# Security Hardening Batch — Implementation Plan

**Source:** SWE/QA audit of v7.33.0 (4-agent sweep + parent verification on disk).
**Scope decision:** Security + coupled efficiency (PR1–4). Pure dedup (`_strip_html`×11, `load_course_config`×9), N+1 page-body fetches (E3), dead-code delete (`push_bridge_handler.py`), and test-gap backfill are **deferred** to a follow-up batch.
**S4 decision:** Both sanitize + sandbox (defense in depth).
**Process:** PR-only. Coder = Codex (verify all writes on disk — Codex runtime is non-persisting on this host; fallback = Sonnet sub-agent coder). Reviewer ≠ coder. Per-PR gate = full `pytest tests/ -q` green + diff read by reviewer. Merge only after both.

File sets are **disjoint across PRs** → independently mergeable. Recommended merge order PR1 → PR2 → PR3 → PR4 (criticality). `requirements.txt` is the only possible shared touch (PR2); PR3/PR4 don't touch it.

---

## PR1 — Token egress: route rogue pagination through canvas_api (S1 + E2)

**Problem.** The v7.8.0 next-link domain guard lives only in `canvas_api.paginated_get`. Three scripts re-implement pagination and send `Authorization: Bearer` to whatever URL a response's `Link: rel="next"` names — full-privilege Canvas PAT to an arbitrary host if a Canvas response is MITM'd/compromised. `criterion_evaluator` additionally `break`s mid-pagination on any non-200 (silent truncation → false audit findings under 429) and has no retry.

**Files:** `scripts/setup_env.py`, `scripts/criterion_evaluator.py`, `scripts/audit_image_alt.py`. New tests in `tests/test_security_fixes.py`.

**Changes.**
1. `setup_env._fetch_all_courses` (`:63-74`): replace the manual `while url:` loop with `canvas_api.paginated_get(url, headers, params=params, canvas_domain=env['domain'])`. Preserve the existing `(courses, error)` return contract: wrap in `try/except (ValueError, requests.RequestException) as e: return None, str(e)`. (`paginated_get` raises on off-domain next-link and on persistent HTTP failure — both map to the existing error channel.)
2. `criterion_evaluator._api_get_all` (`:125-139`): replace loop body with `canvas_api.paginated_get(url, cfg['headers'], params=params, canvas_domain=cfg['domain'])`. `cfg['domain']` confirmed present (`:147,242`).
3. `criterion_evaluator._api_get` (`:116-122`): for the `path.startswith("http")` branch, validate before use — `canvas_api._validate_canvas_response_url(path, cfg['domain'], "criterion_evaluator absolute path")`. Route the GET through `canvas_api._request_with_retry(requests.get, url, cfg['headers'], params=params or {})` so single-object fetches get retry too.
4. `audit_image_alt` `_get` (`:570-580`): replace loop with `canvas_api.paginated_get(url, headers, params=params, canvas_domain=domain)`.

**E2 note.** Routing `criterion_evaluator` through `paginated_get` means a persistent rate-limit now raises instead of silently truncating. `collect_course_data` callers must catch and surface "partial/failed data" rather than reporting truncated counts as authoritative. Add a guard in `collect_course_data` that lets the exception propagate to the report layer with a clear "audit aborted: Canvas API error" message. **Do not** swallow it back into a partial result.

**Acceptance criteria.**
- [ ] All three functions reject an off-domain `next` link (mock a `Link: <https://evil.example/...>; rel="next"` response → raises/returns error; no second GET issued to evil.example). New regression tests assert this.
- [ ] `criterion_evaluator._api_get` rejects an absolute off-domain `path`.
- [ ] A simulated 429 mid-pagination retries (does not truncate); persistent failure raises, and `collect_course_data` surfaces it as an error, not a partial count.
- [ ] `setup_env._fetch_all_courses` preserves `(list, None)` / `(None, errstr)` contract — existing callers unchanged.
- [ ] Full suite green.

---

## PR2 — Staging-server hardening (S2 + S3 + S4 + S7 + S8)

**Files:** `scripts/staging_server.py`, `scripts/unified_staging_review.py`, new `scripts/html_sanitize.py`, new `tests/test_staging_server_security.py`.

### S2 — Host-header validation (DNS-rebinding read-exfil = M3 read side)
Add `_check_host(self)`: build the allowed set from the bound port (`self.server.server_address[1]`) → `{f"127.0.0.1:{port}", f"localhost:{port}"}`. Reject (`403`) when `Host` is missing or not in the set. Call it at the **top of every** `do_GET`, `do_HEAD`, `do_PUT`, `do_DELETE`, `do_OPTIONS`. Make `ALLOWED_ORIGINS` port-aware from the same source (currently hardcodes `:8111`, `:2004-2007`).
- [ ] `Host: evil.com` → 403 on GET and HEAD. `Host: 127.0.0.1:{port}` → served. Missing Host → 403.

### S3 — Kill wildcard CORS + double-emission
`_cors_headers` (`:2117-2120`) hardcodes `Access-Control-Allow-Origin: *`, emitted twice (explicit calls in `_serve_html`/`_json_response`/`handle_staging_file_get` **and** the `end_headers` override at `:2122-2124`). Replace: reflect the request `Origin` **only if** in the (port-aware) allowlist, else emit no ACAO. Consolidate to a single emission — remove the explicit `_cors_headers()` calls before `end_headers()`, keep only the `end_headers` override path; confirm every response routes through `end_headers`.
- [ ] Exactly one `Access-Control-Allow-Origin` header per response; value is an allowlisted origin or absent; never `*`. (Don't rely on the current duplicate-header accident.)

### S4 — Stored XSS: sanitize **and** sandbox
Untrusted bodies (Canvas API page/assignment/discussion bodies, quiz feedback/description) render raw on the privileged `127.0.0.1:8111` origin, which is in the write-allowlist → injected script can drive PUT/DELETE + exfiltrate.

**Sanitize layer — vendored stdlib allowlist (`scripts/html_sanitize.py`), no new dependency.** Recommendation: do **not** add `nh3`/`bleach`. `nh3` is a compiled wheel; `ensure_deps.sh` installs `requirements.txt` at SessionStart across heterogeneous ID machines (incl. the managed-python fallback) — adding a compiled dep expands the very supply-chain surface S5/L2 flag. Implement a small `HTMLParser`-based allowlist sanitizer:
- drop `<script>`, `<style>`, `<iframe>` (except the wrapper we add), `<object>`, `<embed>`, `<link>`, `<meta>`, `<base>`;
- strip every `on*` attribute and any attribute whose value (after entity-decode + whitespace strip + lowercase) begins `javascript:`/`data:text/html`/`vbscript:`;
- keep a conservative tag/attr allowlist sufficient for Canvas content (headings, p, ul/ol/li, a[href], img[src,alt], table family, span/div with style, strong/em, blockquote, pre/code).
(If the reviewer judges a hand sanitizer too risky to own, fall back to `nh3` with a documented sha256-pinned wheel — but sandbox is the hard barrier, so an imperfect sanitizer behind the sandbox is acceptable.)

**Sandbox layer — primary barrier.** Render every untrusted body inside `<iframe sandbox srcdoc="{html_escaped_sanitized_body}" ...>` with `sandbox=""` (no `allow-scripts`, no `allow-same-origin` → null origin, scripts inert, no reach to the staging API). Apply at `unified_staging_review.py:842-854` (page/assignment/discussion/syllabus) and `staging_server.py:661` + the quiz description card (quiz feedback). Height: set a generous `min-height` + `style="width:100%;border:0"`; cross-origin auto-resize is out of scope (scroll acceptable) — note in code comment.
- [ ] A staged page containing `<script>document.title='XSS'</script>` and `<img src=x onerror="fetch('/api/staging/1/pages/x',{method:'DELETE'})">` opened at `/courses/{id}/review`: **no** script execution (verify via Playwright/Preview MCP — no console eval, no DELETE network request), content still visible.
- [ ] Sanitizer unit tests: script/style/iframe stripped; `onerror`/`onclick` stripped; `href="javascript:..."` neutralized; benign tags/attrs preserved.

### S7 — Override `do_HEAD`
Currently unoverridden → falls to `SimpleHTTPRequestHandler.send_head`, serving existence + size of any plugin-root file (`HEAD /.env` → 200 + Content-Length). Override to `405` (or route through the same allowlist as `do_GET`). Run `_check_host` first.
- [ ] `HEAD /.env` → 405 (not 200-with-Content-Length).

### S8 — Remove symlink containment bypass
`_resolve_course_dir` (`:192`): drop the `or raw_course_dir.is_symlink()` clause so containment under `courses_root` is always enforced.
- [ ] A symlinked course dir whose target is outside `courses/` → `ValueError`, not served.

**PR2 acceptance:** all sub-boxes above + full suite green + a browser-verified XSS-no-exec demonstration attached to the PR.

---

## PR3 — Supply chain (S5 + S6 + S9)

**Files:** `hooks/ensure_deps.sh`, `.github/workflows/release.yml`, `.github/workflows/ci.yml`, `.github/workflows/sync-coda-version.yml`.

### S5 — Pin the uv installer (open item M2)
`ensure_deps.sh:63` `curl -LsSf https://astral.sh/uv/install.sh | sh` — unpinned pipe-to-shell RCE surface at SessionStart. Replace with: pin a specific uv version, download its versioned installer to a temp file, `shasum -a 256 -c` against a hash pinned in the script, run only on match, else `warn` + `exit 0` (matches existing failure behavior). Coder fetches the exact installer + hash for the chosen uv version at implementation time and records both in the script with a comment noting how to refresh.
- [ ] Tampered installer (hash mismatch) → aborts, never executes. Happy path still installs uv on a no-system-python machine.

### S6 — SHA-pin GitHub Actions
Pin `actions/checkout`, `actions/setup-python`, `softprops/action-gh-release` to full 40-hex commit SHAs with a trailing `# vX` comment. Coder resolves each tag → SHA at implementation time.
- [ ] Every `uses:` across the three workflow files references a 40-hex SHA.

### S9 — Exclude `.env` from the release zip (defense in depth)
`release.yml:27-32` zip `-x` list omits `.env`. Add `"*/.env" "*.env"`. (Safe today — built from `actions/checkout`, `.env` untracked — but don't rely on `.gitignore` alone.)
- [ ] `-x` list includes the `.env` globs.

**Note:** workflow files alone qualify for the direct-to-`main` exception (repo config gating the pipeline), but `ensure_deps.sh` is plugin code — so PR3 ships as one PR through the normal pipeline.

---

## PR4 — Canvas client efficiency (E1)

**Files:** `scripts/canvas_api.py`. Tests: extend existing `canvas_api` tests.

**Changes.**
1. Add a module-level `requests.Session` (lazy singleton, e.g. `_session()` returning a cached `requests.Session()`); use it inside `_request_with_retry`. Connection reuse removes a TCP+TLS handshake per call.
2. Replace the unconditional `if resp.status_code in (200,201): time.sleep(INTER_REQUEST_DELAY)` (`:193-195`) with a **write-only** delay: sleep only after `POST`/`PUT`/`DELETE`, not GET. Keep the proactive `X-Rate-Limit-Remaining < 10 → sleep(1.0)` throttle (`:188-190`) and all 429/5xx backoff unchanged.

**Risk (flagged for reviewer).** Dropping the GET inter-request delay could increase 429s against the shared `canvas.asu.edu` instance under burst audits. Mitigation already present: the proactive low-remaining throttle + 429 retry/backoff. Reviewer must stress-confirm a GET-heavy audit (e.g. criterion_evaluator on a large course) does not regress into sustained 429s; if it does, fall back to a smaller fixed GET delay (e.g. 0.05s) rather than zero.

**Acceptance criteria.**
- [ ] `requests.Session` reused across calls (no per-call handshake).
- [ ] GET path issues no fixed `sleep`; write path retains a delay; low-remaining throttle + 429 backoff intact.
- [ ] Existing `canvas_api` tests pass unchanged; full suite green.

---

## Release

After PR1–4 merge: bump `.claude-plugin/plugin.json` + `.codex-plugin/plugin.json` + `VERSION` together, add `## X.Y.Z` CHANGELOG section, tag `vX.Y.Z`. Semver: all changes are `fix`/hardening → patch bump, **unless** the S4 sanitizer is treated as a new capability (`feat` → minor). Recommend **minor** (S4 adds `html_sanitize.py` + behavior). Coda sync auto-fires on tag; engineer re-attaches release zip to the Coda page after sync.
