Repository navigation
ACOMMONS-241 Add the approved identifier URL inventory and matcher - #593
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
1e0100a to
73aa693
Compare
46e5342 to
d4b2434
Compare
daniel-teuchert-sonarsource
left a comment
daniel-teuchert-sonarsource
left a comment
There was a problem hiding this comment.
Why do these additions require context checks when existing namespace and identifier-prefix exclusions don’t? If they are intended to follow the existing exclusion policy, we should integrate the domain/path mapping into the normal filter. If context-sensitive treatment is intentional, please explain the distinction and whether existing exclusions should eventually follow it too.
| private static final Pattern CLEARTEXT_AUTHORITY = Pattern.compile( | ||
| "^(?:" + String.join("|", CLEARTEXT_SCHEMES) + ")://(?:[^@\\s/?#]++@)?(?<rest>[^\\s/?#]++)", Pattern.CASE_INSENSITIVE); | ||
|
|
||
| // JS-2698: exact authorities and case-sensitive path + query + fragment patterns. |
There was a problem hiding this comment.
I don't think that we need to mention the JIRA ticket in the code here.
| "^http://([^\\s/?#\\\\]+)([/?#][^\\s\\\\]*)?$", Pattern.CASE_INSENSITIVE); | ||
|
|
||
| /** | ||
| * Recognizes the namespace, schema, vocabulary and protocol identifier inventory shared with SonarJS. |
There was a problem hiding this comment.
I don't think that it makes sense to mention SonarJS here.
Our configuration in analyzer-commons should be as much analyzer agnostic as possible.
The implementation looks okay to me so you probably only have to adjust the comment.
|
@daniel-teuchert-sonarsource The distinction is intentional: JS-2698, which is why we’re adding these URLs, states to preserve the existing exclusions while still reporting actual HTTP requests to the newly added identifier URLs. This PR keeps the new URL matching separate from isSafeWithoutTls() so the JS can apply context-sensitive checks before suppressing an issue. You’re right that the same concern applies to existing namespace and identifier-prefix exclusions. To preserve their current treatment for compitability, rather than because those URLs can never be request destinations. Let me know if you have any questions 😄. |
daniel-teuchert-sonarsource
left a comment
daniel-teuchert-sonarsource
left a comment
There was a problem hiding this comment.
LGTM!
One caveat about completeness: the requested additions are complete, but the resulting list does not preserve every current SonarJS exclusion. The existing SonarJS inventory also contains:
cyclonedx.org — whole host
adlnet.gov — whole host
etherx.jabber.org — whole host
snomed.info — whole host (Only /sct is excluded by this PR)
Please double check if this is intended and if not, add these entries as well.
|
@daniel-teuchert-sonarsource Thank you for pointing that out. I have already discussed with @pierre-loup-tristant-sonarsource he confirmed that we should keep Analyzer Commons’ existing behavior for these four hosts. 👍 |
bb8e977 to
f63dc6f
Compare
Code Review ✅ Approved 🟡 Medium risk 4 closed / 4 findingsExpanding cleartext-protocol exclusions to entire neverssl.com and osmand.net domains could hide genuine insecure HTTP usage from security analysis. Adds the 29 approved JS-2698 identifier URL authority/path patterns and a separate matcher, exported as ✅ 4 closed✅ Quality: npm import test drops existing shape assertions instead of extending them
✅ Cross-PR: SonarJS imports identifier-urls.json, which this package never ships
✅ Quality: First commit references matcher API that only exists in the next commit
✅ Cross-PR: SonarJS helper rejects whitespace-wrapped identifiers that Java accepts
Verification Rules 2/2 met Repos 9 inspected · PRs 2 inspectedFunctional validation No results Rules 2 passed 2 not applicable
Broader impact Repos 9 inspected PRs 2 inspectedCross-repo 9 repositories inspected 0 open impacts
4 more repositories Cross-PR 2 PRs inspected 0 open impacts
Auto-approval Not enabledFor more verification details, comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Add the 29 approved JS-2698 authority/path patterns and a separate identifier URL matcher. Export the inventory as
identifierUrlsincleartext-protocol-exclusions.json, with shared positive and negative regression cases and npm package import verification. ExistingisSafeWithoutTlsbehavior is preserved.Depends on #592; targets
zm/ACOMMONS-237so the diff contains only the identifier additions.Validation: clean Maven verification with coverage passed all 1,261 Java tests and the assembled npm package import check. The Commons JAR is 188,114 bytes, below the unchanged 190,000-byte limit.
https://sonarsource.atlassian.net/browse/ACOMMONS-241
Summary by Gitar
neverssl.comandosmand.netto the domain exclusion pattern inCleartextProtocolFiltercleartext-protocol-corpus.jsonwith positive and negative identifier cases, including lookalike domainsauth0.comOAuth grant-type URLs as safehttp://jabber.org/features/to safe identifier URL prefixesThis will update automatically on new commits.