S5332: FP with Microsoft AD claims

[gian1200 (Giancarlo Calderón Cárdenas)]

• What language is this for?
  Javascript

• Which rule?
  javascript:S5332

• Why do you believe it’s a false-positive/false-negative?
  http string is used as key, not as value.

• Are you using

  • SonarQube - which version? LTA 9.9.6

• How can we reproduce the problem?

  • Use SAML integration with Microsoft AD.
  • Try to read any claim value. Most claim keys are http strings. Examples:
    • “http://schemas.microsoft.com/ws/2008/06/identity/claims/groups”
    • “http://schemas.microsoft.com/identity/claims/displayname”

...
const idpUserInfo = userInfo.identities[0].idpUserInfo;
if (idpUserInfo.attributes) {
	usergroups = idpUserInfo.attributes[
		"http://schemas.microsoft.com/ws/2008/06/identity/claims/groups"
	];
} else {
	usergroups = [];
}
if (idpUserInfo.attributes) {
	username = idpUserInfo.attributes[
		"http://schemas.microsoft.com/identity/claims/displayname"
	];
} else {
	username = idpUserInfo.displayName;
}
...

[sodul (Stephane Odul)]

Disclaimer, I do not code in JavaScript myself, or rather very rarely.

I feel that’s one of the cases where the rule is working as expected and there is no way to programmatically detect that it is a false positive.

That said it seems that these 2 urls are:

• invalid.
• get redirected to https by Microsoft.

> wget http://schemas.microsoft.com/ws/2008/06/identity/claims/groups
URL transformed to HTTPS due to an HSTS policy
--2024-07-08 14:32:22--  https://schemas.microsoft.com/ws/2008/06/identity/claims/groups
Resolving schemas.microsoft.com (schemas.microsoft.com)... 13.107.246.69
Connecting to schemas.microsoft.com (schemas.microsoft.com)|13.107.246.69|:443... connected.
HTTP request sent, awaiting response... 404 Not Found
2024-07-08 14:32:22 ERROR 404: Not Found.

> wget 'http://schemas.microsoft.com/identity/claims/displayname'
--2024-07-08 14:30:29--  http://schemas.microsoft.com/identity/claims/displayname
Resolving schemas.microsoft.com (schemas.microsoft.com)... 13.107.246.69
Connecting to schemas.microsoft.com (schemas.microsoft.com)|13.107.246.69|:80... connected.
HTTP request sent, awaiting response... 307 Temporary Redirect
Location: https://schemas.microsoft.com/identity/claims/displayname [following]
--2024-07-08 14:30:29--  https://schemas.microsoft.com/identity/claims/displayname
Connecting to schemas.microsoft.com (schemas.microsoft.com)|13.107.246.69|:443... connected.
HTTP request sent, awaiting response... 404 Not Found
2024-07-08 14:30:30 ERROR 404: Not Found.

So, at first glance, it seems that you should probably use https for these schemas, but it is possible that these keys are from an API you are calling. If possible fix the API to use urls that are actually working. Again, this is my initial thinking, but more details below.

An other option would be to move these urls to constants and then silence the declaration of the constants so you can use them without additional warnings.

When looking for these URLs I found this C# discussion where not getting the schema urls seems possible: c# - How to configure the claim name of role in IdentityServer4/Identity - Stack Overflow

But it seems that the main problem is how Microsoft decided to design their schemas, which seems to be pretty bad.

SAML 2.0 token claims reference - Microsoft identity platform

I would put this on the infinite pile of very bad Microsoft designs, and I do not think the rule should make exceptions for bad api designs from a major vendor.

[gian1200 (Giancarlo Calderón Cárdenas)]

Thanks for answering.

Agreed, probably a different set of names for their claims could have been defined by them.

However, I’d like to point something out. I’m not doing any kind of request to those URLs.

In JavaScript,

const variable = object.property1;

is equivalent to

const variable = object["property1"];.

This second case is used when property name has special characters.

In my scenario, that URL is the property name of the value I’m trying to read, which is already on local memory (yes, all those attributes come from Microsoft. As shown in your link, all attributes have the same url-like name format)

Reading Add rule S5332 for JS/TS · Issue #2373 · SonarSource/SonarJS · GitHub, it seems that this rule just do literal comparison against a predefined list (with a regex).
I thought that it could be feasible to refine a little bit the rule to allow this general scenario object["xxx"]

[sodul (Stephane Odul)]

I feel that the best approach would be to make these Microsoft hardcoded url keys constants, then silence sonar on these constants since there is not much you can do.

The additional benefits would be that your business logic will become more readable, and less likely to have a bug (imagine you have a typo in these urls if you have the same ones used in multiple places).

const idpUserInfo = userInfo.identities[0].idpUserInfo;
if (idpUserInfo.attributes) {
	usergroups = idpUserInfo.attributes[ID_CLAIMS_GROUPS];
} else {
	usergroups = [];
}
if (idpUserInfo.attributes) {
	username = idpUserInfo.attributes[ID_CLAIMS_DISPLAYNAME];
} else {
	username = idpUserInfo.displayName;
}

You could also start the constants with MS_SCHEMAS = 'http://schemas.microsoft.com/' and base the other constants on that. This way you should get a single warning from the rule.

[Yassin (Yassin Kammoun)]

Hello @gian1200,
Hello @sodul,

Thank you for the very interesting discussion!

Both of you make valid points about the rule and best development practices. While we can all agree that we are facing a bad design here, the rule does make some exceptions for specific URLs (for example, schemas.google.com), and this one from Microsoft is just as legitimate.

Therefore, I created this ticket to make an exception to schemas.microsoft.com.

Cheers,
Yassin