# EditCheck Headless Security Review

**Target:** `gitlab.wikimedia.org/repos/mediawiki/services/editcheck-headless` @ `696b0c7` (branch `main`)
**Changes reviewed:** none — base only, no pending merge requests
**Docs consulted:** `README.md`, `.pipeline/blubber.yaml`, `service-utils.config.yaml`, `.gitlab-ci.yml`
**Reviewer:** Claude (production-focused service audit), run by Caro Medelius
**Date:** 2026-08-14

---

## Methodology / Scope

**Attacker model.** Two actors:

1. **An anonymous or registered wiki user.** They cannot send bytes to this service
   directly — the API "is fully internal, it will never be publicly accessible."
   They *can* edit a wiki page, which the JobQueue then schedules for checking,
   so page content reaches the headless browser. This is the only demonstrated
   public ingress into any part of the system.
2. **A caller on the internal network** (the JobQueue, or anything that can route
   to the pod). There is no authentication or authorization on either transport, so
   this actor is trusted by construction.

**Out of scope:** WMF operators and cluster insiders; `Special:EditCheckHeadless`
and the VisualEditor/EditCheck extension code it loads (different repo — see
recommendation 1); the Linked Artifact Cache itself.

**Reviewed in full** (2,551 lines): `editcheck-headless-server.js` (818),
`editcheck-headless.js` (932), `editcheck-headless-grpc.js` (258),
`editcheck-headless-sitematrix.js` (91), `editcheck-headless-metrics.js` (287),
`editcheck-headless-tracing.js` (165), plus `.pipeline/blubber.yaml`,
`.gitlab-ci.yml`, `service-utils.config.yaml`, `.npmrc`, `package-lock.json`.

**Verification level.** Static tracing only. **The service was never started and no
trigger was fired.** Every claim below is a code-reading argument. Items marked
**[verify]** depend on behaviour or configuration not observable from this repo.

---

## Summary

**No findings.**

No Critical, High, Medium, or Low finding survived the reachability test. Six
code-level issues were identified and all six are recorded under *Not currently
exploitable* — each is a real property of the code, and none has a demonstrated
attacker path today.

The two most substantial (arbitrary-URL SSRF, and Chrome running `--no-sandbox`)
are held back for specific reasons stated inline. Both are cheap to fix and both
are worth fixing; neither is a live vulnerability on the evidence gathered here.

---

## Not currently exploitable

### 1. `wiki` / `wiki_id` accepts any URL; the sitematrix allowlist is never enforced

**Location:** `editcheck-headless-sitematrix.js:73-86`, reached from
`editcheck-headless-server.js:485` (`/check`, `/config`) and
`editcheck-headless-grpc.js:162` (`GetRevisionArtifact`).

```js
if ( /^https?:\/\//i.test( trimmed ) ) {
    return trimmed.replace( /\/+$/, '' );   // any URL, returned verbatim
}
```

**The bug.** The service fetches the full sitematrix at startup
(`editcheck-headless-server.js:670`) and holds ~900 valid wiki base URLs in memory,
but uses that map only as a dbname lookup. Any `http(s)://` value bypasses it — no
host check, no port restriction, no block on RFC-1918, `localhost`, or link-local.
The value flows to `page.goto()` (`editcheck-headless.js:529`), where Chrome loads
and executes whatever the origin serves, and to `fetch()`
(`editcheck-headless-grpc.js:101`) from the pod's network position.

**Why it is not a finding.** No named ingress. `wiki` and `wiki_id` are supplied by
the JobQueue over an internal-only API; no public path that injects an
attacker-controlled value was identified. The remaining argument for filing it —
that the network boundary is the only control and lives outside this repo — reduces
to "operators must not misconfigure the NetworkPolicy," which is not an attacker
capability. It stays here until someone demonstrates a public path that reaches the
`wiki` field.

**Fix worth applying anyway** (the allowlist already exists in memory):

```js
if ( /^https?:\/\//i.test( trimmed ) ) {
    const normalized = trimmed.replace( /\/+$/, '' );
    if ( !allowArbitraryUrls && !Object.values( dbnameMap ).includes( normalized ) ) {
        throw new Error( `Wiki URL not in the sitematrix: "${ normalized }"` );
    }
    return normalized;
}
```

Gate `allowArbitraryUrls` behind an off-by-default flag so `--wikis http://localhost`
still works locally. Two follow-ups, since validating only the first URL is a known
bypass: neither `fetch()` sets a redirect policy and `page.goto()` follows
redirects, so set `redirect: 'manual'` and re-validate `Location`; and if the
JobQueue and LAC can always send a dbname, delete the URL branch in production
instead of validating it.

### 2. Chrome runs `--no-sandbox` with site isolation disabled

**Location:** `editcheck-headless.js:20-55` (`CHROME_ARGS`) — `--no-sandbox`,
`--disable-site-isolation-trials`, `--renderer-process-limit=1`, and
`--disable-features=IsolateOrigins,site-per-process`.

**The bug.** Three mitigations are disabled together: the OS renderer sandbox,
cross-origin process separation, and per-origin renderers. The in-code rationale
for `--no-sandbox` is "required when running as root"; Blubber images do not run as
root by default, so that specific justification may not hold **[verify]** — the
likelier real constraint is that Chrome's sandbox needs user namespaces or
`CAP_SYS_ADMIN` that the pod security policy denies.

**Ingress (this one is real).** Any wiki user edits a page → the JobQueue schedules
a check → `Special:EditCheckHeadless` fetches that page's Parsoid HTML and runs it
through VE. Attacker-authored content reaches a renderer with no OS sandbox. That
is a genuine public path into the browser component.

**Why it is not a finding.** Exploitation requires a Chrome renderer vulnerability,
which was not demonstrated and is not asserted to exist. A removed mitigation with
no working trigger is defence-in-depth, not a live vulnerability. Note this rests on
the manual review alone — semgrep's JS/Node packs have no Puppeteer or browser-flag
coverage, so nothing corroborates it.

**Fix worth applying anyway.** Confirm whether the pod can grant unprivileged user
namespaces or a seccomp profile that lets the sandbox initialise; if so, drop
`--no-sandbox` and keep `--disable-dev-shm-usage`. If it genuinely cannot, document
that as a deployment constraint and compensate (restrictive seccomp/AppArmor,
read-only rootfs, dropped capabilities, egress-limiting NetworkPolicy). The site
isolation flags were added for resource savings (commit `f042819`); measure what
they actually save before keeping them, given the shared cross-origin session
(item 4).

### 3. Outbound `fetch()` calls set no timeout

**Location:** `editcheck-headless-grpc.js:101-106`, `editcheck-headless-sitematrix.js:51-56`.

Neither call sets a `signal`. The service defines a 90 s `--timeout-ms` budget and
applies it to `page.goto()` and the result poll, but not to these; they fall back to
undici's default header/body timeouts (300 s in current undici **[verify]**), over
3× the service's own budget. **Not a finding:** the trigger is a slow upstream wiki,
not an attacker, and `resolvePageTitle` runs before work is enqueued so it does not
block the browser session. **Fix:** pass the existing budget through as
`signal: AbortSignal.timeout( timeoutMs )` (needs threading into the gRPC deps).

### 4. All non-pinned wikis share one serialized browser session

**Location:** `editcheck-headless.js:832-840` (`getSession`), `:700-715` (`enqueue`).

Wikis not named in `--wikis` share a single session running one task at a time
through an unbounded promise chain. `addQueued` increments a gauge but rejects
nothing, and each task can hold the session for up to 90 s, so N queued requests
serialize into N × 90 s. The same session also re-navigates between origins,
sharing one `BrowserContext` (cookies, storage) across different wikis. **Not a
finding:** the only actors who can queue work are trusted, and the trigger is load,
not an attacker. **Fix:** cap queue depth and shed with HTTP 503 /
`RESOURCE_EXHAUSTED`; `editcheck_headless_queue_depth` already exists to alert on.
Separately, use a fresh `BrowserContext` per wiki switch to remove the cross-origin
state question entirely.

### 5. Permissive CORS on a service no browser should call

**Location:** `editcheck-headless-server.js:210-215`, applied by `sendJson` at `:226`.

Every response carries `Access-Control-Allow-Origin: *`. **Not a finding:**
`Access-Control-Allow-Credentials` is not set, so cookies are not sent and
credentialed responses are not exposed, and the data returned (suggestion counts for
public pages) is not sensitive. **Fix worth applying:** delete `setCorsHeaders` and
the `OPTIONS` branch. This is a machine-to-machine API; there is no client to break.

### 6. Raw exception messages in HTTP 500 bodies

**Location:** `editcheck-headless-server.js:557`, `:641`.

`sendError( res, 500, e.message )` returns the raw exception text; puppeteer and
undici messages routinely embed the failing URL, host and port. Stack traces are
correctly kept to the log (`:556`, `:640`). **Not a finding:** only trusted internal
callers can reach it. **Fix:** return a generic message plus a correlation ID —
`x-request-id` is already propagated (`editcheck-headless-tracing.js:39`).

### 7. `parsoidHtml` POST body reintroduces a rejected trust model

`POST /check` accepts up to 10 MiB of caller-supplied HTML
(`editcheck-headless-server.js:589-595`) and feeds it to the browser
(`editcheck-headless.js:247-254`). `project-description.txt` explicitly rejects
"taking results from the user's client" because clients "could generate bad data
either accidentally or maliciously"; this path reopens a narrower version, and
results flow into the shared Linked Artifact Cache. **Not a finding:** only trusted
callers reach it. Worth raising as a design question — if the parameter is not yet
used in production, consider removing it until it is.

### 8. gRPC listener uses `createInsecure()`

`editcheck-headless-server.js:787`. No TLS and no auth. Standard for WMF services
behind an Envoy mesh sidecar that terminates TLS — but that assumption appears
nowhere in this repo. Semgrep independently flagged the sibling
`http.createServer` (`:698`) for the same reason. **[verify]** the mesh config and
document the assumption (recommendation 2).

---

## Reviewed without finding

- **No injection into browser JS.** Every `page.evaluate()` passes values as
  arguments rather than interpolating into source (`editcheck-headless.js:248-253`,
  `:140`, `:150-159`). `title` — the obvious candidate — cannot break out.
- **Prototype pollution.** `dbnameMap` is built with `Object.create( null )` in both
  places (`editcheck-headless-sitematrix.js:15`, `editcheck-headless-server.js:667`),
  so the lookup at `:81` cannot be steered via `__proto__` or `constructor`. Reads
  as deliberate.
- **No dangerous sinks.** No `eval`, `new Function`, `vm.*`, `innerHTML`,
  `document.write`, or deserialization of untrusted data.
- **`execSync` and `spawn` are not injectable.** `getconf CLK_TCK` / `getconf PAGESIZE`
  are constant strings (`editcheck-headless-server.js:249`, `:256`); `spawn`
  (`editcheck-headless.js:354`) passes an argv array with no shell and an
  operator-controlled binary. The Lightpanda path is off by default.
- **Request body cap.** 10 MiB, socket destroyed on overflow (`:177-203`).
- **Metric cardinality is bounded on purpose.** `endpointLabel()` buckets unknown
  paths, commented "labelling each one would let a scanner create time series at
  will" (`editcheck-headless-metrics.js:44-56`). Metrics also sit on a separate port
  (9102), deliberately off the API route.
- **gRPC input validation.** `page_id` checked against `/^\d+$/` and rejected when
  zero (`editcheck-headless-grpc.js:154-158`).
- **Health probes.** Answered before logging and metrics so probes cannot swamp
  either; liveness/readiness split correctly reasoned (`:493-510`).
- **No secrets, in tree or history.** `gitleaks` over all 19 commits: no leaks. An
  independent working-tree pattern scan was also clean.
- **Human-readable source only.** No generated or minified build artifacts committed
  (satisfies the T217351 point of emphasis).

---

## Dependencies

**Two independent scanners, both clean:** `npm audit` — 0 vulnerabilities
(`--package-lock-only --omit=peer --omit=optional`, inside `fresh-node`);
`osv-scanner` — no issues, 145 packages. The same OSV scan runs in CI on every
pipeline via `repos/security/gitlab-ci-security-templates` `generic-osv/osv-ci.yaml`
(ref `v0.1.6`), so this is continuously rechecked. That job is `allow_failure`, so a
new advisory reports without blocking a deploy — a reasonable trade, but a
deliberate one worth stating.

`package-lock.json` (lockfileVersion 3) is structurally sound: 146 entries, all
HTTPS, **zero** missing integrity hashes, two resolved hosts (`registry.npmjs.org`
×145, `gitlab.wikimedia.org` ×1). 146 total packages for a service that drives a
browser, speaks gRPC and exports OTel traces is a small tree, and there is no
webpack in it.

**Version drift** (`npm outdated`) — no known vulnerabilities, so these are
maintenance notes. Two cannot update on their own: `puppeteer-core` 25.3.0 → 25.7.0
(exact pin, no range; it is the browser driver) and `@grpc/proto-loader` 0.7.15 →
0.8.1 (`^0.7.15` does not cross to 0.8.x). The `@opentelemetry/*` packages are one
release behind (0.220.0 → 0.221.0, `instrumentation-undici` 0.30.0 → 0.31.0).
Everything else is current.

**Blind spots — a clean scan does not cover these:**

- **Debian `chromium`**, installed unpinned by `.pipeline/blubber.yaml`. Not in the
  lockfile; no npm-ecosystem scanner will ever see it. Per item 2 it is the
  component that parses attacker-authored content with its sandbox disabled, and it
  is scanned by nothing today.
- **`@wikimedia/service-utils` 2.0.0** — the one first-party dependency, resolved
  from the GitLab registry rather than npmjs. `osv-scanner` reported 145 packages
  against 146 in the lockfile and this is the only non-npmjs entry, so it is
  near-certainly the one skipped: OSV maps packages to ecosystems by registry.
  **[verify]** with `osv-scanner --verbosity=info`. Not alarming — first-party code
  with a valid integrity hash — but the dependency closest to home is the one
  neither tool checks.
- `eslint` `^8.57.1` is end-of-life. Neither scanner flagged it, so EOL here means
  unmaintained, not known-vulnerable. Dev-only and pruned from the production image.

---

## Recommendations

1. **Review `Special:EditCheckHeadless` and the EditCheck extension alongside this
   repo.** This service is a ~2,500-line driver with no complex parsing of its own;
   the actual processing of hostile input (Parsoid HTML → VE data model → edit
   checks) happens in the special page and the RL modules it loads, in a different
   repo. **Reviewing this service alone gives limited assurance.** Both should be
   scoped together.
2. **Document the trust model in the repo.** The "fully internal, never publicly
   accessible" claim exists only in the Phabricator description. Record it next to
   the deployment config along with what it rests on: which NetworkPolicy enforces
   it, that Envoy terminates TLS for the insecure gRPC listener (item 8), and that
   both listeners bind `0.0.0.0` (`.pipeline/blubber.yaml`).
3. **Attach the NetworkPolicy / deployment-charts config to the review request.**
   Items 1 and 5 both reduce to "the network boundary is the only control," and that
   cannot be assessed from this repo. This is the single highest-value addition.
4. **Add regression tests for the security-relevant rejections.** The suite tests
   that an unknown dbname 400s (`test/server.test.js:186-190`); the URL-passthrough
   branch has no test at all, which is part of why it reads as unintentional. If the
   item 1 allowlist lands, test that an off-sitematrix URL also 400s.
5. **Set `min-release-age = 7` in `.npmrc`.** Makes npm refuse to resolve versions
   published in the last 7 days — the standard defence against a compromised
   maintainer publishing a malicious version. Scope it honestly: the lockfile is
   committed and builds install from it with integrity hashes, so this hardens the
   *dependency-update workflow*, not production. **[verify]** the npm in
   `nodejs24-slim` is ≥ 11.10; an unrecognised key is silently ignored, which would
   leave the appearance of protection and none of the substance.
6. **Track the Chromium version.** It is unpinned in `.pipeline/blubber.yaml` and
   invisible to every dependency scanner, while being the most security-critical
   dependency in the image.
7. **Review Lightpanda separately if it is ever adopted.** The code supports it as
   an alternative engine, but it is a young engine with a much smaller security
   track record than Chromium; switching should trigger its own review.