fix(security): escape backslashes in remaining sanitization helpers (CodeQL follow-up)#169
Merged
Merged
Conversation
… fix missed second device-permission site Three follow-ups to PR #167 after CodeQL's fresh scan flagged additional issues: 1. scripts/cdp-bridge/src/tools/device-permission.ts:143 — the second androidKey.replace site was missed by the previous edit. Now also uses escapeRegex(). 2. scripts/learned-actions.mjs (escapeMarkdownTableCell) — escape backslashes FIRST so a pre-existing \| doesn't become \\|. 3. docs-site/scripts/generate-tool-docs.mjs (escapeMdx) — escape backslashes FIRST so a pre-existing \{ doesn't become \\{ after the brace escape. The remaining 3 js/bad-code-sanitization errors (#20 #23 #24) are dismissed as false positives via the API: defense-in-depth runtime validators + JSON.stringify prevent injection but CodeQL's static flow analysis cannot trace the validator.
Lykhoyda
added a commit
that referenced
this pull request
May 19, 2026
…lure (closes #136 sub-issue 1) (#174) * chore: gitignore codex-pair per-developer artifacts codex-pair (ask-llm plugin) is opt-in via a .codex-pair-context.md marker in the project root. The marker enables the PostToolUse hook; its content is the per-file project context the reviewer uses. Marker is per-developer (own model/threshold preferences, own rules) so it does not belong in shared history. Same for the log, cache, and pause sentinel directories — all developer-local. See https://github.com/Lykhoyda/ask-llm marketplace plugin for details. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(device): hard-fail device_screenshot when platform: explicit and raw path fails Closes #136 (sub-issue 1 — re-evidence on #60). PR #142's "graceful degradation" — on raw screenshot failure, fall through to runAgentDevice — was the regression vector. agent-device's --platform routing is exactly the broken path PR #142 was created to avoid. The fallback re- introduced the bug whenever the raw path tripped on transient state (the user's session: OOM-unstable emulator → adb devices reports it as 'offline' → parseAdbDevicesEmu skips it → resolver returns null → fallback fires → iOS screen captured for a platform=android request). New behavior: when platform: is passed explicitly and the raw path fails, return a structured failResult with code=SCREENSHOT_FAILED and meta.reason=('no-device' | 'capture-failed') instead of trying agent-device. The error message names the underlying CLI (xcrun simctl / adb) and the sub-cause so the caller knows what to fix (boot a device vs retry a transient state). Implicit-platform calls (platformExplicit=false) still route through runAgentDevice — backward parity preserved by the existing 'platformExplicit=false → uses runAgentDevice (backward parity)' test. Changes: - src/tools/device-screenshot-raw.ts: tryRawScreenshot now returns a discriminated union {ok:true,path} | {ok:false,reason} so callers can distinguish resolver miss from capturer failure. - src/tools/device-list.ts: captureAndResizeScreenshot returns failResult on explicit-platform raw failure; imports failResult. - test/unit/gh-136-screenshot-raw-platform.test.js: inverted the existing 'falls through' test to assert hard-fail; added a second hard-fail test for the capture-failed surface; tightened the three pre-existing resolver/capturer null-checks to deepEqual on the new union shape. Verified: 1465/1465 cdp-bridge unit tests passing (+1 net new test). Also included: scripts/cdp-bridge/dist/tools/device-permission.js — dist was stale relative to source (PR #169's second escapeRegex site fix never made it to dist). Rebuilt locally; no source change. NOTE: codex-pair flagged an unrelated pre-existing issue in this file: deriveScreenshotPath throws PathTraversalScreenshotError on a '..' path, but no catch wraps the handler, so a bad path becomes an uncaught tool exception. Filed as #136-followup, NOT addressed here. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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.
Follow-up to PR #167 — CodeQL's fresh scan after merge flagged 4 additional issues:
scripts/cdp-bridge/src/tools/device-permission.ts:143— the secondandroidKey.replacesite was missed in fix(security): close all 14 open CodeQL alerts (3 errors + 11 warnings) #167. Now also usesescapeRegex().scripts/learned-actions.mjs(escapeMarkdownTableCell) — escape backslashes FIRST.docs-site/scripts/generate-tool-docs.mjs(escapeMdx) — escape backslashes FIRST.The 3 remaining
js/bad-code-sanitizationerrors (#20 #23 #24) are dismissed via API as false positives — runtime regex validators +JSON.stringifyprevent injection but CodeQL's static flow analyzer can't trace the validators.