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

[Bug] when read claims from JWT is skipping even claims and only reading odds. #2546

Open
2 of 14 tasks
jmborroto opened this issue Mar 31, 2024 · 5 comments
Open
2 of 14 tasks
Assignees
Labels
Bug Product is not functioning as expected Dependency Mismatch Transitive dependency might be at play and create issues resulting in incorrect versions of a class P1 More important, prioritize highly

Comments

@jmborroto
Copy link

jmborroto commented Mar 31, 2024

Which version of Microsoft.IdentityModel are you using? 7.5.0

Where is the issue?

  • M.IM.JsonWebTokens
  • M.IM.KeyVaultExtensions
  • M.IM.Logging
  • M.IM.ManagedKeyVaultSecurityKey
  • M.IM.Protocols
  • M.IM.Protocols.OpenIdConnect
  • M.IM.Protocols.SignedHttpRequest
  • M.IM.Protocols.WsFederation
  • M.IM.TestExtensions
  • M.IM.Tokens
  • M.IM.Tokens.Saml
  • M.IM.Validators
  • M.IM.Xml
  • S.IM.Tokens.Jwt
  • Other (please describe)

Is this a new or an existing app?
The app is on development.

Repro
Attached is a UnitTest project which includes three tests:

  1. The first test is designed to fail in order to demonstrate the error.
  2. The second test includes additional claims to show that the library skips even-numbered claims, yet it passes because the extra claims help by making the real ones able to be read.
  3. The final test demonstrates other issues stemming from this error, such as failing to validate the Issuer, audience, or expiration. These issues depend on whether the preceding number of claims results in an even or odd total.

JWT token Test.zip

Expected behavior
When claims are added to the token, all of them should be retrievable independen of the order or parity.

Actual behavior
The library is skipping one claim in between. To be more precise, the odd-numbered claims are retrievable, while the even-numbered claims are skipped. It appears that everything was fine until version 7.3.1. However, if you test it with version 7.4 or higher, the bug will start to appear.

@jmborroto jmborroto changed the title [Bug] when read claims from JWT is skipping pair claims and only reading odds. [Bug] when read claims from JWT is skipping even claims and only reading odds. Apr 2, 2024
@ArthurSett
Copy link

ArthurSett commented Apr 3, 2024

Same problem. Every second claim is skipped, including the expiration date (which breaks the JWT logic). The JwtPayload.CreatePayload method uses Utf8JsonReader to parse the token and calls reader.Read() each time to read the value and keys forward. Therefore my first guess: it must be a misplaced “reader.Read()” in the code.

I debugged the parsing loop cycle in case of a custom claim-id/name and it led me to the first (reader index at first Entry.Key), second (first Entry.Value) and third (second Entry.Key) reader.Read() in one cycle. When the parsing cycle tries to parse the next key it gets value of the second Entry.

I reverted to 7.3.1 as @jmborroto said and it works again.

@jennyf19 jennyf19 added Bug Product is not functioning as expected P1 More important, prioritize highly labels Apr 5, 2024
@FuPingFranco
Copy link
Contributor

FuPingFranco commented Apr 12, 2024

@jmborroto & @ArthurSett, issue is related to dependency mismatch on the test you provide, please update your transitive dependencies to match Version="7.5.0" for certain transitive dependencies.

Please try the following in on the JWT token Test.csproj.

  <ItemGroup>
    <PackageReference Include="coverlet.collector" Version="6.0.2">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
    <PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="8.0.3" />
    <PackageReference Include="Microsoft.Extensions.Options" Version="8.0.2" />
    <PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" Version="7.5.0" />
    <PackageReference Include="Microsoft.IdentityModel.Protocols" Version="7.5.0" />
    <PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" Version="7.5.0" />
    <PackageReference Include="System.IdentityModel.Tokens.Jwt" Version="7.5.0" />
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.9.0" />
    <PackageReference Include="xunit" Version="2.7.0" />
    <PackageReference Include="xunit.runner.visualstudio" Version="2.5.7">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>

Please confirm if this works to close this issue. Thanks!

@FuPingFranco FuPingFranco reopened this Apr 12, 2024
@FuPingFranco FuPingFranco added the Dependency Mismatch Transitive dependency might be at play and create issues resulting in incorrect versions of a class label Apr 12, 2024
@jmborroto
Copy link
Author

jmborroto commented Apr 13, 2024

This is a new project created only to add the test class and added the MI.JsonWebToken nugget package to make it easy to demonstrate, if any dependency that is needed is not included by default is because the nugget package is not configured to include the correct ones. That also caused issues in our big project where we first notice the issue so if the nugget package is not fix then anyone just installing the nugget package will have the same issue.

@FuPingFranco
Copy link
Contributor

FuPingFranco commented Apr 15, 2024

We are working on this issue but, we are tracking it on a separate issue here #2513 to avoid having multiple issues related to the same bug please refer to the status of #2513.

@hahn-kev
Copy link

Yikes I just ran into this when I installed Openiddict and it referneced a newer version of the identity model packages, but the Identity openIDConnect was an old version being referneced by Aspnet.Core JwtBearer. I hope this can get figured out so it's less painful to debug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Product is not functioning as expected Dependency Mismatch Transitive dependency might be at play and create issues resulting in incorrect versions of a class P1 More important, prioritize highly
Projects
None yet
Development

No branches or pull requests

6 participants