-
Notifications
You must be signed in to change notification settings - Fork 0
Mitigate validator isURL bypass and harden audit #204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /** @file Ensures `pnpm audit` only fails for known, patched validator vulnerability. | ||
| * | ||
| * The validator package currently has no upstream patch release. We vendor the | ||
| * required fix locally and treat the advisory as mitigated when the patched | ||
| * code is present. Any additional vulnerabilities remain fatal. | ||
| */ | ||
|
|
||
| import { spawnSync } from 'node:child_process'; | ||
| import { isValidatorPatched } from '../../security/validator-patch.js'; | ||
| const TARGET_ADVISORY = 'GHSA-9965-vmph-33xx'; | ||
|
|
||
| function runAuditJson() { | ||
| const result = spawnSync('pnpm', ['audit', '--json'], { | ||
| encoding: 'utf8', | ||
| stdio: ['ignore', 'pipe', 'inherit'], | ||
| }); | ||
|
|
||
| if (result.error) { | ||
| throw result.error; | ||
| } | ||
|
|
||
| if (!result.stdout) { | ||
| throw new Error('pnpm audit produced no output to parse'); | ||
| } | ||
|
|
||
| let parsed; | ||
| try { | ||
| parsed = JSON.parse(result.stdout); | ||
| } catch (error) { | ||
| error.message = `Failed to parse pnpm audit JSON: ${error.message}`; | ||
| throw error; | ||
| } | ||
|
|
||
| return { parsed, status: result.status ?? 0 }; | ||
| } | ||
|
|
||
| function main() { | ||
| const { parsed, status } = runAuditJson(); | ||
| const advisories = Object.values(parsed.advisories ?? {}); | ||
|
|
||
| const unexpected = advisories.filter( | ||
| (advisory) => advisory.github_advisory_id !== TARGET_ADVISORY, | ||
| ); | ||
|
|
||
| if (unexpected.length > 0) { | ||
| console.error('Unexpected vulnerabilities detected by pnpm audit:'); | ||
| for (const advisory of unexpected) { | ||
| console.error(`- ${advisory.github_advisory_id}: ${advisory.title}`); | ||
| } | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const targetFinding = advisories.find( | ||
| (advisory) => advisory.github_advisory_id === TARGET_ADVISORY, | ||
| ); | ||
|
|
||
| if (!targetFinding) { | ||
| process.exit(status); | ||
| } | ||
|
|
||
| if (!isValidatorPatched()) { | ||
| console.error( | ||
| 'Validator vulnerability GHSA-9965-vmph-33xx found but local patch missing.', | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| console.info( | ||
| 'Validator vulnerability GHSA-9965-vmph-33xx mitigated by local patch; audit passes.', | ||
| ); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| try { | ||
| main(); | ||
| } catch (error) { | ||
| console.error(error); | ||
| process.exit(1); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| diff --git a/es/lib/isURL.js b/es/lib/isURL.js | ||
| index 368e97d9f41baffe6a8db0164e2da3fa459a0be1..c79fe301aff2fa40b769a8443405f99e4df0f407 100644 | ||
| --- a/es/lib/isURL.js | ||
| +++ b/es/lib/isURL.js | ||
| @@ -91,6 +91,17 @@ export default function isURL(url, options) { | ||
| return false; | ||
| } | ||
| split[0] = url.slice(2); | ||
| + } else { | ||
| + var firstColon = url.indexOf(':'); | ||
| + if (firstColon >= 0) { | ||
| + var potentialProtocol = url.slice(0, firstColon).toLowerCase(); | ||
| + if (options.protocols.indexOf(potentialProtocol) !== -1) { | ||
| + return false; | ||
| + } | ||
| + if (options.require_valid_protocol) { | ||
| + return false; | ||
| + } | ||
| + } | ||
| } | ||
| url = split.join('://'); | ||
| if (url === '') { | ||
| diff --git a/lib/isURL.js b/lib/isURL.js | ||
| index fdd5ea64fb108c2d81bb9b3b1c88abd9c0272bea..00a3d0e3faf2467c6cfbfb5146dd4fd4bbae3e6c 100644 | ||
| --- a/lib/isURL.js | ||
| +++ b/lib/isURL.js | ||
| @@ -97,6 +97,17 @@ function isURL(url, options) { | ||
| return false; | ||
| } | ||
| split[0] = url.slice(2); | ||
| + } else { | ||
| + var firstColon = url.indexOf(':'); | ||
| + if (firstColon >= 0) { | ||
| + var potentialProtocol = url.slice(0, firstColon).toLowerCase(); | ||
| + if (options.protocols.indexOf(potentialProtocol) !== -1) { | ||
| + return false; | ||
| + } | ||
| + if (options.require_valid_protocol) { | ||
| + return false; | ||
| + } | ||
| + } | ||
| } | ||
| url = split.join('://'); | ||
| if (url === '') { |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,12 @@ | ||
| [ | ||
| { | ||
| "addedAt": "2024-01-01", | ||
| "advisory": "GHSA-xxxx-xxxx-xxxx", | ||
| "expiresAt": "2030-01-01", | ||
| "id": "EXAMPLE-0001", | ||
| "introducedBy": "dependency@1.2.3", | ||
| "package": "example-package", | ||
| "reason": "Example exception for development", | ||
| "reviewer": "security@example.com" | ||
| "id": "VAL-2025-0001", | ||
| "package": "validator", | ||
| "advisory": "GHSA-9965-vmph-33xx", | ||
| "introducedBy": "validator@13.15.15", | ||
| "reason": "Local patch hardens isURL protocol parsing until upstream ships a fix.", | ||
| "reviewer": "security@wildside.dev", | ||
| "addedAt": "2025-02-14", | ||
| "expiresAt": "2026-02-14" | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| /** @file Validate audit exception entries against schema and expiry. */ | ||
| import { spawnSync } from 'node:child_process'; | ||
| import Ajv from 'ajv/dist/2020.js'; | ||
| import addFormats from 'ajv-formats'; | ||
| import { isValidatorPatched } from './validator-patch.js'; | ||
|
|
||
| /** | ||
| * Load a JSON file using the import attribute supported by the current Node | ||
|
|
@@ -84,7 +86,68 @@ function assertNoExpired(entries) { | |
| } | ||
| } | ||
|
|
||
| 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 { | ||
|
Comment on lines
+89
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In Useful? React with 👍 / 👎. |
||
| return JSON.parse(result.stdout); | ||
| } catch (error) { | ||
| error.message = `Failed to parse pnpm audit JSON: ${error.message}`; | ||
| throw error; | ||
| } | ||
| } | ||
|
Comment on lines
+89
to
+109
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 |
||
|
|
||
| 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); | ||
| } | ||
| } | ||
|
|
||
| assertValidSchema(data); | ||
| assertNoExpired(data); | ||
|
|
||
| console.log('Audit exceptions valid'); | ||
| const auditJson = runAuditJson(); | ||
| const advisories = Object.values(auditJson.advisories ?? {}); | ||
| assertMitigated(data, advisories); | ||
|
|
||
| console.log('Audit exceptions valid and vulnerabilities accounted for'); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| /** @file Helpers for verifying the locally patched validator dependency. */ | ||
|
|
||
| import { readFileSync } from 'node:fs'; | ||
| import { createRequire } from 'node:module'; | ||
|
|
||
| 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')); | ||
| } | ||
|
|
||
| export function resolveValidatorPath() { | ||
| const rulesetRequire = resolveRulesetRequire(); | ||
| return rulesetRequire.resolve('validator/lib/isURL'); | ||
| } | ||
|
|
||
| export function isValidatorPatched() { | ||
| const validatorPath = resolveValidatorPath(); | ||
| const contents = readFileSync(validatorPath, 'utf8'); | ||
| return contents.includes("var firstColon = url.indexOf(':');"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.