Skip to content

fix: gateway-sign auto-redacted responses - #992

Open
hanakannzashi wants to merge 1 commit into
mainfrom
codex/fix-auto-redact-attestation-892
Open

fix: gateway-sign auto-redacted responses#992
hanakannzashi wants to merge 1 commit into
mainfrom
codex/fix-auto-redact-attestation-892

Conversation

@hanakannzashi

@hanakannzashi hanakannzashi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • use a Gateway signature when auto-redaction changes an attested chat-completion response
  • suppress the provider-signature fetch for that response
  • cover non-streaming and continuous-usage streaming with byte-exact regression tests

#892 was reported for streams. The same auto-redact response rewrite exists for non-streaming completions, so this PR fixes both paths.

Testing

  • cargo test -p api --lib
  • cargo test -p services 'completions::' --lib
  • cargo test -p api --test e2e_all --no-run
  • Focused E2E execution is blocked locally by PostgreSQL authentication during test bootstrap.

Fixes #892

@hanakannzashi
hanakannzashi deployed to Cloud API test env September 1, 2026 05:51 — with GitHub Actions Active
@hanakannzashi
hanakannzashi force-pushed the codex/fix-auto-redact-attestation-892 branch from 7bce550 to 7567819 Compare September 1, 2026 05:55
@hanakannzashi hanakannzashi changed the title fix: gateway-sign auto-redacted streams fix: gateway-sign auto-redacted responses Sep 1, 2026
@hanakannzashi
hanakannzashi deployed to Cloud API test env September 1, 2026 05:55 — with GitHub Actions Active
@claude

claude Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review: fix: gateway-sign auto-redacted streams

Targeted, well-scoped fix, and the core reasoning is right: auto-redact disables the event.raw_passthrough branch (completions.rs:1677-1686) and re-serializes every chunk, so a provider TEE signature over the pre-redaction bytes can never match what the client received. Moving the gateway_signature_enabled computation below maybe_redact so it can observe auto_redact_enabled is the correct placement — nothing between the old and new assignment points reads skip_provider_chat_signature, and the field is only consumed later in create_chat_completion_stream (services/src/completions/mod.rs:1676).

Gating on auto_redact_enabled (requested && !redaction_map.is_empty()) rather than auto_redact_requested is also correct and consistent with the existing passthrough guard — a no-PII request keeps byte-exact provider signing.

Note: I couldn't load the existing inline review threads for this PR, so apologies if any of the below was already covered.

⚠️ model_attestation_supported == None still stores a mismatching provider signature

crates/api/src/routes/completions.rs:468-474

auto_redact_enabled && model_attestation_supported.unwrap_or(false)

The unwrap_or(false) mirrors the fallback policy in chat_stream_usage_mode, but that policy doesn't transfer. There, None disables the rewrite itself — bytes stay byte-exact, so keeping the provider signature is sound. Auto-redact rewrites the chunks unconditionally; None does not turn that off.

Failure path: get_models_with_pricing() errors (completions.rs:1451-1462 — warn and continue), so model_attestation_supported = None for a model that is attested. Then:

  • gateway_signature_enabled = falseskip_provider_chat_signature = false
  • the service layer reads model.attestation_supported == true directly and runs the provider-signature fetch (services/src/completions/mod.rs:141-145, 1676)
  • the client still receives re-serialized, un-redacted bytes

Net: GET /v1/signature/{chat_id} returns a provider_tee signature whose response_hash covers bytes the client never saw. That is worse than the pre-fix state in one respect — a confidently-wrong signature rather than a missing one, with nothing in the response telling the client to distrust it. The new unit test at line 3241 currently pins this behaviour (assert!(!auto_redact_requires_gateway_signature(true, None))).

Suggested fix — fail closed when attestation status is unknown but we know we have mutated the bytes:

fn auto_redact_requires_gateway_signature(
    auto_redact_enabled: bool,
    model_attestation_supported: Option<bool>,
) -> bool {
    // Unlike the usage-rewrite modes, `None` cannot fall back to byte-exact
    // passthrough: auto-redact re-serializes the stream regardless. Suppress
    // the provider fetch rather than store a signature over bytes the client
    // never received.
    auto_redact_enabled && model_attestation_supported.unwrap_or(true)
}

Worth confirming store_chat_signature_and_unpin is a safe no-op for a chat_id that was never pinned (non-attested model) before taking unwrap_or(true). If it isn't, the alternative is to keep the gateway-sign decision as-is but set skip_provider_chat_signature = true whenever auto_redact_enabled — no signature at all is the honest answer here.

Notes (non-blocking)

  • Non-streaming attested + auto-redact has the same shape. completions.rs:2113 re-serializes the un-redacted body while services/src/completions/mod.rs:1834-1841 unconditionally spawns store_chat_signature_from_provider on model.attestation_supportedskip_provider_chat_signature is not consulted on the non-streaming path at all. The comment at completions.rs:2110 documents dropping the signed payload as deliberate, so I am reading it as accepted scope; flagging only because the stored signature is still served over /v1/signature/{chat_id} and will mismatch there too, in case Auto-redacted streams store a provider signature that cannot match client-received bytes #892 was meant to cover it.
  • E2EE + auto-redact is unguarded on the chat plane. /v1/completions rejects both combinations explicitly (completions.rs:3468-3488) and native Anthropic rejects E2EE outright (anthropic.rs:771-782), but maybe_redact never inspects the encryption headers. It stays inert in practice — the detector finds no PII in ciphertext, so redaction_map is empty — but if it ever minted a placeholder, opaque E2EE chunks would get re-serialized. This PR makes that corner better (the signature would at least match the delivered bytes), so no action needed here; it is pre-existing and worth a separate guard.
  • Test coverage narrowed. auto_redact_streaming_splits_placeholder_across_chunks was repurposed rather than added alongside, so the default-stream (no stream_options) content-delta split case lost its dedicated test. auto_redact_unredacts_tool_call_arguments_streaming (line 552) still exercises default-stream tail reassembly, so it is not a hole — but a second test rather than a rename would have kept both.
  • The switch to .content_type() + .bytes(Bytes::from(request_json.clone())) is a good call for a byte-exact hash assertion, and bytes is already a regular dependency of crates/api, so it is available to the integration test.
  • Signature text format matches store_gateway_signature ({request_hash}:{response_hash}), and the tail awaits the store before flushing [DONE] (completions.rs:1960-1998), so the test's fetch-after-stream cannot race.

⚠️

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.

Auto-redacted streams store a provider signature that cannot match client-received bytes

1 participant