fix: serve media/art for filenames containing a literal %

[kentcdodds]

Fixes #125.

route-pattern already percent-decodes wildcard params before a handler runs. Eight path handlers then ran decodeURIComponent again through decodePathParam, which caused two problems:

• A filename like Episode #1 + 50%.mp3, encoded in the RSS feed as 50%25, reached the handler as 50%, so the second decode threw. The route returned 400 Invalid path encoding, and the episode could not be played in podcast clients.
• Names containing a literal %XX sequence were silently resolved to a different file. For example, a request for %41.mp3 served A.mp3.

This PR removes the second decode from every route with a *path wildcard, not only the three named in the issue:

• /media/:token/*path
• /art/:token/*path
• /mcp/widget/:token/*path
• /admin/api/artwork/*path
• /admin/api/media/*path
• /admin/api/media/*path/metadata
• /admin/api/media-stream/*path
• /admin/api/media-analytics/*path

decodePathParam is now unused and has been deleted.

Malformed encodings such as %E0%A4%A never reach these handlers, because the router's strict canonicalization rejects them with a 404 first. The old handler-level "Invalid path encoding" tests fed raw, still-encoded params straight to the handlers, a state that can't happen in production. They are replaced with router-level tests.

The MCP resource template in app/mcp/resources.ts is intentionally left alone. The SDK's UriTemplate.match does not decode, so decoding once there is still correct.

Test Plan

• New app/routes/path-param-decoding.test.ts drives the real router.fetch with URLs built the way the app builds them: encodeURIComponent as in the RSS media URLs, encodeRelativePath as in the MCP tools, and buildItemPodcastArtUrl for artwork. It covers:
  • Episode #1 + 50%.mp3 and Season 100%/Episode %41 & 50%25.mp3 served by /media, by /art (both legacy and .jpg-decorated URLs), and by admin artwork and media-stream.
  • A URL encoding %41.mp3 must not serve A.mp3.
  • Malformed encodings on every affected route return 404 from the router.
• Against main, 5 of the 6 new tests fail with 400 (and one 200 where 404 is expected). On this branch all of them pass.
• npm run validate passes on Node 24, with 271 tests.

Checklist

• Tests updated
• Docs updated (not needed)

Screenshots

N/A (server-side routing fix)

  

Summary by CodeRabbit

• Bug Fixes
  • Media and artwork routes now handle paths containing characters such as spaces, percent signs, #, and + more consistently.
  • Malformed or invalid paths now follow the routes’ existing path-validation behavior instead of returning a separate invalid-encoding error.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 19cda377-07e5-4b9b-b6b8-d17fe8ec37a3

📥 Commits

Reviewing files that changed from the base of the PR and between 5fddfb2 and e1b02c0.

📒 Files selected for processing (16)

• app/helpers/decode-path-param.test.ts
• app/helpers/decode-path-param.ts
• app/routes/admin/api/artwork.ts
• app/routes/admin/api/media-analytics-path.test.ts
• app/routes/admin/api/media-analytics.$path.ts
• app/routes/admin/api/media-stream.ts
• app/routes/admin/api/media.$path.metadata.ts
• app/routes/admin/api/media.$path.ts
• app/routes/admin/api/path-decoding.test.ts
• app/routes/art-route.test.ts
• app/routes/art.ts
• app/routes/mcp/widget-route.test.ts
• app/routes/mcp/widget.ts
• app/routes/media-route.test.ts
• app/routes/media.ts
• app/routes/path-param-decoding.test.ts

💤 Files with no reviewable changes (7)

• app/routes/admin/api/path-decoding.test.ts
• app/routes/mcp/widget-route.test.ts
• app/helpers/decode-path-param.ts
• app/routes/media-route.test.ts
• app/helpers/decode-path-param.test.ts
• app/routes/admin/api/media-analytics-path.test.ts
• app/routes/art-route.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Routes now pass URL path parameters directly to media-path parsers. New tests cover filenames with reserved characters, malformed encodings, and paths that would resolve only after double decoding.

Changes

Path parameter handling

Layer / File(s)	Summary
Pass route parameters directly to path parsers app/helpers/decode-path-param.ts, app/routes/media.ts, app/routes/art.ts, app/routes/mcp/widget.ts, app/routes/admin/api/*	The routes no longer call decodePathParam or return separate invalid-encoding responses. They pass path parameters to media-path parsers; existing format validation remains.
Test path handling across routes app/helpers/decode-path-param.test.ts, app/routes/*test.ts, app/routes/admin/api/*test.ts	New route tests cover reserved characters, malformed encodings, and double decoding. Tests for the removed invalid-encoding responses are deleted.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to e1b02

The change fixes literal-percent filenames without introducing an identified path-handling regression. It is mergeable subject to normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to e1b02

The fix preserves public token and feed-access checks while preventing filenames from being interpreted twice. No newly weakened security control was identified. Protection of administrative endpoints remains unverified, and their existing filesystem resolver lacks an explicit root-containment check.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — Public file access remains scoped by the active token's owning feed. Administrative sinks include file streaming, artwork extraction, metadata replacement, and analytics responses. Their effective external exposure cannot be bounded from application code alone because enclosing deployment authorization is not evidenced.

Trust Boundaries and Controls

• observed — The analytics route has no route-level authentication middleware, and the inspected application router supplies rate limiting, static-file handling, and logging rather than admin authentication. This is preexisting; whether an external access boundary protects the admin API remains unresolved.
• inferred — Removing the second decode eliminates an additional interpretation of attacker-supplied percent sequences without removing downstream token or feed checks. Literal-percent filenames now work with normal encoding; the same downstream filenames could previously be reached using an additional encoding layer, so this correction does not establish new asset authority.

Resilience and Maintainability Implications

• inferred — The unchanged metadata transition is not transactional across rename, permission restoration, cache invalidation, and response generation. Later failures can follow a completed replacement; simultaneous updates have no visible per-file lock, and process interruption can bypass temporary-file cleanup. These are preexisting recovery limitations, not introduced concerns for this PR.
• observed — Public media analytics recording remains downstream of authorization and serving. Recording failures are caught without replacing the media response, and repeated requests create separate events rather than changing access authority.

Hardening Proposals

• proposed — As separate hardening work, establish verifiable authorization coverage for the entire admin API and enforce configured-root containment in the shared filesystem resolver. These proposals address preexisting boundaries rather than defects introduced by the decoding fix.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 9 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: serving media and artwork files whose filenames contain a literal percent sign.
Linked Issues check	✅ Passed	Issue #125 requires literal-percent filenames to work through media and artwork URLs. The PR removes the second URL-decoding step from the affected handlers and passes the router-decoded wildcard to p…
Out of Scope Changes check	✅ Passed	The changes to admin media routes, metadata, analytics, and the MCP widget apply the same double-decoding correction to wildcard routes. The deleted handler-level malformed-encoding tests and new rout…

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

_{Comment @coderabbitai help to get the list of available commands.}