fix(safety): redact model-bound secrets without rejecting turns - #7434
serrrfirat wants to merge 10 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change separates structural prompt validation from credential redaction. It preserves ordinary security text during prompt and memory construction. It redacts credential-shaped values at memory admission and before provider dispatch. ChangesCredential-aware model input
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentLoop
participant MemoryContext
participant ModelInputRedaction
participant ModelGateway
participant Provider
AgentLoop->>MemoryContext: admit untrusted memory
MemoryContext->>ModelInputRedaction: redact model content
ModelInputRedaction-->>MemoryContext: return redacted content
AgentLoop->>ModelGateway: submit model request
ModelGateway->>ModelInputRedaction: redact messages, tools, JSON, and repair content
ModelInputRedaction-->>ModelGateway: return redacted request
ModelGateway->>Provider: dispatch redacted request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Automatic trigger · attempt 1 of 3 · completed in 16m 47s IronLoop completed the review and posted it to GitHub. 🔗 Result |
There was a problem hiding this comment.
🔍 IronLoop review
Three findings: an untrusted credential bypass, an incomplete production recovery path, and stale caller contract tests.
Findings: 🔴 High 1 · 🟠 Medium 2
🔴 High · Reject Basic credentials on untrusted surfaces
Inline on crates/contracts/ironclaw_loop_contracts/src/prompt_text.rs:178. See the inline comment for details.
🟠 Medium · Test the real memory-admission recovery path
Inline on crates/contracts/ironclaw_loop_contracts/src/instruction_bundle.rs:1043. See the inline comment for details.
🟠 Medium · Update caller-level contract expectations
Inline on crates/contracts/ironclaw_loop_contracts/src/prompt_text.rs:178. See the inline comment for details.
Validation
- ✅ Loop-contract test suite — The affected contract crate's unit and contract tests passed.
- ❌ Instruction-bundle caller contract tests — Focused caller tests ran with 25 passing and 6 failing; failures are caused by expectations for the removed path/vocabulary rejection.
- ✅ Production memory-admission tests — Focused admission tests passed and confirmed that path- and credential-marker snippets are still dropped before prompt construction.
Review details
- Run:
65ad8082-4df1-46f8-93b9-a4a768eaa995 - Workflow: Review
- Attempts: 1
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs`:
- Around line 1533-1541: Update the five listed tests in
crates/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs at lines
1533-1541, 1592-1603, 1616-1637, 1641-1652, and 1707-1730: bind the successful
result of each InstructionBundleBuilder::build call, then assert that
materialized_messages contains the exact model_content supplied by the
corresponding skill_instruction_request, covering Authorization vocabulary and
each host-path or instruction-context case.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dac608c9-6f8c-49e6-aaf2-0dbfe72e3c71
📒 Files selected for processing (1)
crates/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rs`:
- Around line 674-679: Move credential admission policy out of
ironclaw_loop_contracts: relocate validate_prompt_text’s authorization parsing,
Basic-auth decoding, and credential classification to the appropriate host,
memory-domain, or security crate, and update
LoopContextSnippet::from_untrusted_memory to use that relocated gate. Remove the
contract crate’s policy dependency and adjust the architecture ceiling only
after the contracts crate contains no workflow decision or parsing logic.
In `@crates/contracts/ironclaw_loop_contracts/src/prompt_text.rs`:
- Around line 195-222: Update contains_credential_value_after_label and its
candidate parsing helpers to skip permitted assignment fillers such as “is”
before evaluating the credential, while preserving authorization scheme
handling. Ensure values like “api key is abc123def456” and “Authorization:
Bearer token ghp_secretvalue123” are detected, and add caller-level regression
coverage through the untrusted-memory admission path that gates insertion into
GenericModelContent.
In `@crates/kernel/ironclaw_host_runtime/src/memory_context.rs`:
- Line 303: Update the maps_benign_snippet_with_reference test to expect the
fixed safe summary “memory context snippet” produced by
LoopContextSnippet::from_untrusted_memory, and remove the assertion that
safe_summary contains provider text while preserving the reference and mapping
assertions.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 91473223-0b00-43d9-bbb3-8421e0f8ffa9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (7)
crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rscrates/contracts/ironclaw_loop_contracts/Cargo.tomlcrates/contracts/ironclaw_loop_contracts/src/host/context.rscrates/contracts/ironclaw_loop_contracts/src/prompt_text.rscrates/kernel/ironclaw_host_runtime/src/memory_context.rscrates/kernel/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs
…6364638 # Conflicts: # crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rs
# Conflicts: # crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/contracts/ironclaw_loop_contracts/src/instruction_bundle.rs (1)
1198-1202: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the filler-separated credential case.
Line 1201 puts the credential directly after
Bearer. This does not test the filler-separated credential pattern that this fix adds. The test can pass before the fix.Use the exact previously accepted credential-plus-filler form. Assert that
InstructionBundleBuilder::buildomits that descriptor.PR summary states that the fix targets credentials separated by filler text. As per coding guidelines, “Every bug fix must add a regression test that fails before the fix.”
🤖 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/contracts/ironclaw_loop_contracts/src/instruction_bundle.rs` around lines 1198 - 1202, Update untrusted_descriptions_with_credential_values_are_omitted_from_the_surface to use the exact previously accepted credential-plus-filler input rather than placing the credential immediately after “Bearer”. Ensure the test invokes InstructionBundleBuilder::build and asserts that the resulting surface omits that descriptor.Source: Coding guidelines
crates/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs (1)
1731-1749: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the recall-framing message is materialized.
The filter removes every message with the
memory-guidance.memory-recall-framingprefix. The test can therefore pass if the new framing message is absent frombundle.materialized_messages.Assert that exactly one matching message exists and that its
model_contentis non-empty before filtering it from the snippet-order assertion.Proposed test fix
+ let recall_framing: Vec<_> = bundle + .materialized_messages + .iter() + .filter(|message| { + message + .content_ref + .as_str() + .starts_with("msg:memory-guidance.memory-recall-framing.") + }) + .collect(); + assert_eq!(recall_framing.len(), 1); + assert!(!recall_framing[0].model_content.is_empty()); + let model_contents: Vec<&str> = bundle .materialized_messages🤖 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/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs` around lines 1731 - 1749, Update the test around the model_contents filter to first collect messages whose content_ref starts with "msg:memory-guidance.memory-recall-framing.", assert exactly one exists, and assert its model_content is non-empty. Then retain the existing filter and snippet-order assertion.crates/contracts/ironclaw_loop_contracts/src/runtime_context/tests.rs (1)
249-281: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPin the byte limit and prompt-surface validation.
With ten 93-byte names, the 512-byte branch should render five names and report
(+5 more). The current assertions only require that09-is absent and that somemoresuffix exists. A larger limit can pass. The test also does not callvalidate_model_safe_text, despite claiming to protect the 4 KiB prompt-surface cap.Assert the exact remainder and validate the rendered text.
Proposed test fix
assert!( !text.contains("09-"), "the last name must be byte-truncated: {text}" ); assert!( - text.contains(" more)"), + text.contains("(+5 more)"), "the byte-truncated remainder folds into the +N more counter: {text}" ); + assert!( + crate::prompt_text::validate_model_safe_text( + text.clone(), + "pending-auth byte bound", + ) + .is_ok(), + "the rendered context must satisfy the prompt surface cap: {text}" + );🤖 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/contracts/ironclaw_loop_contracts/src/runtime_context/tests.rs` around lines 249 - 281, Strengthen pending_extension_auth_line_is_byte_bounded by asserting the exact byte-budget result: five names render and the suffix reports “(+5 more)”. Then pass the rendered text through validate_model_safe_text and assert validation succeeds, preserving coverage of the 4 KiB prompt-surface limit.
🤖 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/contracts/ironclaw_loop_contracts/src/prompt_text.rs`:
- Around line 218-223: Update authorization_scheme_value_candidate to skip
filler tokens when selecting scheme, then select the next non-filler token as
candidate before validating with is_authorization_scheme. Add a caller-level
regression in memory_prompt_context.rs for “Authorization is Bearer
ghp_secretvalue123” through the production admission path, preserving the
expected admitted count of two.
---
Outside diff comments:
In `@crates/contracts/ironclaw_loop_contracts/src/instruction_bundle.rs`:
- Around line 1198-1202: Update
untrusted_descriptions_with_credential_values_are_omitted_from_the_surface to
use the exact previously accepted credential-plus-filler input rather than
placing the credential immediately after “Bearer”. Ensure the test invokes
InstructionBundleBuilder::build and asserts that the resulting surface omits
that descriptor.
In `@crates/contracts/ironclaw_loop_contracts/src/runtime_context/tests.rs`:
- Around line 249-281: Strengthen pending_extension_auth_line_is_byte_bounded by
asserting the exact byte-budget result: five names render and the suffix reports
“(+5 more)”. Then pass the rendered text through validate_model_safe_text and
assert validation succeeds, preserving coverage of the 4 KiB prompt-surface
limit.
In `@crates/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs`:
- Around line 1731-1749: Update the test around the model_contents filter to
first collect messages whose content_ref starts with
"msg:memory-guidance.memory-recall-framing.", assert exactly one exists, and
assert its model_content is non-empty. Then retain the existing filter and
snippet-order assertion.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cd211c18-7f71-4438-8dc2-389e84292a97
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (8)
crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rscrates/contracts/ironclaw_loop_contracts/src/host/context.rscrates/contracts/ironclaw_loop_contracts/src/instruction_bundle.rscrates/contracts/ironclaw_loop_contracts/src/prompt_text.rscrates/contracts/ironclaw_loop_contracts/src/runtime_context/tests.rscrates/kernel/ironclaw_host_runtime/src/memory_context.rscrates/kernel/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs`:
- Around line 1509-1519: Update
instruction_bundle_allows_trusted_skill_authorization_scheme_value in
crates/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs:1509-1519 to
retain the built bundle and assert the Basic authorization string appears in
bundle.materialized_messages. Also update the trusted/installed skill credential
test at crates/kernel/ironclaw_turns/tests/agent_loop_host_contract.rs:1554-1595
to assert both credential strings are present in bundle.materialized_messages,
validating the materialized caller output rather than only successful
construction.
In `@crates/loop/ironclaw_loop_host/src/model_gateway.rs`:
- Around line 2998-3011: Replace or supplement
provider_bound_redaction_covers_stop_sequences with a caller-level test through
complete_model_request, using a recording LlmProvider to capture the dispatched
CompletionRequest. Configure a credential-bearing stop sequence, invoke the
gateway path, and assert the provider receives the replacement value
[REDACTED_SECRET] rather than the original secret; avoid testing
redact_completion_request directly as the sole verification.
- Around line 1720-1735: Update redact_json_string_values to preserve all object
members when redacted keys collide by generating deterministic unique redacted
key names instead of overwriting entries during values.insert. Apply the
identical key mapping to related JSON Schema references such as required, and
add a regression test covering two distinct secret-bearing keys that redact to
the same value.
In `@crates/loop/ironclaw_loop_host/tests/llm_gateway.rs`:
- Around line 168-202: Add a credential-bearing HostManagedModelMessage using
HostManagedModelMessageRole::ToolResult in
gateway_redacts_every_message_role_before_plain_provider_dispatch, then include
its secret in the existing redaction assertions so provider_text verifies the
tool-result content is redacted and the total [REDACTED_SECRET] count is updated
accordingly.
In `@crates/substrates/ironclaw_safety/README.md`:
- Around line 55-57: Update the README invariant describing model-input findings
to identify both transform application boundaries: immediately before provider
dispatch in model_gateway.rs and at memory admission before persisting the
model-visible snippet in memory_context.rs.
In `@crates/substrates/ironclaw_safety/src/model_input_redaction.rs`:
- Around line 174-190: Extend the keeps_security_prose_and_paths_unchanged test
with inputs covering every allowance in is_redaction_marker: the literal values
token, value, key, example, and placeholder, plus values containing an ellipsis
(...). Assert each remains unmodified and preserves its original text, while
keeping the existing security prose and path cases unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 264c89cd-6f2e-4737-9129-56c810a5eb87
📒 Files selected for processing (12)
crates/app/ironclaw_architecture_tests/tests/reborn_extension_specificity.rscrates/contracts/ironclaw_loop_contracts/src/instruction_bundle.rscrates/contracts/ironclaw_loop_contracts/src/prompt_text.rscrates/contracts/ironclaw_loop_contracts/src/runtime_context/tests.rscrates/kernel/ironclaw_host_runtime/src/memory_context.rscrates/kernel/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/kernel/ironclaw_turns/tests/agent_loop_host_contract.rscrates/loop/ironclaw_loop_host/src/model_gateway.rscrates/loop/ironclaw_loop_host/tests/llm_gateway.rscrates/substrates/ironclaw_safety/README.mdcrates/substrates/ironclaw_safety/src/lib.rscrates/substrates/ironclaw_safety/src/model_input_redaction.rs
💤 Files with no reviewable changes (1)
- crates/app/ironclaw_architecture_tests/tests/reborn_extension_specificity.rs
|
Superseded by #7509, opened from an in-repository nearai/ironclaw branch so the Railway preview deployment can run. |
…ai#7509) * fix(loop): allow security prose in recovered context * test(turns): align prompt safety contract coverage * fix(loop): address review feedback on prompt recovery (nearai#7434) * fix(loop): reject filler-separated credentials (nearai#7434) * fix(safety): redact model-bound secrets without rejecting turns * fix(safety): preserve non-secret sha256 fingerprints * test(safety): align channel context with gateway redaction * fix(gateway): address coderabbit review — preserve redacted JSON shape (nearai#7434) * fix(safety): redact quoted structured credentials * fix(safety): scan encoded tool result content * fix(safety): redact structured credential values * fix(safety): redact character-dump credentials * fix(safety): close provider-bound redaction gaps (nearai#7509) * fix(safety): close structured redaction review gaps (nearai#7509) * fix(safety): redact nested schema and URL fragment secrets (nearai#7509) * test(integration): align prompt trust expectation (nearai#7509)
…ai#7509) * fix(loop): allow security prose in recovered context * test(turns): align prompt safety contract coverage * fix(loop): address review feedback on prompt recovery (nearai#7434) * fix(loop): reject filler-separated credentials (nearai#7434) * fix(safety): redact model-bound secrets without rejecting turns * fix(safety): preserve non-secret sha256 fingerprints * test(safety): align channel context with gateway redaction * fix(gateway): address coderabbit review — preserve redacted JSON shape (nearai#7434) * fix(safety): redact quoted structured credentials * fix(safety): scan encoded tool result content * fix(safety): redact structured credential values * fix(safety): redact character-dump credentials * fix(safety): close provider-bound redaction gaps (nearai#7509) * fix(safety): close structured redaction review gaps (nearai#7509) * fix(safety): redact nested schema and URL fragment secrets (nearai#7509) * test(integration): align prompt trust expectation (nearai#7509)
Summary
[REDACTED_SECRET].Change Type
Linked Issue
None.
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo test -p ironclaw_safety --no-fail-fast(299 passed)cargo test -p ironclaw_loop_contracts --no-fail-fastcargo test -p ironclaw_turns --test agent_loop_host_contract --no-fail-fast(87 passed)cargo test -p ironclaw_host_runtime --test memory_prompt_context --no-fail-fast(18 passed)cargo test -p ironclaw_loop_host --test llm_gateway --no-fail-fast(88 passed)cargo test -p ironclaw_loop_host --lib provider_bound_redaction_covers_stop_sequences --no-fail-fastcargo test -p ironclaw_architecture_tests --no-fail-fastRUST_MIN_STACK=16777216 cargo test -p ironclaw_integration_tests --test reborn_integration_golden_payload --no-fail-fast(21 passed)cargo test -p <owning-crate> --features integration: Not applicable; no database-backed behavior changed.Test Strategy
User behavior: Given recovered text containing security vocabulary, host paths, credential-shaped values, or prompt-injection markers, when IronClaw reconstructs and dispatches a model request, benign context remains usable, detected credential values are placeholders, injection-bearing memory snippets are isolated, and the turn continues.
Risk areas:
Tests added or updated:
What the tests prove: detected secrets do not reach provider-visible prompt content, while false positives no longer reject a whole turn or remove unrelated context. Prompt-injection containment remains independent of secret handling.
Security Impact
The model-input boundary now scans immediately before provider dispatch, after all prompt assembly and before prompt-cache hashing. It covers plain/tool/streaming/repair paths; message content across roles; text content parts; plain reasoning and summaries; tool-call argument string keys/values and parse errors; tool descriptions and JSON schema strings/keys; and stop sequences. It logs only aggregate redaction counts, never detected values.
Opaque protocol material that requires exact replay—encrypted/redacted reasoning payloads, signatures, tool-call IDs/names, route/model identifiers, and provider metadata—is not treated as prompt text. Managed credentials remain host-side and continue to use mediated injection rather than prompt construction.
This guarantees redaction for formats recognized by the existing leak detector plus labeled weak values such as
password: letmein; it is not a cryptographic proof that arbitrary unlabeled prose cannot contain a secret. The strongest invariant remains: secrets known to IronClaw must stay in the secret store and never be assembled into model input. The final scan is defense in depth for untrusted text and accidental leakage.Warn-only high-entropy hex findings remain visible unless they are credential-labeled, so ordinary SHA-256 fingerprints do not mutate prompts or prompt-cache identity.
Prompt injection is intentionally not conflated with secret detection. Existing trust envelopes and injection checks still isolate offending untrusted snippets. SkillSpector-style skill trust scanning and NeMo Guardrails-style broader input/output policy remain follow-up layers rather than dependencies of this recovery hotfix.
Reborn Trust-Boundary Checklist
serde(default)fields: none changed.Database Impact
None. Raw stored LLM/thread/memory data is retained; redaction applies to the transient model-facing view.
Blast Radius
Prompt construction contracts, production memory admission, and all loop-host provider request shapes. A regression could leak a recognized secret, corrupt opaque replay data, or reintroduce thread-wide rejection. Caller-level provider captures, memory integration tests, structural contract tests, all-features clippy, and architecture ratchets cover those directions.
Compatibility and Rollback
No wire or storage migration. Removing the unused
base64dependency fromironclaw_loop_contractsshrinks the contract layer. Roll back by reverting this PR; stored data requires no repair. During rollback, the prior credential denylist behavior—and its false-positive thread failures—would return.Follow-up
Review track: C (security/runtime/DB/CI)