Skip to content

fix(security): detect credentials after escaped boundaries - #612

Merged
vladimirrott merged 2 commits into
lacs-project:mainfrom
QinXi-ai:fix/601-escaped-secret-boundaries
Oct 19, 2026
Merged

vladimirrott merged 2 commits into
lacs-project:mainfrom
QinXi-ai:fix/601-escaped-secret-boundaries

Conversation

@QinXi-ai

@QinXi-ai QinXi-ai commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Treat backslash whitespace escapes and percent escapes as left boundaries for bare OpenAI and DeepSeek keys. A key immediately after %20, \n, \r, or \t is now detected, while identifiers such as task-… and prefixsk-… remain clean.

Add regression coverage for escaped keys, an overlong DeepSeek token, modifications to tracked files, and both directions of staged/working-tree divergence.

Related Issue

Fixes #601. Based on @vladimirrott's reproduction and proposed boundary fix, with coverage for all three supported backslash whitespace escapes for both providers.

Validation

  • Tests added or updated: bash tests/release/no-secrets.test.sh passed on a local Linux checkout of the tracked tree plus this patch.
  • Behavior documented in the scanner comments.
  • Security impact considered: synthetic credentials only; existing no-echo and false-positive checks pass.
  • Trust boundary preserved: only the repository credential scanner and its tests change.
  • All upstream CI checks green. The Rust, scripts-lint, docs-and-hygiene, frontend, PostgreSQL contract, dependency review and secret-scan jobs passed. Security audit reports the same four rustls advisories as the base branch.

bash -n and ShellCheck 0.11.0 (--severity=warning) passed for both changed scripts. Before the scanner fix, all eight added escaped-key cases failed as expected. Rust source and Rust test counts are unchanged; the full Rust/GUI workspace suite was not run locally.

Focused mutation checks also passed: the overlong-token case rejects changing DeepSeek's {32} to {32,}, and the staged-test section rejects both --diff-filter=A and reading the working tree instead of git show ":$path". Each unmutated counterpart passed first.

Notes for Reviewers

The existing scanner already reads modified files from the index correctly. The new staged tests protect that behavior against future regressions; they do not change which files are scanned.

This contribution is AI-assisted.

The security-audit failure is inherited from unchanged Cargo.lock: RUSTSEC-2026-0346, 0347, 0348 and 0349 also fail on base commit 10d7483 in main run 38846085049. Resolving those advisories requires a separate rustls dependency update; this PR only changes the scanner and its regression tests.

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @QinXi-ai. The boundary change is the one #601 proposed, and you went past it in the right places: all three backslash escapes for both providers, a 33-character DeepSeek negative so the {32} length stays pinned, and section 5b, which covers a modified tracked file in both staged and working-tree directions. That last part was the half of #601 I cared most about.

Reviewed at 15cfa7b. What I checked:

  • bash tests/release/no-secrets.test.sh passes on the head. Positives went 22 → 30 and negatives 16 → 17.
  • Reverting only the two pattern lines in scripts/check_no_secrets.sh makes the test fail on all 8 new positives. I recorded that as the merge receipt.
  • I also ran targeted mutations: dropping %XX or \\[nrt] from either provider, dropping \t or \r alone, {32} → {32,}, --diff-filter=A. Your tests catch every one.
  • Probes for false positives (task-…, prefixsk-…, risk-assessment-…, a Rust "\ntask-…" string, a %20 URL path before task-…) all stay clean, and the whole tracked tree still scans clean.
  • shellcheck is clean on both files.

One optional gap, not blocking: the only percent fixture is %20, so narrowing %[0-9A-Fa-f]{2} to %20 still passes. A %0A positive per provider next to the existing ones would pin the general form. Send it here or as a follow-up, either works.

The red security-audit was main's fault (four rustls advisories), not yours. #616 fixed it, so I'll update the branch and merge once the board is green.

For a next one, #618 is your shape: the MCP server and the planner read the client machine's /etc/os-release instead of asking the daemon, so a Fedora laptop driving an Ubuntu VM loses 23 Ubuntu read tools. You built the tool list it filters in #348 and aligned the Ubuntu fences in #426, so you know both ends of it. I'll put it on the issue thread once this lands.

@vladimirrott
vladimirrott merged commit 4406e66 into lacs-project:main Oct 19, 2026
13 checks passed
vladimirrott added a commit that referenced this pull request Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

no-secrets: a key after %20 or \n slips past the #578 boundary, and --staged has no case for a modified file

2 participants