fix: prevent include/extends escaping the configured template directory - #982
Open
BrianWillows wants to merge 1 commit into
Open
fix: prevent include/extends escaping the configured template directory#982BrianWillows wants to merge 1 commit into
BrianWillows wants to merge 1 commit into
Conversation
Twig.path.relativePath resolves an include/extends/embed target without
checking that the result stays inside the configured template directory.
Enough '../' segments climb out of it, so a target influenced by untrusted
input reads arbitrary files off disk and inlines them into the rendered
output (CWE-22 / CWE-98).
Verified on 3.0.0: a template in views/ doing
{% include "../SECRET.txt" %} inlined a file placed outside views/.
When a template directory is configured (template.base), reject targets
that resolve outside it. Relative includes within that directory - such as
the existing "{% include "../simple.twig" %}" test fixture - are
unaffected; only paths that climb out of the root are refused. The
reference PHP implementation rejects these paths for the same reason
("Looking outside the configured directories is forbidden").
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Summary
Twig.path.relativePath()resolves an{% include %}/{% extends %}/{% embed %}target without checking that the result stays inside theconfigured template directory. The resolver walks
..segments and, once therunning path is exhausted, simply pushes them:
So enough
../segments escape the views/templates directory and read anarbitrary file, whose content is inlined into the rendered output
(CWE-22 path traversal → CWE-98 local file inclusion).
Reproduced on 3.0.0 — a template inside
views/containing{% include "../SECRET.txt" %}rendered the contents of a file placedoutside
views/.This is exploitable wherever the include target is influenced by untrusted
input: dynamic template/partial selection, a template name taken from a
request, or a user-supplied template. Static includes in trusted templates are
unaffected.
For reference, the PHP implementation's
FilesystemLoaderexplicitly refusesthese paths ("Looking outside the configured directories is forbidden"), so
this is a divergence from the upstream security model rather than intended
behaviour.
Fix
When a template directory has been configured (
template.base), reject targetsthat resolve outside it.
Relative includes within the configured root keep working — including the
repo's own
test/templates/include/relative.twigfixture(
{% include "../simple.twig" %}from a subfolder), which the first version ofthis patch broke and which drove the final approach. Only paths that climb out
of the root are refused.
Verification
mocha -r should).baseset to aviews/directory:{% include "../SECRET.txt" %}→ refused, nothing leaked.{% include "../partial.twig" %}fromviews/sub/→ still renders.Note / question for maintainers
When the include is refused, the thrown
Twig.Errorcurrently surfaces throughthe async path as a secondary
TypeError: Cannot read properties of undefined (reading 'valueOf')rather than cleanly propagating. That's the existing errorplumbing in
twig.async.js/twig.core.jsrather than something this patchintroduces, but I'm happy to follow up so the rejection reports cleanly — let me
know how you'd prefer it handled.
Found and fixed with AI assistance (Claude). Happy to add a regression test.