Skip to content

ACOMMONS-237 Export the S5332 identifier URL inventory through the shared npm package - #592

Merged
ZiadMarey merged 10 commits into
masterfrom
zm/ACOMMONS-237
Nov 12, 2026
Merged

ZiadMarey merged 10 commits into
masterfrom
zm/ACOMMONS-237

Conversation

@ZiadMarey

@ZiadMarey ZiadMarey commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary by Gitar

  • Cleartext-protocol exclusions:
    • Added CleartextProtocolFilter.exportHostPatterns and exportSafeUrlPrefixes to expose existing exclusions for non-JVM analyzers
  • Exports:
    • Added CleartextProtocolFilterExporter and CleartextProtocolCorpusExporter writing JSON files into the npm package
  • npm package:
    • Documented consumer usage in the README and listed the cleartext JSON files in package.json
  • Tests:
    • Added exporter and corpus tests, with the regression corpus shared from Commons test resources
  • Build:
    • Generated cleartext JSON at prepare-package and packaged it through npm.xml
  • Resources:
    • Shared cleartext-protocol-corpus.json via configurations pom.xml resources without packaging it in the Commons JAR

This will update automatically on new commits.

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

ACOMMONS-237

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exports the Java filter’s cleartext exclusions and identifier inventory as JSON, so SonarJS can consume the same configuration without maintaining a duplicate list.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Publishes shared regression examples so consumers can check that their matching behavior agrees with Java. Keeping these examples in the configurations module avoids adding test data to the Commons runtime JAR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Verifies the documented Node.js imports against the assembled npm package and checks its contents against the generated JSON. This catches packaging mistakes that the Java exporter unit tests cannot detect.

Comment thread mise.toml

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adds Node.js to the repository’s tools so the npm package import check can run locally and in CI.

@ZiadMarey
ZiadMarey requested a review from Luqmansonar November 4, 2026 14:12

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see that this PR also introduces new endpoints to filter out.
Can we please do that in a separate PR?
The title of this PR suggests that this change is only about exporting the URL filtering as an npm package.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the clean up!
That makes this already so much easier to review 👍

I compared this PR with the existing secret-classifier exports. I think we can align a few things with that approach:

  1. Reuse RegexTranslator for host patterns. The new exporter writes pattern.pattern() directly, while SecretPatternsExporter calls RegexTranslator.toPortableRegex(...). The current patterns compile in JavaScript, but reusing the translator would protect against future Java-specific regex changes.
  2. Include matching semantics in the JSON. The secret exporter’s match metadata explicitly declares search semantics and case sensitivity. We could similarly declare case-insensitive search for host patterns and case-insensitive prefix matching against the complete URL for safeUrlPrefixes, alongside the README documentation.
  3. Test the exported configuration’s behavior. The new corpus tests call the original Java filter, while the npm test checks imports and JSON equality. In comparison, SecretExclusionCorpusExporterTest loads the exported patterns and verifies that they suppress every known non-secret and preserve every secret candidate. We should similarly exercise the exported host patterns and prefixes here, then run the complete URL corpus against SonarJS because URL parsing differs between runtimes.
  4. Use one shared set of regression cases. Rather than maintaining URL_SAMPLES separately from the filter tests, both should consume shared cases. For secrets, SecretClassifierTest and SecretExclusionCorpusExporter both use exportKnownNonSecretSamples(...). For cleartext, we should follow that shared-source principle while keeping the samples outside the runtime JAR.
  5. Please remove the Node entry in mise.toml and the verify-npm-package-import execution and script for now. Keep Java/Maven verification consistent with the existing exports, and validate npm imports and runtime behavior in SonarJS. As this was sufficient for the secret classifier export I'd argue that it should also be enough here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!
Thanks for the adjustments!

@gitar-bot

gitar-bot Bot commented Nov 12, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 🟡 Medium risk 2 closed / 2 findings

Exporting cleartext URL exclusions to the npm package could suppress insecure-URL findings in consuming analyzers if the matching semantics are misapplied.

Exports the S5332 identifier URL inventory through the shared npm package, with the URL samples map removal and the stacked PR #593 adding identifier samples back. No open issues found; the SonarJS identifier-urls.json import is closed without a fix in this PR.

✅ 2 closed
✅ Cross-PR: #593 still adds identifier samples to the URL_SAMPLES map removed here

📄 sonar-analyzer-commons-configurations/src/main/java/org/sonarsource/analyzer/commons/appsec/export/CleartextProtocolCorpusExporter.java:43-48 📄 commons/src/test/java/org/sonarsource/analyzer/commons/appsec/CleartextProtocolFilterTest.java:171-177
Breaks when #593 is rebased onto this PR and the exporter conflict is resolved by keeping #593's URL_SAMPLES map. The npm corpus would then be built from that map, while the Commons filter tests keep reading the shared JSON resource.

How

  1. This PR removes URL_SAMPLES. toJson() now re-renders commons/src/test/resources/.../cleartext-protocol-corpus.json (CleartextProtocolCorpusExporter.java:43-45).
  2. CleartextProtocolFilterTest.samples() reads that same resource (CleartextProtocolFilterTest.java:171-176), so the exported corpus and the filter tests use one source.
  3. ACOMMONS-241 Add the approved identifier URL inventory and matcher #593 is based on a71f86d, from before this change. It keeps URL_SAMPLES in CleartextProtocolCorpusExporter.java (from line 30) and adds its identifier URL examples there, not to the shared resource.
  4. If the conflict is resolved in favour of ACOMMONS-241 Add the approved identifier URL inventory and matcher #593's file, the larger baseline corpus drops out of the npm export. The new identifier cases would also never reach the Commons filter tests.

Fix

Compared fe00044 with #593 at 1e0100a

Closed: Cross-PR: SonarJS imports identifier-urls.json, which this package never ships

📄 sonar-analyzer-commons-configurations/src/main/packaging/npm/package.json:10-11 📄 sonar-analyzer-commons-configurations/src/main/assembly/npm.xml:33-42 📄 sonar-analyzer-commons-configurations/src/main/java/org/sonarsource/analyzer/commons/appsec/export/CleartextProtocolFilterExporter.java:60-62
Breaks when SonarJS/SonarJS#8325 upgrades to the @sonarsource/analyzer-commons-configurations version built from this PR and Node resolves its JSON import.

How

  1. This PR's package lists only cleartext-protocol-exclusions.json and cleartext-protocol-corpus.json next to the secret files (sonar-analyzer-commons-configurations/src/main/packaging/npm/package.json:10-11, src/main/assembly/npm.xml:33-42). It has no identifier-urls.json.
  2. The authority-to-regex map sits under the identifierUrls key of that document, not at the top level (CleartextProtocolFilterExporter.java:60-62).
  3. SonarJS/SonarJS#8325 imports @sonarsource/analyzer-commons-configurations/identifier-urls.json and reads the domain-to-pattern pairs from the top level of that object (packages/analysis/src/jsts/rules/S5332/identifier-urls.ts:18-24). With this package, that import fails to resolve.
  4. Its test also asserts the inventory is {} (identifier-urls.test.ts:25,30), but this PR ships 29 authorities.

Fix

  • This PR: nothing needed. cleartext-protocol-exclusions.json with a nested identifierUrls object is the format this PR's README documents.
  • SonarJS/SonarJS#8325: import @sonarsource/analyzer-commons-configurations/cleartext-protocol-exclusions.json, build the map from exclusions.identifierUrls, and update the test so it no longer expects an empty inventory (for example, run it against cleartext-protocol-corpus.json).

Compared 5b89733 with SonarJS/SonarJS#8325 at 4281d21

Verification Rules 2/2 met Repos 1 inspected · PRs 2 inspected

Functional validation No results

Rules 2 passed 3 not applicable

✅ Internal artifact registries
✅ Repository license compliance

Broader impact Repos 1 inspected PRs 2 inspected
Cross-repo 1 repository inspected 0 open impacts

SonarSource/SonarJS Inspected

Cross-PR 2 PRs inspected 0 open impacts

PR 8325: JS-2774 Consume the shared identifier inventory. Inspected
PR 593: ACOMMONS-241 Add the approved identifier URL inventory and matcher Inspected

Auto-approval Not enabled

Set up

For more verification details, comment gitar display:verbose.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown

@ZiadMarey
ZiadMarey merged commit 61ee101 into master Nov 12, 2026
11 checks passed
@ZiadMarey
ZiadMarey deleted the zm/ACOMMONS-237 branch November 12, 2026 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants