fix(main): resolve CodeQL alerts #38 and #42 in nx-npm-trust#115
fix(main): resolve CodeQL alerts #38 and #42 in nx-npm-trust#115ThePlenkov merged 4 commits intomainfrom
Conversation
The previous `url.includes('github.com')` check matched github.com anywhere
in the remote URL, including in paths or subdomains such as
https://evil.example/github.com/x/y or https://github.com.attacker.com/x/y.
Replace the substring check with a regex that anchors github.com to the
actual host component for https(s)://, git://, git+ssh://, ssh://, and
SCP-style remotes.
Resolves code-scanning alert #38 (js/incomplete-url-substring-sanitization).
The field stores the configured npm publish MFA *policy* (e.g. `none`, `automatic`), not a user authentication factor or secret. Logging it in the structured JSON report is safe, but the CodeQL heuristic `js/clear-text-logging` fired on the field name `mfa`. Rename clarifies intent and removes the false-positive signal. Resolves code-scanning alert #42 (js/clear-text-logging).
✅ Deploy Preview for adt-cli canceled.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 24 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModified MFA configuration storage key in the check flow from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully resolves both CodeQL security alerts with appropriate fixes. The changes correctly address:
-
Alert #38 (
js/incomplete-url-substring-sanitizationinplugin.ts): Replaced the vulnerable substring check with a comprehensive regex that properly validatesgithub.comas the actual host, preventing URL manipulation attacks. -
Alert #42 (
js/clear-text-loggingincheck.ts): RenamedmfatomfaPolicyto accurately reflect that the field stores a policy value, not sensitive authentication data, eliminating the false positive.
Both fixes are well-implemented, maintain backward compatibility, and include clear documentation explaining the security rationale. No blocking issues identified.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
|
View your CI Pipeline Execution ↗ for commit 69e0e22
☁️ Nx Cloud last updated this comment at |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Latest changes (commit 69e0e22): The refactor addresses a previously noted security concern:
The changes are well-documented with clear JSDoc comments explaining the security rationale. No issues found. Reviewed by minimax-m2.5-20260211 · 450,971 tokens |
There was a problem hiding this comment.
Pull request overview
Fixes two open CodeQL alerts in the internal nx-npm-trust tool by tightening GitHub remote URL detection and avoiding a clear-text-logging false positive in the JSON report output.
Changes:
- Harden GitHub repo detection by matching
github.comonly as the actual remote host (not an arbitrary substring). - Rename the JSON report field from
report.checks.mfatoreport.checks.mfaPolicyto avoid CodeQLjs/clear-text-loggingheuristics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tools/nx-npm-trust/src/plugin.ts | Replaces substring host check with an anchored regex for GitHub remote parsing. |
| tools/nx-npm-trust/src/check.ts | Renames the report field used to record the package MFA publish policy. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Split host extraction into two small paths: `new URL()` for proper URL schemes and a trivial SCP-style regex for `user@host:path`. The combined regex in the previous commit had cyclomatic complexity 31 (> 20 allowed by SonarCloud `typescript:S5843`). Behaviour and the `js/incomplete-url-substring-sanitization` fix are preserved — host match is now anchored via `URL.hostname` or an explicit SCP host capture, so substrings like `github.com.attacker.com` still cannot match. Resolves SonarCloud issue typescript:S5843 on fix/main-health.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… owner/repo) Addresses two bot review findings on PR #115: * Devin Review — the SCP-style regex `^[^@\s]+@([^:]+):(.+)$` falsely matched URL-form remotes that contain both `user@` and a `:port` segment, e.g. `ssh://git@github.com:22/abapify/adt-cli.git`. The port was captured as part of the path and then rejected by parseOwnerRepo, silently disabling trusted-publisher auto-detection. The SCP branch is now skipped whenever the input contains `://`, so URL-form remotes fall through to `new URL()`. * CodeRabbit — the previous shape regex `[^/]+/[^/.]+?` rejected valid GitHub repos with dots in the name (e.g. `owner/repo.name`) and accepted shell metacharacters in the owner segment. The value flows unquoted into a shell command at `plugin.ts` line ~126 (`--trust-repo=${trustRepo}`), so a strict allow-list is enforced before return: `^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$`. This accepts dotted repos and rejects anything that could be interpreted by a shell.
|
Summary
Restores main to a clean state by fixing the two open GitHub Code Scanning
(CodeQL) alerts. Latest CI run on
mainis already green, and SonarCloudfindings are out of scope for this PR.
CI
mainis green (CIworkflow, run24709517306). No CI fix needed.Security (code-scanning)
js/incomplete-url-substring-sanitization— alert Update CI workflow for nx-cloud and ensure fix-ci runs #38 (tools/nx-npm-trust/src/plugin.ts).Replace
url.includes('github.com')with a regex that anchorsgithub.comas the actual host of the remote URL, covering https(s)://, git://,
git+ssh://, ssh://, and SCP-style (
git@github.com:...) remotes.js/clear-text-logging— alert chore: remove redundantversioninput from manual workflow triggers #42 (tools/nx-npm-trust/src/check.ts).Rename
report.checks.mfa→report.checks.mfaPolicy. The field storesthe npm publish policy (
none/automatic), not an auth factor orsecret; the rename removes the false-positive signal without suppressing
the rule.
Quality / AI findings
will be handled separately to keep review scope small.
Test plan
bunx nx format:write— no changesbunx nx run-many -t typecheck lint -p nx-npm-trust— passes (only affected project)versioninput from manual workflow triggers #42 are resolvedSummary by CodeRabbit
Bug Fixes
Refactor