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] JwtPayload.Claim.Value should be culture invariant #2409

Closed
1 of 14 tasks
huaxing-yuan opened this issue Nov 18, 2023 · 0 comments · Fixed by #2453 or #2461
Closed
1 of 14 tasks

[Bug] JwtPayload.Claim.Value should be culture invariant #2409

huaxing-yuan opened this issue Nov 18, 2023 · 0 comments · Fixed by #2453 or #2461
Assignees
Labels
Bug Product is not functioning as expected Regression
Milestone

Comments

@huaxing-yuan
Copy link

huaxing-yuan commented Nov 18, 2023

Which version of Microsoft.IdentityModel are you using?
System.IdentityModel.Tokens.Jwt 7.0.3

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?
c. This is a new app or an experiment.

Repro
Step 1: Set computer localization to one of west european countries, for example "fr-FR", where decimal separator is comma ,.
In these countries, decimal value 10.9 is written as 10,9.
Step 2: Create a JWT with a decimal type claim in the Payload:
Step 3: Get the claim in the payload. The claim.value has a string value 10,9

var credentials = new SigningCredentials(securityKey, algorithm); //whatever signing credentials
var jwtHeader = new JwtHeader(credentials); //whatever headers
var payload = new JwtPayload();
payload.add("numericClaim", 10.9d); //<- here
var token = new JwtSecurityToken(jwtHeader, payload)

Expected behavior
At API level, we would like decimal and datetime data to be represented in invariant culture.
DateTime is perfectly handled with invariant culture: line 522
But all decimal values are not: line 532

claims.Add(new Claim(keyValuePair.Key, dateTime.ToString("o", CultureInfo.InvariantCulture), ClaimValueTypes.DateTime, issuer, issuer));
else if (keyValuePair.Value is bool boolValue)
{
// Can't just use ToString or bools will get encoded as True/False instead of true/false.
if (boolValue)
claims.Add(new Claim(keyValuePair.Key, "true", ClaimValueTypes.Boolean, issuer, issuer));
else
claims.Add(new Claim(keyValuePair.Key, "false", ClaimValueTypes.Boolean, issuer, issuer));
}
else if (keyValuePair.Value != null)
claims.Add(new Claim(keyValuePair.Key, keyValuePair.Value.ToString(), GetClaimValueType(keyValuePair.Value), issuer, issuer));

Actual behavior
Claim.value is string, from what i have obseved, when the claim is added in the payload with a simple ToString(). The claim value is stored with current culture of the computer (for example: fr-FR, which is not exterchangable with another computer en-US)
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-

Claim.Value stores "10,9", which is not expected at API/data exchange level. We would like all numeric values to be stored as standard iso format.

Possible solution
I think the behaviour can be fixed in these two lines, by adding a check on numberic types (decimal, double, float, ...) and use invariant culture version of string conversion:

claims.Add(new Claim(keyValuePair.Key, keyValuePair.Value.ToString(), GetClaimValueType(keyValuePair.Value), issuer, issuer));

claims.Add(new Claim(key, obj.ToString(), GetClaimValueType(obj), issuer, issuer));

Additional context / logs / screenshots / links to code
please find here a screenshot of debug info of a generated token:
image

@keegan-caruso keegan-caruso added this to the 7.2.1 milestone Jan 10, 2024
@keegan-caruso keegan-caruso added the Bug Product is not functioning as expected label Jan 10, 2024
@keegan-caruso keegan-caruso self-assigned this Jan 23, 2024
keegan-caruso pushed a commit that referenced this issue Jan 23, 2024
keegan-caruso added a commit that referenced this issue Jan 26, 2024
* Use invariant culture when formatting string

Fixes #2409

---------

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>
@keegan-caruso keegan-caruso reopened this Jan 26, 2024
keegan-caruso pushed a commit that referenced this issue Jan 26, 2024
keegan-caruso added a commit that referenced this issue Jan 26, 2024
#2453 was incomplete

Fixes #2409

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment