# PRODUCT-3535 Fix Plan (for review — do not implement until approved)

## Affected release lines (both required)

PRODUCT-3535 applies to **two maintained plugin lines** in this repository. The same vulnerable AJAX / URL-building pattern exists on each; fixes and validation must cover **both**.

| Line | Current example | Distribution | Repo pointer |
|------|-----------------|--------------|--------------|
| **1.3.x** | `1.3.12` (`master`, wordpress.org stable) | **Automatic** updates via the WordPress plugin directory | `master`, tags `1.3.*` |
| **2.0.x** | `2.0.7` (tag `2.0.7`) | **Manual** update for sites on the 2.0 line today | tag `2.0.7`, future `2.0.x` tags |

**Implementation expectation:** land the security patch on `master` for the next **1.3.x** wordpress.org release, and **cherry-pick / parallel-release** the same changes onto the **2.0.x** line (tagged `2.0.8+` or as your release process dictates). Do not fix only one line.

**Validation expectation:** record **red** and **green** evidence per line (see [PRODUCT-3535-video-validation.md](PRODUCT-3535-video-validation.md)).

## Problem (confirmed)

CourseStorm WP plugin **1.3.x (≤ 1.3.12) and 2.0.x (≤ 2.0.7)** expose three state-changing `admin-ajax.php` actions without capability or nonce checks:

| Action | Handler | File |
|--------|---------|------|
| `coursestorm_options_save` | `CourseStorm_Admin::save_options()` | `admin/coursestorm-options.php` |
| `coursestorm_settings_sync` | `CourseStorm_Admin::sync_coursestorm_settings()` | `admin/coursestorm-options.php` |
| `coursestorm_sync` | `CourseStorm_Synchronize::run_wp_cron()` | `synchronize.php` |

Only the settings *page* checks `manage_options` (`api_key_options()`). The AJAX handlers do not. The admin JS already sends a `nonce`, but the PHP never verifies it.

Chained impact when `subdomain` is attacker-controlled:

1. **Broken access control / CSRF** — any logged-in user (including Subscriber), or CSRF against an admin, can change plugin settings.
2. **SSRF / host injection** — `CourseStorm_WP_API` concatenates `subdomain` into `https://{subdomain}.coursestorm.com/api/v2` and fetches with `wp_remote_get()` (no allow-list).
3. **Persistent XSS** — `/info` response is stored in `coursestorm-site-info` and later used to build front-end `<script src>` / CSS URLs in `templating.php`, so a poisoned `subdomain` like `evil.example/x` escapes to `evil.example`.

## Evidence

Validation is **manual screen recording** only — see [PRODUCT-3535-video-validation.md](PRODUCT-3535-video-validation.md). Link GREEN (and optionally RED) videos from the PR.

Historical note (1.3.12 baseline): a Subscriber with an invalid nonce could overwrite `coursestorm-settings`; host injection in API/embed URLs was possible. Patched in **1.3.13**.

## Fix (implemented in 1.3.13)

### 1. Authorize all three AJAX entry points

**Important:** do not put unconditional `current_user_can` / nonce checks at the top of `sync_coursestorm_settings()` or `run_wp_cron()` — both are also invoked from trusted internal paths:

- `sync_coursestorm_settings()` ← `save_options` / `_verify_api_credentials` / `start_sync`
- `run_wp_cron()` ← `add_option_coursestorm-settings` / `update_option_coursestorm-settings` hooks

Preferred approach: thin AJAX-only wrappers (or `if ( wp_doing_ajax() ) { ... }` guards at the registered AJAX callbacks) that:

```php
if ( ! current_user_can( 'manage_options' ) ) {
  wp_send_json_error( array( 'message' => 'Forbidden' ), 403 );
}
check_ajax_referer( /* action-specific nonce */, 'nonce' );
// then call the existing method
```

Wire `wp_ajax_*` hooks to those wrappers instead of the raw methods.

Nonce action names should match what `admin.js` / options templates already create:

- options save: existing settings form `_wpnonce` (align action string with `wp_nonce_field` / `check_ajax_referer`)
- manual sync: `coursestorm_manual_sync_nonce`
- settings sync: `coursestorm_manual_settings_sync_nonce`

`save_options()` is AJAX-only today → capability + nonce can live directly in that method (or its wrapper).
### 2. Validate subdomain before any URL build or HTTP

Add a strict validator, e.g. allow only `[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?` (single DNS label, no `/`, `.`, `:`, `@`, whitespace). Reject otherwise before:

- `CourseStorm_WP_API` / `CourseStorm_API` URL construction
- `update_option( 'coursestorm-settings' )`
- writing `coursestorm-site-info`

Normalize to lowercase. Prefer validating at the boundary (`save_options`, `sync_coursestorm_settings`) **and** defensively in the API constructor.

### 3. Harden outbound HTTP

In `lib/coursestorm-wp-api.php`, switch `wp_remote_get()` → `wp_safe_remote_get()`.

Optionally assert constructed host ends with `.coursestorm.{COURSESTORM_TLD}` before fetch.

### 4. Harden embed asset URLs

In `templating.php` (and any other emitters of site-info subdomain):

- Re-validate `coursestorm-site-info->subdomain` before enqueue
- Build URL only from the validated label (never raw concatenation of untrusted stored values)
- Skip enqueue if invalid

This closes the persistent XSS path even if a bad value was already stored.

### 5. Defense-in-depth for stored options

On read of `coursestorm-site-info` / settings subdomain, treat invalid values as unset (or clear them once on upgrade). Avoid leaving a poisoned option active after patch.

### Out of scope / non-goals for this fix

- Changing catalog sync business logic beyond authz + subdomain validation
- Reworking the async task nonce model in `wp-async-task.php` (separate surface; not the reported chain)
- Publishing exploit PoCs or contacting external hosts

## Manual verification (video)

Follow [PRODUCT-3535-video-validation.md](PRODUCT-3535-video-validation.md):

1. **GREEN** — patched **1.3.x** (this PR): authorization, subdomain validation, safe embed URLs.
2. **GREEN** — patched **2.0.x** after cherry-pick (separate release).
3. **RED** — optional baseline clips on vulnerable builds for Jira/history.

### What to show in the GREEN clip

| Check | Green expectation |
|-------|-------------------|
| Authorization | Subscriber cannot change settings; admin save still works |
| Host injection | Invalid subdomain labels rejected before API URL build |
| Embed URL | No script/CSS from attacker hosts; invalid stored labels omitted |
| HTTP API | Outbound catalog fetch uses `wp_safe_remote_get` (code review) |

Also verify admin happy path: Settings → CourseStorm, valid subdomain + nonces, front-end embed on `*.coursestorm.com`.

## Implementation order

1. Shared auth helper + wire into three AJAX entry points (stops BAC/CSRF immediately)
2. Subdomain validator + apply at save/sync/API construct
3. `wp_safe_remote_get` + embed URL hardening
4. Record **GREEN** video on **1.3.x**; attach to PR
5. Cherry-pick / release on **2.0.x**; record **GREEN** video
6. Bump plugin version / changelog per line (1.3.x wordpress.org + 2.0.x manual release)

## Review ask

Please approve or adjust this plan before implementation. In particular confirm:

1. Nonce action names / whether to reuse the existing settings `_wpnonce` for `coursestorm_options_save`
2. Whether invalid already-stored `coursestorm-site-info` should be auto-cleared on upgrade
3. Whether `wp_ajax_nopriv_*` should be explicitly absent (today they are; keep it that way)