Internal refactors: error-leak scrubbing, session accessors, OCSF drift fix - #30
Open
bryanegan wants to merge 2 commits into
Open
Internal refactors: error-leak scrubbing, session accessors, OCSF drift fix#30bryanegan wants to merge 2 commits into
bryanegan wants to merge 2 commits into
Conversation
…aram cleanup Deferred design-review improvements. All internal — no wire/behavior change except the deliberate error-body tightening. - #33 AppError leaks: Internal and TokenExchangeFailed now log their detail via tracing::error! and return a fixed generic body; the raw text could carry pool ids, client ids, ARNs. RefreshFailed deliberately KEEPS its message (it describes the caller's own token state and auth.js surfaces it) — the contract is now explicit in comments rather than accidental. Also rewrote error.rs tests to exercise the REAL into_response (they had re-implemented the mapping, so the scrubbing would not have been caught) and added two leak-assertion tests. - #35 Session accessors: SessionHandle gains tokens()/tokens_opt()/set_tokens()/ destroy(), replacing the same 5-line stringly-typed incantation pasted across 6 handlers + 5 duplicated destroy sites in callback.rs. A malformed `tokens` value now logs WARN (corrupt session) instead of being silently indistinct from logged-out. - #38 Dead param: removed the ignored `_context` from CedarState::authorize; documented on AuthorizeRequest.context why the wire field is accepted-but- ignored (S5) rather than leaving it unexplained. - #49 Vestigial generic: SessionLayer<B> was only ever SessionLayer<AnyBackend>; concretized to Arc<AnyBackend> and dropped the generic from session_middleware. 185 Rust tests pass (was 179), clippy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The design review flagged the 8-positional-arg OCSF API as drift-inviting and noted the drift had already happened. Confirmed: logout.rs carried an inline copy of the auth-protocol mapping that reported `auth_method: "password"` as AUTH_PROTOCOL_OAUTH2 / "OAuth 2.0/OIDC" — wrong data in every logoff event for password users, silently, in whatever SIEM consumes these. - logout.rs now calls the shared ocsf::auth_protocol_from_method() (which was already correct and already used by session.rs). - ocsf.rs had NO tests at all; added a module pinning the (id, name) pairs as a wire contract, a regression guard asserting password != OAuth2, severity/status name coverage, and an emit()-never-panics check. Chose the shared-helper fix over the review's enum/struct redesign: it removes the drift at its source with no call-site churn across 20+ event emissions. The enum refactor stays available if the arg list grows again. 189 Rust tests pass, clippy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deferred design-review improvements. All internal — no wire or behavior change except two deliberate tightenings (noted below).
Security / correctness
InternalandTokenExchangeFailednow log detail viatracing::error!and return a fixed generic body; the raw text could carry pool ids, client ids, ARNs.RefreshFaileddeliberately keeps its message (describes the caller's own token state;auth.jssurfaces it) — the contract is now explicit in comments instead of accidental. This is a response-body change for two 500-class errors.logout.rshad an inline copy of the auth-protocol mapping that reportedauth_method: "password"as OAuth 2.0/OIDC. Every logoff event for password users has been carrying wrong data into whatever SIEM consumes it. Now uses the shared (correct) helper.Test-quality
error.rstests re-implemented the status/body mapping — so the leak-scrubbing above would not have been caught. Rewritten to exercise the realinto_response, plus two leak-assertion tests.ocsf.rshad no tests at all. Added a module pinning the (id, name) pairs as a wire contract + a regression guard for the exact drift.Cleanup (pure internal)
SessionHandlegainstokens()/tokens_opt()/set_tokens()/destroy(), replacing the same stringly-typed 5-line block pasted across 6 handlers and 5 duplicated destroy sites. A malformedtokensvalue now logs WARN (corrupt session) instead of being silently indistinguishable from logged-out._contextparam fromCedarState::authorize; documented on the wire type whycontextis accepted-but-ignored (S5).SessionLayer<B>was only everSessionLayer<AnyBackend>— concretized.189 Rust tests (was 179), clippy clean.
🤖 Generated with Claude Code