8000 Disable FreeMarker ?new() built-in in themes. by gaoyikeshuer · Pull Request #48280 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Disable FreeMarker ?new() built-in in themes. - #48280

Merged
stianst merged 1 commit into
keycloak:mainfrom
gaoyikeshuer:47915-fix-issue
Apr 21, 2026
Merged

Disable FreeMarker ?new() built-in in themes.#48280
stianst merged 1 commit into
keycloak:mainfrom
gaoyikeshuer:47915-fix-issue

Conversation

@gaoyikeshuer
@gaoyikeshuer gaoyikeshuer commented Apr 20, 2026
Copy link
Copy Markdown
Contributor

Closes #47915

@gaoyikeshuer
gaoyikeshuer requested a review from a team as a code owner April 20, 2026 14:46
@gaoyikeshuer
gaoyikeshuer marked this pull request as draft April 20, 2026 14:52
@gaoyikeshuer
gaoyikeshuer commented Apr 21, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

created a login ftl page to test if the fix is working or not.

`

<title>47915 PoC</title>

FreeMarker ?new() PoC

1. Arbitrary class instantiation

<#attempt> <#assign sb = "java.lang.StringBuilder"?new()> ${sb.append("hello-from-").append("?new()").toString()} <#recover> INSTANTIATION FAILED: ${.error}

2. OS command execution via freemarker.template.utility.Execute

<#attempt> <#assign ex = "freemarker.template.utility.Execute"?new()> 
id       -> ${ex("id")}
whoami   -> ${ex("whoami")}
uname -a -> ${ex("uname -a")}
<#recover> EXECUTE FAILED: ${.error}

If you see shell output above, the bug reproduces.

`

before fix:
image

After fix:
image

@gaoyikeshuer
gaoyikeshuer marked this pull request as ready for review April 21, 2026 09:50
rmartinc
rmartinc requested changes 8000 Apr 21, 2026
View reviewed changes
@rmartinc rmartinc left a comment •
edited
Loading
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gaoyikeshuer! Looks good! Please just change the warning to use the typical warning the docs/guides are using in other places. With that it's OK to me.

I also thought about adding a note in the upgrading guide (notable change). But I think that it's OK like it is. If someone thinks different please comment it to @gaoyikeshuer.

Comment thread docs/guides/ui-customization/themes.adoc Resolved
@mabartos
mabartos commented Apr 21, 2026
Copy link
Copy Markdown
Member

@gaoyikeshuer LGTM, just update the warning as @rmartinc and I think it's ready to go.

mabartos
mabartos requested changes Apr 21, 2026
View reviewed changes
Comment thread docs/guides/ui-customization/themes.adoc Resolved
Comment thread docs/guides/ui-customization/themes.adoc Resolved
@gaoyikeshuer
Disable FreeMarker ?new() built-in in themes. Closes keycloak#47915
4ddb8d8 
Signed-off-by: Yike Gao <yikegao8@gmail.com>
mabartos
mabartos approved these changes Apr 21, 2026
View reviewed changes
@mabartos mabartos left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

rmartinc
rmartinc approved these changes Apr 21, 2026
View reviewed changes
@rmartinc rmartinc left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gaoyikeshuer! LGTM too!

@keycloak-github-bot
keycloak-github-bot Bot commented Apr 21, 2026
Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.federation.ldap.LDAPSearchForUsersPaginationTest#testPagination

Keycloak CI - Base IT (5)

java.lang.AssertionError: 

Expected: a collection with size <10>
     but: collection size was <2>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Apr 21, 2026
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@mabartos
mabartos commented Apr 21, 2026
Copy link
Copy Markdown
Member

@rmartinc @gaoyikeshuer I believe it should be backported.

@stianst
stianst merged commit fa58c07 into keycloak:main Apr 21, 2026
85 checks passed
Sivakajan-tech pushed a commit to Sivakajan-tech/keycloak that referenced this pull request Apr 21, 2026
@gaoyikeshuer sivakajan1
Disable FreeMarker ?new() built-in in themes. Closes keycloak#47915 (k…
0e97e0a 
…eycloak#48280)

Signed-off-by: Yike Gao <yikegao8@gmail.com>
Signed-off-by: sivakajan1 <sivakajan@zerobeta.xyz>
@gaoyikeshuer
gaoyikeshuer deleted the 47915-fix-issue branch April 24, 2026 09:54
@stianst stianst mentioned this pull request Jun 1, 2026
Closed
sashyo pushed a commit to tide-foundation/keycloak-IGA that referenced this pull request Jul 20, 2026
@gaoyikeshuer @sashyo
Disable FreeMarker ?new() built-in in themes. Closes keycloak#47915 (k…
0520c5d 
…eycloak#48280)

Signed-off-by: Yike Gao <yikegao8@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stianst stianst stianst approved these changes
@rmartinc rmartinc rmartinc approved these changes
@mabartos mabartos mabartos approved these changes
+1 more reviewer
@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

FreeMarker templates allow instantiation of new objects and even running OS commands

4 participants

@gaoyikeshuer @mabartos @stianst @rmartinc
0