Repository navigation
Conversation
FreeScout asks for the user's password before deleting a mailbox, but users log in through WordPress.org and don't know a FreeScout password. Users now get core's "no password" marker, which makes core skip that prompt: new users when they're added, existing users when they log in through WordPress.org. Break-glass passwords from `wporgsso:password` are kept. Typing the mailbox name replaces the password as the safeguard, and the server refuses deletions without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe WPOrgSSO module now manages FreeScout password state for WordPress.org accounts, including break-glass passwords. Mailbox deletion now prompts administrators to enter the mailbox name and checks that value in the deletion request. ChangesWPOrg SSO access controls
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
actor Administrator
participant mailboxes.js
participant RequireWordPressOrgLogin
participant FreeScoutCore
Administrator->>mailboxes.js: Enter mailbox name
mailboxes.js->>RequireWordPressOrgLogin: Submit delete_mailbox with mailbox_name
RequireWordPressOrgLogin->>FreeScoutCore: Continue request when name matches
Merge Risk: ⚪ Minimal · up to The change replaces the mailbox deletion password prompt with a mailbox-name confirmation and clears non-break-glass passwords. No concrete merge-blocking risk was found, and the tests cover the main paths. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Mailbox-name confirmation prevents accidental deletion but does not replace password reauthentication against a compromised administrator session. Emergency-password issuance also needs coordination with concurrent SSO logins so a newly issued recovery credential is not erased. Existing identity and permission controls limit these risks. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The required confirmation input needs an accessible label communicating its purpose and expected mailbox name.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Updates FreeScout’s WordPress.org sign-in integration so mailbox deletion uses name confirmation instead of an unavailable password, while preserving administrator break-glass passwords.
Changes:
- Assigns core’s no-password marker on user creation and sign-in.
- Adds mailbox-name confirmation in the dialog and server validation.
- Documents the behavior and adds regression tests.
| File | Description |
|---|---|
| freescout.wordpress.net/Modules/WPOrgSSO/tests/PasswordsTest.php | Tests password clearing and break-glass preservation. |
| freescout.wordpress.net/Modules/WPOrgSSO/tests/DeleteMailboxTest.php | Tests deletion confirmation and permissions. |
| freescout.wordpress.net/Modules/WPOrgSSO/Services/Passwords.php | Manages no-password markers and break-glass records. |
| freescout.wordpress.net/Modules/WPOrgSSO/Resources/views/delete_mailbox.blade.php | Adds the confirmation input. |
| freescout.wordpress.net/Modules/WPOrgSSO/README.md | Documents password and deletion behavior. |
| freescout.wordpress.net/Modules/WPOrgSSO/Public/js/mailboxes.js | Integrates confirmation into core’s dialog. |
| freescout.wordpress.net/Modules/WPOrgSSO/Providers/WPOrgSSOServiceProvider.php | Registers assets, UI hooks, and password clearing. |
| freescout.wordpress.net/Modules/WPOrgSSO/Http/Middleware/RequireWordPressOrgLogin.php | Validates mailbox-name confirmation. |
| freescout.wordpress.net/Modules/WPOrgSSO/Http/Controllers/SsoController.php | Clears passwords after WordPress.org sign-in. |
| freescout.wordpress.net/Modules/WPOrgSSO/Console/SetPassword.php | Records administrator break-glass passwords. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The instruction above the field isn't tied to it, so screen readers only announced the placeholder. Give the field an aria-label that carries both the instruction and the expected name. Core clones the template into the dialog, so an id/for pair would end up duplicated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What and why
FreeScout asks for the user's password before an administrator deletes a mailbox. Our users log in only through WordPress.org, so they don't know a FreeScout password and can't answer that prompt. Core skips the prompt for users whose password is its "no password" marker (
User::isDummyPassword()), but WPOrgSSO gave new users a random real password.This gives users core's marker, and replaces the password with typing the mailbox name, GitHub-style.
How it works
No FreeScout password
User::getDummyPassword()). Core requires a password on the create form and hashes it inUser::create(), so the middleware still fills in a random one, and theuser.create_savefilter swaps it for the marker before core saves the user.SsoController::complete()). That's the one point where we know they no longer need a password. Connecting an account (profile orwporgsso:connect) doesn't clear it, since connecting can happen before WordPress.org is enforced, when users still log in with passwords.wporgsso:passwordnow records that it gave an administrator one (optionwporgsso.break_glass.<user id>), and logins skip those administrators. An administrator's older password, from before WordPress.org, can't be told apart from a break-glass one otherwise, so it's cleared;wporgsso:passwordgives them a new one. Someone who stops being an administrator loses theirs at their next login.Hash::check()(Laravel 5.5'sBcryptHasher,password_verify()) returns false for it without throwing. Tested, including the marker and its decrypted value as passwords in break-glass mode.Typing the mailbox name
mailbox.update.after_signaturehook renders a hidden field ("Type the mailbox name to confirm:") with the name;Public/js/mailboxes.jsmoves it into core's delete dialog, keeps the Delete button disabled until the name matches, and addsmailbox_nameto the dialog's request (core's dialog posts a fixed set of fields, so through$.ajaxPrefilter).delete_mailbox(MailboxesController@ajax) unlessmailbox_namematches the mailbox's name exactly. It's case-sensitive; core'sTrimStringsmiddleware trims the input, so spaces around it don't matter. The refusal is a 200 withstatus: error, like core's own errors, so the dialog shows it: "Type the mailbox’s name, Plugin Review, to confirm." Those who can't delete the mailbox get core's answer, without the name.The README describes both.
Testing
PasswordsTestandDeleteMailboxTest(13 tests): marker for new users (not before enforcement), cleared at login for users and administrators' older passwords, break-glass password kept (and cleared for a former administrator), no password login with the marker, the dialog without core's password field (and with it for break-glass administrators), deletion refused with a missing, wrong, differently cased, or array name, allowed with the right one, break-glass administrators needing both, and others getting core's answer.freescout.wordpress.net/phpcs.xml.dist) andnpm run lint:jsare clean.🤖 Generated with Claude Code
Summary by CodeRabbit