audit(theme): defense-in-depth re-sanitization of REST inputs inside handlers

[JohnRDOrazio]

Background

PR #110 review surfaced the question: should the extracted REST handlers re-sanitize their input (`submitter_email`, `url`, `category`, `project_url`, etc.) on entry, even though every route's `args` block already declares a `sanitize_callback` (`sanitize_email` / `esc_url_raw` / `sanitize_text_field` / `sanitize_textarea_field`) that runs during REST dispatch?

Current convention across every extracted handler:

```
includes/handlers/
├── academic-collaboration.php
├── community-channel.php
├── deploy-translation.php
├── flush-opcache.php
├── link-translations.php
├── local-group.php
├── project-status.php
├── refer-community-project.php ← review flagged
├── refer-local-group.php ← review flagged
├── relationship.php
├── send-verification-code.php
├── submit-project-send-code.php
├── submit-project.php ← review flagged
├── team-member.php
├── translate.php
└── update-disposable-domains.php
```

None of them re-sanitize. They all trust the REST framework's `sanitize_callback` to have run by the time the handler sees the request — which is true under the standard `register_rest_route` dispatch path.

Question to settle

Should we flip the convention to defense-in-depth — re-sanitize on entry into the handler — and if so, apply it consistently across all 16 handlers?

Arguments for re-sanitizing:

• The REST args sanitize_callback only runs when the handler is invoked via REST dispatch. If the function is ever called directly (custom plugin code, AJAX shim, future refactor), the sanitisation chain is bypassed.
• The handler's public contract becomes self-contained — readers don't have to cross-reference the args block in functions.php to know what's safe.
• Consistent with WP-VIP / hardened-codebase conventions where every persistence sink and every outbound email is the last line of defense.

Arguments against:

• WP's REST framework is the standard ingress; bypassing it is a misuse pattern. Adding internal sanitisation suggests the function is callable from anywhere, which it isn't.
• Re-sanitising via `sanitize_email` / `esc_url_raw` etc. is idempotent (passing already-clean input through them returns the same value), so the runtime cost is small but the code noise per handler is real.
• 16 handlers × ~5 fields each = ~80 new lines of glue with no behavioural change under the normal call path.

Suggested scope (if accepted)

For each handler, immediately after the `function_exists` guards but before any logic that touches request values:

```php
$email = sanitize_email((string) $request['submitter_email']);
$url = esc_url_raw((string) $request['url']);
$location = sanitize_text_field((string) $request['location']);
$description = sanitize_textarea_field((string) $request['description']);
// ... etc — only the fields the handler actually reads
```

Then replace every `$request['foo']` reference with the corresponding sanitised local. Tests would assert that the persisted post meta / outgoing emails contain the sanitised values, not the raw `$request` input.

Out of scope here

• Sanitising fields the handler doesn't read (no churn for parameters that flow only through the REST args block to never be touched again).
• Validation beyond sanitisation — `is_email()` etc. stay where they are.
• Plugin tree (`wordpress/plugins/cdcf-redis-translations/`) — its three handlers are simpler and don't read URL/email inputs.

Why this is a separate issue, not part of any single PR

Re-sanitisation is a cross-cutting decision. If we apply it to three of the sixteen handlers in isolation (which is what review on #110 effectively proposed), we get:

• Inconsistency between defensive and trusting handlers — future contributors don't know which pattern to follow.
• A "did we get them all" review burden each time a new handler lands.

Resolving the convention once across all 16 is cheaper than tracking the inconsistency over the long tail.

Related

• tests(theme): extend PHPUnit coverage to the remaining functions.php endpoints + helpers #107 — parent test-coverage issue
• audit(theme): handle write-call failures consistently across extracted handlers #109 — sibling cross-handler audit (write-failure handling)
• tests(theme): cover public-submission trio + send-code helpers (refs #107) #110 — the review that surfaced this question

[JohnRDOrazio]

Closed by #120 — resolved as won't-do with the convention documented in AGENTS.md.

Decision

Defense-in-depth re-sanitization inside handler bodies is intentionally not applied. The args-block sanitize_callback declared at registration time (in functions.php's register_rest_route() calls) is the authoritative sanitization layer. PR review of registration changes is the enforcement mechanism.

Why not Option A (full defense-in-depth across 16 handlers)

• No real bypass path exists. Every handler type-hints WP_REST_Request $request and is invoked only via REST dispatch. None are hooked into actions/filters, called from other PHP code, or exposed through admin-ajax. The threat model the issue called out ("if the function is ever called directly...") doesn't match how the codebase actually works.
• The tests can't verify the defense. Brain Monkey already stubs sanitize_email / sanitize_text_field / etc., so adding defensive re-sanitization to a handler body wouldn't change any test outcomes — the tests inject already-clean input. Verifying the defense would require constructing a WP_REST_Request that bypasses REST dispatch, which production callers never do.
• WP's persistence sinks already sanitize downstream. wp_insert_post runs sanitize_post; ACF's update_field runs field-type sanitizers; wp_mail escapes headers (and is_email() already rejects newlines that would enable header injection).
• ~80 lines of idempotent glue across 16 files is real code noise. Five PRs of churn (audit(theme): structured write-response for project-status (refs #109) #115–audit(theme): check update_post_meta returns in translate handler (refs #109) #119 cadence) for a refactor risk that's a generic WP development hazard, not CDCF-specific exposure.

Why not Option C (narrow hybrid — outbound-mail only)

Considered: apply defense-in-depth only to handlers building wp_mail bodies, since email header injection via newlines is the highest-risk vector. Rejected because is_email() already rejects format-violating input upstream, and selective application reintroduces the very inconsistency #111 was trying to settle ("which handlers got the treatment and which didn't?").

What contributors should do (now documented in AGENTS.md)

When adding a new request field: declare sanitize_callback at registration (sanitize_text_field, sanitize_email, esc_url_raw, absint, etc.) — not inside the handler body. Validation beyond sanitization (is_email(), allowlist checks, format guards) stays in the handler where it can return a contextual WP_Error.

Status of the parent test-coverage push

With this closed: #107 closed, #109 closed, #111 closed. Theme suite is at 298 tests / 730 assertions, wordpress-theme Codecov flag at 35%+. The remaining items in functions.php (admin page renderers, meta-box renderers) were explicitly de-scoped in #107 as Tier-4 low-value targets best covered by E2E rather than PHPUnit.