Skip to content

Fix code review findings: panics, detection gaps, and installer issues#12

Merged
sheeki03 merged 3 commits intomainfrom
fix-review-findings
Feb 4, 2026
Merged

Fix code review findings: panics, detection gaps, and installer issues#12
sheeki03 merged 3 commits intomainfrom
fix-review-findings

Conversation

@sheeki03
Copy link
Owner

@sheeki03 sheeki03 commented Feb 4, 2026

Summary

  • Fix UTF-8 slice panics in truncation functions that could crash on multi-byte characters
  • Fix ANSI evidence filter that was always returning empty evidence arrays
  • Strengthen allowlist matching to prevent bypass via subdomain tricks (e.g., evil-github.com)
  • Add sudo env bash detection for pipe-to-interpreter rule
  • Expand Docker flag parsing to prevent image misparsing with --name, -e, etc.
  • Add SHA256 verification to Windows installer for security parity with Unix
  • Fix AUR build by adding base-devel to makedepends
  • Fix bash hook echo in gcloud ssh environments by saving/restoring stty
  • Consolidate duplicate code (levenshtein, write_last_trigger)
  • Improve error messages for policy parse errors and unknown shell types

Test plan

  • cargo fmt --all passes
  • cargo clippy --workspace -- -D warnings passes
  • cargo test --workspace passes (179 tests)
  • New tests added for allowlist boundary matching
  • New test added for sudo env bash detection

- Add UTF-8 safe truncation via util.rs::truncate_bytes to prevent panics
  on multi-byte characters in audit.rs, terminal.rs, environment.rs,
  and the new last_trigger.rs module

- Fix ANSI evidence filter using .contains("escape") instead of "ANSI"

- Strengthen allowlist matching to use host/subdomain boundaries,
  preventing evil-github.com from matching github.com allowlist entry

- Add sudo env bash detection via resolve_env_from_args helper

- Expand Docker value-taking flags (~45 flags) to prevent misparsing
  images when flags like --name are used

- Add SHA256 checksum verification to Windows installer (install.ps1)

- Add base-devel to AUR PKGBUILD makedepends for ring crate compilation

- Fix bash hook terminal echo in gcloud ssh by saving/restoring stty

- Consolidate duplicate levenshtein implementations into util.rs

- Extract write_last_trigger into shared last_trigger.rs module

- Add warning for unknown shell type instead of silent fallback

- Include actual parse error in policy.rs warning messages
@macroscopeapp
Copy link

macroscopeapp bot commented Feb 4, 2026

Make CLI installers verify Windows asset checksums, route shell hook output to stderr under Warp or TIRITH_OUTPUT=stderr, and add UTF‑8 safe truncation (80 bytes for commands, 20 bytes for env values) via tirith_core::util::truncate_bytes

Introduce shared tirith_core::util::{truncate_bytes,levenshtein} and switch redaction and similarity checks to these helpers; expand Docker and env argument parsing to avoid image/interpreter misdetection; add domain allowlist matching with wildcard support; centralize last-trigger writing with UTF‑8 safe command redaction; verify Windows installer downloads against checksums.txt; route shell hook output to stderr when appropriate and preserve terminal state.

📍Where to Start

Start with the shared helpers in tirith_core::util in util.rs, then review their adoption in redaction and argument parsing: Policy.is_allowlisted in policy.rs, Docker parsing in extract.rs, and resolve_env_from_args in rules/command.rs.


Macroscope summarized f8a8e9a.

cmd.to_string()
} else {
format!("{}[...redacted {} chars]", &cmd[..80], cmd.len() - 80)
format!("{}[...redacted {} chars]", prefix, cmd.len() - prefix.len())
Copy link

Choose a reason for hiding this comment

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

🟢 Low

src/audit.rs:100 The message says "redacted X chars" but cmd.len() - prefix.len() counts bytes, not characters. Consider using cmd.chars().count() - prefix.chars().count() or changing "chars" to "bytes".

Suggested change
format!("{}[...redacted {} chars]", prefix, cmd.len() - prefix.len())
format!("{}[...redacted {} bytes]", prefix, cmd.len() - prefix.len())

🚀 Want me to fix this? Reply ex: "fix it for me".

- Add _tirith_output() helper to zsh, bash, and fish hooks
- Auto-detect Warp via TERM_PROGRAM and use stderr instead of /dev/tty
- Add TIRITH_OUTPUT=stderr override for manual control
- Document Warp workaround in troubleshooting.md
Comment on lines +265 to +268
fn domain_matches(host: &str, pattern: &str) -> bool {
let host = host.trim_end_matches('.');
let pattern = pattern.trim_start_matches("*.").trim_end_matches('.');
host == pattern || host.ends_with(&format!(".{pattern}"))
Copy link

Choose a reason for hiding this comment

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

🟡 Medium

src/policy.rs:265 This matches subdomains even when the pattern lacks a *. prefix. Consider checking if the original pattern starts with *. before applying ends_with subdomain matching.

Suggested change
fn domain_matches(host: &str, pattern: &str) -> bool {
let host = host.trim_end_matches('.');
let pattern = pattern.trim_start_matches("*.").trim_end_matches('.');
host == pattern || host.ends_with(&format!(".{pattern}"))
fn domain_matches(host: &str, pattern: &str) -> bool {
let host = host.trim_end_matches('.');
let is_wildcard = pattern.starts_with("*.");
let pattern = pattern.trim_start_matches("*.").trim_end_matches('.');
host == pattern || (is_wildcard && host.ends_with(&format!(".{pattern}")))

🚀 Want me to fix this? Reply ex: "fix it for me".

@sheeki03 sheeki03 merged commit 2b22989 into main Feb 4, 2026
10 checks passed
@sheeki03 sheeki03 deleted the fix-review-findings branch February 4, 2026 17:55
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.

1 participant