Don't use SecureJoin(templateDir, templateName) #3542
Merged
+18
−12
Conversation
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
jandubois
changed the title
AkihiroSuda
reviewed
May 15, 2025
pkg/templatestore/templatestore.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| filePath := filepath.Join(templatesDir, name) |
There was a problem hiding this comment.
Needs a code comment to explain why SecureJoin is not needed
There was a problem hiding this comment.
Also name needs to be validated
There was a problem hiding this comment.
Not sure what that means? You want to drop SecureJoin when processing template:// URLs and instead check templateName here to make sure it doesn't include .. as a substring?
There was a problem hiding this comment.
SecureJoin can remain there in processing URLs, but we still have to make sure that something isn’t calling Read() with an unexpected value.
GoDoc should also explain the expected value of name
The final template path may be a symlink that points somewhere outside the templateDir (e.g. when installed by Homebrew). Also use `path` instead of `filepath` functions to join host and path of template:// URLs; they are not native filepaths and always uses slashes as path delimiters. Signed-off-by: Jan Dubois <[email protected]>
jandubois
force-pushed
the
template-symlink
branch
from
a887af7 to
ef331d0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The final template path may be a symlink that points somewhere outside the templateDir (e.g. when installed by Homebrew).
Also use
pathinstead offilepathfunctions to join host and path of template:// URLs; they are not native filepaths and always uses slashes as path delimiters.Fixes #3540