Conversation
The ApiKeyCheck fairing compared the raw, still-percent-encoded request path against "/api/", while Rocket's router matches on the percent-decoded segment view (rocket::router::collider::paths_match uses req.uri().path().segments()). A request to /%61pi/0/buckets/ therefore failed the fairing's prefix check — skipping auth entirely — and was still dispatched by the router to the real /api/0/buckets/ handler. Full read and write bypass with a single curl and no credentials. This is the same underlying desync as the //api/... bypass fixed in ActivityWatch#588: gate and router disagreed on what the path is. ActivityWatch#588 patched one spelling (leading slashes); this patches the class, by matching on exactly the segment view the router dispatches on. Empty segments are skipped and percent-decoding is applied by the same code path, so double slashes, encoded characters, and double encoding all resolve identically on both sides. Public-path exemptions move to decoded segment slices for the same reason. Reported by Judel <security@pal0x.dev>.
|
Verified end-to-end over real HTTP (not just the in-process test client), both directions, against a server started with Unpatched master ( Bucket listing afterwards confirms the write landed with no credentials: {"authbypass-poc":{"id":"authbypass-poc","type":"test","client":"authbypass-poc",...}}This branch: Bucket listing afterwards: Worth noting the last line: pre-fix, |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThe API-key fairing now uses Rocket’s decoded path-segment representation for authorization decisions.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Incoming request] --> B[Decode path into Rocket segments]
B --> C{First segment is api?}
C -->|No| D[Continue without API-key gate]
C -->|Yes| E{Exact public path?}
E -->|Yes| D
E -->|No| F{Valid bearer API key?}
F -->|Yes| G[Dispatch API handler]
F -->|No| H[Return 401 Unauthorized]
Reviews (2): Last reviewed commit: "test(auth): assert exact 401 on encoded-..." | Re-trigger Greptile |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #636 +/- ##
==========================================
+ Coverage 70.81% 75.77% +4.96%
==========================================
Files 51 62 +11
Lines 2916 5041 +2125
==========================================
+ Hits 2065 3820 +1755
- Misses 851 1221 +370 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile P2: the encoded-path tests asserted "not 200", which a 404 also satisfies — a route miss would have passed as an auth success. - Assert Status::Unauthorized specifically for paths that decode to real /api/ routes. - Add a positive control: the same encoded paths return 200 with a valid key, proving the 401s come from the gate rather than the router failing to match. - /%41PI/ decodes to /API/, which is not a registered route; split it out with an explicit 404 assertion so a future case-insensitive routing change surfaces here instead of silently passing. - Tighten the post-write bucket check and the public-path test to exact statuses for the same reason. Re-verified all three regression tests still fail against the pre-fix gate.
|
Good catch on the P2 — fixed in 2003e5a.
Also tightened the post-write bucket lookup and the public-path test to exact statuses for the same reason. Re-verified all three regression tests still fail against the pre-fix gate. |
|
@greptileai review |
|
CI-green and mergeable (Greptile 5/5) — waiting only on a maintainer click. This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted. |
…ts (#637) * fix(cors): scope the moz-extension wildcard to aw-watcher-web endpoints Both servers trust every Firefox extension origin unconditionally (`moz-extension://.*`), because extension IDs are random per install and cannot be allowlisted up front. The consequence is that any installed extension — with no host permission, so no install-time prompt naming ActivityWatch — can read `/api/0/export` and write events. Deleting the wildcard does not work: host permissions do NOT exempt a Firefox background script from CORS. Verified in Firefox 152 with permissions granted at runtime and an ACAO-only A/B against a mock server; the server's Access-Control-Allow-Origin header is the sole gate. Removing it silently breaks aw-watcher-web. So narrow it instead. Wildcard-matched extension origins now reach only the three endpoints aw-watcher-web actually uses: GET /api/0/info POST /api/0/buckets/<id> POST /api/0/buckets/<id>/heartbeat Everything else — export, import, query, settings, event reads, bucket deletion — is closed to them. That removes the exfiltration half of the problem (app names, window titles, tab URLs, AFK data) with no client change. Origins the user allowlists explicitly via `cors_regex` keep full access. Enforcement is two-stage, because CORS alone only hides responses: blocked requests are rejected with 403 before reaching a handler (a "simple" POST is sent by the browser regardless and would otherwise still execute), and the Access-Control-* headers are stripped so preflights fail. Path matching uses the decoded segment view the router dispatches on, like the #636 fix — a raw-string check would let /%61pi/0/export desynchronize this gate from the handler it protects. Verified in Firefox 152 with an MV2 extension declaring only `permissions: ["storage"]` (host_permissions=[]), against two instances of this build — one with the wildcard opt-in (pre-fix behaviour), one scoped: unscoped.export=200 scoped.export=CORS_ERR unscoped.import=200 scoped.import=CORS_ERR unscoped.ensureBucket=200 scoped.ensureBucket=200 unscoped.heartbeat=200 scoped.heartbeat=200 Reported privately by Judel (security@pal0x.dev). * fix(cors): restrict extension heartbeats to web buckets * docs(cors): disclose residual web-bucket poisoning
2 participants
Summary
The
api_keyauth gate (added in #585, closing #494) can be bypassed completely — read and write — with a singlecurland no credentials:Root cause
ApiKeyCheck::on_requestcompared the raw, still-percent-encoded path:Rocket's router matches on the decoded segment view instead —
rocket::router::collider::paths_matchusesreq.uri().path().segments(), which percent-decodes and skips empty segments. So the gate and the router disagreed about what the path is: the gate saw/%61pi/0/buckets/(not an API path, skip auth), the router sawapi / 0 / bucketsand dispatched to the real handler.This is the same desync as the
//api/...bypass fixed in #588. That fix patched one spelling of the disagreement (leading slashes) while leaving the disagreement itself in place — which is why a second variant surfaced. Reasoning about encodings one at a time invites a third (double encoding, unicode tricks, ...).Fix
Match on exactly the view the router dispatches on:
Percent-decoding and empty-segment skipping now happen in the same code path for gate and router, so double slashes, encoded characters, and double encoding all resolve identically on both sides. This subsumes the #588 fix rather than layering on it (its regression test is kept and still passes). Public-path exemptions move to decoded segment slices for the same reason.
Tests
Three regression tests added. All three fail against the pre-fix gate and pass after — verified by reverting the gate logic locally and re-running:
Coverage: encoded first char, fully encoded segment, mid-segment encoding, encoding combined with the #588 double-slash trick, POST/write bypass (plus confirming the bucket was not created), and decoded-segment parity for the
/api/0/infopublic exemption.cargo test -p aw-servergreen (32 tests),cargo fmt --checkclean, no new clippy warnings.Impact
api_keywas added specifically so other local processes/users can't reach the API (refs ActivityWatch/activitywatch#32, ActivityWatch/activitywatch#1199). This bug defeated exactly that threat model, so I'd agree with the reporter's critical rating — full read/write with no tooling and no credentials.Reported by pal0x, who also flagged that the right fix here is structural rather than per-encoding. Agreed, and that's what this does.