fix(signals): redact /root/ and forward-slash Windows paths in the manifest public-safe guard - #1688
Conversation
…nifest public-safe guard `isFocusManifestPublicSafe` is the public-output safety gate for contributor-issue drafts and decision packs (src/services/contributor-issue-draft.ts, src/services/decision-pack.ts). Its local-filesystem-path alternatives drifted from the canonical PUBLIC_UNSAFE_PATTERN in src/signals/redaction.ts: it omits `/root/` and only matches the backslash Windows form (`C:\\Users\\`), not the forward-slash one (`C:/Users/`). As a result a manifest string containing `/root/<path>` (the home/CWD in containers and most CI runners) or `C:/Users/<path>` passes as public-safe and leaks a local filesystem path into public output, despite the doc comment stating such text 'must not leak ... local filesystem path material'. Sync the path alternatives with the canonical guard: add `/root/` and accept both slash forms (`[A-Z]:[\\/]Users[\\/]`). Adds regression tests covering /Users//home//root//tmp and both Windows forms, plus a relative-path safe case. No linked issue: small, self-evident, self-contained security fix to one guard, no behavior change beyond closing the leak, no API/schema/DB change.
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review — safe to merge
✅ Approved — safe to merge Review summary
Nits — 3 non-blocking
Review context
Contributor next steps
Signal definitions
Review detailsGenerated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative. The patch brings isFocusManifestPublicSafe's filesystem-path check back into parity with redaction.ts by rejecting /root/ and both Windows separator styles, and adds regression coverage for the Unix and Windows cases. The regex change is scoped correctly, and the test covers the newly reachable cases plus a safe relative path. The notable remaining risk is that the path pattern is still duplicated, so future drift remains possible. Nits (2)
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
|
Important 🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪 🔍 Gittensory is reviewing…AI analysis is in progress. This comment will update when the review is complete. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟪 Reviewing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1688 +/- ##
=======================================
Coverage 95.58% 95.58%
=======================================
Files 204 204
Lines 22295 22295
Branches 8053 8053
=======================================
Hits 21310 21310
Misses 408 408
Partials 577 577
🚀 New features to boost your workflow:
|
The bug
isFocusManifestPublicSafeis the public-output safety gate used by contributor-issue drafts (src/services/contributor-issue-draft.ts) and decision packs (src/services/decision-pack.ts). Its local-filesystem-path alternatives have drifted from the canonicalPUBLIC_UNSAFE_PATTERNinsrc/signals/redaction.ts:/Users//home//tmp//root/C:/Users/(forward slash)C:\Users\(backslash)So a manifest string containing
/root/<path>— the home/CWD on containers and most CI runners — or the forward-slash Windows formC:/Users/<path>passes as public-safe and leaks a local filesystem path into public output, even though the function's own doc comment says such text "must not leak ... local filesystem path material".The fix
Sync the path alternatives with the canonical guard: add
/root/and accept both slash forms ([A-Z]:[\\/]Users[\\/]). One-line regex change; the forbidden-term list is untouched.Tests
The existing public-safe invariant test only covered reward/secret terms, never paths — which is why the drift went unnoticed. Added a regression test asserting
/Users/,/home/,/root/,/tmp/, and both Windows forms are rejected, plus a relative-path (src/signals/...) safe case. Full local gate green; codecov patch covers the changed line; OpenAPI unchanged.No linked issue: a small, self-evident, self-contained security fix to one guard — no behavior change beyond closing the leak, no API/schema/DB change.