fix(security): document + opt-in redaction for source-text identity fields (#1920)#2089
Conversation
…ields (#1920) Identity fields (symbol, callee, container, module, macro_name, lint) carry raw source-derived text and are emitted in CI-uploaded receipt artifacts — an info-leak surface for repos that inline secrets near scanned constructs. - Add `source_text_in_identity_fields` to the claim boundary vocabulary (CLAIM_BOUNDARY + CLAIM_BOUNDARY_TEXT) and every artifact schema's claim_boundary_flag enum + docs/schemas/README.md, documenting the surface. - Add `StructuralIdentity::redact_source_text_fields` that clears the source-text-bearing fields while preserving structural anchors (hashes, ast_kind, fingerprints, line/column hints) so matching still works. - Opt-in via `CARGO_ALLOW_REDACT_IDENTITY=1` env var at the finding-construction choke point (allow-rust::finding_builder::push_finding). Tests: redact_source_text_fields clears text fields, preserves anchors; claim-boundary vocabulary test updated (+3); schema-conformance tests pass. Validation: fmt, clippy -D warnings, workspace tests, doc tests, check --mode no-new (0 new findings).
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Summary by CodeRabbit
WalkthroughAdds a ChangesIdentity redaction feature and schema updates
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a7caa3fa5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if std::env::var("CARGO_ALLOW_REDACT_IDENTITY").as_deref() == Ok("1") { | ||
| identity.redact_source_text_fields(); |
There was a problem hiding this comment.
Avoid redacting identities before policy matching
When CARGO_ALLOW_REDACT_IDENTITY=1 is set for the intended CI artifact use case, this clears callee, container, symbol, lint, etc. while the findings are still the inputs to allow_match::score_match, whose selector checks hard-gate on those exact fields. Existing allow entries generated from normal findings commonly include those selectors, so a previously receipted unwrap/unsafe/lint finding will become New and the entry Stale under the redaction env var even though the code did not change. Redaction needs to happen only at artifact serialization/output time, or matching must use an unredacted copy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/allow-core/src/finding.rs (1)
87-101: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRedact
crate_nameas wellcrate_nameis populated fromCargo.tomlpackage.name, so it is source-derived context too. Leaving it out ofredact_source_text_fields()keeps package names visible in CI artifacts.🤖 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/allow-core/src/finding.rs` around lines 87 - 101, The redaction list in StructuralIdentity is missing crate_name, so source-derived package names can still leak into CI artifacts. Update redact_source_text_fields() to treat crate_name the same way as the other source-text fields and ensure StructuralIdentity redaction covers it consistently alongside module, container, symbol, callee, and similar fields.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/allow-core/src/finding.rs`:
- Around line 160-174: The redaction in redact_source_text_fields() is removing
fields that still participate in stable_key() and finding_identity_key(), which
can make distinct findings collide after redaction. Update Finding so redacted
text fields like symbol, callee, container, module, macro_name, and lint are
either replaced with a stable digest/value that preserves uniqueness or are
excluded from the matching key computation, while keeping the structural anchors
used for matching intact.
In `@crates/allow-rust/src/finding_builder.rs`:
- Around line 40-45: Cache the redaction flag once instead of reading
CARGO_ALLOW_REDACT_IDENTITY on every push_finding call, since this is on the
scanner hot path. Add a OnceLock<bool> helper near finding_builder::push_finding
or the redaction branch to initialize the env check once, then reuse that cached
value before calling identity.redact_source_text_fields(). Keep the behavior the
same, just move the env lookup out of the per-finding path.
---
Outside diff comments:
In `@crates/allow-core/src/finding.rs`:
- Around line 87-101: The redaction list in StructuralIdentity is missing
crate_name, so source-derived package names can still leak into CI artifacts.
Update redact_source_text_fields() to treat crate_name the same way as the other
source-text fields and ensure StructuralIdentity redaction covers it
consistently alongside module, container, symbol, callee, and similar fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9df1c9e8-cc98-49a5-b4a0-1d923f978c97
📒 Files selected for processing (19)
crates/allow-core/src/finding.rscrates/allow-core/src/finding_tests.rscrates/allow-report/src/contracts.rscrates/allow-report/src/json_tests.rscrates/allow-rust/src/finding_builder.rsdocs/schemas/README.mddocs/schemas/add.schema.jsondocs/schemas/common.v1.jsondocs/schemas/doctor.schema.jsondocs/schemas/explain.schema.jsondocs/schemas/list.schema.jsondocs/schemas/migrate.schema.jsondocs/schemas/propose.schema.jsondocs/schemas/prune.schema.jsondocs/schemas/receipt.schema.jsondocs/schemas/refresh.schema.jsondocs/schemas/report.schema.jsondocs/schemas/spec-system.schema.jsondocs/schemas/worklist.schema.json
| /// Redact the source-text-bearing identity fields (`symbol`, `callee`, | ||
| /// `container`, `module`, `macro_name`, `lint`) by clearing them, while | ||
| /// preserving the structural anchors (`normalized_snippet_hash`, | ||
| /// fingerprints, `ast_kind`, `line_hint`, `column_hint`) that matching | ||
| /// relies on. Opt-in for CI artifacts where source-text-derived fields are | ||
| /// an info-leak surface (#1920). | ||
| pub fn redact_source_text_fields(&mut self) { | ||
| self.symbol = None; | ||
| self.callee = None; | ||
| self.container = None; | ||
| self.module = None; | ||
| self.macro_name = None; | ||
| self.lint = None; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B2 -A6 '\.stable_key\(\)' --type=rust
rg -n -B2 -A10 'fn stable_key_parts' --type=rust
rg -n -B2 -A10 'stable_identity_key_from_parts' --type=rustRepository: EffortlessMetrics/cargo-allow
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== finding.rs outline ==\n'
ast-grep outline crates/allow-core/src/finding.rs --view expanded || true
printf '\n== finding.rs lines around redaction ==\n'
sed -n '1,260p' crates/allow-core/src/finding.rs | cat -n | sed -n '120,230p'
printf '\n== repository search for stable_key symbols ==\n'
rg -n --hidden --glob '!target' 'stable_key|stable_identity_key|identity key|redact_source_text_fields|CARGO_ALLOW_REDACT_IDENTITY|unwrap_or_default\(\)' crates . || true
printf '\n== repository search for allow-list matching ==\n'
rg -n --hidden --glob '!target' 'allow[-_ ]list|allowlist|matching|suppress|suppression|permit' crates . || trueRepository: EffortlessMetrics/cargo-allow
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== finding.rs relevant functions =='
sed -n '224,260p' crates/allow-core/src/finding.rs | cat -n
echo
echo '== exact stable_key/finding_identity_key call sites in core =='
rg -n -B2 -A4 'finding_identity_key\(|stable_key\(' crates/allow-core crates/allow-policy crates/allow-report crates/cargo-allow crates/allow-rust --type=rust
echo
echo '== tests and docs mentioning hints / redaction =='
rg -n -B2 -A4 'line_hint|column_hint|redact_source_text_fields|CARGO_ALLOW_REDACT_IDENTITY|stable identity key|finding identity key' crates/allow-core crates/allow-report docs --type=rust --type=md
echo
echo '== specific use of stable_key in allow-rust tests or production =='
rg -n -B2 -A6 'identity\.stable_key\(\)|finding_identity_key\(' crates/allow-rust crates/cargo-allow crates/allow-policy --type=rustRepository: EffortlessMetrics/cargo-allow
Length of output: 50387
Redaction should preserve matching entropy
redact_source_text_fields() clears fields that still feed stable_key()/finding_identity_key() (module, container, symbol, callee, macro_name, lint). line_hint/column_hint are review hints only, not part of the key. If redacted findings participate in matching, distinct findings on the same path can collapse onto the same key; replace the cleared text with a stable digest or keep redaction out of matching inputs.
🤖 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/allow-core/src/finding.rs` around lines 160 - 174, The redaction in
redact_source_text_fields() is removing fields that still participate in
stable_key() and finding_identity_key(), which can make distinct findings
collide after redaction. Update Finding so redacted text fields like symbol,
callee, container, module, macro_name, and lint are either replaced with a
stable digest/value that preserves uniqueness or are excluded from the matching
key computation, while keeping the structural anchors used for matching intact.
| // Opt-in: redact source-text-bearing identity fields for CI artifacts | ||
| // where they are an info-leak surface. Structural hashes are preserved. | ||
| // (#1920) | ||
| if std::env::var("CARGO_ALLOW_REDACT_IDENTITY").as_deref() == Ok("1") { | ||
| identity.redact_source_text_fields(); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== repo files of interest ==\n'
git ls-files | rg '(^|/)(rust-toolchain(\.toml)?|Cargo\.toml|Cargo\.lock|finding_builder\.rs)$'
printf '\n== rust toolchain / msrv hints ==\n'
for f in rust-toolchain.toml rust-toolchain Cargo.toml crates/allow-rust/Cargo.toml; do
if [ -f "$f" ]; then
echo "--- $f"
cat -n "$f" | sed -n '1,220p'
fi
done
printf '\n== finding_builder.rs ==\n'
cat -n crates/allow-rust/src/finding_builder.rs | sed -n '1,220p'Repository: EffortlessMetrics/cargo-allow
Length of output: 5735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== push_finding usages ==\n'
rg -n "push_finding\(" crates/allow-rust -g '!target' -g '!**/node_modules/**'
printf '\n== structural identity / redact flag related symbols ==\n'
rg -n "CARGO_ALLOW_REDACT_IDENTITY|redact_source_text_fields|OnceLock|push_finding" crates/allow-rust -g '!target' -g '!**/node_modules/**'
printf '\n== relevant surrounding files ==\n'
for f in crates/allow-rust/src/*.rs; do
echo "--- $f"
sed -n '1,220p' "$f" | rg -n "push_finding|CARGO_ALLOW_REDACT_IDENTITY|redact_source_text_fields|OnceLock|StructuralIdentity|FindingSite|FindingKind"
doneRepository: EffortlessMetrics/cargo-allow
Length of output: 3188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, re, pathlib, json
root = pathlib.Path('.')
candidates = [
'rust-toolchain.toml',
'rust-toolchain',
'Cargo.toml',
'crates/allow-rust/Cargo.toml',
]
for p in candidates:
path = root / p
if path.exists():
print(f'--- {p}')
text = path.read_text(errors='replace').splitlines()
for i, line in enumerate(text[:220], 1):
if re.search(r'(toolchain|rust-version|edition|msrv|minimum supported rust)', line, re.I):
print(f'{i}:{line}')
# print top relevant context for rust-toolchain/Cargo
if p.endswith('rust-toolchain.toml') or p == 'rust-toolchain' or p.endswith('Cargo.toml'):
for i, line in enumerate(text[:80], 1):
print(f'{i}:{line}')
if i >= 80:
break
PY
printf '\n== finding_builder relevant lines ==\n'
sed -n '1,120p' crates/allow-rust/src/finding_builder.rs | nl -ba | sed -n '1,120p'
printf '\n== callers/usages ==\n'
rg -n "push_finding\(" crates/allow-rust -g '!target' -g '!**/node_modules/**'
printf '\n== env/redact symbols ==\n'
rg -n "CARGO_ALLOW_REDACT_IDENTITY|redact_source_text_fields|OnceLock" crates/allow-rust -g '!target' -g '!**/node_modules/**'Repository: EffortlessMetrics/cargo-allow
Length of output: 2509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== toolchain manifests =="
for f in rust-toolchain.toml rust-toolchain Cargo.toml crates/allow-rust/Cargo.toml; do
if [ -f "$f" ]; then
echo "--- $f"
nl -ba "$f" | sed -n '1,160p'
fi
done
echo
echo "== finding_builder.rs excerpt =="
nl -ba crates/allow-rust/src/finding_builder.rs | sed -n '1,140p'
echo
echo "== push_finding call sites =="
rg -n "push_finding\(" crates/allow-rust
echo
echo "== OnceLock usage =="
rg -n "OnceLock|CARGO_ALLOW_REDACT_IDENTITY|redact_source_text_fields" crates/allow-rustRepository: EffortlessMetrics/cargo-allow
Length of output: 248
🌐 Web query:
Rust std::sync::OnceLock stabilized version
💡 Result:
The Rust standard library structure std::sync::OnceLock was stabilized in version 1.70.0 [1][2][3]. It is a thread-safe synchronization primitive that can be written to only once, functioning as a thread-safe alternative to OnceCell [1][4]. The stabilization was implemented through pull request #105587 [5]. At the time of its initial stabilization, other related types like LazyLock and LazyCell were not included and remained experimental, later receiving their own stabilization in subsequent releases [5].
Citations:
- 1: https://doc.rust-lang.org/std/sync/struct.OnceLock.html
- 2: https://doc.rust-lang.org/1.70.0/std/sync/struct.OnceLock.html
- 3: https://doc.rust-lang.org/1.93.1/src/std/sync/once_lock.rs.html
- 4: https://doc.rust-lang.org/stable/std/sync/struct.OnceLock.html
- 5: Partial stabilization of
once_cellrust-lang/rust#105587
Cache the redaction flag once. push_finding reads CARGO_ALLOW_REDACT_IDENTITY for every finding, which is avoidable work in the scanner hot path. allow-rust targets Rust 1.85, so std::sync::OnceLock<bool> is available 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/allow-rust/src/finding_builder.rs` around lines 40 - 45, Cache the
redaction flag once instead of reading CARGO_ALLOW_REDACT_IDENTITY on every
push_finding call, since this is on the scanner hot path. Add a OnceLock<bool>
helper near finding_builder::push_finding or the redaction branch to initialize
the env check once, then reuse that cached value before calling
identity.redact_source_text_fields(). Keep the behavior the same, just move the
env lookup out of the per-finding path.
Product contract
symbol,callee,container,module,macro_name,lint) carry raw source-derived text and are emitted in CI-uploaded receipt artifacts — an info-leak surface for repos that inline secrets near scanned constructs. The surface was undocumented and there was no redaction path.source_text_in_identity_fieldsin every artifact schema + the README vocabulary table + the human claim-boundary text; (2) an opt-inCARGO_ALLOW_REDACT_IDENTITY=1env var clears the source-text-bearing fields at the finding-construction choke point while preserving structural anchors (normalized_snippet_hash, fingerprints,ast_kind, line/column) so matching still works.crates/allow-core/src/finding.rs(redact_source_text_fields),crates/allow-rust/src/finding_builder.rs(env-var gate),crates/allow-report/src/contracts.rs(claim-boundary tag + text),docs/schemas/(all artifact schemas + README).CARGO_ALLOW_REDACT_IDENTITY=1to strip source-text-derived identity fields from the uploaded artifact, eliminating the info-leak surface, while the claim-boundary disclosure informs adopters the surface exists.Tests
redact_source_text_fields_clears_text_but_preserves_anchors— clears the 6 source-text fields, preserves all structural anchors.json_contains_claim_boundary— updated for the new tag count.Validation
cargo fmt --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspace(all ok),cargo test --doc --workspace,check --mode no-new(0 new findings).