Repository navigation
feat(golang): se_last_error gives Go the full error behind a guest's status number, with a leak test - #1104
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (3)
📒 Files selected for processing (17)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe shared guest ABI now records structured error details for failed exports and exposes them through ChangesGuest error reporting
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant GoHost
participant GuestExport
participant SharedABI
participant LastErrorBuffer
GoHost->>GuestExport: call guest export
GuestExport->>SharedABI: invoke export wrapper
SharedABI->>LastErrorBuffer: clear prior error
SharedABI->>LastErrorBuffer: record details when export fails
GoHost->>SharedABI: call se_last_error after failure
SharedABI->>LastErrorBuffer: transfer registered error buffer
LastErrorBuffer-->>GoHost: return packed error
Merge Risk: ⚪ Minimal · up to Failed calls from the encryption and authentication runtimes now include detailed error information. Existing status numbers are preserved, and the new details avoid quoting sensitive input. No concrete merge-blocking problems were identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new diagnostic channel preserves existing status results and has explicit retrieval and cleanup rules. No introduced security defect was established in the inspected paths. Risk remains low rather than minimal because host-side consumption and complete sensitive-data coverage are not yet verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 72.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 142 functions across 16 files. (1 skipped: 1 unsupported.)
Comment |
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 91 | 0 | 71 | 0 |
Every mutant in the changed lines was caught by a test.
c582978 to
7e4e789
Compare
7e4e789 to
2fc8bd6
Compare
|
Rebased onto #1094 at 6efc22b, which fixes the second cipherstash-bot review there (index-only fields and nil interface passthroughs now round-trip). Two conflicts came up, in |
2ef2e86 to
6b896fe
Compare
6b896fe to
647576f
Compare
647576f to
615570b
Compare
d8eaa93 to
c3fb02e
Compare
c3fb02e to
52f49a1
Compare
3220b74 to
b6a9a08
Compare
…hind a status A failing guest export returns one number from the shared status table, and every Rust error behind it is folded away there: the code, message, help and fields #1099 gave each error never reach Go (#1100). The status number stays exactly as it is, the fast path a host acts on, byte-for-byte the vitaminc guest's. Beside it, the full error is now kept for the host to ask for. `last_error` encodes an error as one value in the transport codec every input and output already uses — `code`, `message`, `help`, `url`, `severity`, `fields` (the error's `payload()`) and `causes`, a list of `{code?, message}` — so there is no second encoding. The encoder takes any miette diagnostic and its fields, so this crate still names no library a guest is built over. A cause from another library contributes only what a describer vouches for (an I/O error's kind, a JSON error's kind and position), never its message. `abi::export` is the one wrapper every guest export runs through: it clears the last error when the export starts, makes sure one is recorded when it fails (a `GuestError` for the status when the export recorded nothing), and packs the result. `se_last_error` returns the error as a buffer in the usual packed format, or zero. The error lives in the buffer registry from the moment it is stored, so it is wiped like every other buffer, by `se_dealloc` once the host has it, and by `wipe_all` at shutdown; handing it over empties the slot. `input()` records which pointer/length check failed. Refs #1100 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
…export, with a leak test The crypto and credential guests now run every export through `stack_guest_abi::abi::export`, so each exports `se_last_error` and every failing export leaves an error for it. Every place that maps an error to a status also records it: `status::fail_error`, `fail_dynamic`, `fail_auth` and `fail_profile` record the stack-encrypt, stack-kms, stack-auth or stack-profile error with its fields and return the status the old mapping gave. Where a guest refuses input before any library sees it (bytes that are not the codec, a malformed options object or config, a call out of order) it records a `GuestError` naming what it refused, and the config error names the key, never its value. The status numbers are unchanged and so are their tests. The credential guest's `status_for_auth` matches stack-auth's variants instead of hand-typed `error_code()` strings, so a renamed code can no longer fall through to "other auth failure"; a test pins that every variant gets the status the string table gave it. The crypto guest no longer copies eql-bindings' JSON parser message into a stored-value refusal: serde_json quotes the input it refused, and the input is stored ciphertext. The leak tests (`tests/leak.rs` in each guest) drive every error path a native test can reach with marker values where a caller's data would be — plaintext, contexts, ciphertext, a client key, an access token or access key, a ZeroKMS response body — and assert no marker appears in any encoded error, at any depth, keys included. Each pins the codes it reached, so a path that stops being driven fails there. Reverting either the context-descriptor rule or the JSON-message rule makes them fail. The credential guest's lockfile moves `vitaminc-aead-value` from 0.5.0 to 0.5.1, the version stack-guest-abi builds against and the crypto guest already uses. Refs #1100 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
…kept list The same change as #1103 makes for the stack crates: ERROR_CODES was read only by its own test. The test now builds every GuestError through a match with no wildcard, so a new variant fails to compile until it has a row, and checks each code is in this crate's namespace and snake_case. Refs #1100 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
b6a9a08 to
fcac251
Compare
| /// What was wrong, naming the key and never its value: the config | ||
| /// carries the client key. An unknown key is not named either, since a | ||
| /// value pasted into the wrong place would arrive as one. |
There was a problem hiding this comment.
This is very clumsily worded. Please rewrite to make clearer.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| "a token whose claims are not the claims", | ||
| failure("claims", || { | ||
| use base64ct::{Base64UrlUnpadded, Encoding}; | ||
| let claims = format!(r#"{{"sub": 7, "workspace": "{ACCESS_TOKEN}"}}"#); |
There was a problem hiding this comment.
This scenario cannot detect the leak that it is for. serde reads sub first and stops at invalid type: integer `7`, expected a string. The marker in workspace is never parsed, so it is never quoted.
I verified this. I changed stack_profile::diagnostic::describe_json_error to return error.to_string() (the raw serde message) and the encrypt leak test still passed. The auth leak test failed on its auth.json case, so the mutant was active.
Fix: put the marker where serde quotes it, for example:
let claims = format!(r#"{{"sub": "x", "iss": "x", "services": "{ACCESS_TOKEN}"}}"#);With this change, the test fails on the mutant (a token whose claims are not the claims.message: leak-marker-access-token) and passes on the PR head. It still reaches stack_auth::invalid_token.
| UnknownKey(String), | ||
| } | ||
|
|
||
| impl ConfigError { |
There was a problem hiding this comment.
The ConfigError doc at line 33-35 is now false. It says the detail "is never surfaced across the boundary (statuses leak no config content)". describe() now sends the detail to the host through se_last_error (malformed(e.describe()) in cipher_init). Please change that doc to say that the key name crosses the boundary and the value does not.
3 participants
Summary
Go's Rust WebAssembly runtimes can now return the full error behind a failed call, including its code, message, help, fields, and causes. Previously, Go received only a status number, losing the details added in #1103. The existing status numbers remain unchanged; the new
se_last_errorexport makes details available for the Go integration in #1110.This is the second of three PRs for #1098. It covers both the encryption runtime and the authentication runtime, which manages login files and access tokens.
Changes
stack-guest-abi, the shared runtime interface. The encoded object containscode,message, optional help and URL, severity, structured fields, and causes. External-library causes contribute approved details, such as an I/O error kind or JSON position, rather than their raw messages.se_last_errortransfers the recorded buffer to the host and empties the slot; normal deallocation or shutdown wipes the buffer.Verification
The existing PR description reports the following checks, using Rust 1.94.1. This description edit did not rerun them or check current CI status.
cargo testforstack-guest-abi(16 passed) and both runtimes' native suites passed, including default, EQL, and deterministic encryption configurations. Existing status tests passed unchanged.-D warnings, and rustdoc with-D warningspassed for the root workspace and applicable runtime feature configurations, on native and WebAssembly targets.scripts/check-wasm-imports.pywith the existing import rules and exportedse_last_error.CGO_ENABLED=0 go test ./...fromlanguages/golangpassed against freshly built authentication and all four encryption runtime variants. Go source is unchanged in this PR.7e4e789; only two base dependency files changed. This is historical status, not a claim about current CI.stack-guest-abi(requires nightly; covered by CI) and credential-dependent live tests.Related
Review notes
se_last_errortransfers ownership once; a second read returns zero. Allocation and deallocation do not clear the slot, so the host can free buffers before reading it.STATUS_AUTH_REFRESH_REQUIREDrecords a generic status error because it tells Go to refresh authentication.vitaminc-aead-valuefrom 0.5.0 to 0.5.1, matching the shared interface and encryption runtime.Summary by CodeRabbit