Repository navigation
fix(server): reject incomplete Basic client authentication - #132
Open
cariandrum22 wants to merge 1 commit into
Open
cariandrum22 wants to merge 1 commit into
cariandrum22 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The bounded authentication change is consistently applied and covered by comprehensive endpoint, precedence, and state-preservation tests.
Review effort: Balanced
Findings: None
What changed in this PR
Rejects incomplete Basic authentication across OAuth client endpoints, preventing fallback to other credential methods or public-client handling.
Changes:
- Recognizes bare or undecodable Basic credentials as authentication attempts.
- Preserves authentication precedence for PAR and introspection.
- Adds five-route regression, ordering, and state-preservation tests.
| File | Description |
|---|---|
spec/compliance-matrix.yaml |
Records incomplete Basic handling compliance. |
docs/operations/oauth-conformance.md |
Documents behavior and upgrade impact. |
crates/server/src/client_registry/registry_store/authentication.rs |
Detects incomplete Basic attempts. |
crates/server/src/client_registry/registry_store.rs |
Adds snapshot-failure test support. |
crates/server/src/web/token_endpoint/client_auth.rs |
Uses expanded Basic detection. |
crates/server/src/web/token_lifecycle/client_auth.rs |
Rejects malformed Basic before snapshots. |
crates/server/src/web/par_endpoint/client_auth.rs |
Rejects malformed Basic before identity checks. |
crates/server/src/web/assertion_client/tests/mod.rs |
Registers the new test module. |
crates/server/src/web/assertion_client/tests/incomplete_basic.rs |
Tests endpoint behavior and controls. |
crates/server/src/web/assertion_client/tests/incomplete_basic/boundaries.rs |
Tests admission and error precedence. |
crates/server/src/web/assertion_client/tests/incomplete_basic/effects.rs |
Tests grant and token preservation. |
crates/server/src/web/assertion_client/tests/client_auth_errors.rs |
Shares authentication test helpers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Summary
Treat a supplied Basic scheme as client authentication even when its payload is missing or undecodable. Across token, PAR, device authorization, introspection and revocation, it participates in existing multiple-method rejection and cannot fall through to body credentials or public-client handling. Failed lone Basic returns 401 invalid_client with the existing Basic challenge.
Validation
37 native tests passed: five-route PostgreSQL/Redis fixtures, assertion retry/replay, stored-value and live grant/token preservation, admission ordering, existing assertion authentication/form/grant regressions, PAR shared-store behavior and OAuth conformance/error encoding. Formatting, Clippy, matrix/runtime integrity and hooks passed.
Commands:
nix develop -c cargo test --locked -p aegaeon-server --lib --no-run; the resulting libtest executable ranweb::assertion_client_tests::,web::par_endpoint::authentication_tests::,web::oauth_conformance_tests::andweb::oauth_error_encoding_tests::with--include-ignored --test-threads=1. Also rancargo fmt --all -- --check, Clippy for server/JOSE/FFI with all targets and-D warnings, server lint inventory, runtime drift, matrix validation, strict verified-reference validation and pre-commit checks.Notes
Stacked on #127.
RFC 6749 sections 2.3, 2.3.1 and 5.2; RFC 7521 section 4.2.1. Pure Basic decode rejection now precedes missing outer client_id at plain PAR and registration snapshot errors at introspection. Earlier transport/URI/form admission and public payload-presence helper semantics remain. Operators should remove unintended incomplete Basic headers when using another registered authentication method. Other Authorization scheme/envelope classification and Basic component decoding remain outside this bounded change.