Fix five findings from a security assessment of the plugin
CI / Coding Standards (pull_request) Failing after 28s
CI / Tests (PHP 8.5) (pull_request) Failing after 27s
CI / No Debug Code (pull_request) Successful in 3s
CI / Tests (PHP 8.1) (pull_request) Failing after 39s
CI / Tests (PHP 8.3) (pull_request) Failing after 1m7s
CI / Tests (PHP 8.2) (pull_request) Failing after 1m8s
CI / Static Analysis (pull_request) Successful in 1m17s
CI / Build Plugin Zip (pull_request) Skipped

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:
2026-09-05 11:31:12 -03:00
co-authored by Claude Opus 5
parent 2781243742
commit e522789104
28 changed files with 1246 additions and 50 deletions
+29 -1
View File
@@ -42,6 +42,34 @@ never be used to create a policy-less account (`Auth\EmailConfirmationHandler`):
- `login_init` action redirects any `action=register` request (GET **and** POST) to the registration page before any processing runs.
- `registration_errors` filter is a fail-safe that rejects `register_new_user()` outright.
### Holding signups that came from somewhere else
Blocking core's own form is not the whole story. `users_can_register=1` and
`default_role=us_student` are *site-wide* settings, so they also arm every other
route into `wp_insert_user()` a site happens to have — another plugin's signup
form, a membership add-on. An account minted that way arrives holding
`book_lesson`, with no email confirmed, no studio approval and no policy
acceptance on file, and could book and be billed immediately.
So the pending state is not decided by whichever form created the account. It is
decided once, on `user_register`, by
`Auth\RegistrationLoginGate::holdUnknownSignup()`:
| New account | Result |
|---|---|
| Not a `us_student` | untouched — instructors and everyone else are not this feature's business |
| Created by someone holding `manage_students` (including an admin adding a user in wp-admin) | left active — a deliberate act by someone who could approve it in the next click |
| Anything else | **held** via `RegistrationStatus::hold()` and queued under **Pending Students** |
A hold sets `us_awaiting_approval=1` *and* `us_email_confirmed=1`. The account was
never asked to confirm anything and has no token to answer with, so blocking its
login would strand it; what the hold withholds is `book_lesson`, until a studio
admin approves it.
The plugin's own paths all land in the last row and then say what they meant:
- a self-signup calls `RegistrationStatus::markPending()`, which replaces the hold with a real, unconfirmed pending state (it clears `us_email_confirmed` explicitly for exactly this reason);
- an invited student is approved outright by `RegistrationPage` before the auto-login — the invitation *is* the approval;
- a guardian's child is approved outright by `GuardianService::createChild()`; the account is never signed in to, and queueing every child a family adds would be nonsense.
## Account Lifecycle (self-approval)
State lives entirely in user meta (`Auth\RegistrationStatus`). Only the raw
confirmation token's SHA-256 hash is stored; the token expires after 48h
@@ -198,7 +226,7 @@ No-op when no registration page is set.
- Repository: `Unsupervised\Schedular\Auth\InviteRepository`
- Admin controllers: `Unsupervised\Schedular\Auth\RegistrationController` (invites), `Unsupervised\Schedular\Auth\RegistrationApprovalController` (pending students)
- Frontend: `Unsupervised\Schedular\Auth\RegistrationPage`
- Self-approval flow: `Auth\RegistrationStatus` (lifecycle meta), `Auth\RegistrationLoginGate` (login + booking-cap gate), `Auth\EmailConfirmationHandler` (confirm link + native-form block), `Auth\RegistrationMailer` (emails)
- Self-approval flow: `Auth\RegistrationStatus` (lifecycle meta, including `hold()`), `Auth\RegistrationLoginGate` (login gate, booking-cap gate, and the `user_register` hold), `Auth\EmailConfirmationHandler` (confirm link + native-form block), `Auth\RegistrationMailer` (emails)
- Settings toggle: `Payment\StudioSettings` (`us_registration_mode`, core-option mirror/restore)
- Reuses `Policy\PolicyRepository`, `Policy\PolicyVersionRepository`, `Policy\AcceptanceRepository`
- Schema: `us_invites`; `us_policies.acceptance_scope`. Self-approval adds no tables — state is WordPress user meta.
+79
View File
@@ -0,0 +1,79 @@
# Feature: Data Removal
## Overview
What deleting the plugin takes with it, and how the site owner says so.
WordPress gives an uninstall no interface of its own: `uninstall.php` runs
headless, after the plugin is already gone from the screen, with no opportunity
to ask anything. So the answer is given in advance, on **Access → Plugin
removal**, and read back at uninstall time.
Two things are true at once, and the split below is how both are honoured:
- **A studio's records are irreplaceable.** Lessons taught, payments taken, what
families agreed to and when. A delete during a migration, or a
delete-and-reinstall while troubleshooting, must not be the thing that loses
them. So the data is **kept unless the owner explicitly says otherwise**.
- **Credentials are not records.** A Stripe secret and webhook signing key can be
re-pasted from the Stripe dashboard in under a minute. Live keys sitting in
`wp_options` on a site that no longer has the code to use them are nothing but
exposure. So those are **always** removed.
## Option `us_delete_data_on_uninstall`
`'1'` or `'0'` (default `'0'`). Written only from the Access page.
## Always removed
Whatever the setting says:
| What | Why |
|---|---|
| `us_stripe_secret_key`, `us_stripe_webhook_secret`, `us_stripe_publishable_key`, `us_stripe_mode` | Credentials, not records — cheap to restore, dangerous to leave |
| `us_schedular_latest_release` transient | Cached release metadata; meaningless without the plugin |
| The `us_generate_due_payments` cron event | Deactivation clears it too, but a site whose plugin files simply vanished never ran that hook |
| `users_can_register` / `default_role` | Restored from the snapshot open registration took (`us_registration_prev_*`). These are the *site's* settings, borrowed; leaving them behind would leave the site accepting public signups into a Student role that is about to stop existing. Only acts when a snapshot exists |
## Removed only on an explicit full purge
- Every table in `Schema::TABLES` — all 14, dropped by name.
- Every remaining `us_*` option, including the removal setting itself.
- Every `us_*` user meta key, for all users (`delete_metadata( 'user', 0, $key, '', true )`) — billing overrides, child markers, birth years, pending-signup state.
- The `us_studio_admin`, `us_instructor` and `us_student` roles.
Roles go **only** on a full purge. A site keeping its data is keeping its
students, and a student whose role was deleted out from under them holds no
capabilities at all until the plugin is reinstalled.
`Schema::TABLES` is the single list of tables the plugin owns; the `CREATE TABLE`
statements in `Schema::tables()` spell their own names out, so a new table has to
be added in both places or uninstalling will leave it behind.
## Admin Interface
**Access → Plugin removal** (`manage_options` — the same capability as deleting a
plugin, so the switch and the act it governs are in the same pair of hands):
- A read-only note stating what is removed regardless.
- **Erase everything when the plugin is deleted** — off by default.
Turning it **on** takes the tick *and* the word `DELETE` typed into a confirmation
box. It is the only place in the plugin where a stray click is unrecoverable, so
the checkbox alone is not enough. A refused confirmation reports why and still
saves everything else on the page — a mistyped word must not silently swallow a
capability change made in the same submit.
Turning it **off** needs nothing but unticking the box. Saving the page for some
other reason while it is already on leaves it on, without asking for the word
again.
## Implementation
- `Unsupervised\Schedular\Uninstaller` — the option, and the whole of `run()`
- `uninstall.php` — guards on `WP_UNINSTALL_PLUGIN`, loads the autoloader, calls `Uninstaller::run()`
- `Unsupervised\Schedular\Schema::TABLES` — the table list
- `Unsupervised\Schedular\Auth\AccessSettings` — the setting's UI and the typed confirmation
- `templates/admin/access.php`
## Tests
- `tests/Unit/UninstallerTest.php`
- `tests/Unit/Auth/AccessSettingsTest.php` (the confirmation rules)
## Related
- `payments.md` — the Stripe credentials this always forgets
- `account-registration.md` — the core options open registration borrows
+6 -4
View File
@@ -136,10 +136,12 @@ Self-withdrawal is bounded by the class's **withdrawal deadline** (the instructo
implicit default — a class with no deadline set stays open to withdrawal for its
whole life. Past the deadline `POST /enrollments/{id}/withdraw` rejects the request
with `403 withdrawal_closed`, and the class card shows "Withdrawal has closed —
contact the studio to withdraw." in place of the Withdraw button. The endpoint also
returns `404 not_found` for an unknown enrolment and `403 forbidden` when the
enrolment is not the caller's own; a withdrawal of an already-cancelled enrolment is
idempotent.
contact the studio to withdraw." in place of the Withdraw button. An enrolment that
does not exist and one that is not the caller's own both return the same
`404 not_found`, deliberately: two different answers would let any signed-in student
walk the id space and count the studio's enrolments, and there is nothing they could
do with either answer. A withdrawal of an already-cancelled enrolment is idempotent.
`POST /bookings/{id}/cancel` makes the same trade for the same reason.
The deadline only bounds student **self**-withdrawal. A studio admin can withdraw a
student at any time from the **student detail page** (`Auth\StudentActions::withdrawEnrollment`),
+15 -3
View File
@@ -47,9 +47,21 @@ update for a same-slug plugin and makes core fire the
`us_schedular_latest_release` transient for 6 hours.
3. Strips the leading `v` from the tag and compares against `USC_VERSION`
with `version_compare`; PHP orders `1.0.0-rc.2 < 1.0.0` correctly.
4. When newer, returns the release's first `.zip` asset as the update
package. Core takes over from there: Plugins-screen notice, one-click
update, and WP-Cron auto-updates if enabled.
4. When newer, returns the release's first `.zip` asset **whose download URL
is `https` on `git.unsupervised.ca` itself** as the update package. Core
takes over from there: Plugins-screen notice, one-click update, and
WP-Cron auto-updates if enabled.
The host check is not ceremony. Whatever this returns is downloaded and
unpacked over the installed plugin, so the URL is executable code by
another name — and it arrives in a JSON body. An answer that is not really
the release server's (a hijacked hostname, a tampered response, a repo host
handing out a package hosted somewhere else) would otherwise install
arbitrary code on every site running the plugin, silently for anyone with
auto-updates on. The host must match exactly: `evil-git.unsupervised.ca`,
`git.unsupervised.ca.evil.test` and `cdn.git.unsupervised.ca` are all
refused, and so is plain `http`. An asset that fails the check is skipped
and the scan continues, so one bad asset does not hide a good one.
5. When not newer — the site is current, or the lookup failed — returns a
`no_update` payload (installed version, empty package). This keeps the
plugin in core's `update_plugins` transient so core's `update-supported`