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 2 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>
|
Warning Review limit reached
More reviews will be available in 14 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request hardens security mode authentication in the sunrise.php initialization flow. A deterministic key derivation is replaced with high-entropy random secret generation and persistence, and request validation switches to a timing-safe comparison function to prevent timing attacks. ChangesSecurity mode authentication hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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. |
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