Remove the Java serialization round trip from the bundled Click HiddenField by vharseko · Pull Request #1109 · OpenIdentityPlatform/OpenAM

vharseko · 2026-08-19T15:52:58Z

Removes an unfiltered Java deserialization sink from the bundled Apache Click fork, together with the encoder that fed it.

What was there

ClickUtils.decode(String) performed Base64 → GZIP → ObjectInputStream.readObject() with no ObjectInputFilter, no class allowlist and no depth or size limit. Its only caller was HiddenField.bindRequestValue(), which reached it whenever the field's value class was a Serializable other than the handful the method parses explicitly — and in that path the argument is a request parameter. ClickUtils.encode(Object) produced the format decode read, and its only caller was HiddenField.render().

Why removal rather than an ObjectInputFilter

The sink has no legitimate caller to protect:

nothing outside the Click framework calls addControl(...) anywhere in the product;
the only Click HiddenField instances are the framework's own NonProcessedHiddenField over String and Long (Form.java:1321, :2391), and NonProcessedHiddenField is skipped during binding;
the pages the Click servlet serves are the configurator and upgrader (com.sun.identity.config.*); the deployment ships 13 .htm templates — 11 under /config/**, plus the framework's own /click/error.htm and /click/not-found.htm, which are the default ErrorPage/Page bindings. None of them declares a control, and the configurator pages read request parameters directly;
ClickServlet.processPageRequestParams() binds parameters via OGNL to declared page fields, restricted to primitives / String / Number / Boolean unless a custom TypeConverter is installed — none is registered anywhere in the product.

encode and decode therefore form a closed round trip inside HiddenField that nothing else produces or consumes. An ObjectInputFilter would leave a live readObject() guarded by configuration; deleting the pair cannot regress anything.

com.sun.identity.config.util.TemplatedForm goes at the same time. It extended the upstream Click Form, was referenced from nowhere, and was the one place from which a Click form could plausibly have been introduced.

Behaviour

A submission to a field declared with an unsupported value class now takes the path every other unrecognised value class already took — setValue(aValue), which refuses it because a String is not of the declared value class. No shipped page declares such a field.

The render() side loses its Serializable branch as well, so such a value falls to the existing appendAttributeEscaped("value", getValue()) at HiddenField.java:358 — escaped rather than raw, which is also the safer of the two.

Tests

HiddenFieldTest builds the exact payload the removed encode() produced, wrapping a canary whose readObject records that it ran, then asserts that the bind is refused with an IllegalArgumentException, that the canary stays untouched and that nothing was assigned to the field — for a concrete value class and for Serializable itself. Asserting the refusal rather than swallowing it matters: otherwise the canary assertion would also be satisfied by a bind that never reached the branch under test.

Two more cases check that encode/decode have not come back, and that String, Long, Boolean and an empty submission still bind as before.

Because the diff also moves render(), that side is pinned too. The HiddenField.java:358 call is the branch the deleted Serializable arm used to intercept, so a value class the control does not parse is rendered and the value attribute asserted escaped; the same for a String, with Long and Date covering the remaining two branches.

The tests were verified against the previous code by temporarily reverting the two source files. Four of the seven fail there: the canary is deserialized, the bind is not refused, ClickUtils.decode(String) is still present, and render() emits the raw serialization stream — value="H4sIAAAAAAAA/1vzloG1uIjBM78oXS+/IDUvMyU1rySzpLIgJ7EkLb8oFyyYmKuXnJOZnK2XnJ9XUpSfo+eRmQJU55aZmpMSklpcohKaV1xaUJBfVJKawgABjEwMDBUFAH5jUwJeAAAA" — unescaped. They are regression tests rather than a description of new behaviour.

mvn -o -pl openam-core test -Dtest=HiddenFieldTest   Tests run: 7,    Failures: 0, Errors: 0
mvn -o -pl openam-core test                          Tests run: 1822, Failures: 0, Errors: 0

Not covered here

The upstream org.apache.click:click-nodeps:2.3.0 jar remains a dependency of openam-core and carries its own copy of the same decode. Nothing can reach it. Its only caller there is the upstream HiddenField, which is driven by the upstream org.apache.click.ClickServlet — not mapped in either deployment descriptor, only the fork's servlet is. That servlet could not even be loaded if it were mapped: it extends javax.servlet.http.HttpServlet, and that class is absent from the deployment. Both modules resolve only jakarta.servlet-api-5.0.0, and none of the 381 jars on the openam-server-only classpath carries javax/servlet/http/HttpServlet.class, so loading org.apache.click.ClickServlet fails on superclass resolution.

Dropping the dependency is still a separate piece of work: the fork imports 32 distinct classes from that jar and does not compile without it.

ClickUtils.encode(Object), ClickUtils.decode(String) and com.sun.identity.config.util.TemplatedForm are public in the published openam-core, so removing them is binary-incompatible for anything downstream that reached for them. The repository carries no changelog file; this belongs in the GitHub release notes of the version that ships it.

@leanworld7-netizen

…he bundled Click HiddenField ClickUtils.decode(String) ran Base64, then GZIP, then ObjectInputStream.readObject() over its argument with no ObjectInputFilter, no class allowlist and no depth or size limit. Its only caller was HiddenField.bindRequestValue(), which reached it whenever the field's value class was a Serializable other than the handful the method parses explicitly - and the argument in that path is a request parameter. ClickUtils.encode produced the format decode read, and its only caller was HiddenField.render(). No page in the product binds such a field: nothing outside the Click framework calls addControl, the only Click HiddenField instances are the framework's own NonProcessedHiddenField over String and Long, and the pages the Click servlet serves are the configurator and upgrader, which create no controls. The sink therefore has no legitimate caller to protect, so it is removed rather than filtered: an ObjectInputFilter would leave a live readObject() behind a configuration, whereas deleting a closed round trip that nothing else produces or consumes cannot regress anything. A submission to a field declared with an unsupported value class now takes the same path every other unrecognised class already took - setValue(aValue), which refuses it because a String is not of the declared value class. The test builds the exact payload the removed encode() produced, wrapping a canary whose readObject records that it ran, and asserts the canary stays untouched when the field binds it. Against the previous code that test fails with the canary deserialized. com.sun.identity.config.util.TemplatedForm, which extended the upstream Click Form and was referenced from nowhere, is dropped at the same time: it was the one place from which a Click form could plausibly have been introduced. Reported by GitHub @leanworld7-netizen, and independently twice more.

maximthomas

Verified the reachability argument independently and it holds: no addControl caller outside the Click fork, the only HiddenField instances are NonProcessedHiddenField over String/Long (skipped during binding), TemplatedForm is unreferenced repo-wide including non-Java artifacts, and only the fork's Jakarta ClickServlet is mapped in any web.xml. The removal compiles cleanly — close() and the GZIPOutputStream import survive where still used, and no dangling {@link ClickUtils#encode/decode} javadoc is left. The canary test genuinely fails against the pre-change code.

Two things below; none is a defect in the behaviour being shipped.

The canary tests pass on any `IllegalArgumentException`, including one thrown before the code under test runs (Minor)

openam-core/src/test/java/org/openidentityplatform/openam/click/control/HiddenFieldTest.java:83-88 and :104-108:

try {
    field.bindRequestValue();
} catch (IllegalArgumentException expected) {
}
assertFalse(Canary.deserialized, "the submitted serialization stream must not be deserialized");

Canary.deserialized stays false whenever bindRequestValue() fails to reach the branch under test, not only when it reaches it and refuses — so the pair is satisfied by a bind that never happened. Add argument validation earlier in bindRequestValue() that throws IAE, or soften the unsupported-class path to logger.warn(...); return;, and the test goes green while testing nothing. The path is correct today (setValue(aValue) → setValueObject(String) → IAE at HiddenField.java:233-240), but nothing pins it.

One-line fix — TestNG 6.14.3 (pom.xml:1691-1693) has Assert.assertThrows:

assertThrows(IllegalArgumentException.class, field::bindRequestValue);
assertFalse(Canary.deserialized, "the submitted serialization stream must not be deserialized");

`render()` is changed by this PR and has no test coverage (Minor)

The diff deletes render()'s Serializable arm, which used the unescaped buffer.appendAttribute(...); such a value now falls to appendAttributeEscaped("value", getValue()) at openam-core/src/main/java/org/openidentityplatform/openam/click/control/HiddenField.java:358. The PR's own argument is "escaped rather than raw, which is also the safer of the two" — so that call is now load-bearing, and no test touches render(). HiddenFieldTest.java is the only test file under openam-core/src/test/java/org/openidentityplatform/openam/click/, so there is no prior coverage either.

For a String value class the chain is request parameter → setValue(aValue) (:271-272) → getValue() = getValueObject().toString() (:182-184) → render, with that one escaping call in between. Swapping it back to appendAttribute — the exact form the deleted branch used — would be reflected XSS with the suite green.

Cheap to pin while the fixture is open:

@Test
public void renderEscapesTheValueAttribute() {
    HiddenField text = new HiddenField("text", String.class);
    text.setValue("\"><script>alert(1)</script>");

    HtmlStringBuffer buffer = new HtmlStringBuffer();
    text.render(buffer);

    assertFalse(buffer.toString().contains("<script>"), "the value attribute must be escaped");
}

A Long-classed field asserting value="42" and a Date-classed one asserting epoch millis (HiddenField.java:353-355) would cover the rest of the branch.

Nits

.htm template count: the PR body says "13 .htm templates and all of them live under /config/**". It looks like 11 are under /config/**; /click/error.htm and /click/not-found.htm are the framework's default ErrorPage/Page bindings. Neither declares controls, so the conclusion is unaffected — but a future auditor re-running the argument will hit the discrepancy.
Changelog entry: ClickUtils.encode(Object), ClickUtils.decode(String) and com.sun.identity.config.util.TemplatedForm are all public in the published openam-core. Removal is the right call, but it is binary-incompatible and belongs in the release notes rather than being found by a downstream build break.
Redundant assertion: HiddenFieldTest.java:92-93 — assertFalse(field.getValueObject() instanceof Canary, ...) is always true post-change, since the IAE fires before valueObject is assigned. It does have signal against the pre-change code, so it is not wrong, just weaker than it reads.
Out of scope, spotted while verifying: openam-core/src/main/java/org/openidentityplatform/openam/click/control/Form.java:3061 tests control instanceof org.apache.click.control.Container — the upstream interface, which no fork control implements — so addStatefulFields never recurses into child containers. Harmless today (no Form is ever constructed) and unrelated to this PR; worth a separate issue.

…enFieldTest The canary tests swallowed the IllegalArgumentException, so their assertion was also satisfied by a bind that never reached the branch under test: any future validation added earlier in bindRequestValue(), or a softening of the refusal to a log-and-return, would leave them green while testing nothing. They now assert the refusal itself, and that nothing was assigned to the field. render() is moved by this change and had no coverage. The branch the deleted Serializable arm used to intercept now lands on appendAttributeEscaped(), so a value class the control does not parse is rendered and its value attribute asserted escaped there, with String, Long and Date covering the rest. Four of the seven cases now fail against the pre-change sources, up from three: the added render case emits the raw Base64 serialization stream unescaped.

vharseko · 2026-08-20T18:07:33Z

Thanks — both points taken. Pushed as 3a5a950, test file only; the source diff under review is unchanged.

1. The canary tests pass on any `IllegalArgumentException`

Agreed, and the reasoning is exactly right: the pair was satisfied by a bind that never happened. Both tests now assert the refusal itself:

assertThrows(IllegalArgumentException.class, field::bindRequestValue);

assertFalse(Canary.deserialized, "the submitted serialization stream must not be deserialized");
assertNull(field.getValueObject(), "no object should have been reconstructed from the parameter");

That also settles the "redundant assertion" nit below — assertNull is strictly stronger than assertFalse(... instanceof Canary) and still fails against the pre-change code, where valueObject is a reconstructed Canary. The second test had no getValueObject() check at all; it has one now.

2. `render()` is changed by this PR and has no test coverage

Agreed on the substance, with one correction to the example. new HiddenField("text", String.class) does not reach the call this PR made load-bearing: for valueCls == String.class render() takes the first arm, appendAttributeEscaped("value", String.valueOf(getValue())) at HiddenField.java:351. Line :358 is the trailing else, reachable only for a value class that is neither one of the seven listed nor a Date — precisely what the deleted instanceof Serializable arm used to intercept.

So the test for :358 uses a value class the control does not parse:

public static class Unsupported implements Serializable {
    @Override public String toString() { return "\"><script>alert(1)</script>"; }
}

HiddenField field = new HiddenField("unsupported", Unsupported.class);
field.setValueObject(new Unsupported());
field.render(buffer);
// value="&quot;&gt;&lt;script&gt;alert(1)&lt;/script&gt;"

Your String case is in as well (it pins :351, the only arm a shipped page could reach), plus Long asserting value="42" and Date asserting epoch millis for :353-355.

The :358 case is a genuine regression test, not just a pin. Reverting the two source files, four of the seven cases now fail instead of three:

aSerializableValueClassIsNoLongerDeserialized:115
    Expected IllegalArgumentException to be thrown, but nothing was thrown
anInterfaceValueClassIsNoLongerDeserializedEither:134
    the submitted serialization stream must not be deserialized expected [false] but found [true]
clickUtilsNoLongerCarriesTheSerializationRoundTrip:145
    ClickUtils.decode(String) is an unfiltered readObject() sink and must stay removed
renderEscapesAValueClassTheControlDoesNotParse:191
    unexpected rendering: <input type="hidden" name="unsupported" id="unsupported"
    value="H4sIAAAAAAAA/1vzloG1uIjBM78oXS+/IDUvMyU1rySzpLIgJ7EkLb8oFyyYmKuXnJOZ..."/>

That last line is the old unescaped appendAttribute(ClickUtils.encode(...)) writing a raw serialization stream into the attribute.

mvn -o -pl openam-core test -Dtest=HiddenFieldTest   Tests run: 7,    Failures: 0, Errors: 0
mvn -o -pl openam-core test                          Tests run: 1822, Failures: 0, Errors: 0

Nits

.htm count — you are right, and the PR body is corrected: 13 in total, 11 under /config/**, plus /click/error.htm and /click/not-found.htm as the framework's default ErrorPage/Page bindings. Neither declares a control, so the conclusion stands, but the sentence no longer misstates the split.
Changelog — the repository carries no changelog file, so this is a release-note item rather than a file change. Recorded under "Not covered here" in the PR body: ClickUtils.encode(Object), ClickUtils.decode(String) and com.sun.identity.config.util.TemplatedForm are public in the published openam-core and their removal is binary-incompatible for anything downstream that reached for them.
Form.java:3061 — confirmed, and it is a little worse than described. The fork's Container extends org.openidentityplatform.openam.click.Control and does not implement the upstream interface, so no fork control ever satisfies instanceof org.apache.click.control.Container and the recursion is dead. Had the condition matched an upstream control, the very next line, Container childContainer = (Container) control, casts to the fork interface and would throw ClassCastException. Filing separately; out of scope here as you say.

vharseko · 2026-08-20T18:43:29Z

The Form.java finding is filed as #1110. While writing it up a second instance of the same fork-vs-upstream confusion turned up in the same file — renderControls() at Form.java:2526 tests control instanceof FieldSet against the upstream org.apache.click.control.FieldSet (import at :26), and the fork ships no FieldSet of its own, so that branch is unreachable too. Both are covered there.

vharseko · 2026-08-21T06:47:57Z

Follow-up on the reachability argument you re-verified: it is stronger than the PR body claimed, so I have tightened that paragraph. The upstream org.apache.click.ClickServlet extends javax.servlet.http.HttpServlet, and that class is absent from the deployment — both modules resolve only jakarta.servlet-api-5.0.0, and none of the 381 jars on the openam-server-only classpath carries javax/servlet/http/HttpServlet.class. So the upstream servlet could not be loaded even if it were mapped, and the copy of decode in click-nodeps has no route to a request parameter. No code change; body text only.

The Form.java nit grew into two issues once I swept the whole fork. #1110 now covers the actual root cause — the javax→jakarta repoint was applied to only 16 of the 32 upstream classes the fork imports, which is why instanceof checks in ContainerUtils and Form can never match. #1111 covers the other half, where the upstream classes have no fork twin and reach javax.servlet-bound code: Format, MessagesMap and seven ClickUtils.getLogService() call sites, which fail with NoClassDefFoundError rather than quietly doing nothing. Neither is reachable today, for the same reason this PR's sink was not.

vharseko · 2026-08-21T07:05:00Z

Closing in favour of #1062, which removes the whole Apache Click fork rather than the one sink inside it.

All three source files this PR touched are deleted there — click/control/HiddenField.java, click/util/ClickUtils.java and com/sun/identity/config/util/TemplatedForm.java — along with the other 54 files of org.openidentityplatform.openam.click, click-nodeps and click-extras in openam-core/pom.xml, the click.version property and the OSGi export line in the root pom. GHSA-7j4m-m698-57hp is closed there by deletion, so this narrower change would only have to be reverted on top.

GHSA-7j4m-m698-57hp should now track #1062 instead of this PR. The advisory stays open until that one merges; it is approved and mergeable, with a manual wizard walkthrough outstanding.

Worth keeping from here for whoever audits the advisory:

The description above records why removal was the right call rather than an ObjectInputFilter — no page in the product binds a HiddenField with a Serializable value class, ClickServlet.processPageRequestParams() restricts OGNL binding to primitives / String / Number / Boolean with no TypeConverter registered, and encode/decode formed a closed round trip nothing else produced or consumed. @maximthomas verified that argument independently.
The upstream org.apache.click:click-nodeps:2.3.0 copy of the same decode was never reachable either: org.apache.click.ClickServlet extends javax.servlet.http.HttpServlet, and no jar in the deployment carries that class — both modules resolve only jakarta.servlet-api-5.0.0, and none of the 381 jars on the openam-server-only classpath provides javax/servlet/http/HttpServlet.class. Loading the upstream servlet fails on superclass resolution, mapped or not.
HiddenFieldTest (canary payload built from the removed encode() format, plus render() escaping coverage) is not carried over, since the class under test does not survive Migrate configurator/upgrade wizard from Apache Click to Jakarta Servlets + FreeMarker #1062. It is preserved on the branch fix/ghsa-7j4m-m698-57hp-remove-click-deserialization if it is ever wanted.

Two issues came out of the sweep done while reviewing this PR and are unaffected by the close: #1110 and #1111, both of which #1062 also resolves.

vharseko requested a review from maximthomas August 19, 2026 15:52

vharseko added the security Security fix or hardening (CVE, GHSA, XSS/CSRF/SSRF) label Aug 19, 2026

vharseko requested a review from tsujiguchitky August 19, 2026 15:59

maximthomas requested changes Aug 20, 2026

View reviewed changes

vharseko added tests Test suite: coverage, fixtures, or test infrastructure java Pull requests that update java code refactoring Code cleanup, refactor, dead-code or dependency removal labels Aug 20, 2026

vharseko requested a review from maximthomas August 20, 2026 18:34

vharseko mentioned this pull request Aug 20, 2026

Click fork: half the upstream imports were never repointed, so type checks in ContainerUtils and Form can never match #1110

Open

vharseko mentioned this pull request Aug 21, 2026

Click fork: upstream Format, MessagesMap and ClickUtils.getLogService() are javax.servlet-bound and fail with NoClassDefFoundError #1111

Open

vharseko mentioned this pull request Aug 21, 2026

Migrate configurator/upgrade wizard from Apache Click to Jakarta Servlets + FreeMarker #1062

Open

vharseko closed this Aug 21, 2026

Navigation Menu

Uh oh!

Uh oh!

Remove the Java serialization round trip from the bundled Click HiddenField - #1109

Remove the Java serialization round trip from the bundled Click HiddenField#1109
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix/ghsa-7j4m-m698-57hp-remove-click-deserialization

vharseko commented Aug 19, 2026 •

edited

Loading

Uh oh!

maximthomas left a comment

Uh oh!

vharseko commented Aug 20, 2026

Uh oh!

vharseko commented Aug 20, 2026

Uh oh!

vharseko commented Aug 21, 2026

Uh oh!

vharseko commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

vharseko commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was there

Why removal rather than an ObjectInputFilter

Behaviour

Tests

Not covered here

Uh oh!

maximthomas left a comment

Choose a reason for hiding this comment

The canary tests pass on any IllegalArgumentException, including one thrown before the code under test runs (Minor)

render() is changed by this PR and has no test coverage (Minor)

Nits

Uh oh!

vharseko commented Aug 20, 2026

1. The canary tests pass on any IllegalArgumentException

2. render() is changed by this PR and has no test coverage

Nits

Uh oh!

vharseko commented Aug 20, 2026

Uh oh!

vharseko commented Aug 21, 2026

Uh oh!

vharseko commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vharseko commented Aug 19, 2026 •

edited

Loading

The canary tests pass on any `IllegalArgumentException`, including one thrown before the code under test runs (Minor)

`render()` is changed by this PR and has no test coverage (Minor)

1. The canary tests pass on any `IllegalArgumentException`

2. `render()` is changed by this PR and has no test coverage