[SEC] restrict CORS to authorized extension IDs#581
[SEC] restrict CORS to authorized extension IDs#581RaoufGhrissi wants to merge 1 commit intoActivityWatch:masterfrom
Conversation
325720e to
587bb8c
Compare
Greptile SummaryThis PR tightens the CORS security posture of aw-server-rust by replacing the previous blanket Key changes:
Confidence Score: 4/5Safe to merge with minor follow-up work; the most critical previously-reported bugs (wrong default, |= semantics, wrong regex list for testing mode) are all resolved in this revision. All P0/P1 issues from prior review rounds have been addressed. Remaining findings are P2: the Chrome extension ID is placed in the regex list rather than the exact list (cosmetic), the comma-join serialisation is fragile for regex patterns with commas (edge case, unlikely in practice), and config-file changes after first startup are silently ignored with no log message. None of these block correct operation for the common case. aw-server/src/endpoints/mod.rs — the DB-sync and comma-join serialisation logic warrants a second look before merging. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[build_rocket starts] --> B[Lock datastore Mutex]
B --> C{DB key exists?}
C -- No / First run --> D[Seed DB from config file value]
C -- Yes / Subsequent run --> E[Use DB value, ignore config file]
D --> F[Update in-memory AWConfig]
E --> F
F --> G[cors::cors called with merged AWConfig]
G --> H{cors_allow_aw_chrome_extension?}
H -- true --> I[Push chrome-extension ID to regex list]
H -- false --> J[Skip]
I --> K{cors_allow_all_mozilla_extension?}
J --> K
K -- true --> L[Push moz-extension:.* to regex list]
K -- false --> M[Skip]
L --> N{config.testing?}
M --> N
N -- true --> O[Push localhost:27180 to exact list
Push chrome-extension:.* to regex list]
N -- false --> P[Skip]
O --> Q[AllowedOrigins::some exact + regex]
P --> Q
Q --> R[Rocket server starts]
Reviews (4): Last reviewed commit: "[SEC] restrict CORS to authorized extens..." | Re-trigger Greptile |
aw-server/src/endpoints/cors.rs
Outdated
| let parse_cors_list = |key: &str| -> Vec<String> { | ||
| db.get_key_value(key) | ||
| .map(|s| { | ||
| s.trim_matches('"') | ||
| .split(',') | ||
| .map(|s| s.trim().to_string()) | ||
| .filter(|s| !s.is_empty()) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default() | ||
| }; |
There was a problem hiding this comment.
Fragile JSON string unwrapping via
trim_matches
The settings API stores all values JSON-encoded (see settings.rs line 72: serde_json::to_string(&value.0)). So a string like "moz-extension://abc" is persisted in the database as "\"moz-extension://abc\"".
trim_matches('"') strips surrounding double-quote characters, which works for simple ASCII strings, but is not a correct JSON string deserializer. It will silently produce wrong results for values that contain escaped characters. It also won't handle the case where the user stores a JSON array (e.g., ["ext1", "ext2"]) instead of a comma-separated string.
The idiomatic fix is to use serde_json::from_str::<String>(&s):
let parse_cors_list = |key: &str| -> Vec<String> {
db.get_key_value(key)
.ok()
.and_then(|s| serde_json::from_str::<String>(&s).ok())
.map(|s| {
s.split(',')
.map(|s| s.trim().to_string())
.filter(|s| !s.is_empty())
.collect()
})
.unwrap_or_default()
};
aw-server/src/endpoints/cors.rs
Outdated
| let mut allowed_regex_origins = vec!["chrome-extension://nglaklhklhcoonedhgnpgddginnjdadi".to_string()]; | ||
|
|
||
| let settings_origins = parse_cors_list("settings.cors_origins"); | ||
| drop(db); | ||
|
|
||
| let all_origins = config.cors.iter().cloned().chain(settings_origins); | ||
| for origin in all_origins { | ||
| if origin.starts_with("http://") || origin.starts_with("https://") { | ||
| allowed_exact_origins.push(origin); | ||
| } else if origin.starts_with("chrome-extension://") || origin.starts_with("moz-extension://") { | ||
| allowed_regex_origins.push(origin); | ||
| } else { | ||
| log::warn!("Ignoring invalid CORS origin: '{}'", origin); | ||
| } | ||
| } |
There was a problem hiding this comment.
Firefox extension support is effectively broken for all existing users
The original code included the comment:
Every version of a mozilla extension has its own ID to avoid fingerprinting, so we unfortunately have to allow all extensions to have access to aw-server
This is a fundamental Firefox security property: each installation (and each update) of a Firefox extension gets a new random UUID as its origin (moz-extension://<uuid>). This means:
- There is no single stable ID that a user can add to the allow-list for the ActivityWatch Firefox extension.
- After every extension update, the old ID is invalidated and the user would need to find their new UUID and update the server settings.
- Most users have no easy way to discover their current
moz-extension://UUID.
The PR removes moz-extension://.* without providing any workable alternative for Firefox users. Consider either:
- Keeping
moz-extension://.*as the default (preserving existing behavior) with clear documentation about the risk, or - Using a permanent addon ID via
browser_specific_settings.gecko.idinmanifest.jsonand documenting that users of unofficial builds need the wildcard.
aw-server/src/endpoints/mod.rs
Outdated
| config.address, config.port | ||
| ); | ||
| let cors = cors::cors(&config); | ||
| let cors = cors::cors(&config, &server_state.datastore); |
35a5fe8 to
61a534b
Compare
81acd82 to
b585230
Compare
aw-server/src/config.rs
Outdated
| #[serde(default = "default_true")] | ||
| pub allow_aw_chrome_extension_from_settings: bool, | ||
|
|
||
| #[serde(default = "default_false")] |
There was a problem hiding this comment.
i'm hesitating to set default value to True, so it doesn't impact the actual users
b585230 to
c44c7df
Compare
| fn default_true() -> bool { | ||
| true | ||
| } | ||
|
|
||
| fn default_false() -> bool { | ||
| false | ||
| } |
aw-server/src/config.rs
Outdated
| cors_from_settings: default_cors(), | ||
| cors_regex_from_settings: default_cors(), | ||
| allow_aw_chrome_extension_from_settings: default_true(), | ||
| allow_all_mozilla_extension_from_settings: default_false(), |
There was a problem hiding this comment.
Not a fan of this many additional config keys. Should be prefixed consistently with cors_ such as cors_allow_aw_chrome_extension and cors_allow_all_mozilla_extension.
Curious why the _from_settings is passed along and not resolved/merged into the non-from_settings variants?
There was a problem hiding this comment.
merged with the existing vars, and explained in the readme file
aw-server/src/endpoints/mod.rs
Outdated
| if let Ok(raw) = db.get_key_value("settings.cors") { | ||
| config.cors_from_settings = parse_cors_list(raw); | ||
| } | ||
| if let Ok(raw) = db.get_key_value("settings.cors_regex") { | ||
| config.cors_regex_from_settings = parse_cors_list(raw); | ||
| } | ||
| if let Ok(raw) = db.get_key_value("settings.allow_aw_chrome_extension") { | ||
| config.allow_aw_chrome_extension_from_settings = parse_bool(raw); | ||
| } | ||
| if let Ok(raw) = db.get_key_value("settings.allow_all_mozilla_extension") { | ||
| config.allow_all_mozilla_extension_from_settings = parse_bool(raw); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm a little bit paranoid about letting web UI set CORS like that...
There was a problem hiding this comment.
do you have another suggestion to let non-devs configure cors ?
c44c7df to
4b8e893
Compare
4b8e893 to
102e1f1
Compare
2e6e544 to
e3c3227
Compare
72cdf68 to
51d5a4a
Compare
024a3bf to
8f285a5
Compare
8f285a5 to
823c6fb
Compare
Fixes a security issue where any Firefox extension (moz-extension://.*) could access the ActivityWatch server without any restriction. Previously, the CORS configuration included a wildcard for all Mozilla extensions by default. This commit removes that blanket permission and introduces granular control through both static configuration and the Web UI. We've added 2 new fields to the file configuration (cors_allow_aw_chrome_extension and cors_allow_all_mozilla_extension) and 4 new settings to the Web UI (Fixed origins, Regex origins, and extension-specific shortcuts). The server now merges these settings to determine the final set of authorized origins, ensuring a more secure and flexible configuration. The TOML configuration file values are now used only as an initial seed for the database during the first run. On subsequent runs, any values changed and persisted via the Web UI will take precedence over the config file defaults. Fixed a bug in the web-ui store where changing one setting would cause all other settings to be re-saved with their initial client-side values, unintentionally overwriting database settings with stale defaults. Dependent on: ActivityWatch/aw-webui#795
823c6fb to
907e640
Compare
[SEC] restrict CORS to authorized extension IDs
Fixes a security issue where any Firefox extension (moz-extension://.*) could access the ActivityWatch server without any restriction.
Previously, the CORS configuration included a wildcard for all Mozilla extensions by default. This commit removes that blanket permission and introduces granular control through both static configuration and the Web UI.
We've added 2 new fields to the file configuration (allow_aw_chrome_extension and allow_all_mozilla_extension) and 4 new settings to the Web UI (Fixed origins, Regex origins, and extension-specific shortcuts). The server now merges these settings to determine the final set of authorized origins, ensuring a more secure and flexible configuration.
Dependent on: ActivityWatch/aw-webui#795