HarperFast / HarperFast/prerender-plugin

Crawler-invented malformed URLs become immortal Targets: no validation at discovery + 403 exempt from suppression

Đang mở
#81 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
bug P2
Ngôn ngữ chính
JavaScript
Star
0
Fork
0
Merge trung bình
9 giờ 39 phút
Pull request đã merge (30 ngày)
39

Mô tả

## Summary

Crawlers with broken link extractors invent malformed URLs from our rendered HTML. Discovery creates a
`Target` for each with **no URL validation**, the origin's WAF rejects them with **403**, and 401/403 is
deliberately exempt from suppression — so they can never be retired. They accumulate permanently and
retry forever.

Measured on a production deployment over 24h: **1,264 distinct URLs**, producing **~2,950 renders/day**
that can never succeed. All of them: `sitemapUrl` null (never submitted), `strikes` at max, and **no
`PrerenderedPage` for any device cacheKey — i.e. not one has ever rendered successfully.**

## What has changed since filing — updated 2026-09-16

**The dominant funnel now has a switch, but neither fix in this issue shipped.**

#127 (`prerender-v0.54.0`) added the discovery gate — `ingress.discoveryBots` (a creation-time bot
allowlist) and per-route `ingress.routes[].discoverTargets` — plus
`POST /prerender_admin/discovery-purge` to remove what was already minted. Where a deployment sets
them, the broken-extractor crawlers that invent these URLs (AhrefsBot alone was 81.5% of the junk
misses; DotBot and friends the rest) are exactly the ones left off the list, and this class stops
arriving. #84, which asked for that gate, is closed.

**That is a deployment-configurable mitigation, not a fix, and it does not cover the whole class:**

- An **allowlisted** crawler follows invented links too. Googlebot crawls links wherever it finds them,
including the mangled ones a third party published, so a curated allowlist still admits this shape.
- It does nothing for what has already accumulated — `discovery-purge` deletes by URL prefix and
`sitemapUrl`-null, which does not select *malformed* URLs specifically.
- A deployment that leaves `discoveryBots` at its `['*']` default — the default, chosen so an upgrade
changes no behaviour — is exactly where it was.

Both fixes below are still absent from the tree:

- `maybeSchedule` (`src/http_handlers/bot_request.js`) gates on the route flag and the bot allowlist and
**nothing about the URL itself**.
- `authShaped = (statusCode) => statusCode === 401 || statusCode === 403` in `src/resources/RenderQueue.js`
still suppresses nothing, unconditionally.

## Why it happens

Representative captured URLs (product ids redacted):

```
/product/prd-XXXXXXX/target=
/product/prd-XXXXXXX/url(%22https:/fonts.googleapis.com/css2
/product/prd-XXXXXXX/....jsp'%20defer='defer
/product/prd-XXXXXXX/name@example-vendor.com
/product/prd-XXXXXXX (bare id, slug truncated)
```

These are artefacts of third-party crawlers mis-parsing our own output:

- `url(%22https:/fonts…` is a CSS `url("https://fonts…")` declaration resolved as a **relative page
URL** — note `https://` collapsed to `https:/` by the crawler's normalizer. A single sampled snapshot
contained **200 `url(` occurrences**.
- `target=` is the `target` attribute concatenated onto the href.
- `' defer='defer` is script attributes concatenated.

**Verified NOT our bug:** the same sampled snapshot had 162 `href`s, **0** containing suspicious tokens
and **0** unquoted attributes. Our emitted HTML is well-formed; the mis-parsing is crawler-side.

Worth noting that prerendering enlarges the attack surface: a rendered page is ~2.5x the size of the
origin SSR document with substantially more markup and CSS, so bad parsers have more to mangle than
they would against the origin.

Then the chain closes:

1. crawler requests the malformed URL
2. cache miss → `handlePageScheduling` creates a `Target` — **no URL validation**
3. render → the CDN/WAF answers **403** (these read as injection payloads: quotes, `url(`, `=`
fragments), not 404
4. `RenderQueue` classifies 401/403 as auth-shaped and **deliberately does not suppress** — correct in
general, because an auth failure is usually a broken renderer credential and striking would
mass-delete healthy targets during an outage
5. `failureRetry` retries each ~2x/day, forever

Each decision is individually right. Together they make an immortal class of targets.

## Impact

Compute cost is trivial — ~0.2% of render throughput. The real damage is diagnostic:

`RenderQueue` logs, per occurrence:

> `Prerender got 403 for — auth-shaped, NOT suppressing. If these are widespread, check the
> renderer's origin-bypass credential and the CDN/origin access rules.`

That is exactly the alarm you need for a genuine bypass-token break or a CDN rule change — and it
fires ~2,950 times/day as steady-state noise, so a real incident has to be spotted against that floor.
This matters more as crawl volume grows: the junk population scales with crawl traffic.

## Proposed fix (two parts, complementary)

### 1. Validate URLs at discovery

Reject non-canonical shapes in `handlePageScheduling` **before** creating a `Target`. This prevents the
row, the schedule entries, and every downstream render — the cheapest possible point to stop it, and it
is independent of *who* is asking, which is what makes it cover the allowlisted-crawler case the gate
cannot.

Prefer a positive rule over a blocklist: for a known route, require the path to match that route's
canonical shape (e.g. product slugs restricted to a safe charset). A blocklist of `url(`, quotes,
whitespace, `=`-in-segment etc. would work today but invites an endless game of catch-up.

Should be config-driven and default-on, with a counter for rejected discoveries so the rule's
selectivity is observable (and a too-strict rule is visible rather than silent). The `discovery_gated`
series added in v0.54.0 is the natural home — it already splits by which gate refused.

Note the related finding from #84's measurement: the origin answers a `%252B`-style double-encoded
mutation with a 200 whose canonical is a **newly fabricated** junk URL derived from the mutation. So the
origin manufactures fresh junk URLs from junk input, which crawlers then follow — an argument for
rejecting `%25xx` escapes at discovery specifically, and against ever adopting a page's canonical as a
new Target.

### 2. Let 403 suppress under a never-successful predicate

Keep the mass-outage protection, but narrow it. Suppress on 401/403 only when **all** hold:

- `sitemapUrl` is null (never submitted), **and**
- `strikes >= maxStrikes`, **and**
- no `PrerenderedPage` exists for any device cacheKey (never rendered successfully in its lifetime)

A target meeting all three has never produced a page and was never submitted, so retiring it cannot
lose anything — and any submitted URL keeps the current unconditional protection, which is the case the
exemption was written for. Deletion is self-healing regardless: a later request proxies to origin and
rediscovers the URL.

Part 1 stops new arrivals; part 2 retires what slips past (and anything already accumulated) —
including on deployments running the default `discoveryBots: ['*']`.

## Interim action taken

1,106 of these targets were deleted via `Target.delete()` (which also drops the `RenderSchedule` rows
and `PrerenderedPage` rows — a raw table delete would orphan the schedule row, and `claim` builds jobs
from the schedule alone without checking the target still exists). Every deletion was gated on the
three predicates above; 0 were in a sitemap, 0 had a cached page, 0 errors.

That is a cleanup, not a fix — without part 1 they simply come back on the next crawl.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Start by tracing handlePageScheduling and maybeSchedule in src/http_handlers/bot_request.js to see how discovery creates Targets, then inspect authShaped in src/resources/RenderQueue.js. Define how route-aware URL validation and its rejection counter fit the existing discovery gates. Done means malformed discoveries create no Target, and 401/403 suppression applies only to never-submitted, never-successful targets at max strikes without weakening outage protection.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
javascript
Lĩnh vực
backend
Loại issue
Lỗi
Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
38/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.