Repository navigation
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| ['adnovum.ch', /^\/nevisidm\/ws/], | ||
| ['api.onedoc.ch', /^\/(?:LocationServiceType|profession)/], | ||
| ['archway.archre.com', /^\/user_profile_id/], | ||
| ['auth0.com', /^\/oauth\/grant-type/], | ||
| ['big.arvato-infoscore.de', /^\/BIG_V2/], | ||
| ['bigcontent.io', /^\/cms\/schema/], | ||
| ['cloud.gbtec.de', /^\/rels/], | ||
| ['cv.iptc.org', /^\/newscodes\/digitalsourcetype/], | ||
| ['dashif.org', /^\/(?:guidelines|thumbnail_tile)/], | ||
| ['data.europa.eu', /^\/(?:esco|p27)/], | ||
| ['dbpedia.org', /^\/(?:ontology|property|resource)/], | ||
| ['diaceutics.com', /^\/roles/], | ||
| ['elimu.io', /^\/careplan\/disableSMS/], | ||
| ['experian.com', /^\/communicationV2\/schemas/], | ||
| ['g-p.com', /^\/schema/], |
There was a problem hiding this comment.
💡 Edge Case: Path prefixes have no segment boundary, so unrelated paths match
Every path regex is an open prefix such as /^\/fhir/, /^\/rels/, /^\/sct/ or /^\/schema/, with nothing checking where the path segment ends. As a result, http://hapifhir.io/fhir-proxy, http://cloud.gbtec.de/release-notes, http://snomed.info/sctapi and http://dbpedia.org/resources-api are all treated as identifiers, and their cleartext URLs would go unreported. The tests do check ? and # as boundaries (/GPX?version=1#section), which suggests segment-level matching is what's intended.
Fix: put a boundary after each prefix, e.g. (?=[/?#]|$). Alternatively, make the boundary part of the matcher so individual entries don't each need it.
Require that the matched prefix ends at a path, query or fragment boundary (or the end of the string), except for whole-host '/' entries.:
const suffix = remainder.startsWith('/') ? remainder : `/${remainder}`;
const pattern = IDENTIFIER_URLS.get(match[1].toLowerCase());
if (!pattern) {
return false;
}
const m = pattern.exec(suffix);
return m !== null && (m[0] === '/' || /^(?:[/?#]|$)/.test(suffix.slice(m[0].length)));
Was this helpful? React with 👍 / 👎
|
|
if this needs to be in sync with analyzer commons, let's remove it from SonarJS and add rely on it. analzyer-commons already publishes an npm package that SonarJS depends on. Let's keep a single source of truth |
| * 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' }; |
There was a problem hiding this comment.
🚨 Bug: Imported identifier-urls.json is not in any published Commons package
This commit swaps the inline map for import identifierUrls from '@sonarsource/analyzer-commons-configurations/identifier-urls.json'. The root package.json still pins 2.31.0-5284, and that tarball contains only README.md, secret-patterns.json and secret-exclusion-corpus.json. I unpacked every published version (2.32.0-5319, 2.33.0-5369, 2.34.0-15495, 2.35.0-15509) and none of them ships identifier-urls.json. The files whitelist in the latest package.json also leaves it out. So the TypeScript build fails with a module-not-found error, and so does loading the S5332 helper and its tests. That blocks the whole analysis package, not just this rule.
The fix is to publish a Commons release that includes identifier-urls.json and bump the dependency in package.json and package-lock.json in this PR. If that release isn't ready yet, keep the inline inventory until it is.
Was this helpful? React with 👍 / 👎
| const IDENTIFIER_URLS = new Map<string, RegExp>( | ||
| Object.entries(identifierUrls).map(([domain, pattern]) => [domain, new RegExp(pattern)]), | ||
| ); |
There was a problem hiding this comment.
⚠️ Cross-PR: Empty exported inventory makes every identifier URL fail to match · SonarSource/sonar-analyzer-commons #590
Breaks when SonarJS bumps @sonarsource/analyzer-commons-configurations to a release built from SonarSource/sonar-analyzer-commons#590 before that PR fills in the inventory.
How
CleartextProtocolFilter.IDENTIFIER_URLSisMap.of()(CleartextProtocolFilter.java:228), andIdentifierUrlsExporter.toJson()serializes it, soidentifier-urls.jsoncomes out as{}.- This PR now builds
IDENTIFIER_URLSonly from that JSON (identifier-urls.ts:22-24), which replaces the inline map it removed. - With an empty map,
IDENTIFIER_URLS.get(...)is alwaysundefinedandisKnownIdentifierUrlreturnsfalsefor every URL (identifier-urls.ts:40). - Every case in
identifiersassertstrue(identifier-urls.test.ts:113-115), so they all fail, and S5332 can no longer recognize any identifier URL.
Fix
- This PR: Don't bump the dependency until a Commons release ships a populated
identifier-urls.json. Or keep the inline inventory as the source until then. - SonarSource/sonar-analyzer-commons#590: Fill
IDENTIFIER_URLSwith the full authority-to-pattern inventory before releasing the npm artifact that SonarJS will consume.
Compared 827218c with SonarSource/sonar-analyzer-commons#590 at c0c18f9
Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Comment gitar unblock to override this block and allow merging.
Configure merge blocking · Maintainers can dismiss this review.
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 4 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 417a1c3 | Docs | View more details | Give us feedback! |
| 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); | ||
| }); |
There was a problem hiding this comment.
💡 Quality: Matcher parsing logic is no longer tested; empty-map test is vacuous
This commit removes the roughly 100 lines of matcher tests. The two tests left only check that the shipped inventory is {} and that one URL returns false. With an empty IDENTIFIER_URLS map, isKnownIdentifierUrl returns false for any input, so the second test passes no matter what the authority regex, port handling, case folding, or suffix logic in identifier-urls.ts:34-40 does. None of that parsing code is exercised. The deepEqual(identifierUrls, {}) test also fails as soon as a Commons bump fills in the inventory, and it locks in the current state where the S5332 identifier exception does nothing. A fix is to have the matcher take the inventory as input, e.g. createIdentifierUrlMatcher(inventory), and restore the parsing tests against fixture data, without asserting on the contents of the published JSON.
Build the matcher from an inventory you pass in, so tests can supply fixture data:
export function createIdentifierUrlMatcher(inventory: Record<string, string>) {
const urls = new Map(Object.entries(inventory).map(([d, p]) => [d, new RegExp(p)]));
return (value: string): boolean => {
const match = /^http:\/\/([^\s/?#\]+)([/?#][^\s\]*)?$/i.exec(value);
if (!match) return false;
const remainder = match[2] ?? '';
const suffix = remainder.startsWith('/') ? remainder : `/${remainder}`;
return urls.get(match[1].toLowerCase())?.test(suffix) === true;
};
}
export const isKnownIdentifierUrl = createIdentifierUrlMatcher(identifierUrls);
Was this helpful? React with 👍 / 👎
| export function isKnownIdentifierUrl(value: string): boolean { | ||
| // 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); |
There was a problem hiding this comment.
💡 Cross-PR: Whitespace-padded identifier URLs match in Java but not SonarJS · SonarSource/sonar-analyzer-commons #590
Breaks when sonar-analyzer-commons#590 merges and a later Commons release adds inventory entries, and SonarJS then checks an identifier URL literal that has leading or trailing whitespace.
How
packages/analysis/src/jsts/rules/S5332/identifier-urls.ts:34runs the anchored regex^http://...$on the rawvalue, and[^\s...]rejects any surrounding whitespace.CleartextProtocolFilter.isKnownIdentifierin sonar-analyzer-commons#590 callsurl.strip()before matching.sonar-analyzer-commons-configurations/src/main/packaging/npm/README.md:27(sonar-analyzer-commons#590) tells consumers to strip surrounding whitespace first.- As a result, for an input like
' http://<known>/x ', Java returns true and SonarJS returns false. That goes against the file's own comment, "keep the matching semantics aligned".
Fix
- This PR: Trim surrounding whitespace before running the authority regex, e.g.
.exec(value.trim()), and add a test for a padded URL. - sonar-analyzer-commons#590: Nothing needed.
Compared 417a1c3 with sonar-analyzer-commons#590 at c0c18f9
Strip surrounding whitespace before matching, like Java strip():
const match = /^http:\/\/([^\s/?#\]+)([/?#][^\s\]*)?$/i.exec(value.trim());
Was this helpful? React with 👍 / 👎
|
Code Review 🚫 Blocked 1 closed / 6 findings🟡 Medium risk · The new identifier URL matcher controls URL recognition; parsing or regex mismatches could misclassify candidate identifiers during rule analysis. Adds an identifier URL inventory and matcher for S5332 that loads host-to-path-pattern data from Analyzer Commons JSON, but the PR is blocked. The imported
🚨 Bug: Imported identifier-urls.json is not in any published Commons package📄 packages/analysis/src/jsts/rules/S5332/identifier-urls.ts:18 This commit swaps the inline map for The fix is to publish a Commons release that includes
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
Summary by Gitar
Mapof compiled regexesidentifier-urls.jsonis currently empty, soisKnownIdentifierUrlreturns false for every URLThis will update automatically on new commits.