ci: fix PR #1382 E2E cleanup failure#1413
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)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughUpgrades the security-mode key to a persisted 32-char hex secret with legacy fallback, makes Sunrise unlock timing-safe, adds tests for generation/persistence/legacy behavior, and hardens the E2E workflow by pinning Mailpit and conditionally skipping cleanup steps when checkout artifacts are missing. ChangesSecurity Mode Key Upgrade to High-Entropy Persistent Keys
CI Workflow Robustness Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 docstrings
🧪 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 |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
inc/class-sunrise.php (1)
338-340: ⚡ Quick winSanitize the incoming recovery key before comparing it.
$provided_keynow comes straight from$_GET. Please unslash and sanitize it before thehash_equals()check so this path follows the repo’s input-handling rule without changing valid hex keys.Suggested fix
$provided_key = wu_get_isset($_GET, 'wu_secure'); // phpcs:ignore WordPress.Security.NonceVerification + + if (is_string($provided_key)) { + $provided_key = sanitize_text_field(wp_unslash($provided_key)); + } if (is_string($provided_key) && hash_equals(wu_get_security_mode_key(false), $provided_key)) {As per coding guidelines, "Use wu_clean() or WordPress sanitization functions for input sanitization; custom sanitizers are registered in .phpcs.xml.dist".
🤖 Prompt for 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. In `@inc/class-sunrise.php` around lines 338 - 340, The incoming recovery key stored in $provided_key must be unslashed and sanitized before comparison; update the assignment of $provided_key (used in the hash_equals check against wu_get_security_mode_key(false)) to call wp_unslash on the raw GET value and then sanitize it (preferably with wu_clean() per project guidelines, e.g. $provided_key = wu_clean(wp_unslash(wu_get_isset($_GET, 'wu_secure')))) so only cleaned input reaches the hash_equals() check.Source: Coding guidelines
🤖 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 @.github/workflows/e2e.yml:
- Line 22: Replace the mutable tag used for the Mailpit service by pinning
services.mailpit.image to an immutable sha256 digest instead of
ghcr.io/axllent/mailpit:latest; update the value of services.mailpit.image to
the specific image@sha256:<digest> for a known-good build (obtain the digest
from the registry or CI artifact, e.g., via the registry manifest or docker
pull/inspect) so E2E runs are reproducible and supply-chain stable.
In `@inc/functions/sunrise.php`:
- Around line 98-105: The branch that generates a new $key must be made
first-write-wins: after generating $key (using bin2hex(random_bytes(16))) call
update_network_option(null, 'wu_security_mode_key', $key) but do not immediately
return the generated value; instead re-read the stored option (e.g., via the
corresponding network/site option getter) into $key and return that stored value
so that if another request beat us or the write failed we return the actual
persisted key; keep the existing fallback to wu_get_legacy_security_mode_key()
when $generate is false.
In `@tests/WP_Ultimo/Functions/Sunrise_Functions_Test.php`:
- Around line 144-153: Add a test that covers the persisted-key false path by
first generating and storing the key, then verifying
wu_get_security_mode_key(false) returns the stored 32-character value (not the
6-char legacy fallback). Concretely: call wu_get_security_mode_key(true) to
generate/persist the key (or manually update the network option
'wu_security_mode_key'), fetch the stored value via get_network_option(null,
'wu_security_mode_key'), then assert wu_get_security_mode_key(false) === stored
value and that the stored value has length 32; ensure you clean up/reset the
network option afterwards. Reference: wu_get_security_mode_key and the unlock
flow in inc/class-sunrise.php.
---
Nitpick comments:
In `@inc/class-sunrise.php`:
- Around line 338-340: The incoming recovery key stored in $provided_key must be
unslashed and sanitized before comparison; update the assignment of
$provided_key (used in the hash_equals check against
wu_get_security_mode_key(false)) to call wp_unslash on the raw GET value and
then sanitize it (preferably with wu_clean() per project guidelines, e.g.
$provided_key = wu_clean(wp_unslash(wu_get_isset($_GET, 'wu_secure')))) so only
cleaned input reaches the hash_equals() check.
🪄 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: 8e30d079-fc89-4a0b-8031-9242dd3cd0f0
📒 Files selected for processing (4)
.github/workflows/e2e.ymlinc/class-sunrise.phpinc/functions/sunrise.phptests/WP_Ultimo/Functions/Sunrise_Functions_Test.php
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
|
CLAIM_RELEASED reason=worker_complete runner=superdav42 ts=2026-06-12T11:09:06Z aidevops_version=3.20.56 opencode_version=1.17.3 |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
Summary
Testing
Merged via PR #1413 to main. aidevops.sh v3.20.57 spent 10m on this as a headless bash routine. |
1 participant
Summary
hash_equals()validation.Testing
git diff --checkphp -l inc/functions/sunrise.php && php -l inc/class-sunrise.php && php -l tests/WP_Ultimo/Functions/Sunrise_Functions_Test.phpvendor/bin/phpcs inc/functions/sunrise.php inc/class-sunrise.php tests/WP_Ultimo/Functions/Sunrise_Functions_Test.phppython3 - <<'PY' ... yaml.safe_load(.github/workflows/e2e.yml) ... PYFor #1382
Summary by CodeRabbit
Security
Chores
Tests