Skip to content

fix(web): drop redundant double-decode on public federated routes (500 → graceful)#98

Open
lukaso-bot wants to merge 2 commits into
mainfrom
fix/public-route-double-decode
Open

fix(web): drop redundant double-decode on public federated routes (500 → graceful)#98
lukaso-bot wants to merge 2 commits into
mainfrom
fix/public-route-double-decode

Conversation

@lukaso-bot

Copy link
Copy Markdown
Collaborator

What

repoFromParams in the 4 public permalink/badge routes (result.tsx, badge.ts, issue.tsx, pr.tsx) called decodeURIComponent(c.req.param('projectPath')) — a redundant double-decode. Hono's c.req.param() already percent-decodes (safe try/catch). On a malformed %-bearing path (bad%25 → Hono decodes to bad%), decodeURIComponent("bad%") throws URIError. The throw is inside repoFromParams, called with no try/catch, and there is no app.onErroruncaught HTTP 500 on a public surface.

This is the same bug class the #92 branch already fixed on the service-binding routes (internal.ts) and web-og; that work left the public routes inconsistent.

Fix

Drop the manual decodeURIComponent across all 4 routes (Hono already decoded) and rename projectPathEncprojectPath so the already-decoded value doesn't invite a future re-decode. Load-bearing claim verified by repro against installed Hono 4.12.27: c.req.param('projectPath') returns bad% for a bad%25 path, and decodeURIComponent("bad%") throws URIError.

Tests (TDD)

4 regression tests (one per route) in integration.test.ts, mirroring the #92 internal-route test: seed the cache slot the fixed route keys on (gitlab.com/bad%), hit the malformed bad%25 URL, assert 200. Confirmed RED (expected 500 to be 200) with the bug present, GREEN with the fix. They use gitlab.com (not an Anubis host) so makeProvider doesn't hit the absent relay Durable-Object binding in unit tests — the decode is host-agnostic.

pnpm -r test (282+ pass) · typecheck · build · validate.sh all green.

Scope notes

  • Latent + pre-existing: no real project path contains % (only crafted/garbage URLs hit it; prod SYSTEM=0), and it predates #92. Filed as a focused follow-up rather than a 5th change piled onto the 4 merge-ready PRs.
  • own-url.ts intentionally untouched: it decodes a regex-captured paste string (not a Hono param) — a first decode, correctly. Dropping it would break legit %2F paste handling.
  • internal.ts:105 (same class, service-binding route) is already fixed on the #92 branch, so left to it to avoid a conflict.

🤖 Generated with Claude Code

repoFromParams in the 4 public permalink/badge routes (result, badge, issue,
pr) called decodeURIComponent(c.req.param('projectPath')) — a redundant
double-decode, since Hono's c.req.param() already percent-decodes (safe
try/catch). On a malformed `%`-bearing path (e.g. `bad%25` → Hono decodes to
`bad%`), decodeURIComponent("bad%") throws URIError. The throw is inside
repoFromParams, called with no try/catch, and there is no app.onError → an
uncaught HTTP 500 on a public surface (worse hygiene than the service-binding
routes already fixed on the #92 branch).

Drop the manual decode across all 4 routes (Hono already decoded) and rename
projectPathEnc → projectPath so the already-decoded value doesn't invite a
future re-decode. Verified the Hono behavior by repro against installed
4.12.27. Pinned by 4 regression tests (one per route) mirroring the #92
internal-route test.

Latent in practice (no real project path contains `%`; only crafted/garbage
URLs hit it; prod SYSTEM=0) and pre-existing (predates #92) — a focused
follow-up rather than a 5th change piled onto the merge-ready PRs.

own-url.ts decodes a regex-captured *paste* string (not a Hono param) — a
first decode, correctly — so it is intentionally left untouched.

Co-Authored-By: Claude <noreply@anthropic.com>
@lukaso-bot

Copy link
Copy Markdown
Collaborator Author

Code review: #98

The diff is correct. I checked the make-or-break claim against Hono 4.12.27's source: c.req.param() runs decodeURIComponent on any param containing % (request.js #getDecodedParam), so it decodes both %2F to / and %25 to %. Dropping the redundant decode is right, including for multi-segment GitLab paths like group%2Fsub%2Fproject. The four regression tests are non-vacuous: without the fix, repoFromParams throws before the cache key is built, so the route 500s and the expect(200) goes red.

The issues below are all adjacent to the diff. The changed lines themselves look good.

1. Same uncaught throw still live on /lookup (own-url.ts:16)

The commit justifies leaving own-url.ts alone as "a first decode, correctly." It isn't a redundant decode, true. But it's still an unguarded decodeURIComponent on user input, and it's the same bug class this PR removes from the permalink routes.

fed() at own-url.ts:16 does decodeURIComponent(projectEnc), where projectEnc is captured by ([^/]+). recognizeOwnUrl is called at index.ts:107, outside the surrounding try/catch (the try at index.ts:112 only wraps parseInput), and there is no app.onError (only app.notFound). A malformed % in a pasted federated URL throws URIError and becomes a bare 500:

GET /lookup?q=h/gitlab.gnome.org/p/x%2G/1   ->  500

This is arguably more reachable than the repoFromParams bug: q is a query param on a shareable GET link, and WHATWG query decoding is lenient, so the stray % survives into fed(). A one-line hardening consistent with this PR: move the recognizeOwnUrl(q, ...) call inside the existing try/catch, or wrap the decode in fed().

2. No app.onError: the root cause is still open

The commit cites "no app.onError" as part of why a throw becomes a bare 500, then fixes call sites one at a time. A global app.onError that redirects home (or returns a graceful 500) would be defense-in-depth that catches #1, the internal.ts:105 throw, and the next unguarded parse someone adds. This is the Nth site in the same bug class (#92, this PR, and the other follow-ups). A single backstop may be cheaper than continuing to patch throw sites individually.

3. Four byte-identical repoFromParams copies

badge.ts:36, result.tsx:33, issue.tsx:47, pr.tsx:31 are identical, and this PR edits all four in lockstep. That lockstep is exactly how internal.ts:105 came to drift. Extracting one shared helper collapses it, and this PR is the natural moment since it already opens all four files. Pre-existing, not introduced here.

4. (Coordination) internal.ts:105 has the same double-decode

Still on main, pending #92. Behind isServiceBinding() (fails closed without INTERNAL_SECRET), so only the web-og Service Binding can reach it, and a literal-% project path is uncommon. Just confirm #92 touches that exact line, or the OG renderer 500s for such projects once the public routes are fixed.

…ounce)

recognizeOwnUrl() in the /lookup handler called decodeURIComponent() on the
captured project path before the try/catch. A stray "%" in a pasted federated
link (gitlab.gnome.org/p/x%2G/1) threw URIError and surfaced a bare 500 on a
shareable GET surface. Move it inside the existing try so the catch bounces to
the form like other malformed input. Same double-decode class as fc6a0ae.

Co-Authored-By: Claude <noreply@anthropic.com>
@lukaso-bot

Copy link
Copy Markdown
Collaborator Author

Triage of my own review above — finding 1 applied here, the rest disposed:

1. /lookup malformed-% → APPLIED (3a444a4). Same URIError → 500 class as the permalink routes this PR fixes, but on a GET ?q= link, so more reachable (shareable, lenient query decoding). recognizeOwnUrl() now runs inside the existing try; a stray % bounces to the form like other malformed input instead of 500ing. TDD: a failing test pinned it first (URIError at own-url.ts:16 → 500), the one-line move turns it into a 302. Verified the same 500 live on prod before the fix.

2. app.onError backstop → BACKLOGGED. Sound defense-in-depth — would catch this class, the internal.ts site, and the next unguarded parse in one place. But it's an enhancement, not a correctness bug, so it doesn't belong in this fix. Added to the backlog.

3. Four repoFromParams copies → already BACKLOGGED (the "collapse the number-route copies" refactor deferred from #92's review). Pre-existing, not introduced here.

4. internal.ts:105 → confirmed handled by #92. #92 (574ca60) drops the same double-decode across the /h/:host/{r,i,p} routes. Nothing owed once #92 lands.

Scope note: this PR now covers two surfaces — the permalink/badge route decoders (fc6a0ae) and the /lookup search route (3a444a4) — both the same double-decode/unguarded-decodeURIComponent class. Kept them together rather than a 6th PR.

In-house review of 3a444a4 (Copilot is quota-blocked here, so this is the adversarial pass): minimal and correct, no regression to the happy path (the own redirect is unchanged; publicBaseUrl/new URL moved inside the try is a net improvement). The catch yields reason = 'invalid' for the URIError (no .kind), which is accurate. Non-blocking. CI re-running.

@lukaso-bot

Copy link
Copy Markdown
Collaborator Author

Review — PR 98 (double-decode fix)

The 5 changed source files are correct. Verified that Hono 4.12.27's c.req.param() percent-decodes via a safe try/catch (tryDecode in hono/.../utils/url.js), so the removed decodeURIComponent was a genuine redundant double-decode, and Hono's single decode never throws. Moving recognizeOwnUrl inside the try is a strict improvement. The new tests are valid regressions: they fail on the pre-PR code, pass after.

The same bug class is still live in two sibling files the OG chain depends on. Neither is in this PR's scope, but both are reachable from the permalinks it fixes.


1. packages/web-og/src/index.tsx:55 — public OG route still 500s (same bug)

const { host, projectPath, shaPng } = c.req.param();  // Hono already decoded
...
const decodedPath = decodeURIComponent(projectPath);   // double-decode

For the exact path this PR fixes, /h/<host>/r/bad%25/c/<sha>, the permalink page renders (200) and emits og:image pointing at /h/<host>/r/bad%25/c/<sha>.png. web-og line 49 gets projectPath = "bad%" (Hono-decoded), then line 55 decodeURIComponent("bad%") throws URIError. No try/catch and no app.onError, so the OG card returns 500. Same stray-% input, opposite outcome on the two surfaces. The author comment at line 53 ("regardless of how the router decoded the param") is the misconception: the router already decoded it.

This is the highest-value one to fix: the OG surface is public, and it's the unfurl image for the very permalinks this PR repairs.

2. packages/web/src/routes/internal.ts:105 — same double-decode, service-binding-gated

return resolveResult(c, host, decodeURIComponent(projectPathEnc), sha);

projectPathEnc = c.req.param('projectPath') is already decoded. The decodeURIComponent sits in the argument position, so it throws before resolveResult is entered: uncaught 500, not the 503 that the inner try/catch returns. Reachable only by a caller presenting the matching INTERNAL_SECRET, and for %-paths web-og crashes at its own line 55 first, so latent. But it contradicts the commit message's claim that this bug class "was already fixed on the #92 branch": line 105 is unchanged on the current tree. (Fair to #98: it scoped itself to the public routes.)

3. Cleanup — repoFromParams is copy-pasted across all 4 route files

This PR applied the identical 3-line comment + body edit to badge.ts, issue.tsx, pr.tsx, result.tsx. That's the signal to extract it once, e.g. into packages/web/src/paths.ts (which already groups the RepoRef permalink builders). Four copies of an 11-line function will drift: the next decode/param nuance gets fixed in three and missed in one. Low priority, optional.


Suggested follow-up: a one-line change mirroring this PR onto web-og/src/index.tsx:55 (drop the decode; c.req.param() is already decoded) closes #1. internal.ts:105 can go in the same pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant