fix: prevent API key auth bypass via double slashes#588
fix: prevent API key auth bypass via double slashes#588ErikBjare merged 1 commit intoActivityWatch:masterfrom
Conversation
This commit fixes a vulnerability where API key authentication could be bypassed by sending a request with a double slash (e.g., `//api/0/buckets/...`). The `ApiKeyCheck` fairing relied on `request.uri().path().as_str().starts_with("/api/")`, which evaluated to false for paths starting with `//`, causing the middleware to skip the authentication check. Rocket would then internally normalize the path and route the request successfully, completely bypassing the security measure.
The fix normalizes leading slashes in `ApiKeyCheck` before checking the path prefix, ensuring all API endpoints are properly authenticated regardless of duplicate slashes.
Additionally, this commit updates `aw-client-rust` to prevent it from unintentionally generating requests with double slashes. Because `reqwest::Url`'s string representation automatically includes a trailing slash, the `format!` strings have been adjusted from `{}/api/...` to `{}api/...`.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #588 +/- ##
==========================================
- Coverage 70.81% 68.41% -2.41%
==========================================
Files 51 55 +4
Lines 2916 3239 +323
==========================================
+ Hits 2065 2216 +151
- Misses 851 1023 +172 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Currently affects all rust watchers. |
Greptile SummaryThis PR closes an API key authentication bypass where requests with double-slash paths (e.g. Confidence Score: 5/5Safe to merge — the vulnerability fix is correct, all existing tests pass, and the only finding is a minor missing test case. The normalization logic (trim_start_matches('/') + prepend '/') correctly collapses any number of leading slashes. Constant-time comparison is preserved. The aw-client-rust URL changes are consistent across all call sites and correct given reqwest::Url's trailing-slash serialization. The one remaining finding is a P2 suggestion to add a complementary test for //api/0/info remaining public, which does not block merge. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Rocket
participant ApiKeyFairing
participant Handler
Note over Client,Handler: Before fix — bypass path
Client->>Rocket: GET //api/0/buckets/ (no Bearer token)
Rocket->>ApiKeyFairing: on_request()
ApiKeyFairing->>ApiKeyFairing: path = "//api/0/buckets/"
ApiKeyFairing->>ApiKeyFairing: starts_with("/api/") → false ❌ (skips auth)
ApiKeyFairing-->>Rocket: return (no block)
Rocket->>Rocket: normalize path → /api/0/buckets/
Rocket->>Handler: route matched
Handler-->>Client: 200 OK (bypass!)
Note over Client,Handler: After fix — bypass closed
Client->>Rocket: GET //api/0/buckets/ (no Bearer token)
Rocket->>ApiKeyFairing: on_request()
ApiKeyFairing->>ApiKeyFairing: path = "//api/0/buckets/"
ApiKeyFairing->>ApiKeyFairing: normalized = "/api/0/buckets/"
ApiKeyFairing->>ApiKeyFairing: starts_with("/api/") → true ✅
ApiKeyFairing->>ApiKeyFairing: Bearer token missing → invalid
ApiKeyFairing->>Rocket: redirect to /apikey_fairing
Rocket->>Handler: FairingErrorRoute
Handler-->>Client: 401 Unauthorized
Reviews (1): Last reviewed commit: "fix: prevent API key auth bypass via dou..." | Re-trigger Greptile |
| // Double slash should also require auth | ||
| let res = client | ||
| .get("//api/0/buckets/") | ||
| .header(ContentType::JSON) | ||
| .header(Header::new("Host", "localhost:5600")) | ||
| .dispatch(); | ||
| assert_eq!(res.status(), Status::Unauthorized); | ||
| } |
There was a problem hiding this comment.
Missing test:
//api/0/info should remain publicly accessible
The new test verifies that //api/0/buckets/ requires auth after normalization, but doesn't cover the complementary case: //api/0/info with a double-slash should still be allowed through without credentials (public path). Without this test, a future refactor that normalizes the path before the PUBLIC_PATHS check but in the wrong order could silently break the public endpoint for double-slash requests.
// Suggested addition inside test_api_key_required:
let res = client
.get("//api/0/info")
.header(ContentType::JSON)
.header(Header::new("Host", "localhost:5600"))
.dispatch();
assert_eq!(res.status(), Status::Ok);
This commit fixes a vulnerability where API key authentication could be bypassed by sending a request with a double slash (e.g.,
//api/0/buckets/...). TheApiKeyCheckfairing relied onrequest.uri().path().as_str().starts_with("/api/"), which evaluated to false for paths starting with//, causing the middleware to skip the authentication check. Rocket would then internally normalize the path and route the request successfully, completely bypassing the security measure.The fix normalizes leading slashes in
ApiKeyCheckbefore checking the path prefix, ensuring all API endpoints are properly authenticated regardless of duplicate slashes.Additionally, this commit updates
aw-client-rustto prevent it from unintentionally generating requests with double slashes. Becausereqwest::Url's string representation automatically includes a trailing slash, theformat!strings have been adjusted from{}/api/...to{}api/....