fix(template): reject path traversal in public template-by-id endpoint#3242
Open
baktun14 wants to merge 1 commit into
Open
fix(template): reject path traversal in public template-by-id endpoint#3242baktun14 wants to merge 1 commit into
baktun14 wants to merge 1 commit into
Conversation
The public, unauthenticated GET /v1/templates/{id} endpoint built a file
path by interpolating the caller-supplied id straight into the template
cache path. Hono routes with decodeURI (so a URL-encoded %2F survives as a
single segment) and decodes the param with decodeURIComponent, so a payload
like ..%2F..%2Fsecret arrived at the service as ../../secret and escaped the
cache directory, disclosing arbitrary .json files the API can read.
Add a reusable resolvePathWithinDir() util that resolves a caller-supplied
segment against a base dir and returns null unless it stays strictly inside
it (rejecting traversal, absolute escapes, and null bytes). Guard the read in
getTemplateById() with it: a rejected id logs a TEMPLATE_PATH_TRAVERSAL_BLOCKED
warning and returns null, which the controller maps to a 404 (no info leak).
Legit ids — including ones with dots like ...-DeepSeek-V3.1 — are unaffected.
Covered by util unit tests, service unit tests (traversal returns null without
reading), and a functional test asserting encoded traversal payloads 404 at the
real endpoint.
Fixes CON-428
Contributor
📝 WalkthroughWalkthroughThis PR adds path traversal protection to the template gallery service. A new utility ChangesPath Traversal Protection for Template Gallery
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/test/functional/templates.spec.ts (1)
34-35: ⚡ Quick winAdd explicit non-leak assertion for traversal responses.
A
404alone doesn’t prove sensitive content is absent. Assert the response body does not contain leaked file markers.Suggested test hardening
const response = await app.request(`/v1/templates/${encodedId}`, { method: "GET", headers: new Headers({ "Content-Type": "application/json" }) }); expect(response.status).toBe(404); + const body = await response.text(); + expect(body).not.toContain("root:x:"); + expect(body).not.toContain("/etc/passwd");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/test/functional/templates.spec.ts` around lines 34 - 35, The test currently only asserts response.status === 404; extend it to explicitly assert the response body doesn't contain leaked file markers by adding a negative match on the response payload (e.g. use response.text or response.body depending on the test helper) immediately after expect(response.status).toBe(404);—for example, assert response.text/body does not match a regex for common leak indicators like /<\?xml|<!DOCTYPE|\/etc\/passwd|-----BEGIN|BEGIN|PRIVATE/i to ensure no sensitive file contents are returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/api/test/functional/templates.spec.ts`:
- Around line 34-35: The test currently only asserts response.status === 404;
extend it to explicitly assert the response body doesn't contain leaked file
markers by adding a negative match on the response payload (e.g. use
response.text or response.body depending on the test helper) immediately after
expect(response.status).toBe(404);—for example, assert response.text/body does
not match a regex for common leak indicators like
/<\?xml|<!DOCTYPE|\/etc\/passwd|-----BEGIN|BEGIN|PRIVATE/i to ensure no
sensitive file contents are returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 824e81a8-5026-4c60-8db7-51dc966f01f8
📒 Files selected for processing (5)
apps/api/src/template/services/template-gallery/template-gallery.service.spec.tsapps/api/src/template/services/template-gallery/template-gallery.service.tsapps/api/src/utils/path.spec.tsapps/api/src/utils/path.tsapps/api/test/functional/templates.spec.ts
stalniy
requested changes
Jun 1, 2026
Contributor
stalniy
left a comment
stalniy
left a comment
There was a problem hiding this comment.
overcomplicated implementation but nice catch!
Comment on lines
+248
to
+252
| const cachePath = resolvePathWithinDir(`${this.#galleriesCachePath}/v1/templates`, `${id}.json`); | ||
| if (!cachePath) { | ||
| this.#logger.warn({ event: "TEMPLATE_PATH_TRAVERSAL_BLOCKED", templateId: id }); | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
issue(blocking): seems like it's the wrong place to fix it. It also breaks encapsulation of #templateCachePath function.
To prevent path traversal it's enough to remove all slashes or block them on http schema (zod) level
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3242 +/- ##
==========================================
- Coverage 66.68% 65.48% -1.21%
==========================================
Files 1065 981 -84
Lines 26123 24038 -2085
Branches 6292 5881 -411
==========================================
- Hits 17421 15742 -1679
+ Misses 7609 7236 -373
+ Partials 1093 1060 -33
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
GET /v1/templates/{id}(public, unauthenticated) built a file path by interpolating the caller-suppliedidstraight into the template cache path. Hono routes withdecodeURI(so a URL-encoded%2Fsurvives as a single segment), then decodes the param withdecodeURIComponent, so a payload like..%2F..%2Fsecretarrived at the service as../../secretand escaped the cache directory — disclosing arbitrary.jsonfiles the API process can read (other tenants' cached templates, service-account key JSON, config JSON).Found via an internal repo-wide security audit.
Fixes CON-428
What
resolvePathWithinDir()util (apps/api/src/utils/path.ts) that resolves a caller-supplied segment against a base dir and returnsnullunless it stays strictly inside it (rejects traversal, absolute escapes, and null bytes).getTemplateById()now resolves the read path through the util. A rejected id logs aTEMPLATE_PATH_TRAVERSAL_BLOCKEDwarning and returnsnull, which the controller maps to a 404 (no info leak). The trusted cache-write path is unchanged.{owner}-{repo}-{path}and can contain dots (e.g.…-DeepSeek-V3.1) and other folder-name characters, so an allowlist would risk breaking valid lookups./v1/templates/:id{…\.json}) only redirects and is transitively covered by the service guard — no change needed.Tests
path.spec.ts).nullwithout reading any file; a legit dot-id still resolves to the correct path.test/functional/templates.spec.ts): encoded traversal payloads (..%2F..%2F..%2Fetc%2Fpasswd,%2Fetc%2Fpasswd,..%2F..%2Fsecret, and double-encoded) all return 404 at the real endpoint.No breaking changes; no DB migrations.
Summary by CodeRabbit