# CC Soccer — Security & Correctness Review

**Date:** July 3, 2026
**Reviewer:** Claude (Fable 5, Cowork session)
**Scope:** Custom `ccsoccer` module (PHP controllers/forms/services, routing, checkout panes, order processors, JS, Twig) and `ccsoccer_theme`. Emphasis on commerce/checkout, the group_id UUID consistency the owner flagged, and bot/abuse exposure ahead of opening public registration.
**Baseline:** Cross-referenced against the March 16 and June 13, 2026 reviews to track what is fixed vs. still open, and to catch new issues.

---

## How to read this

The site is about to email its whole user base and open self-registration. That single event changes the threat model: traffic (and bots) spike, and anything gated only by "nobody knows the URL" or "no one would bother" gets tested. The findings below are ordered by what to fix **before** that email goes out.

Two things worth stating up front, because they change the picture:

1. **The `group_id` disconnect you were burned by is genuinely fixed for the checkout path.** All three tournament join branches now write the Team's UUID via a single `getOrCreateTeamGroupId()` helper. No legacy `team_<id>_<id>` strings are written by any live code path anymore. Details in Finding G below.
2. **One subagent flagged "CAPTCHA is disabled" as critical — that is a false alarm, and I want to make sure you don't act on it.** See the "Corrected / non-issues" section. The real captcha risk is smaller and is about *verifying production*, not enabling modules.

---

## Summary

Status legend: ✅ Fixed (Jul 3) · 🗒️ Documented, pending decision · ⏸️ Deferred · ⬜ Open (not started)

| ID | Severity | Issue | Status |
|----|----------|-------|--------|
| **A** | **CRITICAL** | Group action forms (accept/decline/leave/remove/invite/nudge/delete) have no CSRF protection | ✅ Fixed (Jul 3) |
| **B** | **HIGH** | Checkout completion has no idempotency guard — a replayed "place" event re-spends credits | ✅ Fixed (Jul 3) |
| **C** | **HIGH** | Credits charged at checkout aren't reconciled against what's actually deducted at placement | ✅ Fixed (Jul 3) |
| **D** | **MEDIUM** | Override extend/nudge/revoke are state-changing GET routes, no CSRF | ✅ Fixed (Jul 3) |
| **E** | **MEDIUM** | Jersey pane trusts submitted `variation_id` without confirming it's a jersey | ⬜ Open |
| **F** | **MEDIUM** | Full-season race at completion takes payment but creates no registration, no refund flag | ✅ Fixed (Jul 3) |
| **G** | **MEDIUM** | `invitee_email` fee-waiver can be spoofed by changing account email | ⏸️ Deferred (before team fees scale) |
| **H** | **MEDIUM** | User/email enumeration + directory harvest via invite autocomplete | 🗒️ Documented, discuss w/ Caleb |
| **I** | **LOW/MED** | Authorize.net **test** creds and reCAPTCHA **secret** key committed to git | ⬜ Open |
| **J** | **INFO** | Financial/PII permissions could carry `restrict access: true`; `view reports` is coarse (bundles reports + per-player credit history). Access is board/TD/admin only — not exposed to players or public. | ✅ Flag added (Jul 3); split optional |
| **K** | **LOW** | Prior open items: raw exception messages, inline `onchange`, admin-notes escaping | ⬜ Open |

---

## Resolution Log

### July 3, 2026 (same day — Fable 5 review session)

**Fixed and committed** (deploy: code only; run `drush cr` per environment; no config import or db update):

- **A — CSRF on group actions.** Added `_csrf_token: 'TRUE'` to all seven group
  routes and restricted accept to POST. Converted the pending-invitations banner
  decline link to a render element so it carries a valid token.
  Files: `ccsoccer.routing.yml`, `src/Controller/RegistrationController.php`.
- **B — Checkout idempotency guard.** `onOrderPlace` now returns early if the order
  is already marked processed, and sets that flag after credits + registrations
  (before confirmation emails). Prevents double credit spend on a re-dispatched
  place transition. File: `src/EventSubscriber/OrderCompleteSubscriber.php`.
- **C — Credit reconciliation.** `processCredits` compares credits actually deducted
  against the amount discounted at checkout; on a shortfall it logs an alert and
  flags the order `ccsoccer_credit_shortfall`. Same file as B.
- **D — CSRF on override actions.** Added `_csrf_token: 'TRUE'` to override
  extend/nudge/revoke, and rebuilt the active-overrides table as a render array so
  the action links carry a valid token. Files: `ccsoccer.routing.yml`,
  `src/Controller/OverrideController.php`.
- **F — Paid-but-unregistered season race.** Both season dead-end branches now flag
  the order `ccsoccer_season_registration_failed` (with reason + counts) instead of
  silently keeping the payment. Same file as B/C.
- **J — Permission hygiene (downgraded to informational).** Confirmed these surfaces
  are board/TD/admin only — not exposed to players or the public — so the earlier
  "exposes every player's ledger" wording was corrected. Added `restrict access: true`
  to the financial / PII / mass-communication permissions and clarified the
  `view reports` description. File: `ccsoccer.permissions.yml`. The optional split of
  `view reports` into a separate finance permission is deferred as a design choice.

**Documented, pending decision:**

- **H — Invite email harvest.** Full analysis and recommended fix (mask the email +
  stop sending raw email in the response; inviting already resolves email from uid)
  written up in `INVITE_EMAIL_MASKING_PROPOSAL.md`. Andrew to discuss format with
  Caleb before implementing. Leaning toward masking.

**Deferred (documented):**

- **Flagged-orders admin report.** The new flags from C and F (plus the existing
  tournament flags — see SESSION_HANDOFF item 18) have no admin surfacing yet.
  Design + rationale in `FLAGGED_ORDERS_REPORT_PROPOSAL.md`. Deliberately deferred:
  at ~200 players/season, a player who pays without a product self-reports, and all
  flags log at error level to dblog. Revisit if volume grows.
- **G — `invitee_email` fee-waiver.** The group_id disconnect that caused past pain
  is already fixed; the residual email-match waiver is low-risk at current fees and
  is deferred until the productized team-fee flow scales (next year). Docblock
  cleanup (`GroupInvitationsForm.php:24`, `Team.php:655-657`) still outstanding.

**Open — not started (recommended order):**

- **E** — Jersey pane: verify the submitted variation is a jersey before adding.
- **I** — Scrub + rotate Authorize.net test creds and the reCAPTCHA secret from git
  (low urgency; sandbox / domain-locked, and live creds are already out of git).
- **K** — Generic exception messages, move the inline `onchange`, escape admin notes
  on any future display.

---

## CRITICAL

### A · Group action forms have no CSRF protection

**Routes (`ccsoccer.routing.yml`):** `invitation.accept` (1109), `invitation.decline` (1117), `group.leave` (1134), `group.remove_member` (1125), `group.nudge` (1091), `group.delete` (1100), `group.invite` (1082).
**Templates:** `ccsoccer-group-manage.html.twig`, `ccsoccer-my-registrations.html.twig`.

Verified: only 3 routes in the entire 1707-line routing file carry `_csrf_token: 'TRUE'`, and none of them are these. The controller handlers check *ownership* (e.g. `removeMember` confirms the member belongs to the current user's group) but never validate a CSRF token. Worse, `invitation.accept` and `invitation.decline` have **no `methods:` restriction**, so they fire on a plain GET — an `<img src=".../invitation/42/accept">` on any web page is enough.

Every logged-in player is affected. A malicious page can silently force a victim to accept/decline invitations, leave their group, remove teammates, or fire invite/nudge emails in their name. This has been open since the June 13 review, which also rated it critical. It is the single most important thing to fix before the launch email.

**Fix:** Add `_csrf_token: 'TRUE'` to all seven routes; restrict accept/decline to `methods: [POST]`; render a token in each form (cleanest is to convert these hand-rolled Twig `<form>` buttons to Form API, or emit `csrf_token('group-action-' ~ id)` as a hidden field and validate it in the controller). This is a half-day of work and closes the biggest hole.

---

## HIGH

### B · No idempotency guard on checkout completion → replayed event re-spends credits

**File:** `src/EventSubscriber/OrderCompleteSubscriber.php` (`onOrderPlace`, ~76–89; `processCredits`, ~227–259).

`onOrderPlace` runs on every `commerce_order.place.post_transition` with no "already processed" flag. Registration *creation* is protected by existing-registration queries, but **credit spend and donation are not**. If the place transition is ever re-dispatched — an admin re-placing an order, a re-saved order, a gateway retry that re-triggers the workflow, or future code calling `applyTransitionById('place')` again — `useCredits()` and `donateCredits()` run a second time and deduct the player's balance again with no offsetting effect on the (already-locked) order total. This silently burns real credit balance.

**Fix:** At the top of `onOrderPlace`, check `$order->getData('ccsoccer_completion_processed')`; return if set. Set and save it once processing succeeds. This one guard also hardens Findings C and F.

### C · Credits charged at checkout aren't reconciled with what's actually deducted

**Files:** `CheckoutPane/CreditsPane.php` (~302–325 apply, ~232 validate); `OrderCompleteSubscriber.php` (~227–259) → `Service/CreditManagerService.php` (`useCredits`, ~307–373).

The credit *discount* is locked onto the order total at the credits step. The *actual balance deduction* happens later at placement, FIFO, and silently caps at whatever is still available. Between those two moments the balance can drop (a concurrent order, an expired credit, an admin adjustment). Result: the order total was reduced by, say, $30 of credits but only $18 gets deducted — the league eats $12, with nothing flagged. `useCredits()` returns the actual amount used, but the return value isn't compared to what was charged.

**Fix:** In `processCredits`, compare `$actual_used` to the charged `amount_cents`; on mismatch, log an alert and set an order data flag for admin review. Ideally re-check the balance immediately before the place transition.

---

## MEDIUM

### D · Override extend/nudge/revoke are GET state-changes with no CSRF

**Routes:** `override.extend` (971), `override.nudge` (984), `override.revoke` (997). **Controller:** `OverrideController::extendOverride/nudgeOverride/revokeOverride`.

No `methods:` restriction and no `_csrf_token`; the handlers call `$override->save()` / `markCancelled()` directly. Gated by `manage waitlist`, so blast radius is admin-only, but a CSRF against a logged-in waitlist admin can revoke overrides or shift expirations. Convert to POST + `_csrf_token`, or route through a Drupal ConfirmForm (which gives CSRF for free).

### E · Jersey pane trusts submitted `variation_id`

**File:** `CheckoutPane/JerseySelectionPane.php` (~147–165).

`submitPaneForm` loads whatever `jersey_variation` id was posted and adds it to the cart with no check that it's actually a jersey (`JERSEY-%` SKU / jersey bundle). The `<select>` options are jersey-only, but a crafted POST can add any active variation — including a $0 or admin-only variation — into a registration order and reach completion. Price still comes from the variation (so this isn't direct price forgery), but it's an unintended-product-injection hole. The sibling `PlayerInfoPane::addJerseyToCart` resolves by expected SKU and is safe; mirror that here — verify the loaded variation's SKU/bundle before `addEntity`.

### F · Full-season race takes payment but creates no registration and no refund flag

**File:** `OrderCompleteSubscriber.php` (~309–344 season; team-full branches ~585, ~633).

When a season is full/inactive at completion, the code logs and returns without creating the registration — and the `TODO: Consider refunding` is unimplemented. The player has paid, the order shows complete, but no registration exists and nothing surfaces to an admin. The tournament team-full branches at least flag the order; the season branch doesn't. At minimum, set an order data flag (mirror the tournament branches) so these surface in an admin view; ideally trigger a refund workflow.

### G · `invitee_email` fee-waiver can be spoofed — and a note on group_id (mostly good news)

**Files:** `TournamentTeamPane.php` (`getInvitedTeams`, ~506–544); `Commerce/TeamPaidOrderProcessor.php` (~78–136); `OrderCompleteSubscriber.php` (~569–618).

**group_id (the thing that bit you before): fixed.** All three tournament checkout branches now route `group_id` through `getOrCreateTeamGroupId()`, which uses the Team's UUID and persists it. Season registrations use the invitation's UUID, consistent with `GroupController`. No legacy `team_<id>_<id>` string is written by any live path (it survives only in the historical migration hook). Two stale docblocks still *describe* the old format as current — `GroupInvitationsForm.php:24` and `Team.php:655-657` — and `Team::getRosterStats` still counts by team id "because group_id would be 0," a condition that no longer holds for checkout-created teams. Worth updating the comments and re-checking `getRosterStats` doesn't double-count, but the data bug itself is resolved.

**The remaining soft spot:** the "join an invited team" path resolves entitlement three ways, one being `invitee_email == account email`. Drupal doesn't re-verify email on change, so a user can set their account email to a known-invited address and pass the gate — and if that team is `team_paid`, get their registration fee zeroed. This is acknowledged in the `TeamPaidOrderProcessor` docblock and rated low-risk at $35. It stays low **only while team fees are small**; before you scale team-paid next year, gate the waiver on the `invitee` entity-reference match (not email), or require verified email.

### H · User/email enumeration and directory harvest via invite autocomplete

**File:** `GroupController::userSearch` (~594–693).

The invite autocomplete returns real name **and email address** for any match, to any logged-in user, at 30 queries/min. Self-registration is open, so one throwaway account can harvest the member directory (names + emails) — a spam/phishing goldmine right as you email everyone. The invite flood limits (10/5min invite, 5/10min nudge, 48h per-invite cooldown) are good and mitigate email-bombing, but they don't stop *reading* the directory. Return only display name + an opaque uid from the autocomplete (not raw email), and/or restrict the endpoint to users who actually have a group to invite into. Minor related nit: this one search site omits `escapeLike($term)` (the other three include it) — not SQLi (still parameterized), just inconsistent LIKE-wildcard handling.

---

## LOW

### I · Secrets in git

Confirmed still in the repo:
- `config/sync/commerce_payment.commerce_payment_gateway.authorize_net_test.yml` — Authorize.net **test/sandbox** `api_login` + `transaction_key`. Low severity (sandbox only), but flagged critical in two prior reviews and still not scrubbed. Your **live** credentials are correctly kept out of git (in `settings.local.php`), which is the important part.
- `config/sync/recaptcha.settings.yml` — the reCAPTCHA **secret** key (server-side-only by design). It's domain-locked and `recaptcha.settings` is in `config_ignore` (so it doesn't push to prod), which limits impact, but the secret still shouldn't live in the repo or in `SESSION_HANDOFF.md`.

For both: move values to `settings.local.php` config overrides, rotate the keys, and scrub history (BFG or `git filter-repo`). These are sandbox/domain-locked so this is cleanup, not an emergency.

### J · Permission hygiene / granularity (INFORMATIONAL — not an exposure)

**Correction (Jul 3):** an earlier draft of this finding described `view reports`
as "exposing every player's credit ledger," which was misleadingly alarmist. To be
clear: **nothing here is visible to players or the public.** Verified against
config — `view reports` is granted only to `board_member`, `tournament_director`,
and `administrator`. The reports pages (`/admin/ccsoccer/reports/*`), the Credits
Overview (`/admin/ccsoccer/credits`), and per-player credit ledgers
(`/user/{user}/ccsoccer-credits`) are all board/TD/admin only. Regular players,
anonymous users, `beta_tester`, and `content_editor` do **not** have access.

What remains are two minor, non-urgent points:

1. **`restrict access: true` hygiene flag.** Previously only `administer ccsoccer`
   set it. This flag doesn't change who has access — it only makes Drupal's
   permissions page show the "security-sensitive, grant with care" warning. Added
   Jul 3 to the financial / PII / mass-communication permissions: `view reports`,
   `issue credits`, `manage registrations`, `manage waitlist`, `send notifications`,
   and the four report permissions. Purely operational permissions (seasons,
   rosters, schedules, game status, team names, series rules) intentionally left
   without it.

2. **`view reports` is coarse (optional split).** It bundles two capabilities —
   seeing aggregate reports AND viewing any single player's full credit history
   (`PlayerCreditsController::access`). Consequence: a **tournament director** also
   gets the ability to page through every player's credit ledger. If board members
   and tournament directors seeing player credit balances is acceptable (they're
   trusted officials — arguably it's their job), there is nothing to fix. Only if
   you want a role that sees tournament reports but **not** individual financial
   histories would you split "view arbitrary users' credit ledgers" into a dedicated
   finance permission. Deferred as a design choice, not a defect. (The `view
   reports` description was also updated in-code to state that it grants viewing any
   player's credit history, so the coarse scope is visible when assigning roles.)

### K · Prior open items still present (from June 13)

Lower-urgency but not yet done: raw `$e->getMessage()` returned to the browser in several JSON responses (H-1); inline `onchange` handler in `ReportController.php:393` (H-2 — the interpolated value is server-built, so not directly injectable, but it defeats CSP); admin free-text `notes` written unescaped in the deposit forfeit/refund forms (M-1 — currently *not rendered* anywhere, so latent, becomes live only if a future template prints it with `|raw`/`#markup`). Standardize render arrays on `#plain_text`/`Html::escape()` when you touch these.

---

## Corrected / non-issues (don't spend time here)

- **"CAPTCHA/reCAPTCHA modules are disabled" — FALSE.** In Drupal's `core.extension.yml`, a module line like `captcha: 0` means *enabled with weight 0*, not off. `captcha`, `image_captcha`, and `recaptcha` all sit under the `module:` section (lines 3–86), so all three are enabled. Additionally, `recaptcha.settings` and `captcha.captcha_point.*` are in `config_ignore`, so the captcha configuration is environment-specific and the disabled state you'd see in `config/sync` is the *local* dev state by design (per your own SESSION_HANDOFF: "Domain-locked to production. LOCAL intentionally disabled"). **The real, smaller action:** load the production contact / register / login / password-reset forms and confirm the reCAPTCHA widget actually renders and blocks — because that config is `config_ignore`'d and can't be verified from the repo. The contact-form spam you're seeing is better addressed with the geo-blocking already on your TODO (Smart IP + Country Block) than by touching captcha.
- **SQL injection in the recently-changed search code — none.** All four multi-term search sites use `escapeLike()` + parameterized entity/DBTNG queries. Safe.
- **Open redirect — none.** All redirects use server-generated `Url::fromRoute()`, not request input.
- **Email header injection — none.** Recipients are validated with `filter_var(...FILTER_VALIDATE_EMAIL)`; subject/body are server-side templates.
- **File upload (profile pictures) — safe.** Extension + size validated, MIME re-checked via `getimagesize()`, admin-gated. (Site-wide `article`/`media` image fields have no `max_filesize` — a minor DoS/disk nit, not urgent.)
- **Masquerade — no escalation.** Gated to the `administrator` role; unmasquerade link is CSRF-protected.
- **Prices are never trusted from the client.** Season/tournament/deposit prices come from the product variation server-side; registration quantities are forced to 1; discount percent is clamped to `0 < p ≤ 100`. Anonymous purchase of registration products is blocked at the entry routes and again in `OrderCompleteSubscriber`.

---

## Recommended order before the launch email

1. **A** — CSRF on the seven group routes (+ POST-only on accept/decline). The one true critical.
2. **B** — Idempotency guard in `onOrderPlace`. Cheap, protects real money, also shores up C and F.
3. **C, F** — Credit reconciliation + flag the paid-but-unregistered season race.
4. **H** — Stop returning raw emails from the invite autocomplete (before you hand everyone a login).
5. **D, E** — CSRF on override GET routes; jersey-variation allow-list.
6. **Verify on production:** reCAPTCHA widget actually renders on contact/register/login/pass; `dev_mode = FALSE`; Devel uninstalled; `error_level: hide`; `hash_salt` and `trusted_host_patterns` set. (Several are in your existing pre-launch checklist.)
7. **Then cleanup:** G (docblocks + email-waiver gating before team fees scale), I (scrub/rotate secrets), J (permission `restrict access`), K (prior escaping items).

Nothing here blocks the soft launch you've already done. Items A, B, C, F, and H are the ones I'd want closed before the mass email drives real traffic and real bots at the site.