Skip to content

JS-2774 Consume the shared identifier inventory. #8325

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.

Sign up for GitHub

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

Draft
ZiadMarey wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

JS-2774 Consume the shared identifier inventory. #8325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions packages/analysis/src/jsts/rules/S5332/identifier-urls.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* SonarQube JavaScript Plugin
* Copyright (C) SonarSource Sàrl
* mailto:info AT sonarsource DOT com
*
* You can redistribute and/or modify this program under the terms of
* the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
* See the Sonar Source-Available License for more details.
*
* You should have received a copy of the Sonar Source-Available License
* along with this program; if not, see https://sonarsource.com/license/ssal/
*/
import { strict as assert } from 'node:assert';
import { describe, it } from 'node:test';
import identifierUrls from '@sonarsource/analyzer-commons-configurations/identifier-urls.json' with { type: 'json' };
import { isKnownIdentifierUrl } from './identifier-urls.js';

describe('S5332 identifier URL matcher', () => {
it('uses an empty identifier inventory', () => {
assert.deepEqual(identifierUrls, {});
});

it('does not match URLs when the inventory is empty', () => {
assert.equal(isKnownIdentifierUrl('http://identifier.invalid/schema'), false);
});
Comment on lines +23 to +29

@gitar-bot gitar-bot Bot Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: Tests never exercise the URL parsing or matching logic

Both tests depend on the inventory being empty. One asserts identifierUrls deep-equals {}, which will fail as soon as Commons ships any entry. The other only checks that an empty map returns false. So the authority/remainder regex, lowercasing only the authority, keeping the port, adding / before ?/# suffixes, and running the per-domain regex all go untested. A bug in any of them would pass. Drive isKnownIdentifierUrl with a fixture inventory (for example, factor the matcher to accept a Map<string, RegExp>) and test port, case, query, fragment and non-matching paths. Drop the assertion that the upstream data is empty.

Was this helpful? React with 👍 / 👎

});
41 changes: 41 additions & 0 deletions packages/analysis/src/jsts/rules/S5332/identifier-urls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* SonarQube JavaScript Plugin
* Copyright (C) SonarSource Sàrl
* mailto:info AT sonarsource DOT com
*
* You can redistribute and/or modify this program under the terms of
* the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
* See the Sonar Source-Available License for more details.
*
* You should have received a copy of the Sonar Source-Available License
* along with this program; if not, see https://sonarsource.com/license/ssal/
*/

import identifierUrls from '@sonarsource/analyzer-commons-configurations/identifier-urls.json' with { type: 'json' };

@gitar-bot gitar-bot Bot Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Cross-Repo: Commons npm package doesn't publish identifier-urls.json

Fix here and in SonarSource/sonar-analyzer-commons

Breaks in: SonarSource/sonar-analyzer-commons - sonar-analyzer-commons-configurations/src/main/packaging/npm/package.json - files

This file imports @sonarsource/analyzer-commons-configurations/identifier-urls.json. In SonarSource/sonar-analyzer-commons, the files list in sonar-analyzer-commons-configurations/src/main/packaging/npm/package.json ships only README.md, secret-patterns.json and secret-exclusion-corpus.json. The 2.31.0-5284 tarball that SonarJS pins matches that: I downloaded it and it has no identifier-urls.json. So the module can't be resolved, and TypeScript build, bundling and the new test all fail. Commons has to generate and publish the inventory in a new npm release, and this repo has to move its pin to that release.

Here: Once Commons publishes a release that contains identifier-urls.json, bump @sonarsource/analyzer-commons-configurations in package.json and regenerate package-lock.json. Until then, keep this PR in draft so master doesn't break.

There: In sonar-analyzer-commons-configurations, generate identifier-urls.json (the domain-to-regex map) into the npm packaging dir, add it to files in src/main/packaging/npm/package.json, and publish a new version.

In SonarSource/sonar-analyzer-commons - sonar-analyzer-commons-configurations/src/main/packaging/npm/package.json:

"files": [
  "README.md",
  "secret-patterns.json",
  "secret-exclusion-corpus.json",
  "identifier-urls.json"
]

Was this helpful? React with 👍 / 👎


// The inventory is maintained in Analyzer Commons; keep the matching semantics aligned.

@gitar-bot gitar-bot Bot Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Cross-PR: Padded URLs match in the Java filter but not in SonarJS · SonarSource/sonar-analyzer-commons #590

Breaks when sonar-analyzer-commons#590 is released with a populated inventory and the S5332 rule checks a URL string that has leading or trailing whitespace.

How

  1. CleartextProtocolFilter.isKnownIdentifier calls url.strip() before matching (commons/src/main/java/org/sonarsource/analyzer/commons/appsec/CleartextProtocolFilter.java:244).
  2. isKnownIdentifierUrl runs /^http:\/\/.../i directly on value with no trim, so any surrounding whitespace makes exec return null (packages/analysis/src/jsts/rules/S5332/identifier-urls.ts:34).
  3. So the same padded identifier URL is excluded by Java analyzers but still reported by SonarJS, even though the comment at identifier-urls.ts:20 says the matching semantics are kept aligned.

Fix

  • This PR: Trim value the same way as Java's strip() before running the authority regex, and add a test for a padded URL.
  • sonar-analyzer-commons#590: Nothing needed. Optionally list the whitespace stripping in the README's consumer matching rules.

Compared 4281d21 with sonar-analyzer-commons#590 at c0c18f9

Was this helpful? React with 👍 / 👎

// A match identifies a possible identifier; the caller must also check its use in the AST.
const IDENTIFIER_URLS = new Map<string, RegExp>(
Object.entries<string>(identifierUrls).map(([domain, pattern]) => [domain, new RegExp(pattern)]),
);

/**
* The Commons npm package shares the inventory as JSON. Its Java matcher,
* CleartextProtocolFilter.isKnownIdentifier(), is not exposed through that package.
* SonarJS therefore performs URL parsing and regex matching here in Node.js using the shared data.
*/
export function isKnownIdentifierUrl(value: string): boolean {

@gitar-bot gitar-bot Bot Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: isKnownIdentifierUrl is never called by the S5332 rule

The PR title says S5332 now consumes the shared identifier inventory. But isKnownIdentifierUrl is only imported by its own test: rule.ts, rule.lib.ts and rule.aws.ts don't reference it. So the rule still reports http:// identifier URLs exactly as before and the change has no effect on behavior. Either call the matcher from the cleartext-protocol check in rule.lib.ts (with the AST-usage check the comment requires), or describe the PR as adding the helper only.

Was this helpful? React with 👍 / 👎

// Do not use URL.hostname (drops ports), URL.host (normalizes :80), or a lowercased
// URL (changes identifier spelling). Only the authority is case-insensitive.
const match = /^http:\/\/([^\s/?#\\]+)([/?#][^\s\\]*)?$/i.exec(value);
if (!match) {
return false;
}
const remainder = match[2] ?? '';
const suffix = remainder.startsWith('/') ? remainder : `/${remainder}`;
return IDENTIFIER_URLS.get(match[1].toLowerCase())?.test(suffix) === true;
}
Loading