refactor: update partials resolution to support relative paths and improve error handling #458
Conversation
…prove error handling
…prove error handling
ReneWerner87
requested review from
Copilot,
efectn,
gaby and
sixcolors
and removed request for
a team
Summary of ChangesHello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the partials resolution mechanism within the Mustache template engine to enable the use of relative paths for including template snippets. This change addresses a reported issue by making template organization more intuitive and flexible. Additionally, the error handling for cases where partials cannot be found has been significantly improved, offering clearer diagnostic information to developers. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@Durandle46 can you check and test with your codebase |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the partial template resolution logic to support relative paths and improves error handling, which is a great enhancement for usability. However, a critical Path Traversal vulnerability has been introduced in mustache/mustache.go due to insufficient sanitization of partial names. This is particularly concerning when using the default mustache.New() constructor, which reads directly from the local filesystem without proper directory protections. Proper validation is needed to ensure partial names cannot escape the base directory.
| func (p fileSystemPartialProvider) lookupCandidates(partial string) []string { | ||
| addCandidate := func(candidates []string, candidate string) []string { | ||
| for _, existing := range candidates { | ||
| if existing == candidate { | ||
| return candidates | ||
| } | ||
| } | ||
| return append(candidates, candidate) | ||
| } | ||
|
|
||
| addExtension := func(raw string) string { | ||
| if strings.HasSuffix(raw, p.extension) { | ||
| return raw | ||
| } | ||
| return raw + p.extension | ||
| } | ||
|
|
||
| base := filepath.ToSlash(strings.TrimSpace(p.baseDir)) | ||
| base = strings.TrimSuffix(base, "/") | ||
| if base == "." || base == "/" { | ||
| base = "" | ||
| } | ||
|
|
||
| clean := filepath.ToSlash(strings.TrimSpace(partial)) | ||
| clean = strings.TrimPrefix(clean, "./") | ||
|
|
||
| candidates := make([]string, 0, 2) | ||
| if clean != "" { | ||
| candidates = addCandidate(candidates, addExtension(clean)) | ||
| if base != "" { | ||
| candidates = addCandidate(candidates, addExtension(path.Join(base, clean))) | ||
| } | ||
| } | ||
|
|
||
| return candidates | ||
| } |
There was a problem hiding this comment.
The lookupCandidates method does not sanitize the partial input, leading to a Path Traversal vulnerability. When the engine is initialized using mustache.New(), it uses the local filesystem directly (p.fileSystem is nil). A malicious template could use ../ sequences in a partial tag (e.g., {{> ../../../etc/passwd }}) to read arbitrary files from the server, escaping the intended template directory. More robust sanitization is required to prevent access outside the intended template directory. This involves cleaning the path and then checking for directory traversal attempts or absolute paths to ensure the resolved path remains within the baseDir.
WalkthroughThis PR addresses a bug where Mustache template partials fail to load silently. The fix implements multi-candidate path resolution for partial lookup, allowing partials to be resolved relative to an engine's base directory while maintaining backward compatibility. Updated tests and documentation reflect the corrected partial path resolution behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mustache/README.md (1)
57-62:⚠️ Potential issue | 🟡 MinorPre-existing version mismatch in the example code.
Line 17 instructs users to install
mustache/v4, but the Go example on line 61 importsmustache/v3. This wasn't introduced by this PR, but since you're updating this section, it might be worth fixing in the same pass.
🧹 Nitpick comments (1)
mustache/mustache.go (1)
78-82: Consider also stripping a leading./frombasefor consistency.After
filepath.ToSlashandTrimSuffix("/"), the code checks for"."and"/"but not"./". If someone passes"./"as the directory,basewould remain"./"after these checks, thenpath.Join("./", clean)would clean it anyway. Not a bug, but trimming"./"here (like you do forcleanon line 85) would make the logic symmetric.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
mustache/README.mdmustache/mustache.gomustache/mustache_test.gomustache/views/index.mustachemustache/views/relative.mustache
🧰 Additional context used
🧬 Code graph analysis (1)
mustache/mustache_test.go (1)
mustache/mustache.go (2)
New(99-112)NewFileSystemPartials(120-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: compare
🔇 Additional comments (9)
mustache/mustache.go (3)
34-59: Looks good overall — solid multi-candidate lookup with clear error reporting.The fallback logic is well-structured: try each candidate, capture the first error, and surface a descriptive message listing all tried paths. The verbose logging will help users debug partial resolution issues — directly addressing the silent-failure bug in issue
#383.
61-96:path.Joincleans away the leading./— verify this is intentional.On line 91,
path.Join(base, clean)normalizes the result, so whenbaseis"./views", the candidate becomes"views/partials/header.mustache"(without the leading./). This works becauseos.ReadFileresolves both identically relative to CWD, but it means the first candidate (partials/header.mustache) and the joined candidate (views/partials/header.mustache) will never be equal — so the dedup inaddCandidatewill never trigger for the commonNew("./views", ...)case. That's fine functionally, but worth noting.One edge case: if
partialis an empty or whitespace-only string,cleanbecomes""and no candidates are generated, hitting the"no partial candidates generated"fallback inGet. This seems reasonable.
145-147: LGTM — propagating verbosity at load time.Clean approach to wire engine-level
Verboseinto the partials provider duringLoad().mustache/mustache_test.go (2)
51-65: Good test coverage for the new relative partials feature.The test exercises the core bug fix — rendering partials via relative paths from the engine's base directory. Assertions are clear and consistent with the existing test patterns.
99-113:Test_FileSystempartials base updated to"./views"— aligns with new resolution logic.Changing
partialsFSfromhttp.Dir(".")tohttp.Dir("./views")is consistent with the updated partial paths inindex.mustache(nowpartials/headerinstead ofviews/partials/header). This makes the test accurately reflect the documented usage pattern.mustache/views/relative.mustache (1)
1-3: LGTM — clean test fixture for relative partials.Uses
partials/headerandpartials/footerpaths consistent with the updated resolution logic.mustache/views/index.mustache (1)
1-3: Partial paths updated to be relative to engine directory — consistent with the new resolution behavior.mustache/README.md (2)
24-28: Documentation examples updated to match new partial path convention — LGTM.
69-71: Helpful clarification on partial resolution behavior.The comment clearly explains that partials resolve relative to the engine directory / filesystem root and that full paths remain compatible for backward compatibility.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Pull request overview
Refactors Mustache partial resolution so partials can be referenced relative to the engine’s views directory (and improves observability when partial lookup fails), addressing gofiber/template issue #383.
Changes:
- Updated Mustache partial lookup to try multiple candidate paths and return richer errors (plus optional verbose logs).
- Adjusted example templates/docs to use
{{> partials/...}}instead of{{> views/partials/...}}. - Added/updated tests and fixtures for the new partial path expectations (including filesystem-partials behavior).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mustache/mustache.go | Adds candidate-based partial lookup and improved error reporting/logging; wires provider into New(...) and filesystem constructors. |
| mustache/mustache_test.go | Adds a relative-partials render test and updates filesystem-partials root to ./views. |
| mustache/views/index.mustache | Updates partial includes to partials/... form. |
| mustache/views/relative.mustache | Adds a template fixture using partials/... includes. |
| mustache/README.md | Updates documentation examples to the new partial include style and clarifies resolution behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| candidates = addCandidate(candidates, addExtension(clean)) | ||
| if base != "" { | ||
| candidates = addCandidate(candidates, addExtension(path.Join(base, clean))) | ||
| } |
There was a problem hiding this comment.
lookupCandidates currently tries the raw partial path before the engine baseDir candidate. That means a partials/header reference can resolve to a file relative to the process working directory even when an engine directory is configured, which is contrary to the README statement that partials resolve relative to the engine directory / filesystem root and can pick the wrong file if duplicates exist. Prefer trying baseDir + partial first (when baseDir is set), and fall back to the raw path for compatibility.
| candidates = addCandidate(candidates, addExtension(clean)) | |
| if base != "" { | |
| candidates = addCandidate(candidates, addExtension(path.Join(base, clean))) | |
| } | |
| if base != "" { | |
| candidates = addCandidate(candidates, addExtension(path.Join(base, clean))) | |
| } | |
| candidates = addCandidate(candidates, addExtension(clean)) |
| if firstErr == nil { | ||
| firstErr = fmt.Errorf("no partial candidates generated") | ||
| } | ||
| return "", fmt.Errorf("render: partial %q does not exist (tried: %s): %w", partial, strings.Join(candidates, ", "), firstErr) | ||
| } |
There was a problem hiding this comment.
The new partial lookup/error-wrapping behavior isn’t covered by a failing-path test. Add a test that renders a template referencing a missing partial and assert that Render (or Load, depending on mustache behavior) returns an error containing the partial name and attempted candidates (and that it’s not silently ignored).
| // Or from an embedded system | ||
| // Note that with an embedded system the partials included from template files must be | ||
| // specified relative to the filesystem's root, not the current working directory | ||
| // engine := mustache.NewFileSystem(http.Dir("./views", ".mustache"), ".mustache") | ||
| // Partials are resolved relative to the engine directory / filesystem root. | ||
| // For compatibility, full paths also work when present in your templates. | ||
| // engine := mustache.NewFileSystem(http.Dir("./views"), ".mustache") |
There was a problem hiding this comment.
The README example shows http.Dir(...) in the (commented) embedded/filesystem snippet but the import block in the example doesn’t include net/http. Add the import so users can copy/paste and uncomment the snippet without compile errors.
1 participant
fixes #383
Summary by CodeRabbit