v1.0 tag-cut: fail-closed brief-block, HMAC body-parse code, loopback warning (PR 4/4)#16
v1.0 tag-cut: fail-closed brief-block, HMAC body-parse code, loopback warning (PR 4/4)#16tachyon-beep wants to merge 1 commit into
Conversation
…rning
Three pre-tag-cut code fixes with tests:
- SEC-01 (crates/clarion-storage/src/query.rs): `entity_briefing_block_reason`
used to return None (= unblocked) when serde_json::from_str on the
`properties` JSON failed. A plugin emitting malformed properties JSON
could silently disable the WP5 briefing block. Now returns
Some("malformed_properties_json") on parse failure; routes to 403
BRIEFING_BLOCKED via the existing call site at http_read.rs:705-712.
- CI-02 (crates/clarion-cli/src/http_read.rs): HMAC middleware body-read
failure used to return ErrorCode::InvalidPath ("request body is
invalid"). Federation clients pattern-matching on `code` would mis-
route a transport/IO failure as a path-validation failure. Switched
to ErrorCode::Internal (500) — the closed error envelope does not
need a new variant; the outer RequestBodyLimitLayer already 413s
oversized client bodies, so reaching this inner path implies a real
transport/IO failure rather than a normal request shape.
- SEC-02 code half (crates/clarion-cli/src/http_read.rs): when both
`auth_token` and `identity_secret` resolve to None and the bind is
loopback, emit a tracing::warn! [TRUST] banner at startup. The
operator-facing trust statement was added in the previous commit
(docs/operator/secret-scanning.md, docs/operator/clarion-http-read-api.md);
this commit adds the visible runtime signal.
Tests added:
- entity_briefing_block_reason_fails_closed_on_malformed_json
- hmac_middleware_body_read_failure_is_not_invalid_path
- spawn_emits_loopback_no_token_trust_warning
Full CI floor green: cargo fmt/clippy/build/nextest (525 passed)/doc/deny;
ruff/ruff format/mypy --strict/pytest (142 passed); sprint_1_walking_skeleton
and wp5_secret_scan e2e.
Closes gap-register items SEC-01, CI-02, and the code half of SEC-02.
There was a problem hiding this comment.
Pull request overview
Pre–v1.0 tag-cut hardening for the HTTP read surface and briefing-block enforcement, with regression tests, to avoid silent secret exposure and improve client error routing.
Changes:
- Fail-closed
entity_briefing_block_reasonwhenpropertiesJSON is malformed (treat as briefing-blocked rather than unblocked). - Return
INTERNAL(500) instead ofINVALID_PATHon HMAC middleware body-read failure, with a targeted regression test. - Emit an operator-visible startup warning when binding to loopback with neither bearer-token nor HMAC identity configured, with a regression test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/clarion-storage/tests/query_helpers.rs | Splits prior tolerant parsing test and adds a fail-closed malformed-JSON regression test for briefing-block extraction. |
| crates/clarion-storage/src/query.rs | Changes briefing-block reason extraction to return a synthetic block reason on JSON parse failure. |
| crates/clarion-cli/src/http_read.rs | Adds loopback-no-auth startup warning, changes HMAC body-read failure mapping to 500/INTERNAL, and adds regression tests for both. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Extract the `briefing_blocked` reason from an entity's `properties` JSON | ||
| /// column. Shared with `clarion-mcp` (which makes the same call inline) so | ||
| /// federation read surfaces enforce the block uniformly. | ||
| pub fn entity_briefing_block_reason(properties_json: &str) -> Option<String> { | ||
| serde_json::from_str::<serde_json::Value>(properties_json) | ||
| .ok()? | ||
| .get("briefing_blocked")? | ||
| .as_str() | ||
| .map(str::to_owned) | ||
| // Fail-closed: malformed properties JSON treated as briefing-blocked to prevent secret exposure. | ||
| let Ok(value) = serde_json::from_str::<serde_json::Value>(properties_json) else { | ||
| return Some("malformed_properties_json".to_owned()); | ||
| }; | ||
| value.get("briefing_blocked")?.as_str().map(str::to_owned) |
| let Ok(body_bytes) = to_bytes(body, HTTP_BODY_LIMIT_BYTES).await else { | ||
| // CI-02 fix: a body read failure here is not a path-validation | ||
| // problem. The outer `RequestBodyLimitLayer` already rejects | ||
| // oversized bodies with the framework's 413; reaching this branch | ||
| // means a transport-layer IO failure or a body that could not be |
| "[TRUST] HTTP API serving on loopback without authentication; any \ | ||
| local process on this host can read the catalogue. Set \ | ||
| identity_token_env or token_env for multi-tenant safety." |
| let probe = TcpListener::bind(("127.0.0.1", 0)).expect("probe bind"); | ||
| let bind: SocketAddr = probe.local_addr().expect("probe local addr"); | ||
| drop(probe); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e5804bb7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return json_error( | ||
| StatusCode::BAD_REQUEST, | ||
| ErrorCode::InvalidPath, | ||
| "request body is invalid", | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| ErrorCode::Internal, | ||
| "request body could not be read", |
There was a problem hiding this comment.
In require_hmac_identity, all to_bytes failures are now forced to 500 INTERNAL, but this branch is also reached for oversized streamed/chunked request bodies on protected routes (where RequestBodyLimitLayer cannot pre-reject via Content-Length). That means a client-side payload-too-large condition is misreported as a server fault, which can trigger incorrect retry behavior and server-error alerting for normal client mistakes. Please special-case length-limit failures here (e.g., 413) instead of classifying every body read error as INTERNAL.
Useful? React with 👍 / 👎.
|
Superseded — the v1.0 tag-cut work in this stacked chain has all landed on |
Summary
Three pre-tag-cut code fixes with regression tests. Single commit, 3 files, +215/-12 lines.
SEC-01 — Fail-closed
entity_briefing_block_reasoncrates/clarion-storage/src/query.rs— whenserde_json::from_stron a row'spropertiesJSON failed, the function returnedNone(= unblocked), silently disabling the WP5 briefing block for malformed-JSON rows. Now returnsSome("malformed_properties_json"), which routes via the existinghttp_read.rs:705-712call site to a 403BRIEFING_BLOCKEDresponse.Test:
entity_briefing_block_reason_fails_closed_on_malformed_json. Existing tolerant test split to preserveNonefor valid-JSON-but-non-object shapes (null,[]).CI-02 — HMAC body-parse error code
crates/clarion-cli/src/http_read.rs:426-431— HMAC middleware body-read failure used to returnErrorCode::InvalidPath("request body is invalid"), which would mis-route a transport/IO failure as a path-validation failure for a federation client switching oncode. Switched toErrorCode::Internal(500). The outerRequestBodyLimitLayer(line 367) already returns 413 for oversized client bodies, so reaching this inner path implies a real transport/IO failure rather than a normal client request shape.Test:
hmac_middleware_body_read_failure_is_not_invalid_path— asserts the code field isINTERNALand explicitly NOTINVALID_PATH.SEC-02 (code half) — Loopback-no-token startup warning
crates/clarion-cli/src/http_read.rs— when bothauth_tokenandidentity_secretresolve toNoneand the bind is loopback, emit atracing::warn![TRUST]banner at startup. The operator-facing trust statement landed in PR 3 (docs/operator/secret-scanning.md,docs/operator/clarion-http-read-api.md); this commit adds the visible runtime signal.Test:
spawn_emits_loopback_no_token_trust_warning.Closes (filigree)
clarion-b10e116c04— SEC-01 fail-closed entity_briefing_block_reasonclarion-7dc9fcc72a— CI-02 federation error code on body parse failureclarion-42f4fee904— SEC-02 (code half — doc half in PR 3)Test plan
Full CI floor green on this branch:
cargo fmt --check/clippy -D warnings/build/nextest run --workspace --all-features(505 passed, 2 skipped) /doc -D warnings/deny checkruff/ruff format --check/mypy --strict/pytest(142 passed)tests/e2e/sprint_1_walking_skeleton.shCARGO_BUILD=0 tests/e2e/wp5_secret_scan.shThe 505 vs RC1's 525 delta is the absence of the dogfood-orientation MCP tool tests (a32c162 + 4dd7b63), which remain on the
RC1branch as post-1.0 work.🤖 Generated with Claude Code