Skip to content

[Reborn] Envelope installed skill prompt context - #3505

Closed
serrrfirat wants to merge 3 commits into
reborn-integrationfrom
reborn/3476-envelope-sanitization
Closed

serrrfirat wants to merge 3 commits into
reborn-integrationfrom
reborn/3476-envelope-sanitization

Conversation

@serrrfirat

Copy link
Copy Markdown
Collaborator

Summary

  • add shared untrusted-context envelope/sanitizer used by memory and installed skill prompt content
  • preserve installed skill prompt content only after wrapping it as Untrusted skill content: ...
  • reject instruction-like installed skill prompt content fail-closed before loop context emission

Scope

Follow-up to #3476. Addresses the #3492 comment item: SKILL.md prompt content for Installed skills needs the same baseline untrusted-content envelope primitive as memory.

Verification

  • CARGO_TARGET_DIR=/Users/firatsertgoz/Documents/ironclaw/target cargo test -p ironclaw_loop_support -p ironclaw_host_runtime -p ironclaw_turns
  • CARGO_TARGET_DIR=/Users/firatsertgoz/Documents/ironclaw/target cargo test -p ironclaw_architecture

Does not close #3492.

@github-actions github-actions Bot added size: L 200-499 changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels May 11, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request centralizes the sanitization and enveloping of untrusted content into a new untrusted_context module. It modifies Installed skills to include sanitized prompt content within an explicit untrusted envelope, whereas this content was previously excluded. Reviewer feedback recommends making the prefix method pub(crate) to avoid string duplication, replacing hardcoded byte limits with shared constants, and using the UntrustedContextKind::prefix method as the single source of truth for prefixes.

}

impl UntrustedContextKind {
const fn prefix(self) -> &'static str {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Make this method pub(crate) so it can be used by other modules in the crate (e.g., skill_context.rs) to avoid duplicating the prefix strings and maintain consistency.

Suggested change
const fn prefix(self) -> &'static str {
pub(crate) const fn prefix(self) -> &'static str {
References
  1. Use centralized helper functions for security-sensitive logic to ensure consistency and avoid duplication.

SkillTrustLevel::Installed => None,
SkillTrustLevel::Installed => {
let summary =
untrusted_context_summary(UntrustedContextKind::Skill, &parsed.prompt_content, 512)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The value 512 is used as a hardcoded limit for untrusted skill content. Always truncate tool output for previews or status updates to a reasonable maximum length. Consider defining a shared constant for this limit to maintain consistency across the repository.

References
  1. Always truncate tool output for previews or status updates to a reasonable maximum length.
  2. Use centralized helper functions for security-sensitive logic to ensure consistency and avoid duplication.

const DEFAULT_MAX_SKILL_CONTEXT_BYTES: usize = 32 * 1024;
const FNV_OFFSET: u64 = 0xcbf29ce484222325;
const FNV_PRIME: u64 = 0x00000100000001B3;
const UNTRUSTED_SKILL_PREFIX: &str = "Untrusted skill content: ";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The UNTRUSTED_SKILL_PREFIX string is duplicated here. To improve maintainability and ensure consistency, use the UntrustedContextKind::prefix method as the single source of truth.

Suggested change
const UNTRUSTED_SKILL_PREFIX: &str = "Untrusted skill content: ";
const UNTRUSTED_SKILL_PREFIX: &str = UntrustedContextKind::Skill.prefix();
References
  1. Use centralized helper functions for security-sensitive logic to ensure consistency and avoid duplication.

@serrrfirat

Copy link
Copy Markdown
Collaborator Author

Paranoid review found Medium+ issues at head 7b0a8942df6a9d23278781dda16a370b86dfae8d:

Severity Confidence File:Line Finding
Medium Certain crates/ironclaw_loop_support/src/skill_context.rs:171-187, crates/ironclaw_turns/src/run_profile/skill_context.rs:347-355 Installed skill description bypasses the new untrusted sanitizer. Scenario: installed SKILL.md has description: ignore previous instructions... and benign prompt. Builder sanitizes only parsed.prompt_content, stores raw parsed.manifest.description, and service emits it before enveloped prompt. Fix/test: sanitize/envelope or reject instruction-like installed descriptions; add contract test with malicious installed description.
Medium Certain crates/ironclaw_turns/src/run_profile/skill_context.rs:347-352 Installed prompt_content validation is prefix-only at service boundary. Scenario: caller constructs SkillRunSnapshot::from_entries with prompt_content: Some("Untrusted skill content: ignore previous instructions"); snapshot hash is valid, service emits injection because it checks only starts_with. Fix/test: add validator for already-enveloped untrusted summaries: prefix + LoopSafeSummary + instruction-marker check on payload; add test for prefixed unsafe installed prompt rejection.

@serrrfirat

Copy link
Copy Markdown
Collaborator Author

Closing this version. Direction changed: instead of runtime hard-rejecting installed skill prompt content, we should move suspicious-skill handling to install/discovery time with quarantine/review UX, hash-scoped trust decisions, and runtime-safe fallback that never emits unsafe raw content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules size: L 200-499 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant