Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decode<JwtHeader> not working #456

Closed
thoron opened this issue Jan 12, 2023 · 25 comments · Fixed by #459
Closed

Decode<JwtHeader> not working #456

thoron opened this issue Jan 12, 2023 · 25 comments · Fixed by #459
Assignees
Labels

Comments

@thoron
Copy link

thoron commented Jan 12, 2023

Regression issue in Decode

When upgrading from 9.0.3 to 10.0.0 decode header will cease to function correctly, all fields will be null:

var tokenString = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c";
var serializer = new JsonNetSerializer();
var urlEncoder = new JwtBase64UrlEncoder();
var decoder = new JwtDecoder(serializer, urlEncoder);

var header = decoder.DecodeHeader<JwtHeader>(tokenString);
Console.WriteLine(header.Algorithm);

For 9.0.3

HS256

For 10.0.0

 // Empty string or null
@abatishchev abatishchev self-assigned this Jan 12, 2023
@abatishchev
Copy link
Member

hi @thoron, what's your target platform?

I'm unable to reproduce the issue. I've put your token into TestData and these Header related tests. After updating the asserts accordingly they all have passed.

@thoron
Copy link
Author

thoron commented Jan 13, 2023

@abatishchev I encountered the issue when using a win11 computer. Now using a Linux computer (Pop 22.04) I cannot reproduce it either. Both were targeting net7.0.
I do not currently have access to a Windows computer, I will try to repro again on Monday and if I cannot not I will close this issue and blame cosmic background radiation.

I could reproduce it on my Linux machine when using same target (net7.0) and dotnet version (7.0.102)

[Test]
public void TestJwt()
{
	var tokenString = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c";
	var serializer = new JsonNetSerializer();
	var urlEncoder = new JwtBase64UrlEncoder();
	var decoder = new JwtDecoder(serializer, urlEncoder);

	var header = decoder.DecodeHeader<JwtHeader>(tokenString);
	Assert.AreEqual("HS256", header.Algorithm); // null
}

@abatishchev
Copy link
Member

I have only Windows machine but I'll try to reproduce it on different frameworks too.

@abatishchev abatishchev added bug and removed question labels Jan 13, 2023
@abatishchev
Copy link
Member

I see, so you can reproduce it now.
Does it still happen if you downgrade the target version to net6.0? And please confirm it does not happen if you downgrade the version of JWT to 9.x.
It might be a bug in STJ, not JWT though :(

@thoron
Copy link
Author

thoron commented Jan 17, 2023

Using .NET 7 SDK with target net6.0 can to reproduce the issue when using JWT 10.0.0. Downgrading fixes the issue.

@PeterHagen
Copy link

I'm running into a similar issue, accept the problem comes from using JsonProperty attributes. I made a little unit test for this:

using JWT;
using JWT.Algorithms;
using JWT.Builder;
using JWT.Serializers;
using Newtonsoft.Json;

namespace JwtTokenTest;

public class UnitTest1
{
    [Fact]
    public void DecodeWithJsonPropertyNotWorkingTest()
    {
        var secret = "ahu9ar9hgiurehagh;areifuiawehfiawheuifhaewhfi;ewhiguahierug";
        var myValue = "abcd";
        var encoded = CreateToken(secret, new OAuthPayloadModel()
        {
            AccessToken = myValue,
            Exp = DateTimeOffset.UtcNow.AddSeconds(26600).ToUnixTimeSeconds()
        });
        Assert.NotNull(encoded);
        
        var oauth2 = DecodeToken<OAuthPayloadModel>(secret, encoded);
        Assert.Null(oauth2?.AccessToken);
        // Assert.Equal(myValue, oauth2.AccessToken);
    }
    
    [Fact]
    public void DecodeWithoutJsonPropertyNotWorkingTest()
    {
        var secret = "ahu9ar9hgiurehagh;areifuiawehfiawheuifhaewhfi;ewhiguahierug";
        var myValue = "abcd";
        var encoded = CreateToken(secret, new OAuthPayloadWithoutJsonPropertyModel()
        {
            AccessToken = myValue,
            Exp = DateTimeOffset.UtcNow.AddSeconds(26600).ToUnixTimeSeconds()
        });
        Assert.NotNull(encoded);
        
        var oauth2 = DecodeToken<OAuthPayloadWithoutJsonPropertyModel>(secret, encoded);
        Assert.NotNull(oauth2?.AccessToken);
        Assert.Equal(myValue, oauth2.AccessToken);
    }
    
    /// <summary>
    /// https://github.com/jwt-dotnet/jwt
    /// </summary>
    protected string CreateToken(string secret, object payload)
    {
        if (string.IsNullOrEmpty(secret))
            throw new Exception("Api secret not set");

        var algorithm = new HMACSHA256Algorithm(); 
        var serializer = new JsonNetSerializer();
        var urlEncoder = new JwtBase64UrlEncoder();
        var encoder = new JwtEncoder(algorithm, serializer, urlEncoder);
        var token = encoder.Encode(payload, secret);

        return token;
    }

    protected T DecodeToken<T>(string secret, string token) where T : class, new()
    {
        return new JwtBuilder()
            .WithAlgorithm(new HMACSHA256Algorithm()) // symmetric
            .WithSecret(secret)
            .MustVerifySignature()
            .Decode<T>(token);
    }
    
    public class OAuthPayloadModel
    {
        [JsonProperty("HA")]
        public string AccessToken { get; set; }
        
        [JsonProperty("EX")]
        public long? Exp { get; set; }
    }
    
       
    public class OAuthPayloadWithoutJsonPropertyModel
    {
        public string AccessToken { get; set; }
        
        public long? Exp { get; set; }
    }
}

Both unit test will run fine, but the first one,DecodeWithJsonPropertyNotWorkingTest, should NOT get a null value from decode.

This issue is with .Net 7 (7.0.101) on macOS and in production on Linux (latest version also)

@PeterHagen
Copy link

PeterHagen commented Jan 17, 2023

I'm running into a similar issue, accept the problem comes from using JsonProperty attributes. I made a little unit test for this:

...

I found the issue. It seems that on encoding I specify the JsonSerializer, while on de decoder I didn't:

    protected string CreateToken(string secret, object payload)
    {
        if (string.IsNullOrEmpty(secret))
            throw new Exception("Api secret not set");

        return  new JwtBuilder()
            .WithJsonSerializer(new JsonNetSerializer())
            .WithAlgorithm(new HMACSHA256Algorithm()) // symmetric
            .WithSecret(secret)
            .MustVerifySignature()
            .Encode(payload);
    }

    protected T DecodeToken<T>(string secret, string token) where T : class, new()
    {
        return  new JwtBuilder()
            .WithJsonSerializer(new JsonNetSerializer())
            .WithAlgorithm(new HMACSHA256Algorithm()) // symmetric
            .WithSecret(secret)
            .MustVerifySignature()
            .Decode<T>(token);
    }

I guess that version 10 maybe default switch from json serialised, therefor breaking my code. Setting the serializer on encoding and decoding solves my problem.

@hartmark
Copy link
Contributor

hartmark commented Jan 17, 2023

I found this issue too. It's because now it decodes using System.Text.Json instead of Json.NET for newer .net versions.

@abatishchev This is the first non-beta nuget for which #401 is live, right?

Previously it uses Json.NET and for that you can have a global state for the serializer settings, now from 10.0.0.0 and upwards it will use System.Text.Json as default.

One thing with System.Text.Json is that it has no global state anymore, probably to make it more thread safe and so on. But it will break some previous assumptions in older code.

I have this code to set the Json.NET global serializer settings to serialize as camel-case.

JsonConvert.DefaultSettings = () => new JsonSerializerSettings
{
    ContractResolver = new CamelCasePropertyNamesContractResolver(),
    Formatting = Formatting.None
};

So previously my code just serialized my JWTs with the following code:

var token = JwtBuilder.Create()
    .WithAlgorithm(new NoneAlgorithm())
    .AddHeader("vrsn", "1")
    .WithSecret(Secret);

To make it use Json.NET again you can just build your tokens with this code instead, note the last line:

  var token = JwtBuilder.Create()
      .WithAlgorithm(new NoneAlgorithm())
      .AddHeader("vrsn", "1")
      .WithSecret(Secret)
      .WithJsonSerializer(new JsonNetSerializer())

@abatishchev
Copy link
Member

Hi everyone, thanks for chiming in and sorry for the troubles. I hoped making STJ the serializer by default would improve things, not the other way around.

This issue is with .Net 7 (7.0.101) on macOS and in production on Linux (latest version also)

Do you know if an issue was filed on them?

This is the first non-beta nuget for which #401 is live, right?

Correct. Multiple beta versions were published and I hoped that would be tested by the users. But since it's a pre-release naturally some people try to avoid them (me included).

WithJsonSerializer(new JsonNetSerializer())

Yes, that's the current workaround.

Previously it uses Json.NET and for that you can have a global state for the serializer settings, now from 10.0.0.0 and upwards it will use System.Text.Json as default.

What I can do is to roll back the change and make Json.Net the serializer by default again, let's say in version 10.1.0. What everyone would think?

Meanwhile, if someone could contribute to #407 it'd be very much appreciated and could have had caught this issue earlier.

@hartmark
Copy link
Contributor

I think it's good that there is no longer any hidden global state for the serializer. This is however a new major version so some change in behaviour will be expected.

So I don't it's needed to rollback to JSON.net as the default. Perhaps having it mentioned in the release notes would be good however.

I have no experience with setting up pipelines but having one for Linux would be nice.

@PeterHagen
Copy link

PeterHagen commented Jan 17, 2023

Personally, no 'rollback' is necessary for me. Is there any reference (possible breaking change) about this change on the release notes? That should be enough, in my opinion. I have to admit, I just updated the nuget packages in my project without thinking about anything. And... no unit tests...

The reason why it didn't work for me, was because of using [JsonProperty("...")] attributes on the properties, which is not supported by System.Text.Json. Using the .WithJsonSerializer(new JsonNetSerializer()) should be sufficient.

@abatishchev
Copy link
Member

abatishchev commented Jan 17, 2023

Is there any reference (possible breaking change) about this change on the release notes?

Yes, I've started keeping CHANGELOG.md, per the suggestion by @cmeeren.

The reason why it didn't work for me, was because of using [JsonProperty("...")]

In your own code? Here's how it "worked around" in the library's:

#if MODERN_DOTNET
using JsonProperty = System.Text.Json.Serialization.JsonPropertyNameAttribute;
#else
using Newtonsoft.Json;
#endif

Just in case it'd be helpful.

@PeterHagen
Copy link

PeterHagen commented Jan 17, 2023

https://github.com/jwt-dotnet/jwt/blob/05fce3601db0fe7280a14605b45c40d6cba399ce/src/JWT/Builder/JwtHeader.cs#L1-5

Just in case it'd be helpful.

In my case it's being mixed, so using System.Text.Json and NewtonSoft's JsonProperty. So that won't work.

I'm afraid I cheered a bit too early this afternoon. I now created 2 tests, where I for use JsonNetSerializer, and for the other the SystemTextSerializer. I also added the JsonProperty for both. SystemTextSerializer works, but JsonNetSerializer doesn't:

using System;
using JWT.Algorithms;
using JWT.Builder;
using JWT.Serializers;
using Newtonsoft.Json;
using Xunit;

namespace WillowMedia.Insight.Tests;

public class JwtNewtonSoftTests
{
    public class TokenModel
    {
        [Newtonsoft.Json.JsonProperty("AT")]
        public string AccessToken { get; set; }

        public long? Exp { get; set; }
    }

    [Fact]
    public void NoneAlgorithmEncodeDecodeTest()
    {
        var Secret = "wgaeg43g35h44a3hq4w55a3";
        var token = JwtBuilder.Create()
            .WithAlgorithm(new NoneAlgorithm())
            .WithSecret(Secret)
            .WithJsonSerializer(new JsonNetSerializer());

        var encoded = token.Encode(new TokenModel()
        {
            AccessToken = "abc123",
            Exp = DateTimeOffset.UtcNow.AddHours(1).ToUnixTimeSeconds() 
        });
        Assert.NotNull(encoded);
        
        token = JwtBuilder.Create()
            .WithAlgorithm(new NoneAlgorithm())
            .WithSecret(Secret)
            .WithJsonSerializer(new JsonNetSerializer());

        var payloadDecoded = token.Decode<TokenModel>(encoded);
        Assert.NotNull(payloadDecoded?.AccessToken);
    }   
}

Could you please try if this works for you?

@abatishchev
Copy link
Member

abatishchev commented Jan 18, 2023

Found the issue, will fix and release an update version shortly.

Long story short: the initial idea was to replace NS.J with STJ but then it evolved into allowing them side-by-side. But the header class accidentally stayed working only for the former option. This code precisely causes the issue:

#if MODERN_DOTNET
using JsonProperty = System.Text.Json.Serialization.JsonPropertyNameAttribute;
#else
using Newtonsoft.Json;
#endif

Fixed version:

#if MODERN_DOTNET
using System.Text.Json.Serialization;
#endif
using Newtonsoft.Json;

Sorry for the troubles! And thank you everyone for helping to identify the issue.

@abatishchev
Copy link
Member

abatishchev commented Jan 18, 2023

Pushed version 10.0.1.

@PeterHagen
Copy link

Thanks, but unfortunately there is no change for me. I can't use the JsonProperty or JsonPropertyName attributes. I removed them so at least it works

@abatishchev
Copy link
Member

I can't use the JsonProperty or JsonPropertyName attributes.

Can you describe your scenario and/or environment? I bet there is more than one way to customize/override (de)serialization behavior.

@PeterHagen
Copy link

PeterHagen commented Jan 24, 2023

I have this test (xunit new project) with NewtonSoft.Json:

using JWT.Algorithms;
using JWT.Builder;
using JWT.Serializers;

namespace JwtTests;

public class JwtNewtonSoftTests
{
    public class TokenModel
    {
        /// <summary>
        /// Our accesstoken
        /// </summary>
        [Newtonsoft.Json.JsonProperty("AT")]
        public string AccessToken { get; set; }

        /// <summary>
        /// Exp is the expiration date in seconds since epoch
        /// </summary>
        public long? Exp { get; set; }
    }
    
    public const string Secret = "wgaeg43g35h44a3hq4w55a3";

    [Fact]
    public void NoneAlgorithmEncodeDecodeTest()
    {
        var token = JwtBuilder.Create()
            .WithAlgorithm(new NoneAlgorithm())
            .WithSecret(Secret)
            .WithJsonSerializer(new JsonNetSerializer());

        var model = new TokenModel()
        {
            AccessToken = "abc123",
            Exp = DateTimeOffset.UtcNow.AddHours(1).ToUnixTimeSeconds()
        };
        var encoded = token.Encode(model);
        Assert.NotNull(encoded);
        
        token = JwtBuilder.Create()
            .WithAlgorithm(new NoneAlgorithm())
            .WithSecret(Secret)
            .WithJsonSerializer(new JsonNetSerializer());

        var payloadDecoded = token.Decode<TokenModel>(encoded);
        Assert.Equal(model.AccessToken, payloadDecoded.AccessToken);
    }
    
    [Fact]
    public void HMACSHA256AlgorithmEncodeDecodeTest()
    {
        var jwtToken = JwtBuilder.Create()
            .WithAlgorithm(new HMACSHA256Algorithm())
            .WithSecret(Secret)
            .WithJsonSerializer(new JsonNetSerializer());

        var model = new TokenModel()
        {
            AccessToken = "abc123",
            Exp = DateTimeOffset.UtcNow.AddHours(1).ToUnixTimeSeconds()
        };
        var encoded = jwtToken.Encode(model);
        Assert.NotNull(encoded);

        jwtToken = JwtBuilder.Create()
            .WithAlgorithm(new HMACSHA256Algorithm())
            .WithSecret(Secret)
            .WithJsonSerializer(new JsonNetSerializer());
        
        var payloadDecoded = jwtToken.Decode<TokenModel>(encoded);
        Assert.Equal(model.AccessToken, payloadDecoded.AccessToken);
    }
}

These both fail

@abatishchev
Copy link
Member

abatishchev commented Jan 24, 2023

Even in version 10.0.1? And succeeds in version 9.0.3?

@PeterHagen
Copy link

Yes, this is with 10.0.1

@abatishchev abatishchev reopened this Jan 24, 2023
@PeterHagen
Copy link

Would you like me to publish the test project somewhere?

@abatishchev
Copy link
Member

Sure, please. I think this would be the right place for it: https://github.com/jwt-dotnet/jwt/blob/main/tests/JWT.Tests.Common/Builder/JwtBuilderDecodeTests.cs

@hartmark
Copy link
Contributor

Good finding, seems when I wrote this I didn't take into account that the name of the properties might have been overriden by the attribute.

@PeterHagen I used your tests as base and added them into the repo in the linked PR above

@PeterHagen
Copy link

I forgot to mention that this also applies to using System.Text.Json with the JsonPropertyNameAttribute, and the correct serializer

@abatishchev
Copy link
Member

Fixed by #464. Thanks to @hartmark! Please try version 10.0.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

4 participants