fix(security): resolve CodeQL HIGH/MEDIUM code-scanning findings by pallakatos · Pull Request #32 · Azure/kars

Pal Lakatos-Toth (pallakatos) · 2026-04-24T08:39:31Z

Summary

Fixes all real-code CodeQL findings (HIGH + MEDIUM). Pure security hardening — no behavior changes.

Alerts resolved

HIGH

Alert	Location	Fix
`js/clear-text-logging` #181, #182	`plugin.ts:163-164`	Added `redactSecrets()` (Bearer/tokens/JWTs) around module logger
`js/insecure-temporary-file` #199	`plugin.ts:4611`	Banner marker moved to `~/.cache/azureclaw/banner-printed` (0700/0600)
`js/insecure-temporary-file` #229	`plugin.ts:1492`	`/tmp/.offload-start-<id>` → `fs.mkdtempSync(path.join(os.tmpdir(), 'offload-'))`
`js/file-system-race` #189	`plugin.ts:5387`	`statSync`+`readFileSync` → `openSync`+`fstatSync`+`readSync` (single fd)
`js/file-system-race` #200	`mesh-plugin/connection.ts:634`	Same TOCTOU fix — single fd
`js/file-system-race` #201	`plugin.ts:1609`	Same TOCTOU fix — single fd
`js/reflected-xss` #183	`commands/mesh.ts:185`	`result.error` HTML-escaped in OAuth callback page

MEDIUM

Alert	Location	Fix
`js/indirect-command-line-injection` #230	`plugin.ts:1556`	`execSync` with shell → `execFileSync("find", [...])` (no shell)
`js/log-injection` #185	`commands/mesh.ts:549`	`sanitizeForLog()` strips CR/LF/tabs
`js/log-injection` #186, #187	`stepper.ts:158, 166`	Same CR/LF stripping in `kvLine`/`checkLine`

Verification

✅ cd cli && npm run typecheck
✅ cd cli && npm run build
✅ cd cli && npm test → 242/244 (2 pre-existing skipped)
✅ cd mesh-plugin && npm run typecheck && npm test → 36/36
✅ cd cli && npm run lint → 0 errors, 24 pre-existing warnings (unchanged)

Not addressed here

js/http-to-file-access docs(r4+r5): release-readiness docs — STRIDE, AGT boundary, roadmap, BC, runbook, RC1 changelog #188 + js/file-access-to-http release(r1): cross-runtime AgentMesh first-class tool wrappers #184 (medium, mesh.ts:142/476): expected flows — user's own OAuth identity to the user's own file, user's own configured registry URL read from that same file. Will dismiss in GH UI as false positives.
note-level unused-local-variable and useless-assignment-to-local: pre-existing style findings; deferred to a follow-up cleanup PR.

Risk

Low. Changes are mechanical hardening (escape, redact, single-fd read, no-shell exec). Test suite fully green.

Resolves the following GitHub code-scanning alerts: HIGH: - clear-text-logging (plugin.ts:163-164): add redactSecrets() that masks Bearer tokens, API keys, and JWTs before console.log/warn in the module logger. - insecure-temporary-file (plugin.ts:1492, 4611): replace predictable /tmp/.offload-start-<requestId> marker with fs.mkdtempSync() per-request directory; move the banner dedupe marker from /tmp/.azureclaw-banner-printed to ~/.cache/azureclaw/banner-printed (user-private, mode 0700/0600). - file-system-race / TOCTOU (plugin.ts:1609, 5387; mesh-plugin/connection.ts:634): replace statSync→readFileSync with openSync+fstatSync+readSync so size check and read happen on the same fd. - reflected-xss (commands/mesh.ts:185): HTML-escape result.error in the OAuth callback HTML response (new escapeHtml helper). MEDIUM: - indirect-command-line-injection (plugin.ts:1556): replace execSync with shell-interpolated `find` command by execFileSync("find", [...args]), removing the shell entirely. Size limit enforced via .slice(0, 50) on JS side instead of piping through `head`. - log-injection (commands/mesh.ts:549; stepper.ts:158, 166): strip CR/LF/ tabs from untrusted strings before passing to console.log/error via new sanitizeForLog() helper in stepper.ts and mesh.ts. Verification: - cd cli && npm run typecheck ✓ - cd cli && npm run build ✓ - cd cli && npm test → 242/244 (2 pre-existing skipped) - cd mesh-plugin && npm run typecheck ✓ - cd mesh-plugin && npm test → 36/36 Remaining medium alerts at mesh.ts:142 (http-to-file-access via saveIdentity) and mesh.ts:476 (file-access-to-http via registryUrl) are expected flows — user's own OAuth-verified identity flowing to the user's own identity file, and the user's own configured registry URL being read from that same file. Will be dismissed as false positives in the GH alerts UI. Note-level unused-variable and useless-assignment alerts are pre-existing non-security style findings; deferred to a later cleanup PR to keep this one focused on security fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

        } else {
          console.error(
-            chalk.red(`  ✘ Verification failed: ${result.error ?? "Unknown error"}`)
+            chalk.red(`  ✘ Verification failed: ${sanitizeForLog(result.error ?? "Unknown error")}`)


-  console.log(`${prefix}${k} ${chalk.bold(value)}`);
+  const k = sanitizeForLog(key).padEnd(12);
+  const prefix = icon ? `  ${sanitizeForLog(icon)} ` : "  ";
+  console.log(`${prefix}${k} ${chalk.bold(sanitizeForLog(value))}`);


 export function checkLine(ok: boolean, text: string): void {
  const icon = ok ? chalk.green("✓") : chalk.yellow("○");
-  console.log(`  ${icon} ${text}`);
+  console.log(`  ${icon} ${sanitizeForLog(text)}`);


…+ PEM Original redactor only matched secrets preceded by a keyword (Bearer/token/ api_key/etc.). A bare AzureClaw one-time pairing token like 'azcp_1_eyJjb250cm9sbGVyX2FtaWQi...' logged on its own would slip through. Now also redacts: - azcp_N_<payload> one-time pairing tokens (any version) - -----BEGIN .. KEY-----...-----END .. KEY----- PEM blocks - Basic HTTP auth headers (same as Bearer) - Extended keyword list: handoff_token, admin_token, pairing_token, invite_code, access_token, refresh_token, authorization Export redactSecrets and add 9 unit tests covering every pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- ReDoS in redactSecrets PEM regex (alert #270): bound character classes ({1,40}) and body length ({0,8192}?) so catastrophic backtracking is impossible on crafted '-----BEGIN -----' inputs. - TOCTOU at mesh_transfer_file (alert #268): drop the pre-open statSync isFile/size guard; do kind + size + read entirely via the same fd (openSync → fstatSync → readSync → closeSync). - log-injection at stepper.ts/mesh.ts (alerts #265-#267): switch sanitizeForLog to the classic CodeQL-recognized pattern (split .replace calls for \r, \n, \t) so the JS query models it as a sanitizer. - Unused sanitizeForLog in plugin.ts (alert #269): remove; redactSecrets alone wraps the _log sinks. cli tests: 251/253 pass. mesh-plugin tests: 36/36. typecheck clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Reflect the controls added in #28 (review/w1a), #31 (vulnerable transitive deps), #32 (CodeQL HIGH/MEDIUM closure) and f3eb8ae (sandbox entrypoint hardening) in the security-facing docs. No code changes. docs/security.md - New 'Admin-plane hardening' table under Layer 7 covering: constant-time admin-token compare, #[serde(deny_unknown_fields)] on SpawnRequest/HandoffMeta, ROUTER_ADMIN_ALLOW_IPS, ADMIN_ALLOWED_ORIGINS, canonical 'Authorization: Bearer' header, handoff middleware no-localhost-bypass, per-request size caps, trace-id sanitization, controller-set-only AZURE_IMDS_ENDPOINT/AZURE_AD_ENDPOINT test overrides. Each row carries file:line citations into inference-router/. - New 'Operator CLI & Plugin Hardening' section covering the CodeQL fixes: redactSecrets() (Bearer/JWT/PEM/azcp_*/keyword secrets, ReDoS-bounded), sanitizeForLog() (CRLF/tab strip, CodeQL-recognized split-replace pattern), escapeHtml() on the OAuth callback page, mkdtempSync() per-request tmpdir, openSync+fstatSync+readSync TOCTOU-safe reads, execFileSync('find', [...]) no-shell exec. - New 'Sandbox entrypoint hardening' subsection: EPERM-tolerant chmod/fchmod, AGT_POLICY_DIR profile-leak fix, plugin/SDK/node_modules re-chowned to root RO, regression test asserts every hardening invariant. - New 'Supply-chain & dependency hygiene' subsection: cargo audit CI job (closed RUSTSEC-2026-0098/-0099/-0104 via rustls-webpki bump), npm overrides (uuid/xml2js/lodash), vendored Python wheel + Go toolchain bumps, AgentMesh Cargo.lock bumps, fuzz/proptest targets. docs/security-validation.md - New 'Cross-cutting Hardening' section maps each new control to its automated test (cli/src/redact.test.ts, sandbox-hardening.test.ts, inference-router unit tests, cargo audit, npm audit, cargo fuzz) and provides a one-block reproduction recipe. README.md - Updated 'Admin token' row in Security Model: canonical 'Authorization: Bearer', x-azureclaw-admin deprecation, constant-time compare, ROUTER_ADMIN_ALLOW_IPS / ADMIN_ALLOWED_ORIGINS allowlists. docs/threat-model.md and CHANGELOG.md were already accurate and unchanged. Co-authored-by: Pal Lakatos-Toth <pallakatos@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(security): address CodeQL HIGH/MEDIUM code-scanning findings Resolves the following GitHub code-scanning alerts: HIGH: - clear-text-logging (plugin.ts:163-164): add redactSecrets() that masks Bearer tokens, API keys, and JWTs before console.log/warn in the module logger. - insecure-temporary-file (plugin.ts:1492, 4611): replace predictable /tmp/.offload-start-<requestId> marker with fs.mkdtempSync() per-request directory; move the banner dedupe marker from /tmp/.azureclaw-banner-printed to ~/.cache/azureclaw/banner-printed (user-private, mode 0700/0600). - file-system-race / TOCTOU (plugin.ts:1609, 5387; mesh-plugin/connection.ts:634): replace statSync→readFileSync with openSync+fstatSync+readSync so size check and read happen on the same fd. - reflected-xss (commands/mesh.ts:185): HTML-escape result.error in the OAuth callback HTML response (new escapeHtml helper). MEDIUM: - indirect-command-line-injection (plugin.ts:1556): replace execSync with shell-interpolated `find` command by execFileSync("find", [...args]), removing the shell entirely. Size limit enforced via .slice(0, 50) on JS side instead of piping through `head`. - log-injection (commands/mesh.ts:549; stepper.ts:158, 166): strip CR/LF/ tabs from untrusted strings before passing to console.log/error via new sanitizeForLog() helper in stepper.ts and mesh.ts. Verification: - cd cli && npm run typecheck ✓ - cd cli && npm run build ✓ - cd cli && npm test → 242/244 (2 pre-existing skipped) - cd mesh-plugin && npm run typecheck ✓ - cd mesh-plugin && npm test → 36/36 Remaining medium alerts at mesh.ts:142 (http-to-file-access via saveIdentity) and mesh.ts:476 (file-access-to-http via registryUrl) are expected flows — user's own OAuth-verified identity flowing to the user's own identity file, and the user's own configured registry URL being read from that same file. Will be dismissed as false positives in the GH alerts UI. Note-level unused-variable and useless-assignment alerts are pre-existing non-security style findings; deferred to a later cleanup PR to keep this one focused on security fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(security): broaden redactor to catch bare azcp_*_ pairing tokens + PEM Original redactor only matched secrets preceded by a keyword (Bearer/token/ api_key/etc.). A bare AzureClaw one-time pairing token like 'azcp_1_eyJjb250cm9sbGVyX2FtaWQi...' logged on its own would slip through. Now also redacts: - azcp_N_<payload> one-time pairing tokens (any version) - -----BEGIN .. KEY-----...-----END .. KEY----- PEM blocks - Basic HTTP auth headers (same as Bearer) - Extended keyword list: handoff_token, admin_token, pairing_token, invite_code, access_token, refresh_token, authorization Export redactSecrets and add 9 unit tests covering every pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(codeql): repair regressions from prior security PR - ReDoS in redactSecrets PEM regex (alert #270): bound character classes ({1,40}) and body length ({0,8192}?) so catastrophic backtracking is impossible on crafted '-----BEGIN -----' inputs. - TOCTOU at mesh_transfer_file (alert #268): drop the pre-open statSync isFile/size guard; do kind + size + read entirely via the same fd (openSync → fstatSync → readSync → closeSync). - log-injection at stepper.ts/mesh.ts (alerts #265-#267): switch sanitizeForLog to the classic CodeQL-recognized pattern (split .replace calls for \r, \n, \t) so the JS query models it as a sanitizer. - Unused sanitizeForLog in plugin.ts (alert #269): remove; redactSecrets alone wraps the _log sinks. cli tests: 251/253 pass. mesh-plugin tests: 36/36. typecheck clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Pal Lakatos-Toth <pallakatos@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Reflect the controls added in #28 (review/w1a), #31 (vulnerable transitive deps), #32 (CodeQL HIGH/MEDIUM closure) and 22197bc (sandbox entrypoint hardening) in the security-facing docs. No code changes. docs/security.md - New 'Admin-plane hardening' table under Layer 7 covering: constant-time admin-token compare, #[serde(deny_unknown_fields)] on SpawnRequest/HandoffMeta, ROUTER_ADMIN_ALLOW_IPS, ADMIN_ALLOWED_ORIGINS, canonical 'Authorization: Bearer' header, handoff middleware no-localhost-bypass, per-request size caps, trace-id sanitization, controller-set-only AZURE_IMDS_ENDPOINT/AZURE_AD_ENDPOINT test overrides. Each row carries file:line citations into inference-router/. - New 'Operator CLI & Plugin Hardening' section covering the CodeQL fixes: redactSecrets() (Bearer/JWT/PEM/azcp_*/keyword secrets, ReDoS-bounded), sanitizeForLog() (CRLF/tab strip, CodeQL-recognized split-replace pattern), escapeHtml() on the OAuth callback page, mkdtempSync() per-request tmpdir, openSync+fstatSync+readSync TOCTOU-safe reads, execFileSync('find', [...]) no-shell exec. - New 'Sandbox entrypoint hardening' subsection: EPERM-tolerant chmod/fchmod, AGT_POLICY_DIR profile-leak fix, plugin/SDK/node_modules re-chowned to root RO, regression test asserts every hardening invariant. - New 'Supply-chain & dependency hygiene' subsection: cargo audit CI job (closed RUSTSEC-2026-0098/-0099/-0104 via rustls-webpki bump), npm overrides (uuid/xml2js/lodash), vendored Python wheel + Go toolchain bumps, AgentMesh Cargo.lock bumps, fuzz/proptest targets. docs/security-validation.md - New 'Cross-cutting Hardening' section maps each new control to its automated test (cli/src/redact.test.ts, sandbox-hardening.test.ts, inference-router unit tests, cargo audit, npm audit, cargo fuzz) and provides a one-block reproduction recipe. README.md - Updated 'Admin token' row in Security Model: canonical 'Authorization: Bearer', x-azureclaw-admin deprecation, constant-time compare, ROUTER_ADMIN_ALLOW_IPS / ADMIN_ALLOWED_ORIGINS allowlists. docs/threat-model.md and CHANGELOG.md were already accurate and unchanged. Co-authored-by: Pal Lakatos-Toth <pallakatos@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-code-quality Bot found potential problems Apr 24, 2026

View reviewed changes

Comment thread cli/src/plugin.ts Fixed

github-advanced-security Bot found potential problems Apr 24, 2026

View reviewed changes

github-code-quality Bot found potential problems Apr 24, 2026

View reviewed changes

Comment thread cli/src/plugin.ts Fixed

github-advanced-security Bot found potential problems Apr 24, 2026

View reviewed changes

Comment thread cli/src/plugin.ts Fixed

Pal Lakatos-Toth (pallakatos) merged commit 2ebb9b3 into main Apr 24, 2026
15 checks passed

Pal Lakatos-Toth (pallakatos) deleted the fix/codeql-code-findings branch April 24, 2026 09:26

This was referenced Apr 24, 2026

refactor: extract IMeshTransport abstraction + rewrite identity onto Node crypto (Phase 1 of #27) #29

Merged

docs(security): align security docs with recent hardening fixes #33

Merged

Navigation Menu

Repository navigation

fix(security): resolve CodeQL HIGH/MEDIUM code-scanning findings - #32

fix(security): resolve CodeQL HIGH/MEDIUM code-scanning findings#32
Pal Lakatos-Toth (pallakatos) merged 3 commits into
mainfrom
fix/codeql-code-findings

Pal Lakatos-Toth (pallakatos) commented Apr 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Pal Lakatos-Toth (pallakatos) commented Apr 24, 2026

Summary

Alerts resolved

HIGH

MEDIUM

Verification

Not addressed here

Risk

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants