Skip to content
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

FileLoader::normalizePath() strips leading ../ #138

Closed
spaze opened this issue Oct 1, 2016 · 8 comments · Fixed by #139
Closed

FileLoader::normalizePath() strips leading ../ #138

spaze opened this issue Oct 1, 2016 · 8 comments · Fixed by #139

Comments

@spaze
Copy link
Contributor

@spaze spaze commented Oct 1, 2016

First, thanks for making Latte! ☕

While migrating to Latte 2.4 I've find the following issue, if I do this in a presenter:

$this->createTemplate()->setFile('../../foo/bar.latte);

and there's

{include waldo.latte}

in bar.latte, rendering fails with

_RuntimeException: Missing template file foo/waldo.latte_

I've tracked it down to an issue in Latte\Loaders\FileLoader::normalizePath() where in the first foreach iteration when $res is still empty then if it hits $part === '..' condition then the $part is just thrown away because it tries to array_pop from an empty array.

The issue can be demonstrated by adding

Assert::same('../../tests', $loader->getReferredName('../tests', '../file'));

to Loaders.FileLoader.phpt.

Hope this is at least a bit understandable.

The workaround is to never pass leading ../ to setFile() (and use realpath() to remove these) but since I've already spent quite a while figuring out where's the problem, the proposed fix is an incoming pull request.

@OzzyCzech

This comment has been minimized.

Copy link

@OzzyCzech OzzyCzech commented Oct 3, 2016

Have same problem after upgrade to 2.4

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Oct 3, 2016

$this->createTemplate()->setFile('../../foo/bar.latte); seems like code smell (i.e. relative paths are code smell), you should use absolute path.

@OzzyCzech

This comment has been minimized.

Copy link

@OzzyCzech OzzyCzech commented Oct 3, 2016

I know (do not agree) but It should work - it's valid path and valid use case

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Oct 3, 2016

I agree that normalizePath should be fixed, but still I have to point out that using relative paths is a security problem and you should never use them :)

@dg dg closed this in #139 Oct 3, 2016
dg added a commit that referenced this issue Oct 3, 2016
We just remember these and the add them to the rest of the parts
dg added a commit that referenced this issue Oct 3, 2016
@spaze

This comment has been minimized.

Copy link
Contributor Author

@spaze spaze commented Oct 3, 2016

@dg Thanks! Why is that a problem?

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Oct 3, 2016

Because you never know against what it is relative.

@spaze

This comment has been minimized.

Copy link
Contributor Author

@spaze spaze commented Oct 3, 2016

Sure, it's always current directory which can be changed, of course. Maybe I just misunderstood what you said and thought you mean it always is a problem, sorry. It's clear now, thanks.

@spaze

This comment has been minimized.

Copy link
Contributor Author

@spaze spaze commented Oct 3, 2016

Just a side note, this discussion made me revisit my initial workaround for the original issue and realizing I'd need to work around it harder I've moved my files elsewhere, which made the code easier and solved the issue for me. So yeah, a happyend, thanks @dg :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.