Standardize attestation error envelopes#753
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Review: Standardize attestation error envelopesTraced the change end-to-end against Verified correct
Non-blocking notes
✅ Approved — clean, scoped, and well-covered by the added tests. |
PierreLeGuen
left a comment
There was a problem hiding this comment.
Reviewed the attestation error-envelope changes against the linked issue, the route diff, the shared ErrorResponse model, service AttestationError sources, OpenAPI schema registration, docs references, and relevant tests/CI metadata.
No substantive issues found. Local checks run: cargo test -p api routes::attestation::tests and cargo fmt --all -- --check. GitHub reports Security audit, Lint, and Test Suite passing.
Evrard-Nil
left a comment
There was a problem hiding this comment.
🤖 Reviewed independently at f464a01 (built, clippy, unit tests — green). Safe to approve.
- Standardizes both attestation endpoints onto the shared OpenAI-compatible
ErrorResponse { error: { message, type, param, code } }(drops the route-local{error: String}) — now consistent with the rest of cloud-api. - Upstream-auth-masking invariant SAFE: the only backend-wrapping variant,
ProviderError(attestation report path,services/attestation/mod.rs:764), still maps to 503 (attestation.rs:392-397) — a backend 401/403/407 during attestation never surfaces as client 4xx. The signature endpoint can only yieldSignatureNotFound/RepositoryError, so no backend error reaches it. Error message body is byte-identical to pre-PR (e.to_string()) — no new info-leak surface. - The
503→400change for invalid client params (bad nonce / unsupportedsigning_algo, via the new pre-lookupvalidate_signing_algo) is a correctness improvement (stops blaming our backend for client mistakes). - The string→object error-body shape change is intentional/documented (#657) and aligns attestation with every other endpoint; no e2e asserts on the old shape; 4 new unit tests pass; CI green.
- NIT only:
ClientErroris a dead variant (never constructed) — harmless.
LGTM.
Problem
Fixes #657.
GET /v1/signature/{chat_id}returned a non-standard error shape:{ "error": "Signature not found: ..." }Other API errors use the shared envelope with
error.message,error.type,error.param, anderror.code, so clients that normalize errors throughresponse.error.messageneeded a signature-endpoint special case.The issue thread also called out the same local string-only envelope on
GET /v1/attestation/report, plus invalid nonce/signing parameter errors being reported as HTTP 503 instead of client-side 400s.Change
ErrorResponse { error: String }with the sharedcrate::models::ErrorResponseenvelope./v1/signature/{chat_id}errors into standard envelopes:404withtype: not_found_error400withtype: invalid_request_error500withtype: internal_server_errorsigning_algovalidation for/v1/signature/{chat_id}so unsupported algorithms return400before repository lookup./v1/attestation/reportinvalid client parameters to400, including nonce and signing algorithm errors, while preserving503for provider unavailability.signing_algo, and supported algorithms.What Else Is Needed
errorvalues as strings; they should readerror.messageafter this change.Tests
cargo test -p api --lib attestationcargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo test --lib --bins