Enforce path containment in FolderTheme.getTemplate()#50098
Open
MdTanwer wants to merge 10 commits into
Open
Conversation
Route template resolution through ResourceLoader.getFile() so getTemplate() applies the same normalization and root-path guard as getResourceAsStream(). Closes keycloak#49961 Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens theme template resolution by ensuring FolderTheme.getTemplate() enforces path containment (consistent with getResourceAsStream()), preventing ../-style traversal from escaping the theme directory. It also adds a regression test to validate both valid and traversal-like template names.
Changes:
- Route
FolderTheme.getTemplate()throughResourceLoader.getFile(...)to apply the existing containment check. - Add a new
FolderThemeTest.testGetTemplatePathContainmentcovering a normal template and traversal/encoded traversal-like names.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| services/src/main/java/org/keycloak/theme/FolderTheme.java | Uses ResourceLoader.getFile(themeDir, name) to enforce path containment when resolving templates. |
| services/src/test/java/org/keycloak/theme/FolderThemeTest.java | Adds a regression test for template path containment behavior. |
Initialize FreeMarker with the bundled VERSION_2_3_32 to avoid legacy 2.3.0 defaults and opt into current security and correctness improvements. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: MdTanwer <122739350+MdTanwer@users.noreply.github.com>
| assertNotNull(theme.getTemplate("login.ftl")); | ||
| assertNull(theme.getTemplate(NONE + "forbidden.ftl")); | ||
| assertNull(theme.getTemplate(SINGLE + "forbidden.ftl")); | ||
| assertNull(theme.getTemplate(DOUBLE + "forbidden.ftl")); |
Member
There was a problem hiding this comment.
This is not testing changes as it is passing without your fix.
|
|
||
| assertNotNull(theme.getTemplate("login.ftl")); | ||
| assertNull(theme.getTemplate(NONE + "forbidden.ftl")); | ||
| assertNull(theme.getTemplate(SINGLE + "forbidden.ftl")); |
Member
There was a problem hiding this comment.
Same here - This is not testing changes as it is passing without your fix.
Author
There was a problem hiding this comment.
Updated the test to follow ResourceLoaderTest — using assertGetTemplate(..., false, true) for ../forbidden.ftl so we prove the file exists outside themeDir but is rejected, and removed the encoded literal path checks that passed without the fix.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: MdTanwer <122739350+MdTanwer@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: MdTanwer <122739350+MdTanwer@users.noreply.github.com>
…hub.com/MdTanwer/keycloak into fix/folder-theme-path-containment-49961
| public void testGetTemplatePathContainment() throws Exception { | ||
| Path tempDirectory = Files.createTempDirectory("folder-theme-test"); | ||
| File themeDir = new File(tempDirectory.toFile(), "login"); | ||
| org.junit.Assert.assertTrue("Failed to create theme directory: " + themeDir, themeDir.mkdirs()); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: MdTanwer <122739350+MdTanwer@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: MdTanwer <122739350+MdTanwer@users.noreply.github.com>
…hub.com/MdTanwer/keycloak into fix/folder-theme-path-containment-49961
Use assertGetTemplate with expectFileToExist so the regression test proves the target file exists outside themeDir but is rejected. Remove encoded literal path assertions that passed without the fix. Closes keycloak#49961
d407cbc to
795204d
Compare
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
FolderTheme.getTemplate()throughResourceLoader.getFile()for parity withgetResourceAsStream()../or encoded traversal sequences that escapethemeDirtestGetTemplatePathContainmentcovering valid and traversal pathsTest plan
mvn test -Dtest=FolderThemeTest,ResourceLoaderTestinservicesmodulelogin.ftl)Closes Themes: FolderTheme.getTemplate() missing path-containment check #49961