Mitigate validator isURL bypass and harden audit#204
Open
leynos wants to merge 1 commit into
Open
Conversation
Apply a local patch to validator's isURL implementation and wire custom audit scripts to treat the advisory as mitigated once the patch is present.
Reviewer's GuideThis PR hardens the security audit and advisory flow by patching the validator isURL helper to block protocol parsing bypasses, vendoring the patch in package.json, and revamping audit scripts (root and frontend) to parse pnpm audit JSON, enforce exceptions for GHSA-9965-vmph-33xx, and wire the new flow through the Makefile. Sequence diagram for the new audit flow with validator patch enforcementsequenceDiagram
participant Makefile
participant "pnpm run audit"
participant "run-audit.mjs"
participant "isValidatorPatched()"
participant "pnpm audit"
participant User
User->>Makefile: make audit
Makefile->>"pnpm run audit": Run audit script
"pnpm run audit"->>"run-audit.mjs": Execute audit logic
"run-audit.mjs"->>"pnpm audit": Run pnpm audit --json
"pnpm audit"-->>"run-audit.mjs": Return advisories JSON
"run-audit.mjs"->>"isValidatorPatched()": Check if validator is patched
"isValidatorPatched()"-->>"run-audit.mjs": Return patch status
alt Unexpected vulnerabilities
"run-audit.mjs"->>User: Print error and exit 1
else Only validator advisory present and patched
"run-audit.mjs"->>User: Print success and exit 0
end
Class diagram for validator patch verification helpersclassDiagram
class validator_patch_js {
+resolveValidatorPath()
+isValidatorPatched()
}
validator_patch_js : -resolveRulesetRequire()
validator_patch_js : +readFileSync
validator_patch_js : +createRequire
class run_audit_mjs {
+runAuditJson()
+main()
}
run_audit_mjs --> validator_patch_js : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Failed
(1 file with Bumpy Road Ahead)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| validate-audit.js | 1 critical rule | 10.00 → 9.84 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
Comment on lines
+111
to
+144
| function assertMitigated(entries, advisories) { | ||
| if (advisories.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const exceptionsById = new Map(entries.map((entry) => [entry.advisory, entry])); | ||
| const unexpected = []; | ||
|
|
||
| for (const advisory of advisories) { | ||
| const exception = exceptionsById.get(advisory.github_advisory_id); | ||
| if (!exception) { | ||
| unexpected.push(advisory); | ||
| continue; | ||
| } | ||
|
|
||
| if ( | ||
| advisory.github_advisory_id === 'GHSA-9965-vmph-33xx' && | ||
| !isValidatorPatched() | ||
| ) { | ||
| console.error( | ||
| 'Validator vulnerability GHSA-9965-vmph-33xx reported but local patch is missing.', | ||
| ); | ||
| process.exit(1); | ||
| } | ||
| } | ||
|
|
||
| if (unexpected.length > 0) { | ||
| console.error('pnpm audit reported vulnerabilities without exceptions:'); | ||
| for (const advisory of unexpected) { | ||
| console.error(`- ${advisory.github_advisory_id}: ${advisory.title}`); | ||
| } | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
❌ New issue: Bumpy Road Ahead
assertMitigated has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughIntroduce a custom audit flow replacing direct pnpm audit calls. Add a Node script to run and validate audit output, enforce an exception policy, and verify a local patch for validator. Configure pnpm patchedDependencies and add a validator patch file. Extend security validation utilities and exception records. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant MK as Makefile
participant PN as pnpm
participant FE as frontend-pwa run-audit.mjs
participant SA as pnpm audit --json
participant VP as validator-patch check
Dev->>MK: make audit
MK->>PN: pnpm run audit
PN->>FE: node ./scripts/run-audit.mjs
FE->>SA: spawn pnpm audit --json
SA-->>FE: JSON advisories + status
FE->>VP: isValidatorPatched()
alt Unexpected advisories present
FE-->>Dev: Print unexpected advisories
FE->>Dev: exit 1
else Only GHSA-9965-vmph-33xx
alt Patch missing
FE-->>Dev: Error: validator not patched
FE->>Dev: exit 1
else Patch present
FE-->>Dev: OK: advisory mitigated by patch
FE->>Dev: exit 0
end
else No advisories
FE-->>Dev: Forward original status
end
sequenceDiagram
autonumber
participant Sec as security/validate-audit.js
participant SA as pnpm audit --json
participant EX as audit-exceptions.json
participant VP as validator-patch.js
Sec->>EX: Load exceptions (schema + expiry)
Sec->>SA: runAuditJson()
SA-->>Sec: advisories[]
Sec->>Sec: Map advisories vs exceptions
alt Any unexpected advisories
Sec-->>Sec: Collect unexpected list
Sec->>Sec: Exit with failure
else Only GHSA-9965-vmph-33xx present
Sec->>VP: isValidatorPatched()
alt Patched
Sec-->>Sec: Proceed
Sec-->>User: "Audit exceptions valid and vulnerabilities accounted for"
else Not patched
Sec-->>User: Error: missing local patch
Sec->>Sec: Exit with failure
end
else None present
Sec-->>User: "Audit exceptions valid and vulnerabilities accounted for"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The audit JSON parsing and advisory filtering logic is duplicated between security/validate-audit.js and frontend-pwa/scripts/run-audit.mjs—consider extracting a shared utility to avoid drift.
- Rather than calling process.exit directly inside assertMitigated, throw descriptive errors to improve control flow and make the audit script more testable.
- The patchedDependencies field in package.json depends on pnpm’s patch support; ensure your CI/install environments are pinned to a pnpm version that honors this entry so the validator fix is applied.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The audit JSON parsing and advisory filtering logic is duplicated between security/validate-audit.js and frontend-pwa/scripts/run-audit.mjs—consider extracting a shared utility to avoid drift.
- Rather than calling process.exit directly inside assertMitigated, throw descriptive errors to improve control flow and make the audit script more testable.
- The patchedDependencies field in package.json depends on pnpm’s patch support; ensure your CI/install environments are pinned to a pnpm version that honors this entry so the validator fix is applied.
## Individual Comments
### Comment 1
<location> `security/validate-audit.js:89-98` </location>
<code_context>
+import { isValidatorPatched } from '../../security/validator-patch.js';
+const TARGET_ADVISORY = 'GHSA-9965-vmph-33xx';
+
+function runAuditJson() {
+ const result = spawnSync('pnpm', ['audit', '--json'], {
+ encoding: 'utf8',
</code_context>
<issue_to_address>
**suggestion:** Error handling for missing stdout differs from run-audit.mjs.
The current approach returns an empty advisories object when stdout is missing, whereas run-audit.mjs throws an error. Please update this implementation to match run-audit.mjs for consistent error handling and to prevent silent failures.
</issue_to_address>
### Comment 2
<location> `security/validator-patch.js:21` </location>
<code_context>
+export function isValidatorPatched() {
+ const validatorPath = resolveValidatorPath();
+ const contents = readFileSync(validatorPath, 'utf8');
+ return contents.includes("var firstColon = url.indexOf(':');");
+}
</code_context>
<issue_to_address>
**suggestion:** Patch detection relies on a specific code string.
Hardcoding this string makes the detection brittle to upstream changes. Consider using a checksum or a more distinctive signature for improved reliability.
Suggested implementation:
```javascript
import { createHash } from 'crypto';
const PATCHED_VALIDATOR_CHECKSUM = 'your_patched_sha256_checksum_here'; // Replace with actual checksum
function computeChecksum(contents) {
return createHash('sha256').update(contents, 'utf8').digest('hex');
}
export function isValidatorPatched() {
const validatorPath = resolveValidatorPath();
const contents = readFileSync(validatorPath, 'utf8');
const checksum = computeChecksum(contents);
return checksum === PATCHED_VALIDATOR_CHECKSUM;
}
```
1. You need to compute the SHA256 checksum of your patched validator file and replace `'your_patched_sha256_checksum_here'` with the actual value.
2. Make sure the `crypto` module is available in your environment (Node.js).
</issue_to_address>
### Comment 3
<location> `security/validate-audit.js:89` </location>
<code_context>
}
}
+function runAuditJson() {
+ const result = spawnSync('pnpm', ['audit', '--json'], {
+ encoding: 'utf8',
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new runAuditJson and assertMitigated functions.
These functions introduce new logic for parsing audit output and handling advisories. You must provide both behavioural and unit tests to verify their correct operation, including error handling and edge cases.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 4
<location> `security/validate-audit.js:111` </location>
<code_context>
+ }
+}
+
+function assertMitigated(entries, advisories) {
+ if (advisories.length === 0) {
+ return;
</code_context>
<issue_to_address>
**issue (review_instructions):** Provide behavioural and unit tests for assertMitigated to ensure correct handling of advisories and exceptions.
The assertMitigated function contains critical logic for security exception handling. You must add tests that cover scenarios with expected and unexpected advisories, and verify correct process exit behavior.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `frontend-pwa/scripts/run-audit.mjs:12` </location>
<code_context>
}
}
+function runAuditJson() {
+ const result = spawnSync('pnpm', ['audit', '--json'], {
+ encoding: 'utf8',
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for runAuditJson and main in run-audit.mjs.
The new audit script introduces logic for parsing audit output and handling vulnerabilities. You must provide tests to verify correct parsing, error handling, and process exit codes for all relevant scenarios.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 6
<location> `security/validator-patch.js:18` </location>
<code_context>
+ return rulesetRequire.resolve('validator/lib/isURL');
+}
+
+export function isValidatorPatched() {
+ const validatorPath = resolveValidatorPath();
+ const contents = readFileSync(validatorPath, 'utf8');
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for isValidatorPatched and resolveValidatorPath.
These functions are essential for verifying the presence of the validator patch. You must provide tests that confirm correct detection of the patch and handle error cases (e.g., missing files).
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 7
<location> `patches/validator@13.15.15.patch:1` </location>
<code_context>
+diff --git a/es/lib/isURL.js b/es/lib/isURL.js
+index 368e97d9f41baffe6a8db0164e2da3fa459a0be1..c79fe301aff2fa40b769a8443405f99e4df0f407 100644
+--- a/es/lib/isURL.js
</code_context>
<issue_to_address>
**issue (review_instructions):** Demonstrate the validator patch with behavioural and unit tests.
You must provide tests that verify the patched validator's isURL function behaves as intended, especially for the security-relevant cases addressed by the patch.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+89
to
+98
| function runAuditJson() { | ||
| const result = spawnSync('pnpm', ['audit', '--json'], { | ||
| encoding: 'utf8', | ||
| stdio: ['ignore', 'pipe', 'inherit'], | ||
| }); | ||
|
|
||
| if (result.error) { | ||
| throw result.error; | ||
| } | ||
|
|
There was a problem hiding this comment.
suggestion: Error handling for missing stdout differs from run-audit.mjs.
The current approach returns an empty advisories object when stdout is missing, whereas run-audit.mjs throws an error. Please update this implementation to match run-audit.mjs for consistent error handling and to prevent silent failures.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
Comment on lines
+89
to
+103
| function runAuditJson() { | ||
| const result = spawnSync('pnpm', ['audit', '--json'], { | ||
| encoding: 'utf8', | ||
| stdio: ['ignore', 'pipe', 'inherit'], | ||
| }); | ||
|
|
||
| if (result.error) { | ||
| throw result.error; | ||
| } | ||
|
|
||
| if (!result.stdout) { | ||
| return { advisories: {} }; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
In runAuditJson, the script treats a missing stdout as an empty audit result and continues to print “Audit exceptions valid…”. If pnpm audit --json fails before producing output (network outage, registry error, authentication problems), stderr is printed but the function returns {advisories: {}} and the process exits with 0. This bypasses the audit entirely and hides real failures. The helper should propagate the command’s exit status or throw when no JSON is produced so audit failures stop the build.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
security/validate-audit.js (1)
111-144: Flatten mitigation logic and accumulate failures (addresses CodeScene nesting)Reduce nesting, log all issues, and then exit once.
Apply:
-function assertMitigated(entries, advisories) { - if (advisories.length === 0) { - return; - } - - const exceptionsById = new Map(entries.map((entry) => [entry.advisory, entry])); - const unexpected = []; - - for (const advisory of advisories) { - const exception = exceptionsById.get(advisory.github_advisory_id); - if (!exception) { - unexpected.push(advisory); - continue; - } - - if ( - advisory.github_advisory_id === 'GHSA-9965-vmph-33xx' && - !isValidatorPatched() - ) { - console.error( - 'Validator vulnerability GHSA-9965-vmph-33xx reported but local patch is missing.', - ); - process.exit(1); - } - } - - if (unexpected.length > 0) { - console.error('pnpm audit reported vulnerabilities without exceptions:'); - for (const advisory of unexpected) { - console.error(`- ${advisory.github_advisory_id}: ${advisory.title}`); - } - process.exit(1); - } -} +function assertMitigated(entries, advisories) { + if (advisories.length === 0) return; + + const exceptionsById = new Map(entries.map((e) => [e.advisory, e])); + const unexpected = []; + let hasErrors = false; + + for (const advisory of advisories) { + if (!exceptionsById.has(advisory.github_advisory_id)) { + unexpected.push(advisory); + continue; + } + if ( + advisory.github_advisory_id === 'GHSA-9965-vmph-33xx' && + !isValidatorPatched() + ) { + console.error( + 'Validator vulnerability GHSA-9965-vmph-33xx reported but local patch is missing.', + ); + hasErrors = true; + } + } + + if (unexpected.length > 0) { + console.error('pnpm audit reported vulnerabilities without exceptions:'); + for (const advisory of unexpected) { + console.error(`- ${advisory.github_advisory_id}: ${advisory.title}`); + } + hasErrors = true; + } + + if (hasErrors) process.exit(1); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
Makefile(1 hunks)frontend-pwa/package.json(1 hunks)frontend-pwa/scripts/run-audit.mjs(1 hunks)package.json(2 hunks)patches/validator@13.15.15.patch(1 hunks)security/audit-exceptions.json(1 hunks)security/validate-audit.js(2 hunks)security/validator-patch.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
package.json
📄 CodeRabbit inference engine (AGENTS.md)
package.json: ESM‑only packages/apps; configure Vite accordingly; publish only ESM or static assets
Provide Bun scripts for fmt, lint, typecheck, dev, build, preview, test, audit, audit:snyk; prefer bun run/bunx
Export explicit public entry points for libraries via package.json exports/types; avoid wildcard re‑exports
Use caret (^) versions for all direct frontend dependencies; avoid *, >=, or tag aliases; use ~ only with justification
Files:
package.json
{package.json,.browserslistrc}
📄 CodeRabbit inference engine (AGENTS.md)
Define modern evergreen browser support via browserslist; drop legacy where feasible
Files:
package.json
🧬 Code graph analysis (2)
security/validate-audit.js (2)
frontend-pwa/scripts/run-audit.mjs (3)
runAuditJson(38-38)advisories(39-39)unexpected(41-43)security/validator-patch.js (1)
isValidatorPatched(18-22)
frontend-pwa/scripts/run-audit.mjs (2)
security/validate-audit.js (3)
result(90-93)advisories(150-150)unexpected(117-117)security/validator-patch.js (1)
isValidatorPatched(18-22)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (10)
Makefile (1)
143-146: LGTM! Audit flow properly integrated.The change from direct
pnpm auditinvocation topnpm run auditcorrectly delegates to the new script-based validation workflow, ensuring the validator patch is verified before allowing advisories.package.json (2)
37-37: LGTM! Audit script correctly configured.The audit script properly invokes the validation orchestrator that will verify patch application and advisory status.
48-50: Verify patch file existence and format.The patchedDependencies entry maps validator@13.15.15 to patches/validator@13.15.15.patch. Ensure the patch file exists and that pnpm correctly applies it during installation.
Run the following script to verify:
#!/bin/bash # Description: Verify patch file exists and can be applied by pnpm # Check patch file exists if [ ! -f "patches/validator@13.15.15.patch" ]; then echo "ERROR: Patch file not found at patches/validator@13.15.15.patch" exit 1 fi # Verify patch file is a valid unified diff if ! head -n 1 "patches/validator@13.15.15.patch" | grep -q "^diff "; then echo "WARNING: Patch file may not be a valid unified diff" fi # Check if validator 13.15.15 is actually used in dependencies rg -n "validator.*13\.15\.15" pnpm-lock.yaml echo "Patch file exists and validator version found in lockfile"patches/validator@13.15.15.patch (1)
1-44: Patch logic correctly mitigates protocol bypass.The patch adds validation to reject URLs with recognized protocols that lack the standard
://separator. This prevents bypass attempts likehttp:example.comwhich might evade standard URL validation.The logic:
- Checks URLs not starting with
//- Extracts substring before first colon as potential protocol
- Rejects if the protocol is recognised (in
options.protocols) because it indicates a malformed URL- Also rejects if
require_valid_protocolis setBoth ES and CommonJS versions receive identical patches, ensuring consistent behaviour.
Verify the patch doesn't break legitimate edge cases:
#!/bin/bash # Description: Search for existing validator usage patterns that might be affected # Find validator isURL usage with custom protocols rg -n "isURL.*protocols.*:" --type ts --type js -A 5 -B 2 # Check for usage with require_valid_protocol option rg -n "require_valid_protocol" --type ts --type js -A 2 -B 2 # Find test files that might need updating fd -e test.ts -e test.js -e spec.ts -e spec.js | xargs rg -l "isURL"frontend-pwa/scripts/run-audit.mjs (3)
12-35: LGTM! Robust audit JSON retrieval.The function correctly:
- Spawns pnpm audit with JSON output
- Handles spawn errors
- Validates stdout exists before parsing
- Wraps JSON parsing with detailed error messages
- Returns both parsed data and exit status
37-72: LGTM! Audit validation logic is sound.The validation flow correctly:
- Filters unexpected advisories (non-target)
- Exits fatally if any unexpected vulnerabilities exist
- Passes through if target advisory absent (allows clean audits)
- Verifies patch application when target advisory present
- Provides clear success/failure messages
Consider extracting the advisory ID to a configuration file for easier maintenance:
#!/bin/bash # Description: Check if multiple audit scripts exist that might need the same advisory ID # Find other audit-related scripts or configs fd -e mjs -e js -e json | xargs rg -l "GHSA-9965-vmph-33xx|TARGET_ADVISORY" # Check for security configuration files fd "security|audit" -t f -e json -e js
74-79: LGTM! Top-level error handling ensures failures are caught.The try-catch wrapper correctly logs unexpected errors and exits with code 1, preventing silent failures.
frontend-pwa/package.json (1)
14-14: LGTM! Audit script correctly delegates to validation script.The audit script now invokes the custom run-audit.mjs script, which enforces patch verification and targeted advisory validation as designed.
security/audit-exceptions.json (1)
3-10: Time-bounded exception looks correctIDs, advisory, provenance, and dates align with the new validation flow.
Run to verify patchedDependencies is present and points to the patch:
#!/bin/bash # Verify pnpm patchedDependencies contains validator@13.15.15 rg -nC2 -g package.json -P '"patchedDependencies"\s*:\s*\{[\s\S]*?"validator@13\.15\.15"\s*:\s*"patches/validator@13\.15\.15\.patch"'security/validate-audit.js (1)
149-153: Verify advisories shape across pnpm versionsGuard against schema drift by tolerating arrays at top-level and nested locations.
Run:
#!/bash # Print raw audit JSON keys to confirm advisories shape in your CI environment. pnpm audit --json | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>{const j=JSON.parse(s);console.log(Object.keys(j));console.log("advisories keys:", j.advisories && Object.keys(j.advisories).length);});'
Comment on lines
+89
to
+109
| function runAuditJson() { | ||
| const result = spawnSync('pnpm', ['audit', '--json'], { | ||
| encoding: 'utf8', | ||
| stdio: ['ignore', 'pipe', 'inherit'], | ||
| }); | ||
|
|
||
| if (result.error) { | ||
| throw result.error; | ||
| } | ||
|
|
||
| if (!result.stdout) { | ||
| return { advisories: {} }; | ||
| } | ||
|
|
||
| try { | ||
| return JSON.parse(result.stdout); | ||
| } catch (error) { | ||
| error.message = `Failed to parse pnpm audit JSON: ${error.message}`; | ||
| throw error; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add timeout and tighten result handling for pnpm audit
Prevent indefinite hangs; fail with a clear message on timeout; accept non-zero status when JSON is provided.
Apply:
function runAuditJson() {
- const result = spawnSync('pnpm', ['audit', '--json'], {
- encoding: 'utf8',
- stdio: ['ignore', 'pipe', 'inherit'],
- });
+ const result = spawnSync('pnpm', ['audit', '--json'], {
+ encoding: 'utf8',
+ stdio: ['ignore', 'pipe', 'inherit'],
+ timeout: 60_000,
+ });
if (result.error) {
throw result.error;
}
- if (!result.stdout) {
+ if (result.timedOut) {
+ throw new Error('pnpm audit timed out after 60s');
+ }
+
+ if (!result.stdout || !result.stdout.trim()) {
return { advisories: {} };
}
try {
return JSON.parse(result.stdout);
} catch (error) {
error.message = `Failed to parse pnpm audit JSON: ${error.message}`;
throw error;
}
}🤖 Prompt for AI Agents
In security/validate-audit.js around lines 89 to 109, add a 60_000ms timeout to
the spawnSync call to prevent hangs, check result.timedOut and throw a clear
Error('pnpm audit timed out after 60s') if true, and tighten stdout handling by
treating empty or whitespace-only stdout as no advisories (return { advisories:
{} }) — leave behavior that accepts non-zero exit codes intact so JSON output is
still parsed when present.
Comment on lines
+6
to
+11
| function resolveRulesetRequire() { | ||
| const workspaceRequire = createRequire(new URL('../frontend-pwa/package.json', import.meta.url)); | ||
| const orvalRequire = createRequire(workspaceRequire.resolve('orval/package.json')); | ||
| const coreRequire = createRequire(orvalRequire.resolve('@orval/core/package.json')); | ||
| return createRequire(coreRequire.resolve('@ibm-cloud/openapi-ruleset/package.json')); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Harden dependency resolution and add failure context
Guard resolution with try/catch and emit actionable errors. Current chain will throw if the graph shifts (e.g., hoisting or dep rename), breaking the audit flow without signal.
Apply:
function resolveRulesetRequire() {
- const workspaceRequire = createRequire(new URL('../frontend-pwa/package.json', import.meta.url));
- const orvalRequire = createRequire(workspaceRequire.resolve('orval/package.json'));
- const coreRequire = createRequire(orvalRequire.resolve('@orval/core/package.json'));
- return createRequire(coreRequire.resolve('@ibm-cloud/openapi-ruleset/package.json'));
+ try {
+ const workspaceRequire = createRequire(new URL('../frontend-pwa/package.json', import.meta.url));
+ const orvalRequire = createRequire(workspaceRequire.resolve('orval/package.json'));
+ const coreRequire = createRequire(orvalRequire.resolve('@orval/core/package.json'));
+ return createRequire(coreRequire.resolve('@ibm-cloud/openapi-ruleset/package.json'));
+ } catch (err) {
+ const hint = 'Failed to resolve @ibm-cloud/openapi-ruleset via (frontend-pwa → orval → @orval/core). Did the dependency graph change?';
+ throw new Error(`${hint} Root cause: ${err instanceof Error ? err.message : String(err)}`);
+ }
}
Comment on lines
+18
to
+21
| export function isValidatorPatched() { | ||
| const validatorPath = resolveValidatorPath(); | ||
| const contents = readFileSync(validatorPath, 'utf8'); | ||
| return contents.includes("var firstColon = url.indexOf(':');"); |
Contributor
There was a problem hiding this comment.
Fix fragile patch detection; avoid audit bypass
Do not rely on a single substring. Implement a deterministic check against the actual patch file hunks for validator/lib/isURL so the result cannot drift.
Apply:
-import { readFileSync } from 'node:fs';
+import { readFileSync, existsSync } from 'node:fs';
+import { fileURLToPath } from 'node:url';
@@
export function isValidatorPatched() {
- const validatorPath = resolveValidatorPath();
- const contents = readFileSync(validatorPath, 'utf8');
- return contents.includes("var firstColon = url.indexOf(':');");
+ try {
+ const validatorPath = resolveValidatorPath();
+ const contents = readFileSync(validatorPath, 'utf8');
+ // Prefer verifying against the repo patch to avoid brittle heuristics.
+ const patchUrl = new URL('../patches/validator@13.15.15.patch', import.meta.url);
+ if (existsSync(fileURLToPath(patchUrl))) {
+ const patch = readFileSync(patchUrl, 'utf8');
+ const addedLines = [];
+ let inTarget = false;
+ for (const line of patch.split(/\r?\n/)) {
+ if (line.startsWith('diff --git ')) {
+ inTarget = line.includes('validator') && line.includes('isURL');
+ continue;
+ }
+ if (!inTarget) continue;
+ if (line.startsWith('+++ ') || line.startsWith('--- ')) continue;
+ if (line.startsWith('+') && !line.startsWith('+++')) {
+ const l = line.slice(1).trim();
+ if (l) addedLines.push(l);
+ }
+ }
+ if (addedLines.length > 0) {
+ return addedLines.every((l) => contents.includes(l));
+ }
+ }
+ // Fallback: fail closed rather than guessing.
+ return false;
+ } catch {
+ // Fail closed to avoid a false pass in the audit gate.
+ return false;
+ }
}🤖 Prompt for AI Agents
In security/validator-patch.js around lines 18 to 21, the current
patch-detection relies on a single substring which is brittle; replace it with a
deterministic verification that reads the repository patch file and ensures all
added hunk lines for validator/lib/isURL are present in the installed file. Add
imports for existsSync from 'node:fs' and fileURLToPath from 'node:url'; resolve
the patch file via new URL('../patches/validator@13.15.15.patch',
import.meta.url) and fileURLToPath, parse the patch to collect only added lines
within the validator/isURL diff hunk (skip diff headers and +++/--- lines), then
return true only if every collected added line is found in the validator file
contents; if the patch file is missing, parsing fails, or no added lines are
found, fail closed by returning false, and wrap the logic in try/catch to return
false on errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68efa51663ac8322bc16aa73ed2fa8e4
Summary by Sourcery
Apply a local patch to fix the validator isURL protocol bypass and enforce it in CI by extending audit scripts to parse pnpm audit JSON and verify that only the known GHSA-9965-vmph-33xx advisory is exempted when the patch is applied; update package.json, Makefile, and add helper modules and patch file to integrate this hardened audit flow.
New Features:
Enhancements:
Build:
CI: