Skip to content

refactor(host_api): centralize system-sentinel id minting and pin authority contract - #4584

Open
matiasbenary wants to merge 7 commits into
nearai:mainfrom
matiasbenary:security/pr-4575-tests-and-docs
Open

matiasbenary wants to merge 7 commits into
nearai:mainfrom
matiasbenary:security/pr-4575-tests-and-docs

Conversation

@matiasbenary

Copy link
Copy Markdown
Contributor

Summary

  • Add a blessed TenantId::system_sentinel() / UserId::system_sentinel() constructor as the only sanctioned
    path to mint a SYSTEM_RESERVED_ID-valued id, and route the four legitimate call sites (ResourceScope::system,
    GovernorBackedAccountant, ThreadScope::to_resource_scope, TurnScope::to_resource_scope) through it instead
    of the general from_trusted escape hatch. The point is auditability: every authority-elevating constructor is
    now git grep-able as system_sentinel.
  • Reuse the same constructor inside the sentinel-aware Deserialize impl so the JSON round-trip path and the
    in-process path mint the sentinel through identical code.
  • Document the security contract on the accepts_system_sentinel macro arm: opting a new id kind in is a

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions github-actions Bot added size: M 50-199 changed lines risk: low Changes to docs, tests, or low-risk modules contributor: regular 2-5 merged PRs labels Jun 9, 2026
@matiasbenary
matiasbenary marked this pull request as ready for review June 9, 2026 02:38
Copilot AI review requested due to automatic review settings June 9, 2026 02:38
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR standardizes construction of the “system” sentinel IDs across crates and updates serde behavior so ResourceScope::system() can round-trip through JSON while keeping the sentinel value unconstructible via normal new() validators.

Changes:

  • Introduces system_sentinel() as the blessed constructor for sentinel-valued IDs and updates call sites to use it.
  • Updates the string_id! macro to generate strict vs. sentinel-aware Deserialize implementations (TenantId/UserId only).
  • Adds host API contract tests pinning JSON round-trip behavior and ensuring non-opted-in ID types reject the sentinel.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
crates/ironclaw_turns/src/scope.rs Switches fallback user_id construction to UserId::system_sentinel()
crates/ironclaw_threads/src/contract.rs Switches fallback user_id construction to UserId::system_sentinel()
crates/ironclaw_loop_support/src/budget_accountant.rs Switches fallback user_id construction to UserId::system_sentinel() and drops direct constant usage
crates/ironclaw_host_api/src/resource.rs Routes ResourceScope::system() through the new sentinel constructors
crates/ironclaw_host_api/src/ids.rs Extends string_id! to support strict vs sentinel-aware deserialization and adds system_sentinel()
crates/ironclaw_host_api/tests/host_api_contract.rs Adds tests pinning sentinel behavior across serialization/deserialization and ID kinds

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/ironclaw_host_api/src/ids.rs Outdated
Comment on lines 185 to 197
impl<'de> Deserialize<'de> for $name {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
let value = String::deserialize(deserializer)?;
// Admit the system sentinel; `Self::new` rejects its
// control bytes, so JSON round-trip needs an explicit bypass.
if value == crate::SYSTEM_RESERVED_ID {
return Ok(Self::system_sentinel());
}
Self::new(value).map_err(serde::de::Error::custom)
}
Comment on lines 66 to +70
/// validation rejects, so no user-supplied identifier can collide.
pub fn system() -> Self {
Self {
tenant_id: TenantId::from_trusted(SYSTEM_RESERVED_ID.to_string()),
user_id: UserId::from_trusted(SYSTEM_RESERVED_ID.to_string()),
tenant_id: TenantId::system_sentinel(),
user_id: UserId::system_sentinel(),
Comment on lines +221 to +224
// Sentinel stays unreachable through ordinary construction; only
// `from_trusted` may produce it, so user input can never collide.
assert!(TenantId::new(SYSTEM_RESERVED_ID).is_err());
assert!(UserId::new(SYSTEM_RESERVED_ID).is_err());
serrrfirat
serrrfirat approved these changes Jun 9, 2026 •

@serrrfirat serrrfirat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code Review Summary — #4584

Verdict: REQUEST_CHANGES — 2 High findings (1 security, 2 tests missing critical coverage)

# Sev Category File Finding
1 🔴 High Security ids.rs:194 @deserialize_with_sentinel admits sentinel from untrusted JSON
2 🟠 Medium Security ids.rs:179 system_sentinel() is pub — any crate can mint system-authority IDs
3 🔴 High Tests scope.rs:103 TurnScope::to_resource_scope() sentinel fallback never tested
4 🔴 High Tests contract.rs:33 ThreadScope::to_resource_scope() sentinel fallback never tested
5 🟡 Medium Tests budget_accountant.rs:397 GovernorBackedAccountant no-actor branch never exercised
6 🟡 Medium Tests filesystem_service.rs:510 (not in diff) No integration test for ownerless ThreadScope through full call site
7 ⚪ Low Performance ids.rs:194 Sentinel branch discards owned String, double-allocates
8 ⚪ Low Tests host_api_contract.rs:221 Stale comment contradicts new system_sentinel() API
9 ⚪ Low Conventions ids.rs:121 (not in diff) from_trusted doc still says "sentinel values" — contradicts system_sentinel guidance

Reviewers: security ✅ bugs ✅ performance ✅ tests ✅ conventions ✅


Finding #6 (Medium — Tests — not in diff)

crates/ironclaw_threads/src/filesystem_service.rs — No integration test drives FilesystemSessionThreadService with ownerless ThreadScope through the full call site. .claude/rules/testing.md requires tests through the caller (not just the helper) when a transform gates a side effect and there is at least one wrapper. ThreadScope::to_resource_scope() is called at 20+ sites in filesystem_service.rs; a scope with owner_user_id=None routes all those writes to the sentinel path with no end-to-end coverage.

Fix: add tests::filesystem_thread_service_stores_and_reads_ownerless_scope covering ensure_thread + load_context_window with owner_user_id=None.


Finding #9 (Low — Conventions — not in diff)

crates/ironclaw_host_api/src/ids.rs:121 — The from_trusted doc-comment says "Reserved for sentinel values that intentionally contain bytes the validator rejects". The @sentinel_constructor arm added in this same PR explicitly says callers must use system_sentinel() instead. The stale doc actively misdirects readers.

Fix: update from_trusted doc to say: "Construct without validation. For non-sentinel trusted string rehydration (e.g. DB row values). To mint the system sentinel, use system_sentinel() instead."

Comment thread crates/ironclaw_host_api/src/ids.rs Outdated
// Admit the system sentinel; `Self::new` rejects its
// control bytes, so JSON round-trip needs an explicit bypass.
if value == crate::SYSTEM_RESERVED_ID {
return Ok(Self::system_sentinel());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 High — Security — #1: @deserialize_with_sentinel admits sentinel from any untrusted JSON source

Before this diff, TenantId and UserId used @deserialize_strict which delegated to Self::new() → validate_scope_id() → rejects \x1f control bytes. No caller-supplied JSON could ever produce a sentinel-valued ID.

The new @deserialize_with_sentinel arm fires before validation with zero trust context: any Deserializer — including HTTP request bodies, queue messages, WASM host-call payloads, inter-process pipes — that supplies "\x1fSYSTEM\x1f" will successfully deserialize into a TenantId/UserId bearing the system sentinel. resource.rs still documents "no user-supplied identifier can ever collide", but that invariant is now broken.

All structs containing TenantId/UserId (ResourceScope, etc.) inherit this widened attack surface. Any future HTTP/RPC endpoint adding one of these types as a request-body field silently becomes an authority-elevation vector.

Fix: Do not admit the sentinel through the public Deserialize impl. Define a separate serde module used only in explicitly trusted deserialization sites (DB row reads, trusted inter-process messages):

// Separate trusted serde helper, used only with #[serde(with = "...")]
pub mod trusted_serde {
    pub fn deserialize<'de, D: serde::Deserializer<'de>>(d: D) -> Result<TenantId, D::Error> {
        let v = String::deserialize(d)?;
        if v == crate::SYSTEM_RESERVED_ID { return Ok(TenantId::system_sentinel()); }
        TenantId::new(v).map_err(serde::de::Error::custom)
    }
}
// Default Deserialize stays strict:
impl<'de> Deserialize<'de> for TenantId {
    fn deserialize<D: serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
        let v = String::deserialize(d)?;
        Self::new(v).map_err(serde::de::Error::custom)
    }
}

/// `ResourceScope::is_system` returns `true` only when BOTH the
/// tenant and user ids of a scope come from this constructor;
/// no authority decision may key on a single field.
pub fn system_sentinel() -> Self {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟠 Medium — Security — #2: system_sentinel() is pub — any crate can mint system-authority IDs without audit

system_sentinel() is pub on pub types TenantId/UserId. Every downstream crate — product adapters, integration glue, WASM shims — can call it to produce an ID that passes ResourceScope::is_system(). from_trusted() was already pub but anonymous; system_sentinel() is semantically named, prominently doc-commented as the "blessed path", and therefore far more discoverable. A developer writing a new service could call it to produce a "system" scope for what they believe is a privileged operation, bypassing per-tenant isolation without any capability check or audit event. Documentation-as-security is not an access-control mechanism.

Fix: Restrict to pub(crate), or if cross-crate use is genuinely needed, gate on a zero-sized capability token so the type system enforces the restriction:

// Option A (simplest):
pub(crate) fn system_sentinel() -> Self { ... }

// Option B (cross-crate with type-system enforcement):
pub fn system_sentinel(_: &crate::SystemSentinelAuthority) -> Self { ... }

scope.user_id = self
.explicit_owner_user_id()
.cloned()
.unwrap_or_else(UserId::system_sentinel);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 High — Tests — #3: TurnScope::to_resource_scope() sentinel fallback path never tested

When explicit_owner_user_id() returns None, user_id falls back to UserId::system_sentinel(). No test asserts this branch executes or that the resulting ResourceScope carries user_id == SYSTEM_RESERVED_ID with is_system() == false (partial sentinel). Wrong user attribution silently routes billing and storage under the system slot.

Fix:

// crates/ironclaw_turns/tests/ (or inline)
#[test]
fn turn_scope_to_resource_scope_uses_sentinel_when_no_explicit_owner() {
    let scope = TurnScope::new(
        TenantId::new("tenant-abc").unwrap(), None, None,
        ThreadId::new("thread-abc").unwrap(),
    );
    let rs = scope.to_resource_scope();
    assert_eq!(rs.user_id.as_str(), SYSTEM_RESERVED_ID);
    assert!(!rs.is_system(), "partial sentinel must not classify as system");
}

}),
user_id: self
.owner_user_id
.clone()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 High — Tests — #4: ThreadScope::to_resource_scope() sentinel fallback (owner_user_id=None) never tested

ThreadScope::to_resource_scope() uses UserId::system_sentinel() when owner_user_id is None. All test helpers set owner_user_id: Some(...) — no test constructs a ThreadScope with owner_user_id: None and asserts the resulting ResourceScope has user_id == SYSTEM_RESERVED_ID and is_system() == false. Infrastructure/ownerless threads silently land in the sentinel user slot without coverage.

Fix:

#[test]
fn thread_scope_to_resource_scope_uses_sentinel_when_owner_absent() {
    let scope = ThreadScope {
        tenant_id: TenantId::new("tenant-x").unwrap(),
        agent_id: AgentId::new("agent-x").unwrap(),
        owner_user_id: None,
        project_id: None, mission_id: None,
    };
    let rs = scope.to_resource_scope();
    assert_eq!(rs.user_id.as_str(), SYSTEM_RESERVED_ID);
    assert!(!rs.is_system());
}

@@ -397,7 +396,7 @@ impl GovernorBackedAccountant {
.actor
.as_ref()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Medium — Tests — #5: GovernorBackedAccountant no-actor branch never exercised

resource_scope() uses .unwrap_or_else(UserId::system_sentinel) when LoopRunContext::actor is None. Every test helper always sets an actor. No test verifies that pre_model_call with actor=None routes reservation to the sentinel user account rather than panicking or using a wrong scope.

Fix: tests::pre_model_call_uses_sentinel_user_when_actor_is_absent covering LoopRunContext without actor

Comment thread crates/ironclaw_host_api/src/ids.rs Outdated
let value = String::deserialize(deserializer)?;
// Admit the system sentinel; `Self::new` rejects its
// control bytes, so JSON round-trip needs an explicit bypass.
if value == crate::SYSTEM_RESERVED_ID {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚪ Low — Performance — #7: Sentinel branch discards owned String, double-allocates

Pre-PR: return Ok(Self::from_trusted(value)) reused the String already allocated by String::deserialize. Post-PR: return Ok(Self::system_sentinel()) calls SYSTEM_RESERVED_ID.to_string() for a new allocation, then drops value. Every sentinel deserialization hit now performs 2 heap allocations where 1 sufficed. Neither system_sentinel nor from_trusted carries #[inline], so the compiler is not guaranteed to optimize this away.

Fix: In the @deserialize_with_sentinel arm only, reuse the owned string:

if value == crate::SYSTEM_RESERVED_ID {
    return Ok(Self::from_trusted(value)); // reuse owned String
}

This preserves the auditability goal (the equality check above is the sentinel gate) while avoiding the extra allocation.

assert_eq!(decoded.tenant_id.as_str(), SYSTEM_RESERVED_ID);
assert_eq!(decoded.user_id.as_str(), SYSTEM_RESERVED_ID);

// Sentinel stays unreachable through ordinary construction; only

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚪ Low — Tests — #8: Stale comment contradicts new system_sentinel() API

This comment says "only from_trusted may produce it" but the PR adds system_sentinel() as the blessed cross-crate constructor (and the adjacent system_sentinel_constructor_produces_the_reserved_value test pins exactly this). The comment now contradicts the new API contract.

Fix:

// Sentinel stays unreachable through ordinary construction; the blessed
// minting paths are `TenantId::system_sentinel()` / `UserId::system_sentinel()`
// (and the lower-level `from_trusted` escape hatch they wrap),
// so user input can never collide.

@coderabbitai

coderabbitai Bot commented Jun 15, 2026 •

Copy link
Copy Markdown

Review Change Stack

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 408a85c9-28eb-4fd9-a12d-1affc7e34309

📥 Commits

Reviewing files that changed from the base of the PR and between b770052 and 5701d66.

📒 Files selected for processing (2)
  • crates/ironclaw_host_api/src/resource.rs
  • crates/ironclaw_host_api/tests/host_api_contract.rs

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added explicit system_sentinel() constructors for system-level identifier types (TenantId, UserId) to support system-scoped operations.
  • Refactor

    • Updated system scope creation and ownerless fallbacks to use the new sentinel constructors, keeping normal identifier validation strict.
  • Tests

    • Expanded contract tests to verify JSON round-tripping for system scopes and to ensure sentinel values are rejected for ordinary (non-system) identifiers, including near-miss string cases.

Walkthrough

Introduces a dedicated system_sentinel() constructor on TenantId and UserId via a new accepts_system_sentinel mode in the string_id! macro. ResourceScope gains sentinel-aware serde helpers so SYSTEM_RESERVED_ID is accepted only for its own fields. All from_trusted(SYSTEM_RESERVED_ID...) call sites across budget_accountant, ThreadScope, and TurnScope are replaced with the new constructor, and contract/security tests are added throughout.

Changes

System Sentinel Constructor Rollout

Layer / File(s) Summary
string_id! macro sentinel mode and TenantId/UserId wiring
crates/ironclaw_host_api/src/ids.rs
Adds an accepts_system_sentinel macro variant that generates pub fn system_sentinel() -> Self and a strict @deserialize_strict arm keeping on-wire deserialization rejecting; updates from_trusted docs; wires TenantId and UserId to the new mode.
ResourceScope sentinel-aware deserialization
crates/ironclaw_host_api/src/resource.rs
Annotates tenant_id and user_id fields with #[serde(deserialize_with)] targeting new internal helpers that map SYSTEM_RESERVED_ID to system_sentinel() and validate all other values; updates ResourceScope::system() to use the new constructors.
Host API sentinel contract and security tests
crates/ironclaw_host_api/tests/host_api_contract.rs
Adds tests for ResourceScope::system() round-trip, sentinel constructor values, rejection of SYSTEM_RESERVED_ID on bare ID types, near-miss string rejection, lookalike string classification, and is_system() requiring both fields to carry the sentinel simultaneously.
Callsite migration in loop_support, threads, and turns
crates/ironclaw_loop_support/src/budget_accountant.rs, crates/ironclaw_threads/src/contract.rs, crates/ironclaw_turns/src/scope.rs
Replaces from_trusted(SYSTEM_RESERVED_ID...) with UserId::system_sentinel() in resource_scope fallback logic across all three call sites; each gains new unit tests asserting fallback value and that is_system() remains false.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 A sentinel hops in, named with care,
No raw from_trusted left to snare.
system_sentinel() — the one true gate,
Near-miss strings rejected by fate.
The macro arms stretch, the tests run bright,
This rabbit declares the sentinels right! 🌟

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning Summary is present and substantive, but required sections (Change Type, Validation, Security Impact, Reborn checklist, Database Impact, Blast Radius, Rollback Plan, Review track) are missing or unchecked. Complete remaining template sections: check Change Type boxes, specify validation steps taken, document security impact and reborn trust-boundary considerations, note database/rollback/blast radius, assign review track.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed Title follows Conventional Commits style with clear scope and summary accurately describing the refactoring work.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added size: L 200-499 changed lines and removed size: M 50-199 changed lines labels Jun 15, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
crates/ironclaw_host_api/src/ids.rs (1)

230-236: ⚡ Quick win

Clarify the macro comment: sentinel is NOT admitted during bare ID JSON deserialization.

The comment says "admitting [crate::SYSTEM_RESERVED_ID] during JSON deserialization" but the @deserialize_strict arm is still used for these types, so the bare TenantId/UserId Deserialize impl rejects the sentinel. Sentinel admission only happens via ResourceScope's field-level deserialize_with helpers. Consider rewording to avoid confusion:

-// SECURITY-CRITICAL: `accepts_system_sentinel` opts these id types into
-// admitting [`crate::SYSTEM_RESERVED_ID`] during JSON deserialization and
-// exposes the `system_sentinel()` constructor — the only blessed path that
-// can mint a sentinel-valued id. Every other id kind stays strict and
-// rejects the sentinel on the wire. Adding this flag to any new id type
-// requires security review: any HTTP/RPC endpoint that deserializes such an
-// id from an untrusted request body becomes an authority-elevation vector.
+// SECURITY-CRITICAL: `accepts_system_sentinel` exposes the `system_sentinel()`
+// constructor — the only blessed path for minting a sentinel-valued id. The
+// bare id's `Deserialize` impl stays STRICT (rejects the sentinel on the wire);
+// sentinel admission during deserialization is scoped to trusted-persistence
+// shapes like `ResourceScope`'s field-level `deserialize_with` helpers.
+// Adding this flag to any new id type requires security review.
🤖 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/ironclaw_host_api/src/ids.rs` around lines 230 - 236, The comment
block for the `accepts_system_sentinel` flag at lines 230-236 incorrectly
implies that the sentinel is admitted during bare JSON deserialization of id
types like TenantId and UserId. Clarify the comment to accurately reflect that
the `@deserialize_strict` arm still rejects the sentinel during bare
deserialization, and that sentinel admission only occurs via ResourceScope's
field-level `deserialize_with` helpers. The flag should be described as exposing
the `system_sentinel()` constructor (the blessed path to mint sentinel-valued
ids) while maintaining strict deserialization at the bare id type level.
🤖 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.

Nitpick comments:
In `@crates/ironclaw_host_api/src/ids.rs`:
- Around line 230-236: The comment block for the `accepts_system_sentinel` flag
at lines 230-236 incorrectly implies that the sentinel is admitted during bare
JSON deserialization of id types like TenantId and UserId. Clarify the comment
to accurately reflect that the `@deserialize_strict` arm still rejects the
sentinel during bare deserialization, and that sentinel admission only occurs
via ResourceScope's field-level `deserialize_with` helpers. The flag should be
described as exposing the `system_sentinel()` constructor (the blessed path to
mint sentinel-valued ids) while maintaining strict deserialization at the bare
id type level.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 28a501f8-4748-4299-a22a-e6277931d802

📥 Commits

Reviewing files that changed from the base of the PR and between 06db5af and b770052.

📒 Files selected for processing (6)
  • crates/ironclaw_host_api/src/ids.rs
  • crates/ironclaw_host_api/src/resource.rs
  • crates/ironclaw_host_api/tests/host_api_contract.rs
  • crates/ironclaw_loop_support/src/budget_accountant.rs
  • crates/ironclaw_threads/src/contract.rs
  • crates/ironclaw_turns/src/scope.rs

Copilot AI review requested due to automatic review settings June 15, 2026 03:16
@github-actions github-actions Bot added size: M 50-199 changed lines and removed size: L 200-499 changed lines labels Jun 15, 2026

Copilot AI 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.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comment on lines +230 to +236
// SECURITY-CRITICAL: `accepts_system_sentinel` opts these id types into
// admitting [`crate::SYSTEM_RESERVED_ID`] during JSON deserialization and
// exposes the `system_sentinel()` constructor — the only blessed path that
// can mint a sentinel-valued id. Every other id kind stays strict and
// rejects the sentinel on the wire. Adding this flag to any new id type
// requires security review: any HTTP/RPC endpoint that deserializes such an
// id from an untrusted request body becomes an authority-elevation vector.
Comment on lines +175 to +180
/// SECURITY-CRITICAL: this is the blessed path for minting a
/// sentinel-valued id. It exists so every authority-elevating
/// constructor is `git grep`-able as `system_sentinel`, rather
/// than hidden behind the more general `from_trusted` escape
/// hatch. New code that needs the sentinel must call this
/// method — never `from_trusted(SYSTEM_RESERVED_ID.to_string())`.
Comment on lines +241 to +243
// SECURITY GUARD (PR review finding #1): NO bare id type — not even the
// sentinel-aware TenantId/UserId — admits the sentinel during its own
// `Deserialize`. Sentinel admission lives exclusively on `ResourceScope`'s

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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/ironclaw_host_api/src/resource.rs`:
- Around line 68-69: Replace the `from_trusted(raw)` calls with
`system_sentinel()` in the deserialization helpers to maintain the auditability
contract. When deserializing TenantId and checking if raw equals
SYSTEM_RESERVED_ID, use the sentinel-aware `system_sentinel()` constructor
instead of `from_trusted(raw)` to ensure these authority-elevating constructor
calls are discoverable via git grep and align with the behavior of
ResourceScope::system(). This fix applies to multiple locations in the
deserialization logic.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 408a85c9-28eb-4fd9-a12d-1affc7e34309

📥 Commits

Reviewing files that changed from the base of the PR and between b770052 and 5701d66.

📒 Files selected for processing (2)
  • crates/ironclaw_host_api/src/resource.rs
  • crates/ironclaw_host_api/tests/host_api_contract.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

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/ironclaw_host_api/src/resource.rs`:
- Around line 68-69: Replace the `from_trusted(raw)` calls with
`system_sentinel()` in the deserialization helpers to maintain the auditability
contract. When deserializing TenantId and checking if raw equals
SYSTEM_RESERVED_ID, use the sentinel-aware `system_sentinel()` constructor
instead of `from_trusted(raw)` to ensure these authority-elevating constructor
calls are discoverable via git grep and align with the behavior of
ResourceScope::system(). This fix applies to multiple locations in the
deserialization logic.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 408a85c9-28eb-4fd9-a12d-1affc7e34309

📥 Commits

Reviewing files that changed from the base of the PR and between b770052 and 5701d66.

📒 Files selected for processing (2)
  • crates/ironclaw_host_api/src/resource.rs
  • crates/ironclaw_host_api/tests/host_api_contract.rs
🛑 Comments failed to post (1)
crates/ironclaw_host_api/src/resource.rs (1)

68-69: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Deserialize helpers bypass system_sentinel(), breaking the stated auditability contract.

PR objective: "make every authority-elevating constructor discoverable via git grep for system_sentinel" and "The same constructor is reused in the sentinel-aware Deserialize implementation".

The code calls from_trusted(raw) instead of system_sentinel(). This means git grep system_sentinel will miss these two call sites, and the JSON round-trip path diverges from ResourceScope::system().

Proposed fix
 fn deserialize_system_aware_tenant_id<'de, D>(deserializer: D) -> Result<TenantId, D::Error>
 where
     D: serde::Deserializer<'de>,
 {
     let raw = String::deserialize(deserializer)?;
     if raw == SYSTEM_RESERVED_ID {
-        Ok(TenantId::from_trusted(raw))
+        Ok(TenantId::system_sentinel())
     } else {
         TenantId::new(raw).map_err(serde::de::Error::custom)
     }
 }

 fn deserialize_system_aware_user_id<'de, D>(deserializer: D) -> Result<UserId, D::Error>
 where
     D: serde::Deserializer<'de>,
 {
     let raw = String::deserialize(deserializer)?;
     if raw == SYSTEM_RESERVED_ID {
-        Ok(UserId::from_trusted(raw))
+        Ok(UserId::system_sentinel())
     } else {
         UserId::new(raw).map_err(serde::de::Error::custom)
     }
 }

Also applies to: 80-81

🤖 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/ironclaw_host_api/src/resource.rs` around lines 68 - 69, Replace the
`from_trusted(raw)` calls with `system_sentinel()` in the deserialization
helpers to maintain the auditability contract. When deserializing TenantId and
checking if raw equals SYSTEM_RESERVED_ID, use the sentinel-aware
`system_sentinel()` constructor instead of `from_trusted(raw)` to ensure these
authority-elevating constructor calls are discoverable via git grep and align
with the behavior of ResourceScope::system(). This fix applies to multiple
locations in the deserialization logic.

This branch has not been deployed

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

Labels

contributor: regular 2-5 merged PRs risk: low Changes to docs, tests, or low-risk modules size: M 50-199 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants