feat(server): add opt-in API key authentication via Bearer token#585
Conversation
When api_key is set in config.toml, all API endpoints except /api/0/info require an Authorization: Bearer <key> header. Requests without a valid key receive 401 Unauthorized. Implemented as a Rocket Fairing (same pattern as HostCheck) so it applies globally without modifying individual endpoints. By default api_key is unset, meaning auth is disabled — existing setups are unaffected. Closes ActivityWatch#494 Refs ActivityWatch/activitywatch#32, ActivityWatch/activitywatch#1199
Greptile SummaryThis PR adds opt-in API key authentication to Previously-reported issues are all addressed in this revision: Confidence Score: 5/5Safe to merge — all previously-reported P0/P1 issues are addressed; only P2 suggestions remain. All critical prior findings (timing attack, empty-key bypass, web UI blocked) are resolved. The two remaining observations are both P2: a missing test for static-path pass-through and the config template discoverability gap. Neither blocks correctness or security of the feature. aw-server/src/endpoints/apikey.rs (missing static-path test), aw-server/src/config.rs (api_key omitted from generated config template) Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant ApiKeyCheck (Fairing)
participant Router
participant Handler
Note over ApiKeyCheck (Fairing): on_request fires before routing
Client->>ApiKeyCheck (Fairing): OPTIONS /api/0/buckets
ApiKeyCheck (Fairing)-->>Router: pass through (CORS preflight)
Router->>Handler: OPTIONS catch-all
Handler-->>Client: 200 OK
Client->>ApiKeyCheck (Fairing): GET / (static asset)
ApiKeyCheck (Fairing)-->>Router: pass through (not /api/)
Router->>Handler: root_index
Handler-->>Client: 200 OK (web UI)
Client->>ApiKeyCheck (Fairing): GET /api/0/info
ApiKeyCheck (Fairing)-->>Router: pass through (PUBLIC_PATHS)
Router->>Handler: server_info
Handler-->>Client: 200 OK
Client->>ApiKeyCheck (Fairing): GET /api/0/buckets (no token)
ApiKeyCheck (Fairing)->>ApiKeyCheck (Fairing): redirect_unauthorized → /apikey_fairing
ApiKeyCheck (Fairing)->>Router: GET /apikey_fairing
Router->>Handler: FairingErrorRoute
Handler-->>Client: 401 Unauthorized
Client->>ApiKeyCheck (Fairing): GET /api/0/buckets (Bearer valid)
ApiKeyCheck (Fairing)->>ApiKeyCheck (Fairing): ct_eq → match
ApiKeyCheck (Fairing)-->>Router: pass through
Router->>Handler: buckets_get
Handler-->>Client: 200 OK
Reviews (3): Last reviewed commit: "fix(server): scope API key auth to /api/..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let valid = match auth_header { | ||
| Some(value) => { | ||
| if let Some(token) = value.strip_prefix("Bearer ") { | ||
| token == api_key | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
| None => false, | ||
| }; |
There was a problem hiding this comment.
The token == api_key comparison on line 116 short-circuits on the first mismatched byte, leaking timing information that a network-adjacent attacker can use as an oracle to guess the key character by character. This is especially relevant for the stated use case of non-localhost network exposure. Consider using the subtle crate's ConstantTimeEq or comparing HMAC digests of both values.
| let valid = match auth_header { | |
| Some(value) => { | |
| if let Some(token) = value.strip_prefix("Bearer ") { | |
| token == api_key | |
| } else { | |
| false | |
| } | |
| } | |
| None => false, | |
| }; | |
| let valid = match auth_header { | |
| Some(value) => { | |
| if let Some(token) = value.strip_prefix("Bearer ") { | |
| // Use constant-time comparison to prevent timing oracle attacks | |
| token.len() == api_key.len() | |
| && token | |
| .bytes() | |
| .zip(api_key.bytes()) | |
| .fold(0u8, |acc, (a, b)| acc | (a ^ b)) | |
| == 0 | |
| } else { | |
| false | |
| } | |
| } | |
| None => false, | |
| }; |
| async fn on_request(&self, request: &mut Request<'_>, _: &mut Data<'_>) { | ||
| let api_key = match &self.api_key { | ||
| None => return, // auth disabled | ||
| Some(k) => k, | ||
| }; |
There was a problem hiding this comment.
If a user sets api_key = "" in config, the guard accepts any request that sends Authorization: Bearer (Bearer followed by an empty token), since "Bearer ".strip_prefix("Bearer ") yields Some("") and "" == "" is true. Filtering this out at the point the key is read keeps the authentication logic clean.
| async fn on_request(&self, request: &mut Request<'_>, _: &mut Data<'_>) { | |
| let api_key = match &self.api_key { | |
| None => return, // auth disabled | |
| Some(k) => k, | |
| }; | |
| let api_key = match &self.api_key { | |
| None => return, // auth disabled | |
| Some(k) if k.is_empty() => { | |
| warn!("api_key is configured but empty — authentication is effectively disabled"); | |
| return; | |
| } | |
| Some(k) => k, | |
| }; |
| ) -> rocket::route::Outcome<'r> { | ||
| let err = HttpErrorJson::new( | ||
| Status::Unauthorized, | ||
| "Missing or invalid API key. Set 'Authorization: Bearer <key>' header.".to_string(), | ||
| ); | ||
| Outcome::from(request, err) | ||
| } |
There was a problem hiding this comment.
WWW-Authenticate response header
RFC 7235 §4.1 requires that a 401 response include at least one WWW-Authenticate challenge so HTTP clients can discover the supported scheme automatically. Without it, generic HTTP clients have no machine-readable signal to prompt for credentials.
(This requires a small refactor of FairingErrorRoute::handle to build a Response manually and append .raw_header("WWW-Authenticate", "Bearer realm=\"aw-server\"").)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #585 +/- ##
==========================================
- Coverage 70.81% 68.39% -2.42%
==========================================
Files 51 55 +4
Lines 2916 3237 +321
==========================================
+ Hits 2065 2214 +149
- Misses 851 1023 +172 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…empty-key guard, fmt
|
Addressed both security findings from the Greptile review (commit 7266413):
Also fixed the All 6 apikey unit tests pass. |
|
@greptileai review |
| /// Paths that are always accessible without authentication. | ||
| const PUBLIC_PATHS: &[&str] = &["/api/0/info"]; |
There was a problem hiding this comment.
PUBLIC_PATHS only exempts /api/0/info, so all static web UI routes (/, /css/, /js/, /fonts/, etc.) are gated behind Bearer auth. A browser cannot send Authorization: Bearer headers for ordinary page navigation, so the ActivityWatch web UI becomes completely inaccessible as soon as a user sets api_key. This breaks the primary user interface for anyone enabling this feature on a desktop install.
Restricting the check to the /api/ subtree — keeping static asset routes public — would match the stated intent of "protecting the API":
| /// Paths that are always accessible without authentication. | |
| const PUBLIC_PATHS: &[&str] = &["/api/0/info"]; | |
| /// Paths that are always accessible without authentication. | |
| const PUBLIC_PATHS: &[&str] = &["/api/0/info"]; | |
| /// Only enforce auth for paths under this prefix. | |
| const PROTECTED_PREFIX: &str = "/api/"; |
Then in on_request, replace the PUBLIC_PATHS check with:
// Only enforce auth on API routes; static web-UI assets are always public.
if !request.uri().path().starts_with(PROTECTED_PREFIX) {
return;
}
// Within the API, /api/0/info is always public.
if PUBLIC_PATHS.contains(&request.uri().path().as_str()) {
return;
}
|
In |
|
@greptileai review |
|
@TimeToBuildBob Did you see #585 (comment), how is the config structure looking? |
|
@TimeToBuildBob Also, I kinda thought there were already some kind of API key loading and generation in aw-server-rust?? Or is this just an old/stale PR? |
|
@ErikBjare Answering both questions: 1. Is this PR stale / does aw-server-rust already have API key code? You may be thinking of one of:
2. Config structure (per your 11:04 comment) api_key = "your-secret-key-here"
cors = []i.e. it ships flat alongside I'll push a follow-up that nests it under a heading. The natural shape, given the existing flat fields, would be address = "127.0.0.1"
port = 5600
[auth]
api_key = "your-secret-key-here"Or if you'd prefer, |
|
|
|
@TimeToBuildBob You seem to not be very responsive on these repos... I need to explicitly tag you. |
Move api_key from the top-level AWConfig into a new AWAuthConfig
sub-struct, which serialises as an [auth] table in config.toml:
[auth]
api_key = "your-secret-key-here"
This keeps top-level config.toml keys clean and gives auth settings
a dedicated, extensible namespace.
All 6 apikey unit tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@ErikBjare Done — pushed Config now looks like: address = "127.0.0.1"
port = 5600
[auth]
api_key = "your-secret-key-here"All 6 unit tests still pass. Existing configs without an |
2 participants
Summary
Adds opt-in API key authentication to aw-server-rust. When
api_keyis set inconfig.toml, all API endpoints except/api/0/inforequire anAuthorization: Bearer <key>header.Implementation: Rocket Fairing (same pattern as the existing
HostCheckfairing) — applies globally without modifying individual endpoints.Config (
~/.config/activitywatch/aw-server-rust/config.toml):Leave unset (default) to disable authentication — existing setups are unaffected.
Behavior
GET /api/0/info— always public (health check / version endpoint)OPTIONSpreflight requests — always pass through (CORS)Authorization: Bearer <api_key>Tests
5 new unit tests covering:
Notes
Closes #494
Refs ActivityWatch/activitywatch#32, ActivityWatch/activitywatch#1199