fix(security): harden high scan findings#258
Conversation
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| const comparatorTokens = spec | ||
| .split(/[\s,]+/) | ||
| .map((token) => token.trim()) | ||
| .filter(Boolean); | ||
|
|
||
| if ( | ||
| comparatorTokens.length > 0 && | ||
| comparatorTokens.every((token) => /^(?:<=|>=|<|>|=)\s*v?\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?$/.test(token)) | ||
| ) { | ||
| return comparatorTokens.every((token) => evaluateComparator(token)); | ||
| } | ||
|
|
There was a problem hiding this comment.
versionMatches should keep comparator+version pairs together, otherwise spaced ranges like <= 0.0.2 fall through to true and match every version — can we split only on commas or extract comparator tokens with a regex?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
skills/clawsec-nanoclaw/lib/advisories.ts around lines 78-163, specifically in
versionMatches where you build comparatorTokens (lines 122-133) and evaluateComparator
(lines 103-117), the tokenizer splits on whitespace so a comparator like "<= 0.0.2"
becomes separate tokens ("<=" and "0.0.2"), causing the every() validation to fail and
the function to incorrectly fall back to returning true. Refactor comparator parsing to
extract full operator+version pairs with a regex (e.g., find all occurrences of optional
operator followed immediately by a semver version, allowing whitespace between operator
and version) instead of spec.split(/[ \s,]+/). After token extraction, evaluate each
comparator token; if the spec contains comparator operators but cannot be tokenized into
valid pairs, return false rather than true so invalid/partially parsed ranges don’t
match everything.
There was a problem hiding this comment.
Commit 8157697 addressed this comment by replacing the whitespace split with regex-based comparator extraction that keeps operator+version pairs together, including spaced forms like <= 0.0.2. It also returns false when comparator operators are present but cannot be parsed into valid pairs, preventing the прежний fallback-to-true behavior.
| /** | ||
| * @param {HookDescriptor} hook | ||
| * @param {string} targetPath | ||
| * @param {string} _targetPath | ||
| * @param {number} timeoutMs | ||
| * @returns {Promise<Vulnerability[]>} | ||
| */ | ||
| async function evaluateHook(hook, targetPath, timeoutMs) { | ||
| async function evaluateHook(hook, _targetPath, timeoutMs) { | ||
| const findings = []; | ||
| const invocationTimeoutMs = Math.max(1000, timeoutMs); | ||
|
|
||
| for (const eventKey of hook.events) { | ||
| const safeEvent = buildEvent(eventKey, "safe baseline input", targetPath); | ||
| const safeContext = { | ||
| skillPath: hook.hookDir, | ||
| agentPlatform: "openclaw", | ||
| dastMode: true, | ||
| targetPath: path.resolve(targetPath), | ||
| event: eventKey, | ||
| }; | ||
|
|
||
| const safeResult = await invokeHookHarness(hook, safeEvent, safeContext, invocationTimeoutMs); | ||
| const inspection = await inspectHookHandler(hook, invocationTimeoutMs); |
There was a problem hiding this comment.
Can we move const inspection = await inspectHookHandler(...) outside the for (const eventKey of hook.events) loop and reuse it per hook? It only depends on handlerPath/exportName, so this repeats the same spawn/read/analysis for every event.
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`skills/clawsec-scanner/scripts/dast_runner.mjs` around lines 472-517 inside
`evaluateHook`, the code currently calls `await inspectHookHandler(hook,
invocationTimeoutMs)` once per `eventKey`, even though `inspectHookHandler` only depends
on the hook descriptor and returns the same inspection result for each event. Refactor
by running `inspectHookHandler` a single time before the `for (const eventKey of
hook.events)` loop, storing the result (including `timedOut`, `parseError`, `stderr`,
and `parsed`), and then reusing it inside the loop to generate the same per-event
findings/IDs/messages. If any part of `inspectHookHandler`’s output is actually
event-specific, adjust `inspectHookHandler` to accept `eventKey` (or document/verify the
invariant) before moving it; otherwise ensure the change preserves the existing
vulnerability text while eliminating redundant spawns.
There was a problem hiding this comment.
Commit 8157697 addressed this comment by moving inspectHookHandler(hook, invocationTimeoutMs) before the for (const eventKey of hook.events) loop and reusing the single inspection result for every event. The per-event findings still get generated, but the redundant spawn/read/analysis work now happens only once per hook.
| const semverPattern = String.raw`v?\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?`; | ||
|
|
||
| const parseVersion = (ver: string): number[] | null => { | ||
| const match = ver.match(new RegExp(`^${semverPattern}$`)); | ||
| if (!match) return null; | ||
| const parts = match[0].replace(/^v/, '').match(/^(\d+)\.(\d+)\.(\d+)/); | ||
| if (!parts) return null; | ||
| return [parseInt(parts[1], 10), parseInt(parts[2], 10), parseInt(parts[3], 10)]; |
There was a problem hiding this comment.
parseVersion doesn't guard against a null match before accessing match[0], and also strips prerelease data so versionMatches() treats 1.2.3-beta.1 as equal to 1.2.3 — should we add a null guard (returning null for malformed input) and preserve prerelease segments while only stripping build metadata after +?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
In skills/clawsec-nanoclaw/lib/advisories.ts around lines 89-96, refactor
`parseVersion()` to: 1. Add a null guard after `ver.match(...)` — if `match` is falsy,
return `null` immediately instead of throwing on `match[0]` access. Ensure all
downstream callers of `parseVersion()` handle a possible `null` return gracefully. 2.
Return both the numeric components and the prerelease segment (strip only build metadata
after `+`), then update the comparator/equality logic (`=`, `>=`, `<=`, and any
caret/tilde paths) to compare prereleases using SemVer rules (prerelease < release;
prerelease identifiers compared lexically/numerically per spec), so prereleases no
longer collapse into the same match as the corresponding release.
There was a problem hiding this comment.
Commit a0c50e4 addressed this comment by adding a null check in parseVersion() and returning null for non-matching inputs instead of touching match[0]. It also preserves prerelease identifiers, strips only build metadata, and updates version comparison/range logic so prereleases no longer collapse into the base release.
| const evaluateComparator = (comparator: string): boolean => { | ||
| const match = comparator.trim().match(new RegExp(`^(<=|>=|<|>|=)?\\s*(${semverPattern})$`)); | ||
| if (!match) return false; | ||
|
|
||
| const operator = match[1] || '='; |
There was a problem hiding this comment.
The new comparator evaluation block duplicates the semver parsing/comparison already in skills/hermes-attestation-guardian/lib/semver.mjs and skills/clawsec-suite/hooks/clawsec-advisory-guardian/lib/version.mjs, should we reuse the shared helper instead of copying this logic here?
Want Baz to fix this for you? Activate Fixer
| const semverPattern = String.raw`v?\d+\.\d+\.\d+(?:-[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?`; | ||
| const semverRegex = new RegExp( | ||
| String.raw`^v?(\d+)\.(\d+)\.(\d+)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?$` | ||
| ); |
There was a problem hiding this comment.
semverPattern is unused here, so with noUnusedLocals this may fail the TypeScript build — can we drop it and keep only semverRegex, or wire it in if needed?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
skills/clawsec-nanoclaw/lib/advisories.ts around lines 96-99 inside versionMatches,
remove the unused semverPattern declaration (it’s defined but never referenced).
Ensure only semverRegex remains (or, if you prefer, rewrite semverRegex to use
semverPattern) so the helper compiles cleanly under noUnusedLocals/noUnusedParameters.
| // Tilde range (~1.2.3): patch-level compatibility (1.2.x where x >= 3) | ||
| if (spec.startsWith('~')) { | ||
| if (vParts[0] !== specParts[0]) return false; | ||
| if (vParts[1] !== specParts[1]) return false; | ||
| return vParts[2] >= specParts[2]; | ||
| const upperBound = { major: specParts.major, minor: specParts.minor + 1, patch: 0, prerelease: [] }; | ||
| return compareVersions(vParts, specParts) >= 0 && compareVersions(vParts, upperBound) < 0; | ||
| } |
There was a problem hiding this comment.
The new versionMatches duplicates the caret/tilde/prerelease semver logic already in other packages, so we now have three copies to keep in sync — should we extract a shared helper like skills/shared/semver.ts and import it here?
Want Baz to fix this for you? Activate Fixer
1 participant
User description
Summary
Security benefit
Testing
node skills/clawsec-nanoclaw/test/security-hardening.test.mjsnode skills/clawsec-clawhub-checker/test/reputation_check.test.mjsnode skills/clawsec-scanner/test/dast_harness.test.mjsnode skills/clawsec-scanner/test/reviewer_regressions.test.mjspython3 utils/validate_skill.py skills/clawsec-nanoclawpython3 utils/validate_skill.py skills/clawsec-clawhub-checkerpython3 utils/validate_skill.py skills/clawsec-scanner(passes with existing README recommendation warning)/opt/homebrew/bin/npx tsc --noEmitPATH="/opt/homebrew/bin:$PATH" /opt/homebrew/bin/npx eslint . --ext .ts,.tsx,.js,.jsx,.mjs --max-warnings 0 --ignore-pattern '.worktrees/**'PATH="/opt/homebrew/bin:$PATH" /opt/homebrew/bin/npm run buildnode skills/clawsec-scanner/scripts/dast_runner.mjs --target skills/clawsec-scanner/hooks --format jsongit diff --checkNote: the literal repo lint command without the
.worktrees/**ignore fails locally because this checkout contains an ignored.worktrees/pr-255/distartifact with bundled JS output; source lint passes with that local artifact excluded.Generated description
Below is a concise technical summary of the changes proposed in this PR:
Harden the ClawHub reputation gate, NanoClaw integrity service, and NanoClaw advisory matching by blocking explicit malicious verdicts, validating integrity IPC request IDs, and adding comparator-range semantics before persisting results. Replace the scanner’s DAST execution harness with a static inspection flow that only reads handler sources for coverage and risk signals so untrusted hook code is never imported or executed.
blockOnMaliciousScannerData), mock-testing the behavior, and updating the skill metadata/CHANGELOG to reflect the new0.0.5release.Modified files (5)
Latest Contributors(2)
validateRequestId/resolveResultPath), and bumping the skill metadata to0.0.7.Modified files (6)
Latest Contributors(2)
readHookSource, detect exports, collect risk signals, reuse that output across events, and log coverage-only findings while documenting the new behavior and testing that hooks are not executed.Modified files (7)
Latest Contributors(2)