-
Notifications
You must be signed in to change notification settings - Fork 1
fix: default UNIFIEDPUSH_ENABLED to false #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,8 +102,12 @@ impl Config { | |
| fcm_enabled: env::var("FCM_ENABLED") | ||
| .unwrap_or_else(|_| "true".to_string()) | ||
| .parse()?, | ||
| // Default false: the UnifiedPush dispatch path POSTs to the | ||
| // client-supplied device token treated as a URL, so enabling | ||
| // the backend by omission opens an SSRF surface. Operators | ||
| // opt in explicitly. | ||
| unifiedpush_enabled: env::var("UNIFIEDPUSH_ENABLED") | ||
| .unwrap_or_else(|_| "true".to_string()) | ||
| .unwrap_or_else(|_| "false".to_string()) | ||
| .parse()?, | ||
| batch_delay_ms: env::var("BATCH_DELAY_MS") | ||
| .unwrap_or_else(|_| "5000".to_string()) | ||
|
|
@@ -246,4 +250,63 @@ mod tests { | |
| msg | ||
| ); | ||
| } | ||
|
|
||
| /// UnifiedPush must be opt-in. Its dispatch path POSTs to the | ||
| /// client-supplied device token treated as a URL, so a deployment that | ||
| /// simply forgets the variable must not end up with the backend live. | ||
| #[test] | ||
| fn unifiedpush_defaults_to_disabled() { | ||
| let _guard = ENV_MUTEX.lock().unwrap(); | ||
| std::env::remove_var("UNIFIEDPUSH_ENABLED"); | ||
| std::env::set_var("NOSTR_RELAYS", "wss://relay.example.com"); | ||
|
|
||
| let result = Config::from_env(); | ||
|
|
||
| std::env::remove_var("NOSTR_RELAYS"); | ||
|
Comment on lines
+260
to
+265
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Restore the prior environment values after each test. These tests remove or overwrite values supplied by the test process, then leave them unset. A later test can observe unintended defaults when the suite runs with
📍 Affects 1 file
🤖 Prompt for AI Agents |
||
|
|
||
| let config = result.expect("Config::from_env MUST succeed on defaults"); | ||
| assert!( | ||
| !config.push.unifiedpush_enabled, | ||
| "UNIFIEDPUSH_ENABLED MUST default to false" | ||
| ); | ||
| } | ||
|
|
||
| /// The opt-in still works: setting the variable explicitly enables it. | ||
| #[test] | ||
| fn unifiedpush_honours_explicit_opt_in() { | ||
| let _guard = ENV_MUTEX.lock().unwrap(); | ||
| std::env::set_var("UNIFIEDPUSH_ENABLED", "true"); | ||
| std::env::set_var("NOSTR_RELAYS", "wss://relay.example.com"); | ||
|
|
||
| let result = Config::from_env(); | ||
|
|
||
| std::env::remove_var("UNIFIEDPUSH_ENABLED"); | ||
| std::env::remove_var("NOSTR_RELAYS"); | ||
|
|
||
| let config = result.expect("Config::from_env MUST succeed on explicit opt-in"); | ||
| assert!( | ||
| config.push.unifiedpush_enabled, | ||
| "UNIFIEDPUSH_ENABLED=true MUST enable the backend" | ||
| ); | ||
| } | ||
|
|
||
| /// FCM keeps its permissive default: it does not take a client-supplied | ||
| /// URL, so the fail-open concern that motivates the UnifiedPush default | ||
| /// does not apply, and flipping it would change existing deployments. | ||
| #[test] | ||
| fn fcm_default_is_unchanged() { | ||
| let _guard = ENV_MUTEX.lock().unwrap(); | ||
| std::env::remove_var("FCM_ENABLED"); | ||
| std::env::set_var("NOSTR_RELAYS", "wss://relay.example.com"); | ||
|
|
||
| let result = Config::from_env(); | ||
|
|
||
| std::env::remove_var("NOSTR_RELAYS"); | ||
|
|
||
| let config = result.expect("Config::from_env MUST succeed on defaults"); | ||
| assert!( | ||
| config.push.fcm_enabled, | ||
| "FCM_ENABLED MUST keep defaulting to true" | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This opt-in default does not protect users following the documented Docker setup:
README.mdanddocs/deployment.mddirect them to rundocker-compose up, whiledocker-compose.yml:13explicitly setsUNIFIEDPUSH_ENABLED=true, overriding both this template and the new Rust default. Consequently, the reference Docker deployment still enables the client-controlled URL dispatch—and its SSRF surface—without an operator opting in; update the Compose environment tofalseor remove the override.Useful? React with 👍 / 👎.