Fix five findings from a security assessment of the plugin
The assessment looked for three things: whether students can reach each other's bookings, whether payment settings can be dodged, and whether the plugin opens a way into the rest of the install. The student-isolation and payment paths held up. These are what did not. - The front-end login form told WordPress not to work out whether the site was secure, so on HTTPS every student's session cookie was issued without the Secure flag. wp_signon() only derives it from is_ssl() when the second argument is left at its default; an explicit false reads like "no preference" and is not. - The update check took whatever download URL the release API returned and handed it to core, which unpacks it over the installed plugin. The package must now be https on git.unsupervised.ca exactly, compared on the parsed host so a lookalike name cannot pass. - Uninstalling dropped 2 of 14 tables and left the Stripe secret and webhook signing key in wp_options. Removal is now a choice made in advance on Access -> Plugin removal: records are kept unless the owner opts in (with a typed confirmation), while credentials and the borrowed core registration settings go every time. - Open registration switches on the site-wide users_can_register and makes Student the default role, arming any other signup form on the site to mint students who could book and be billed immediately. The pending state is now decided once, on user_register, rather than by whichever form created the account. - Cancel and withdraw answered "not yours" differently from "does not exist", which let a signed-in student enumerate the studio's bookings. Both now give the same 404. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
@@ -11,12 +11,59 @@ namespace Unsupervised\Schedular\Auth;
|
||||
*
|
||||
* Both checks key solely off the pending user meta, so invite- and
|
||||
* admin-created students (which carry none of it) are unaffected.
|
||||
*
|
||||
* It also decides which new accounts land in that pending state to begin with —
|
||||
* see {@see holdUnknownSignup()}, which closes the gap left by open registration
|
||||
* turning the site's own `users_can_register` on.
|
||||
*/
|
||||
class RegistrationLoginGate {
|
||||
|
||||
public function register(): void {
|
||||
add_filter( 'wp_authenticate_user', [ $this, 'blockUnconfirmed' ], 10, 1 );
|
||||
add_filter( 'user_has_cap', [ $this, 'withholdBookingWhilePending' ], 10, 4 );
|
||||
add_action( 'user_register', [ $this, 'holdUnknownSignup' ], 10, 1 );
|
||||
}
|
||||
|
||||
/**
|
||||
* Hold any student account created by an unauthenticated request that did not
|
||||
* come through the studio's own signup form.
|
||||
*
|
||||
* Enabling open registration switches the site's `users_can_register` on and
|
||||
* makes Student the default role for a new user, because that is what the
|
||||
* studio's registration page needs. But those are *site-wide* settings: they
|
||||
* also arm every other route into `wp_insert_user()` the site happens to have
|
||||
* — another plugin's signup form, a membership add-on — and an account minted
|
||||
* that way arrives holding `book_lesson`, with no email confirmed, no studio
|
||||
* approval, and no policy acceptance on file. It could book and be billed
|
||||
* immediately.
|
||||
*
|
||||
* So the state is decided here, at the one point every path passes through,
|
||||
* rather than trusted to whichever form happened to create the account:
|
||||
*
|
||||
* - **Not a student** — instructors and everyone else are none of this
|
||||
* feature's business.
|
||||
* - **Created by staff** (anyone holding `manage_students`, which includes an
|
||||
* administrator adding a user from wp-admin) — a deliberate act by someone
|
||||
* who could have approved them anyway; approving their own creation is
|
||||
* ceremony, so the account is left active.
|
||||
* - **Anything else** — held, and queued for review under **Pending
|
||||
* Students**.
|
||||
*
|
||||
* The studio's own paths land in the last case and then say what they meant:
|
||||
* a self-signup calls {@see RegistrationStatus::markPending()} (which replaces
|
||||
* the hold with a real, unconfirmed pending state), and an invited student and
|
||||
* a guardian's child are approved outright by the code that creates them.
|
||||
*/
|
||||
public function holdUnknownSignup( int $userId ): void {
|
||||
if ( $userId <= 0 || ! RoleManager::isStudent( $userId ) ) {
|
||||
return;
|
||||
}
|
||||
|
||||
if ( current_user_can( RoleManager::CAP_MANAGE_STUDENTS ) ) {
|
||||
return;
|
||||
}
|
||||
|
||||
RegistrationStatus::hold( $userId );
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user