Security: use a high-entropy random key for the security-mode disable URL#1382
Security: use a high-entropy random key for the security-mode disable URL#1382vuckro wants to merge 3 commits into
Conversation
The unauthenticated ?wu_secure=KEY query string that turns the network-wide recovery "security mode" off used substr(md5(admin_email), 0, 6) as the key — only ~24 bits and derived from a commonly public/guessable value, so an attacker could compute or brute-force it and remotely disable the admin's safe-mode lockdown. Generate a 128-bit random key once (random_bytes, since this runs from sunrise before pluggable.php) and store it as a network option, and compare it with hash_equals(). The key is already displayed on the settings screen, so the documented "copy this URL to disable security mode" workflow is unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSecurity-mode authentication now uses a persisted 16-byte random hex secret (with a legacy fallback) and validates incoming ChangesSecurity mode authentication hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inc/class-sunrise.php`:
- Line 338: The code currently casts wu_get_isset($_GET, 'wu_secure') to string
and passes it to hash_equals, which can trigger "Array to string conversion" for
inputs like ?wu_secure[]=x; update the check in the sunrise bootstrap (around
the hash_equals call) to first retrieve the value via wu_get_isset($_GET,
'wu_secure'), verify it is a string (e.g. is_string(...) or ensure it's scalar
and specifically a string), and only then call
hash_equals(wu_get_security_mode_key(), $value). If the value is not a string,
skip the hash_equals branch (treat as non-matching) to avoid warnings and
potential log spam. Ensure you reference the wu_get_isset call, the 'wu_secure'
GET key, and the hash_equals/wu_get_security_mode_key invocation when
implementing the guard.
In `@inc/functions/sunrise.php`:
- Around line 91-96: The code in Sunrise::load() currently auto-generates and
persists a new wu_security_mode_key on first read (using get_network_option,
random_bytes, bin2hex, update_network_option), which silently invalidates
pre-issued recovery URLs; instead, stop creating a new random key during
ordinary reads — if wu_security_mode_key is missing, temporarily accept the
legacy/derived key (compute the legacy value the same way the Settings screen
previously derived it) until an admin explicitly sets or a migration persists a
new random key; move the update_network_option(random key) call out of
Sunrise::load() into an explicit migration/upgrade routine or the settings save
path so the secret is only rotated when persisted intentionally, and ensure
load() checks both the persisted option and the legacy-derived key for
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5778e68c-3587-413b-b8e2-f685b86e88c3
📒 Files selected for processing (2)
inc/class-sunrise.phpinc/functions/sunrise.php
|
CLAIM_RELEASED reason=worker_complete runner=superdav42 ts=2026-06-10T13:55:22Z aidevops_version=3.20.46 opencode_version=1.16.2 |
|
Permission check failed for this PR (HTTP unknown from collaborator permission API). Unable to determine if @vuckro is a maintainer or external contributor. A maintainer must review and merge this PR manually. This is a fail-closed safety measure — the pulse will not auto-merge until the permission API succeeds. aidevops.sh v3.20.52 plugin for OpenCode v1.17.3 with gpt-5.5 spent 2m and 47,696 tokens on this as a headless worker. |
|
Implemented a compatible follow-up branch/PR for the failing checks: #1413. Evidence:
Verification run locally on the follow-up branch:
Blocked from updating this fork PR branch directly, so #1413 is the upstream-compatible replacement/follow-up for the same change set plus the CI failure fix. |
* fix(security): use a high-entropy random key for security-mode disable The unauthenticated ?wu_secure=KEY query string that turns the network-wide recovery "security mode" off used substr(md5(admin_email), 0, 6) as the key — only ~24 bits and derived from a commonly public/guessable value, so an attacker could compute or brute-force it and remotely disable the admin's safe-mode lockdown. Generate a 128-bit random key once (random_bytes, since this runs from sunrise before pluggable.php) and store it as a network option, and compare it with hash_equals(). The key is already displayed on the settings screen, so the documented "copy this URL to disable security mode" workflow is unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(security): preserve legacy security mode recovery URL * ci: guard e2e cleanup before checkout * fix: address security mode review feedback --------- Co-authored-by: vuckro <maribel_waters@howtocore.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Rebase needed — PR has merge conflicts and no
|
2 participants
Summary
The unauthenticated
?wu_secure=KEYquery string that turns the network-widerecovery security mode off used
substr(md5(admin_email), 0, 6)as the key— only ~24 bits and derived from a commonly public/guessable value. An attacker
could compute or brute-force it and remotely disable the admin's safe-mode
lockdown (re-enabling all other plugins).
Changes
random_bytes— this runs from sunrise,before
pluggable.php) and store it as a network option.hash_equals().The key is already displayed on the Settings screen ("copy this URL to disable
security mode"), so the documented recovery workflow is unaffected — the
settings note now shows the new random URL.
Summary by CodeRabbit