# CC Soccer — Security Review
**Date:** June 13, 2026
**Reviewed by:** Claude (Cowork session)
**Scope:** Full codebase review of the custom `ccsoccer` module — PHP controllers, forms, services, routing, JS, Twig templates, and configuration. Cross-referenced against the March 16, 2026 assessment.

---

## Context

The March 16, 2026 assessment identified 16 findings. Several were fixed; several were deferred to "production deployment." The site is now live at ccsoccer.com. This review re-examines what is still open and identifies new issues introduced since March.

---

## Summary

| Severity | Count | Action |
|----------|-------|--------|
| **CRITICAL** | 2 | Fix before removing IP whitelist |
| **HIGH** | 3 | Fix within days |
| **MEDIUM** | 4 | Fix before public announcement |
| **LOW** | 2 | Plan for near term |

---

## CRITICAL

### C-1 · Authorize.net Test Credentials Still in Git

**File:** `config/sync/commerce_payment.commerce_payment_gateway.authorize_net_test.yml`

The file still contains plaintext credentials:
```
api_login: 6GbB775ypB
transaction_key: 3Jp868acL35znH76
client_key: 2EtDfbm3fsreuWQc9Hn43uxEySh693h8WYe7Lkp3pc68DTPDgC2MTBwyg646UA7p
```

This was flagged CRITICAL in March and deferred to "production deployment." The site is now live. Even though these are test-mode credentials, they are permanently baked into git history and are callable against the Authorize.net sandbox. If the production gateway ever shared an API login or if these were reused, the exposure would be real.

**Fix:** Remove the values from the YAML, move them to `settings.local.php` as config overrides, and rotate both test and production credentials:

```php
// settings.local.php (test server)
$config['commerce_payment.commerce_payment_gateway.authorize_net_test']['configuration']['api_login'] = 'YOUR_LOGIN';
$config['commerce_payment.commerce_payment_gateway.authorize_net_test']['configuration']['transaction_key'] = 'YOUR_KEY';
$config['commerce_payment.commerce_payment_gateway.authorize_net_test']['configuration']['client_key'] = 'YOUR_CLIENT_KEY';
```

Then scrub the old values from git history using BFG Repo Cleaner:
```bash
bfg --replace-text credentials.txt ccsoccer-d11.git
git reflog expire --expire=now --all && git gc --prune=now --aggressive
git push --force
```

---

### C-2 · Group Action Forms Have No CSRF Protection

**Files:** `templates/ccsoccer-group-manage.html.twig`, `templates/ccsoccer-my-registrations.html.twig`
**Routes:** `ccsoccer.invitation.accept`, `ccsoccer.invitation.decline`, `ccsoccer.group.leave`, `ccsoccer.group.remove_member`, `ccsoccer.group.nudge`, `ccsoccer.group.delete`, `ccsoccer.group.invite`

These player-facing actions are implemented as raw HTML `<form method="post">` buttons with no CSRF token and no `_csrf_token: 'TRUE'` in `ccsoccer.routing.yml`. A malicious page can trigger them silently against any logged-in player:

```html
<!-- Attacker's page — silently removes a player from their group -->
<form action="https://ccsoccer.com/my-group/42/leave" method="post">
  <input type="submit" value="Click here for free jerseys">
</form>
```

The March fix correctly added `fetchWithCsrf()` to all AJAX/JS routes, but the Twig form-based routes were missed. This is the highest-risk unfixed issue because it affects every logged-in player.

**Fix:** Add `_csrf_token: 'TRUE'` to each of the seven routes above, then inject the token into each Twig template. Drupal generates the token via `url.path.is_front` or you can pass it from the controller:

```php
// In GroupController::manage()
$build['#attached']['drupalSettings']['ccsoccer']['csrfToken'] =
  \Drupal::service('csrf_token')->get('group-actions');
```

Or the simpler approach — include the token as a hidden field generated in each form:

```twig
{# In each <form> block #}
<input type="hidden" name="csrf_token" value="{{ csrf_token('group-action-' ~ registration.id()) }}">
```

And validate it in each controller method:
```php
$token = $request->request->get('csrf_token');
if (!\Drupal::service('csrf_token')->validate($token, 'group-action-' . $registration->id())) {
  throw new \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException();
}
```

---

## HIGH

### H-1 · Exception Messages Returned to Browser

**Files (examples):**
- `PlayerSkillController.php` — `'error' => 'Failed to save: ' . $e->getMessage()` (returned in JSON response)
- `TournamentScheduleController.php` — `'message' => 'Error: ' . $e->getMessage()` (returned in JSON response)
- `CancelRegistrationForm.php` — `'error' => $e->getMessage()` (returned in JSON response)
- `TournamentCancelRegistrationForm.php` — `'error' => $e->getMessage()` (JSON + messenger)
- `ReportController.php` — `addError('Error generating report: ' . $e->getMessage())` (shown to user)
- `RosterBuilderController.php` — `$this->t('Error: @error', ['@error' => $e->getMessage()])` (shown to user)

Raw exception messages can expose database schema, file paths, table names, internal logic, and stack details. This was flagged in March (#12) and is not yet fixed.

**Fix:** Log the full message, return a generic one to the client:

```php
} catch (\Exception $e) {
  $this->logger->error('Save failed for user @uid: @msg', [
    '@uid' => $this->currentUser()->id(),
    '@msg' => $e->getMessage(),
  ]);
  return new JsonResponse(['error' => 'Save failed. Please try again or contact support.'], 500);
}
```

---

### H-2 · Inline `onchange` Handler in ReportController

**File:** `src/Controller/ReportController.php`, line 393

```php
'#attributes' => ['onchange' => 'window.location.href = "/admin/ccsoccer/reports/tournament-deposits?tournament=" + this.value'],
```

Inline event handlers bypass Content Security Policy and are a marker for XSS risk. This was flagged in March (#8) and is still present.

**Fix:** Remove the inline handler and move it to a JS file that uses `drupalSettings` for the URL:

```php
// Controller
'#attributes' => ['class' => ['tournament-deposit-select']],
'#attached' => ['library' => ['ccsoccer/report-navigation']],
```

```js
// report-navigation.js
once('tournament-deposit-select', '.tournament-deposit-select').forEach(function(el) {
  el.addEventListener('change', function() {
    window.location.href = '/admin/ccsoccer/reports/tournament-deposits?tournament=' + this.value;
  });
});
```

---

### H-3 · Authorize.net Test Credentials Not Yet Moved to settings.local.php on Production

This is the configuration-management companion to C-1. Even after scrubbing git history, you need to confirm that the **production** `settings.local.php` is passing live credentials via config overrides — and that the production config export does **not** include the `authorize_net_test` gateway at all (since it shouldn't exist in production). Verify with:

```bash
ssh ccsoccer
grep -r "api_login\|transaction_key\|client_key" ~/ccsoccer-d11/config/
```

---

## MEDIUM

### M-1 · Stored XSS Risk in Admin Notes Fields

**Files:** `TournamentDepositForfeitForm.php`, `TournamentDepositRefundForm.php`

Admin users write free-text `notes` to registrations. If those notes are ever rendered via `Markup::create()` or `#markup` without escaping, a compromised admin account (or a social-engineering attack on an admin) could inject persistent XSS. The March report (#11) flagged this and it remains open.

**Fix:** When reading notes back for display, always use `Html::escape()`:

```php
use Drupal\Component\Utility\Html;
// ...
'#markup' => Html::escape($registration->get('notes')->value ?? ''),
```

Or use `#plain_text` in the render array instead of `#markup`.

---

### M-2 · Several Admin AJAX Routes Lack `_csrf_token`

**Routes (all admin-permission-gated, but still worth adding):**
`ccsoccer.roster_builder_save`, `ccsoccer.roster_builder_move`, `ccsoccer.roster_builder_merge_to_group`, `ccsoccer.roster_builder_create_group`, `ccsoccer.roster_builder_reorder_teams`, `ccsoccer.schedule_builder_move`, `ccsoccer.schedule_builder_swap`, `ccsoccer.schedule_builder_workbench_*`, `ccsoccer.season_schedule_snapshot_*`, and tournament equivalents.

These are all behind `_permission: 'manage seasons'` or `'generate rosters'`, and the JS that calls them does use `fetchWithCsrf()` — so the risk is lower. However, defense-in-depth means the route itself should also declare `_csrf_token: 'TRUE'` so a future refactor (or a direct curl call from a compromised admin session) cannot bypass it.

**Fix:** Add `_csrf_token: 'TRUE'` to all POST routes in `ccsoccer.routing.yml`. For routes where the JS already sends the token via `X-CSRF-Token` header, Drupal will validate it automatically when the route requires it.

---

### M-3 · Inconsistent `Html::escape()` vs `htmlspecialchars()`

The codebase mixes two escaping approaches in render arrays:
- Some places use `htmlspecialchars($value)` (correct for raw string contexts)
- Some places use `'#markup' => $value` without any escaping (risky)
- The `Markup::create()` calls signal "this is safe HTML" to Drupal's renderer — which suppresses auto-escaping

This was March finding #15. The safest convention for render arrays is:
- Use `'#plain_text'` for plain strings (Drupal escapes automatically)
- Reserve `Markup::create()` only for HTML you've fully constructed yourself with all user data escaped
- Never use `'#markup' => $user_data` without escaping

---

### M-4 · Entity Access Not Checked on Season/Tournament Clone Parameters

**Files:** `SeasonForm.php`, `TournamentForm.php`

When cloning a season or tournament, the source entity ID comes from a URL parameter. If the form doesn't verify the current user has access to the source entity, an admin-level user could clone entities they shouldn't be able to read. This was March finding #16.

**Fix:** Add an explicit access check in `buildForm()`:

```php
if ($source_season_id) {
  $source = $this->entityTypeManager->getStorage('season')->load($source_season_id);
  if (!$source || !$source->access('view', $this->currentUser())) {
    throw new \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException();
  }
}
```

---

## LOW

### L-1 · `settings.local.php` Contains Clickatell Plaintext Password

**File:** `web/sites/default/settings.local.php` (local dev only — not in git)

The local dev `settings.local.php` has:
```php
$config['ccsoccer.notifications']['clickatell_password'] = 'S0cc3r';
```

This file is `.gitignore`-protected and shouldn't reach production. However, the fact that it's a short, guessable password (`S0cc3r`) is worth noting — if the Clickatell API credentials were ever compromised, this password provides little resistance. Recommend rotating to a strong random password in the Clickatell portal, and storing it as an environment variable rather than inline in the settings file.

---

### L-2 · `dev_mode` and Error Verbosity Must Be Confirmed Off on Production

The local `settings.local.php` sets `$settings['dev_mode'] = TRUE`. Production must have `FALSE`. Likewise, error display must be hidden and the Devel module uninstalled. These were March items (#9, #10) listed as "do at production deployment."

Verify production state with:
```bash
ssh ccsoccer
cd ~/ccsoccer-d11
vendor/bin/drush pml --status=enabled | grep devel
vendor/bin/drush cget system.logging error_level
```

Expected: devel not in the list; `error_level: hide`.

---

## Status of March 2026 Findings

| # | Finding | March Status | Current Status |
|---|---------|-------------|----------------|
| 1 | Authorize.net credentials in git | 🔶 Deferred | ❌ **Still in repo — NOW CRITICAL** |
| 2 | Database dump in repo | ⚠️ Delete | ✅ No .sql files found in repo root |
| 3 | Empty hash_salt | 🔶 Deferred | 🔶 Confirm production override is set |
| 4 | Trusted host patterns | 🔶 Deferred | 🔶 Confirm production `settings.local.php` has patterns |
| 5 | Missing CSRF on AJAX (JS) | ✅ Fixed | ✅ `fetchWithCsrf()` confirmed in all JS |
| 6 | Insufficient access checks | ✅ Fixed | ✅ Verified `hasPermission()` on AJAX controllers |
| 7 | XSS via innerHTML (JS) | ✅ Fixed | ✅ Confirmed safe DOM methods |
| 8 | Inline JS event handler | ⚠️ Still needed | ❌ **Still present — ReportController.php:393** |
| 9 | Devel module | 🔶 Deferred | 🔶 Verify disabled on production |
| 10 | Dev services config | 🔶 Deferred | 🔶 Verify `settings.local.php` not deployed |
| 11 | Stored XSS in notes fields | ⚠️ Still needed | ❌ **Still open** |
| 12 | Error messages expose details | ⚠️ Still needed | ❌ **Still open — multiple files** |
| 13 | Rate limiting | ✅ Fixed | ✅ Flood control confirmed on key routes |
| 14 | CSP headers | 🔶 Deferred | 🔶 Implement before public launch |
| 15 | Inconsistent HTML escaping | ⚠️ Still needed | ❌ **Still open** |
| 16 | Entity access on clone params | ⚠️ Still needed | ❌ **Still open** |

**New findings since March:**

| # | Finding | Severity |
|---|---------|----------|
| N-1 | Group action forms have no CSRF protection | **CRITICAL** |
| N-2 | Exception messages returned to browser | **HIGH** (consolidated from March #12) |

---

## Recommended Fix Order

**Before removing the IP whitelist:**

1. **C-2** — Add CSRF tokens to the 7 group action routes and Twig forms
2. **C-1** — Remove Authorize.net credentials from the config YAML, rotate them, and scrub git history
3. **H-1** — Replace `$e->getMessage()` with generic errors in JSON responses and messengers

**Before public announcement:**

4. **H-2** — Move the `onchange` handler out of `ReportController.php`
5. **M-1** — Escape notes field output
6. **M-2** — Add `_csrf_token: 'TRUE'` to all remaining admin POST routes
7. Confirm production checklist: `hash_salt`, `trusted_host_patterns`, `dev_mode = FALSE`, Devel uninstalled, error level hidden, CSP headers active

**Near term:**

8. **M-3** — Standardize on `#plain_text` / `Html::escape()` across render arrays
9. **M-4** — Add access check on clone source entity in SeasonForm/TournamentForm
10. **L-1** — Rotate Clickatell password to a strong random value