Unsupervised/unsupervised-scheduler
2
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases 19 Wiki Activity

Fix five findings from a security assessment of the plugin #197

Merged
thatguygriff merged 2 commits from security/assessment-hardening into main 2026-09-05 15:00:33 +00:00
Conversation 1 Commits 2 Files Changed 28
+1251 -50
Kydoimos commented 2026-09-05 14:32:03 +00:00
Member
Copy Link
Copy Source

A security assessment of the plugin, and fixes for everything it turned up.

The brief was three questions: can students reach each other's bookings, can
payment settings be dodged, and does the plugin open a way into the rest of the
WordPress install. The first two held up. Every read path is scoped through
GuardianService::householdIds() / canActFor(), LessonBooker::resolveOffering()
correctly refuses a substituted cheaper offering, and the Stripe webhook fails
closed. SQL is prepared throughout, every admin page checks a capability and a
nonce, and templates escape their output.

Five things did not hold up. All five are fixed here.

1. Front-end login issued auth cookies without the Secure flag

src/Auth/LoginPage.php called wp_signon( $credentials, false ). Core only
derives the flag from is_ssl() when that argument is left at its default empty
string — an explicit false reads like "no preference" and is not. So on an
HTTPS site every student signing in through [us_student_login] got the plain
AUTH_COOKIE, which a browser will send over http://. Anyone able to watch the
network and provoke a single plaintext request to the domain could lift a
signed-in session. Sessions started at wp-login.php were never affected.

Now passes the credentials and nothing else. The test asserts the call takes
exactly one argument, so re-introducing a second one fails.

2. Update packages were installed from wherever the API said

UpdateChecker took the release asset's browser_download_url verbatim and
handed it to core, which downloads and unpacks it over the installed plugin. The
URL is executable code by another name, and it arrives in a JSON body — a
hijacked hostname or a tampered response could have pointed every site running
the plugin at any file on the internet, silently for anyone with auto-updates on.

The package must now be https on git.unsupervised.ca. The check is on the
parsed host, not the string, because a name that merely contains the right one
is the whole trick: evil-git.unsupervised.ca satisfies an endsWith check,
git.unsupervised.ca.evil.test a startsWith one, and
https://[email protected]/x.zip reads like the real host to a
person. An asset that fails is skipped and the scan continues, so one bad asset
cannot mask a good one.

3. Uninstalling left live API keys and all PII behind

uninstall.php dropped 2 of the 14 tables in Schema.php and deleted 2 options.
Left behind: the Stripe secret key and webhook signing secret, payments,
credits, intake answers (including children's), policy acceptances, guardian
links, invites, and every us_* user meta. Deleting a plugin is the one moment
an admin expects credentials to be revoked.

WordPress gives an uninstall no interface — uninstall.php runs headless, after
the plugin is already gone from the screen — so the answer is now given in
advance, on Access → Plugin removal (manage_options, the same capability as
deleting a plugin). The split is deliberate:

  • Always removed, whatever the setting: the Stripe credentials, the release
    transient, the cron event, and the users_can_register / default_role
    snapshot restore. Credentials are re-pastable from the Stripe dashboard in a
    minute; leaving live keys on a site that no longer has the code to use them was
    the finding, so making that opt-in would not have fixed it.
  • Opt-in full purge: all 14 tables (via a new Schema::TABLES), every us_*
    option, every us_* user meta for all users, and the three roles. Roles go
    only on a purge — a kept student whose role was deleted holds no capabilities
    at all until a reinstall.

Turning the purge on takes the tick and the word DELETE typed; a refused
confirmation says so and still saves the rest of the page. Turning it off, or
saving the page while it is already on, needs nothing.

4. Open registration armed every other signup form on the site

Enabling it switches on the site-wide users_can_register and makes Student the
default role, which is what the studio's registration page needs — but those are
site-wide. Core's own form is blocked, but any other signup route (another
plugin's form, a membership add-on) then minted a us_student holding
book_lesson, with no email confirmed, no approval and no policies agreed to. It
could book and be billed immediately.

The pending state is now decided once, on user_register, at the one point every
path passes through:

New account Result
Not a us_student untouched
Created by someone holding manage_students left active — a deliberate act by someone who could approve it in the next click
Anything else held, and queued under Students → Pending Students

A hold marks the account awaiting approval and email-confirmed: nobody asked it
to confirm anything and it has no token to answer with, so blocking its login
would strand it. What is withheld is book_lesson. The plugin's own paths land
in the last row and then say what they meant — a self-signup calls markPending()
(which now clears us_email_confirmed explicitly, or the hold would have let it
skip confirmation), while invited students and guardians' children are approved
outright by the code that creates them.

5. Booking ids could be enumerated

POST /bookings/{id}/cancel and /enrollments/{id}/withdraw returned 404 for a
nonexistent id but 403 for someone else's — enough for any signed-in student to
walk the id space and learn how many lessons and enrolments the studio holds.
Both now give the same 404. The booking form's own 403 stays: there the id
came from a list of people the caller may act for, so "not yours" is a correction
they need, not a fact they lack.

Tests

composer test, composer lint and composer cs were not run locally — this
machine has no PHP, Composer or vendor/, so CI is the first execution. Flagging
it rather than claiming a green run I did not see. Please hold the merge until the
CI jobs report.

What is covered:

  • tests/Unit/Auth/LoginPageTest.php — wp_signon() is called with exactly one argument
  • tests/Unit/Update/UpdateCheckerTest.php — eight-case provider of untrusted package URLs (unrelated host, lookalike prefix and suffix, subdomain, credentials-in-host, plain http, no scheme, empty), plus a good zip still being found after a bad one
  • tests/Unit/UninstallerTest.php — credentials forgotten even when data is kept; keeping data drops nothing and removes no roles; a full purge drops every table Schema::TABLES declares; core registration settings restored from the snapshot, and left alone when there is none
  • tests/Unit/Auth/AccessSettingsTest.php — the tick alone does not enable the purge, the tick plus the typed word does, a near miss does not, an already-on setting survives an unrelated save, and unticking turns it off
  • tests/Unit/Auth/RegistrationLoginGateTest.php — held / left active / ignored, per the table above
  • tests/Unit/Auth/RegistrationStatusTest.php — hold(), and markPending() clearing the confirmed flag
  • tests/Unit/Auth/RegistrationPageTest.php, tests/Unit/Guardian/GuardianServiceTest.php — an invited student and a guardian's child are not left waiting for approval
  • tests/Unit/Booking/BookingEndpointTest.php, tests/Unit/GroupClass/EnrollmentEndpointTest.php — the two refusals are now indistinguishable

Notes

  • No version bump: nothing in the DDL changed, so the Schema.php rule in
    CLAUDE.md does not apply, and bumping mid-cycle would collide with the release
    workflow's own bump PR. Schema::TABLES is a name list for the uninstaller; the
    CREATE TABLE statements still spell their own names out, so a new table has to
    be added in both places.
  • Docs: docs/features/data-removal.md is new; account-registration.md,
    plugin-self-update.md and group-classes.md amended. CHANGELOG entries are
    under the open ## [1.5.6] section.

🤖 Generated with Claude Code

A security assessment of the plugin, and fixes for everything it turned up. The brief was three questions: can students reach each other's bookings, can payment settings be dodged, and does the plugin open a way into the rest of the WordPress install. **The first two held up.** Every read path is scoped through `GuardianService::householdIds()` / `canActFor()`, `LessonBooker::resolveOffering()` correctly refuses a substituted cheaper offering, and the Stripe webhook fails closed. SQL is prepared throughout, every admin page checks a capability *and* a nonce, and templates escape their output. Five things did not hold up. All five are fixed here. --- ### 1. Front-end login issued auth cookies without the Secure flag `src/Auth/LoginPage.php` called `wp_signon( $credentials, false )`. Core only derives the flag from `is_ssl()` when that argument is left at its default empty string — an explicit `false` reads like "no preference" and is not. So on an HTTPS site every student signing in through `[us_student_login]` got the plain `AUTH_COOKIE`, which a browser will send over `http://`. Anyone able to watch the network and provoke a single plaintext request to the domain could lift a signed-in session. Sessions started at `wp-login.php` were never affected. Now passes the credentials and nothing else. The test asserts the call takes exactly one argument, so re-introducing a second one fails. ### 2. Update packages were installed from wherever the API said `UpdateChecker` took the release asset's `browser_download_url` verbatim and handed it to core, which downloads and unpacks it over the installed plugin. The URL is executable code by another name, and it arrives in a JSON body — a hijacked hostname or a tampered response could have pointed every site running the plugin at any file on the internet, silently for anyone with auto-updates on. The package must now be `https` on `git.unsupervised.ca`. The check is on the *parsed host*, not the string, because a name that merely contains the right one is the whole trick: `evil-git.unsupervised.ca` satisfies an endsWith check, `git.unsupervised.ca.evil.test` a startsWith one, and `https://[email protected]/x.zip` reads like the real host to a person. An asset that fails is skipped and the scan continues, so one bad asset cannot mask a good one. ### 3. Uninstalling left live API keys and all PII behind `uninstall.php` dropped 2 of the 14 tables in `Schema.php` and deleted 2 options. Left behind: the Stripe **secret key** and **webhook signing secret**, payments, credits, intake answers (including children's), policy acceptances, guardian links, invites, and every `us_*` user meta. Deleting a plugin is the one moment an admin expects credentials to be revoked. WordPress gives an uninstall no interface — `uninstall.php` runs headless, after the plugin is already gone from the screen — so the answer is now given in advance, on **Access → Plugin removal** (`manage_options`, the same capability as deleting a plugin). The split is deliberate: - **Always removed**, whatever the setting: the Stripe credentials, the release transient, the cron event, and the `users_can_register` / `default_role` snapshot restore. Credentials are re-pastable from the Stripe dashboard in a minute; leaving live keys on a site that no longer has the code to use them was the finding, so making that opt-in would not have fixed it. - **Opt-in full purge**: all 14 tables (via a new `Schema::TABLES`), every `us_*` option, every `us_*` user meta for all users, and the three roles. Roles go *only* on a purge — a kept student whose role was deleted holds no capabilities at all until a reinstall. Turning the purge on takes the tick **and** the word `DELETE` typed; a refused confirmation says so and still saves the rest of the page. Turning it off, or saving the page while it is already on, needs nothing. ### 4. Open registration armed every other signup form on the site Enabling it switches on the site-wide `users_can_register` and makes Student the default role, which is what the studio's registration page needs — but those are *site-wide*. Core's own form is blocked, but any other signup route (another plugin's form, a membership add-on) then minted a `us_student` holding `book_lesson`, with no email confirmed, no approval and no policies agreed to. It could book and be billed immediately. The pending state is now decided once, on `user_register`, at the one point every path passes through: | New account | Result | |---|---| | Not a `us_student` | untouched | | Created by someone holding `manage_students` | left active — a deliberate act by someone who could approve it in the next click | | Anything else | held, and queued under **Students → Pending Students** | A hold marks the account awaiting approval *and* email-confirmed: nobody asked it to confirm anything and it has no token to answer with, so blocking its login would strand it. What is withheld is `book_lesson`. The plugin's own paths land in the last row and then say what they meant — a self-signup calls `markPending()` (which now clears `us_email_confirmed` explicitly, or the hold would have let it skip confirmation), while invited students and guardians' children are approved outright by the code that creates them. ### 5. Booking ids could be enumerated `POST /bookings/{id}/cancel` and `/enrollments/{id}/withdraw` returned `404` for a nonexistent id but `403` for someone else's — enough for any signed-in student to walk the id space and learn how many lessons and enrolments the studio holds. Both now give the same `404`. The booking form's own `403` stays: there the id came from a list of people the caller may act for, so "not yours" is a correction they need, not a fact they lack. --- ## Tests **`composer test`, `composer lint` and `composer cs` were not run locally** — this machine has no PHP, Composer or `vendor/`, so CI is the first execution. Flagging it rather than claiming a green run I did not see. Please hold the merge until the CI jobs report. What is covered: - `tests/Unit/Auth/LoginPageTest.php` — `wp_signon()` is called with exactly one argument - `tests/Unit/Update/UpdateCheckerTest.php` — eight-case provider of untrusted package URLs (unrelated host, lookalike prefix and suffix, subdomain, credentials-in-host, plain http, no scheme, empty), plus a good zip still being found after a bad one - `tests/Unit/UninstallerTest.php` — credentials forgotten even when data is kept; keeping data drops nothing and removes no roles; a full purge drops every table `Schema::TABLES` declares; core registration settings restored from the snapshot, and left alone when there is none - `tests/Unit/Auth/AccessSettingsTest.php` — the tick alone does not enable the purge, the tick plus the typed word does, a near miss does not, an already-on setting survives an unrelated save, and unticking turns it off - `tests/Unit/Auth/RegistrationLoginGateTest.php` — held / left active / ignored, per the table above - `tests/Unit/Auth/RegistrationStatusTest.php` — `hold()`, and `markPending()` clearing the confirmed flag - `tests/Unit/Auth/RegistrationPageTest.php`, `tests/Unit/Guardian/GuardianServiceTest.php` — an invited student and a guardian's child are not left waiting for approval - `tests/Unit/Booking/BookingEndpointTest.php`, `tests/Unit/GroupClass/EnrollmentEndpointTest.php` — the two refusals are now indistinguishable ## Notes - No version bump: nothing in the DDL changed, so the `Schema.php` rule in `CLAUDE.md` does not apply, and bumping mid-cycle would collide with the release workflow's own bump PR. `Schema::TABLES` is a name list for the uninstaller; the `CREATE TABLE` statements still spell their own names out, so a new table has to be added in both places. - Docs: `docs/features/data-removal.md` is new; `account-registration.md`, `plugin-self-update.md` and `group-classes.md` amended. CHANGELOG entries are under the open `## [1.5.6]` section. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Kydoimos added 2 commits 2026-09-05 14:57:46 +00:00
Fix five findings from a security assessment of the plugin 1847159e31 
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]>
Fix the CI failures in the uninstaller work
CI / Coding Standards (pull_request) Successful in 28s
Details
CI / No Debug Code (pull_request) Successful in 3s
Details
CI / Tests (PHP 8.1) (pull_request) Successful in 38s
Details
CI / Tests (PHP 8.3) (pull_request) Successful in 40s
Details
CI / Tests (PHP 8.2) (pull_request) Successful in 49s
Details
CI / Static Analysis (pull_request) Successful in 1m0s
Details
CI / Tests (PHP 8.5) (pull_request) Successful in 57s
Details
CI / Build Plugin Zip (pull_request) Skipped
Details
ae1a62883e 
Both were mine, and both were in code the earlier commit could not run.

The four test failures shared one cause: UninstallerTest stubbed get_option
with an arrow function, which captures by value, so every read answered from
a snapshot of the options taken at setUp — before the test set any and before
the run wrote any. Every assertion that depended on reading back what had
just been written therefore saw an empty store. The file's other stubs
already use by-reference closures; this one now does too.

The phpcs error is WordPress.DB.PreparedSQL.NotPrepared on the table drop.
The sniff cannot follow $sql across the null guard that PHPStan requires
(prepare() is nullable), and unlike the repositories — which call through a
typed $this->db property the sniff does not track at all — the uninstaller
calls the global $wpdb, so the sniff sees it. Silenced explicitly, with the
reason.

composer test (996 tests, 2871 assertions), composer lint and composer cs all
pass locally on PHP 8.4.

Co-Authored-By: Claude Opus 5 <[email protected]>
Kydoimos force-pushed security/assessment-hardening from 62392fdece to ae1a62883e 2026-09-05 14:57:46 +00:00 Compare
thatguygriff merged commit f58585be0d into main 2026-09-05 15:00:33 +00:00
thatguygriff deleted branch security/assessment-hardening 2026-09-05 15:00:33 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
thatguygriff
Labels
Clear labels
bug
Something isn't working
 enhancement
Extends an existing feature
 feature
New user-facing capability
 payments
Billing, Stripe, receipts, reporting
 security
Security vulnerability or hardening
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
Kydoimos (Kydoimos) thatguygriff (James Griffin-Allwood)
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Unsupervised/unsupervised-scheduler#197