-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial support for resolving relative paths #343
Conversation
… allow for different types of relative path resolving
|
||
Path templatePath = Paths.get(parentPath); | ||
Path folderPath = templatePath.getParent() != null ? templatePath.getParent() : Paths.get(""); | ||
if (folderPath != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that folderPath
would ever be null, but the build was failing because Travis CI
was saying there was a possible null dereference. 🤷♂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits but I think this looks good.
|
||
@Override | ||
public String resolve(String path, JinjavaInterpreter interpreter) { | ||
if (path.startsWith("./")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we allow ../
straight up without having to do ./../
since that is a bit redundant?
} | ||
return path; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra line
default Optional<LocationResolver> getLocationResolver() { | ||
return Optional.empty(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I worry about here is the possibility that it could allow an arbitrary directory traversal exploit. I think that's only possible when using the FileLocator
, but could you verify?
That should only be possible using |
This PR adds support for resolving resource locations, such when using relative paths in a file system.
LocationResolver
interface, which resolves a string to the absolute location of a resource (i.e. resolves a relative path to an absolute path).ResourceLocator
can have an optionalLocationResolver
. I decided to have this live in theResourceLocator
since the behavior should be fairly linked.resolveResourceLocation()
method toJinjavaInterpreter
which uses theLocationResolver
of the currentResourceLocator
to resolve the location of a resource.include
,import
,extends
, andfrom [..] import
tags to resolve the location before retrieving a resource and before adding it to thecurrentPathStack
(this is to make sure all paths in the stack are absolute).RelativePathResolver
which is aLocationResolver
that resolves relative paths beginning with./
based on thecurrentPathStack
and thecurrent_path
context value. I also added tests for each of the tags mentioned above to test this behavior using theRelativePathResolver
. (I wasn't sure if this class should live in Jinjava or not, but felt it was a good basic example of aLocationResolver
.)