Repository navigation
fix(auth): enforce middleware on merged GraphQL and federation routes - #336
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
![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: 6b756cab8e
ℹ️ 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".
| .layer(axum_middleware::from_fn_with_state( | ||
| auth_state, | ||
| auth::auth_middleware, | ||
| )) |
There was a problem hiding this comment.
When AuthConfig.enabled is true, this outer layer rejects legitimate federation protocol requests before their handlers can validate federation credentials: heartbeat accepts the documented X-Federation-PSK header, and registration accepts its PSK in the request body, whereas auth_middleware recognizes only X-API-Key or Bearer credentials. Consequently, peers following the existing federation contract now receive 401 responses unless they are also provisioned with an unrelated general API credential; exempt the PSK-authenticated peer routes or integrate federation identities into this authentication boundary.
Useful? React with 👍 / 👎.
| .layer(axum_middleware::from_fn_with_state( | ||
| auth_state, | ||
| auth::auth_middleware, | ||
| )) |
There was a problem hiding this comment.
When authentication is enabled, a Reader credential can no longer issue the read-only POST /federation/query operation: the newly applied middleware sends this path through required_permission, but is_execute_path does not include /federation/query, so the POST fallback requires Write and returns 403 even though the built-in reader role explicitly has Execute. Add this endpoint to the execute-path classification so read-only clients retain query access.
Useful? React with 👍 / 👎.
| .layer(axum_middleware::from_fn_with_state( | ||
| auth_state, | ||
| auth::auth_middleware, | ||
| )) |
There was a problem hiding this comment.
When authentication is enabled, any Writer can submit the GraphQL updatePlannerConfig mutation because this middleware authorizes only the envelope POST /graphql, which maps to Write. The equivalent REST operation, PUT /planner/config, is explicitly classified as Admin, so routing GraphQL solely through method-based RBAC creates a role bypass for the same configuration change. Inspect the GraphQL operation or enforce permissions in the resolver before allowing this admin mutation.
Useful? React with 👍 / 👎.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1576270e1
ℹ️ 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".
| .layer(axum_middleware::from_fn_with_state( | ||
| auth_state, | ||
| auth::auth_middleware, | ||
| )) |
There was a problem hiding this comment.
When authentication is enabled, any Writer credential can now POST /federation/deregister/{store_id} because the outer middleware classifies this unlisted POST route as Write; the handler then removes the peer without requiring its federation PSK. This lets an ordinary data writer disrupt federation membership, so classify peer deregistration as Admin or enforce an equivalent permission in the handler.
Useful? React with 👍 / 👎.
…tions and v1 consumer contract (#337) Independent code-grounded reconnaissance as of main `5876bc67e8962e3b276fb0161664e0b3a420a64c`. Adds three review documents: - `docs/reviews/2026-10-29-verisimdb-as-built-recon.adoc` — source-derived architecture, conceptual ER, eight-modality/persistence matrix, actual lifecycle, P0/P1/P2 discrepancy register, tests and formal proof obligations, independent Mathesis/QuandleDB/JanusKey/Marches reuse, prioritized engineering programme. - `docs/consumer-interface-contract-v1-observed.adoc` — actual flatten-vs-nested HTTP ABI, operations, HTTP statuses, feature/configuration guarantees and explicit non-guarantees. - `docs/reviews/2026-10-29-hypatia-verified-handover.adoc` — only verified downstream integration facts. No rewrite of the Octad schema; accepted generic term **modality profile** distinguished from **modality set** and **consonance subject**. References separate code fixes #333, #334, #335, #336. Makes no soundness claims beyond observed source/test status. No data mutation.
P0 security boundary
build_routerapplied authentication/RBAC middleware to the REST router before merging separately constructed GraphQL and federation routers. In Axum, a layer on an existing Router does not automatically wrap routes merged afterward. The GraphQL endpoint includes create and delete octad mutations.Repair
auth_middlewareto the outermost combined router after merging GraphQL and federation.Scope and unresolved problems
AuthConfig.enabled = true. The application currently constructsAuthState::default()(authentication disabled), and secure deployment configuration is an independent P0.POST /graphqlas Write even for read-only GraphQL operations. The safe default is restrictive; a follow-up needs operation-aware permission checks.No data migration or destructive operation. Draft pending CI and security review.