🛡️ fix: defense-in-depth for ANSI injection, .git root walk, and Unix backslash collision (Aikido)#152
Merged
Merged
Conversation
added 3 commits
July 20, 2026 21:41
…641757)
Mitigates Aikido finding group 30641757 (priority 35, MEDIUM):
"ANSI escape sequence injection in search output" — indexed content
could embed CSI/OSC sequences (e.g. \x1b[2J clears screen, \x1b]0;...\x07
rewrites window title) that the host terminal would execute on print.
Changes:
- Add `sanitize_for_terminal(&str) -> String` helper in src/search/mod.rs
Strips: CSI sequences (ESC [ ... <final 0x40-0x7E>),
OSC sequences (ESC ] ... (BEL | ESC \)),
single-char escape sequences (ESC <0x40-0x5F>),
and stray control chars except \n and \t.
Safe on truncated input — never panics.
- Apply to every user-controllable println! site in search/mod.rs:
* print_result: result.path, result.kind, result.signature,
result.context, result.context_prev/next lines,
result.content lines, snippet
* sync_database: file.path display, deleted-file path string
* compact path: result.path
* query string in standard output header
- Add 9 unit tests covering CSI/OSC/single-char/control-char/unicode/
empty/truncated-input cases.
The `colored` crate wraps content but does not sanitize inner escapes;
sanitization happens BEFORE .bright_green() / .dimmed() / etc. so the
color wrapper cannot be broken out of.
Local cargo check blocked by pre-existing MSYS2 link.exe issue
(documented in PR #151) — no errors in src/search/mod.rs. cargo fmt
passes.
…:walk Mitigates Aikido finding group 30641794 (priority 38, MEDIUM): "Local client can register `.git` dir as repo, search excluded Git metadata" — exposes internal/sensitive files (objects, config, refs) via search results. Root cause: `FileWalker::walk`'s `filter_entry` closure short-circuits on `entry.depth() == 0` (the root entry), so the ALWAYS_EXCLUDED name check is bypassed when the user points the indexer at a directory whose own name is `.git` (or `node_modules`, `target`, etc.). Fix: validate `self.root.file_name()` at the top of `walk()` and bail! with an actionable error if the name matches an ALWAYS_EXCLUDED entry. Covers every caller uniformly — CLI `index`, HTTP `/repos`, `doctor`, `sync_database`, watcher — without needing to patch each callsite. Pre-existing depth==0 short-circuit intentionally left in place (now unreachable for excluded names; still correct for normal roots whose names are not in the list). Test: `test_rejects_excluded_named_root` builds a temp `.git` dir, asserts walk() returns Err with "Refusing to index" + ".git" in the message, and verifies a sibling non-excluded root walks normally.
…th (Aikido 30641757)
Mitigates Aikido finding group 30641757 (priority 46, MEDIUM):
"Improper Input Validation — backslash path collision on Unix".
Companion finding to the ANSI escape injection already fixed in
stage 1/3 (same group, different priority).
THREAT MODEL
On Unix, backslash is a legal filename character (not a path
separator). A file literally named `foo\bar.rs` is distinct from
`foo/bar.rs` (which lives in subdirectory `foo`). The previous
`normalize_path` / `normalize_path_str` unconditionally ran
`.replace('\\', "/")`, collapsing both into the key `foo/bar.rs`.
This caused silent HashMap collisions in `FileMetaStore`: one
file's chunks would overwrite the other's metadata, leading to
stale search results, missed re-indexing, or wrong chunk IDs.
FIX
Gate the backslash-to-forward-slash conversion behind `#[cfg(windows)]`:
- Windows: backslash IS a path separator — conversion is required
for HashMap consistency across canonicalize/Notify/raw APIs.
- Unix: preserve backslash literally; it is part of the filename
and must not be normalized away.
The `trim_start_matches(r"\\?\")` (UNC prefix strip) runs
unconditionally on both platforms — it is a no-op on Unix in
practice but defensive in case a Windows-style path string leaks
into a Unix process via config/migration.
TESTS
- Added `test_normalize_path_preserves_unix_backslash_filenames`
(cfg(not(windows))): asserts `foo/bar.rs` and `foo\bar.rs`
normalize to distinct keys.
- Gated 12 Windows-specific tests with `#[cfg(windows)]` because
they explicitly assert backslash conversion using hardcoded
`C:\...` / `\\?\C:\...` inputs. These tests document Windows
behavior and have no meaning on Unix after the fix.
Files changed: src/cache/file_meta.rs (+50 / −2 net)
Validation:
- `cargo fmt --check src/cache/file_meta.rs` PASS
- `cargo check --lib --tests` fails ONLY at the pre-existing
MSYS2 `/usr/bin/link` vs MSVC `link.exe` link step (no errors
reference file_meta.rs). Authoritative validation will run in
GitHub CI.
This was referenced Jul 22, 2026
This was referenced Jul 23, 2026
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.
Companion defense-in-depth branch to PR #151 (which shipped the Critical fixes). This PR closes 3 additional Aikido findings (all MEDIUM severity).
What ships
sanitize_for_terminal()helper insrc/search/mod.rsstrips CSI/OSC/Fe escape sequences + stray control chars per ECMA-48, applied at every user-controllableprintln!site before theColorizewrapper. 9 unit tests..gitdir as repo, exposing internal Git metadata via searchFileWalker::walk()entry refuses to walk any root whose name is inALWAYS_EXCLUDED(.git,.svn,node_modules, etc.). Actionable error: "Point the indexer at the parent project directory instead." 1 unit test (reject + sibling-accept).foo\bar.rs(literal) collapsed withfoo/bar.rs(separator) → HashMap key collision → silent metadata corruptionnormalize_path_str()now gates.replace(\\, "/")behind#[cfg(windows)]. UNC-strip remains unconditional.normalize_pathdelegates tonormalize_path_str(incidental DRY win). 1 new Unix test; 16 Windows-only tests correctly gated.Scope
3 commits, 3 source files only:
src/search/mod.rs(+173 / −12)src/file/mod.rs(+51 / −0)src/cache/file_meta.rs(+59 / −2)Total: +283 / −14. No scope creep, no docs/config noise.
Local validation caveat
Same as PR #151:
cargo check --lib --testsfails at link step locally — MSYS2/usr/bin/link(GNU coreutils) shadows MSVClink.exeand no Windows SDK is installed. Pre-existing host issue, unrelated to this change.cargo fmt --checkPASSES on all three files. CI on GitHub Actions will run the authoritative gate.Review
All 4 reviews PASS (3 per-stage + 1 final). No critical/important issues. Two minor cosmetic notes deferred (doc-comment drift at
file_meta.rs:59, commit-message test count).Out of scope (deliberately deferred)
cargo updateworkflowsafe_canonicalize(...).unwrap_or_else(|_| ...)sites — already audited; the only HTTP-facing one (src/serve/mod.rs:3167) is already correct