[#1111] Click fork: stop calling javax.servlet-bound upstream ClickUtils

[maximthomas]

The fork is compiled against jakarta.servlet, but click-nodeps 2.3.0 is not: upstream ClickUtils.getLogService() resolves a javax.servlet.Servlet- Context that is absent from the classpath, and reads a thread local that only the upstream servlet populates. Route these calls to the fork's own ClickUtils.getLogService() instead.

ContainerUtils imported org.apache.click.util.ClickUtils, shadowing the same-package fork class, so its unqualified call resolved upstream too. Its LogService import is repointed as well: the upstream interface is javax.servlet-bound via onInit(ServletContext).

No behaviour change; nothing reaches this code today.

fixes #1111 issue

[vharseko]

Code review

Reviewed the diff, compiled openam-core from this branch (clean, no build break) and traced reachability of every touched method through XmlConfigService, web.xml and the configurator pages. The direction is right; two items below should be addressed before merge.

1. The sweep is incomplete in the file it edits — ContainerUtils.java:40-46

The import block this PR touched still pulls three control classes from upstream:

import org.apache.click.control.Button;
import org.apache.click.control.FieldSet;
import org.apache.click.control.Label;

The fork's Button and Label extend the fork Field, so every instanceof filter in this class is checked against a type the fork's controls can never be (lines 552, 1129, 1135, 1178, 1184, 1208, 1214, 1245, 1251, 1302). Consequences on live paths:

• getInputFields() / getFieldMap() / getHiddenFields() never exclude a fork Label or Button, so Form.clearValues() calls setValue(null) on a Button (wipes its caption) and validate() runs field validation on Labels;
• copyContainerToObject() copies a Button's caption into the target bean property;
• ContainerUtils.getButtons(form) always returns an empty list for a fork Form.

This is the same defect class the PR sets out to fix, three lines from the imports it corrected. (FieldSet has no fork equivalent, so that one is merely dead code — but it should go too.)

2. getResourceAsStream anchor swap is a real behaviour change — ClickUtils.java:1154, 1573, 1679

The PR description says "No behaviour change; nothing reaches this code today". Both halves look inaccurate:

• getResourceAsStream(name, aClass) tries TCCL.getResourceAsStream(name) first, but ClassLoader.getResourceAsStream does not strip a leading /. For "/click.xml", "/<pkg>/<Control>.files" and any slash-prefixed <file> entry from click.xml, that first branch always returns null, so aClass.getResourceAsStream(name) is the only resolver. Changing the anchor class therefore moves resolution from click-nodeps.jar's loader to openam-core.jar's.
• The code is reachable: web.xml registers org.openidentityplatform.openam.click.ClickServlet (openam-server-only/src/main/webapp/WEB-INF/web.xml:362, openam-federation/OpenFM/src/main/resources/xml/noconsole/web.xml:31), and XmlConfigService.deployFiles → ClickUtils.deployFile runs on every boot.

The change may well be the correct one — but it is worth stating as intentional rather than as a no-op.

Same bug class, still present elsewhere

• ClickUtils.java:68 — import org.apache.click.control.ActionLink shadows the fork's own ActionLink. getCssSelector() is declared over the fork Control, so the instanceof ActionLink branch at line 1461 is unreachable for every control the fork can produce; the method returns a[name=xxx] instead of a[class*=-xxx], i.e. exactly the case that branch documents as broken.
• ErrorReport.java — still fully-qualifies org.apache.click.util.ClickUtils at lines 201, 427, 439, 593, 607, 635, 656, even though the fork defines both getBundle and escapeHtml. Unlike autoPostRedirect, this class is on a live path (ClickServlet.handleException → ErrorPage → ErrorReport), so the fork keeps a hard reference into the javax.servlet-compiled jar in reachable code. Those two methods happen to be servlet-free today, which is the only reason it works.

Worth fixing while in the area

• ClickUtils.java:537 — the newly routed getLogService() now sits inside a catch block and can itself throw: getLogService() → getConfigService() → Context.getThreadLocalContext() → ContextStack.peek(), which throws RuntimeException("No Context available on ThreadLocal Context Stack") on an empty stack. autoPostRedirect is a public static utility callable from any servlet/filter thread, so a broken client connection can surface as an unrelated RuntimeException with the real IOException discarded. Worth guarding with Context.hasThreadLocalContext() or a nested try/catch.
• ClickUtils.java:1679-1681 — IOUtils.readLines(is) runs before the fileList == null check, so that guard is unreachable and a missing .files descriptor throws NullPointerException out of ClickServlet init instead of logging "there are no files to deploy". The stream is also never closed. Relevant here because the PR changed which class anchors that exact lookup, and no fork control currently ships a .files descriptor.
• ClickUtils.java:1190-1195 — getConfigService() is now the mandatory hop for every rerouted getLogService() call, and its failure message tells the operator to register <servlet-class>org.apache.click.ClickServlet</servlet-class>. Following that advice installs the upstream, javax.servlet-bound servlet, which never sets the fork's ConfigService attribute key — so the diagnostic makes the failure permanent. Should name org.openidentityplatform.openam.click.ClickServlet.

Pre-existing in the copied upstream code (separate issue, not this PR)

Noting these because they sit on or beside the lines touched here:

• autoPostRedirect (ClickUtils.java:492-500) writes target into an HTML attribute and the parameter values into a <textarea> via HtmlStringBuffer.append(), which does no escaping — so a quote in target or a </textarea><script> in a value injects into an auto-submitting page. The neighbouring appendAttribute("name", key) already escapes, showing the intent; appendEscaped is the fix.
• ClickUtils.java:530 sets Content-Length from buffer.length() (a char count) while writing buffer.toString().getBytes() (platform-default bytes), so any non-ASCII parameter yields a short Content-Length and a truncated or hanging response.
• ClickUtils.java:541 — close(os) after close(gos) is redundant; GZIPOutputStream.close() already closes the wrapped stream.

Suggestion

The fix is a hand-sweep of individual call sites with nothing preventing recurrence, and it is already demonstrably incomplete. A build-time guard scoped to org/openidentityplatform/openam/click/** — Checkstyle IllegalImport or forbidden-apis on org.apache.click.**, with an explicit allowlist for the types genuinely shared (Stateful, Format, PropertyUtils, HtmlStringBuffer) — would catch the next occurrence. There are currently no tests under the fork package either.

[maximthomas]

Fixed items 1, 3, 4 and 7 in 21c3743; description corrected for item 2. Notes on what I verified, and two places where the picture turned out different.

4 — ErrorReport. This is worse than either of us wrote, and it's the reason the PR matters.

The finding was right, but "those two methods happen to be servlet-free today, which is the only reason it works" doesn't hold: it wasn't working. Upstream org.apache.click.util.ClickUtils cannot be linked without javax.servlet at all — it fails during verification, before any method body runs. Probe with click-nodeps on the classpath and javax.servlet absent, i.e. the deployed shape:

javax.servlet ABSENT: ok
escapeHtml            FAILED: NoClassDefFoundError: javax/servlet/ServletOutputStream
PropertyUtils POJO    FAILED: NoClassDefFoundError: javax/servlet/ServletOutputStream
HSB append            -> hi
HSB appendEscaped     FAILED: NoClassDefFoundError: javax/servlet/ServletOutputStream

ErrorReport.toString() takes the production branch (click.xml ships <mode value="production"/>) straight into getBundle("click-control", locale), and ClickServlet.handleException → ErrorPage.onInit builds one for every unhandled page error. So any error in the configurator produced a NoClassDefFoundError in place of the error page. Fixed, and the fork's getBundle is byte-identical to upstream's (both TCCL → ResourceBundle.getBundle), so click-control.properties inside click-nodeps.jar still resolves.

The allowlist needs correcting before #1110 uses it. Same probe: PropertyUtils and HtmlStringBuffer are not safe, so the proposed allowlist of Stateful, Format, PropertyUtils, HtmlStringBuffer is wrong on three of four entries.

• PropertyUtils.getValue on a POJO links upstream ClickUtils via toGetterName and dies. Only the Map path short-circuits ahead of it. There is a live call at ContainerUtils.java:272, and the catch (Exception) at :284 will not catch it, since NoClassDefFoundError is an Error.
• HtmlStringBuffer is safe only for append/toString; appendEscaped and appendAttributeEscaped fail. Both files here use only the safe subset, but the explicit import shadows an identical fork class in the same package, so reaching for appendEscaped later is a runtime Error with no compile-time signal.
• Format (ClickUtils.java:73, live at :959, pushed into the Velocity model) is Context-bound throughout.

Stateful and TemplateException are the two I can confirm clean.

1 — control imports. Fixed: Button and Label repointed, FieldSet and its three guards removed. Removal is a strict no-op — upstream FieldSet implements upstream Control, so it is type-incompatible with the fork's Container.getControls(), and the fork has no class that is both Field and Container.

One correction on the consequences. Nothing instantiates a fork Form, Button or Label anywhere in the tree — TemplatedForm extends org.apache.click.control.Form (upstream) and is itself unreferenced. So clearValues() wiping a Button caption, validate() on Labels and copyContainerToObject() copying a caption can't occur. The imports still had to go, but this one was latent, unlike item 4.

2 — getResourceAsStream. Reachability correction accepted: XmlConfigService:256 → deployFiles → :1425 → ClickUtils.deployFile → :1573 runs at boot, and "nothing reaches this code today" was over-broad. Description updated to state the swap as intentional. The resolution claim doesn't hold, though, for a different reason at each site:

• "/click.xml" (:1154) and the .files descriptor (:1679) are absolute, so ClassLoader.getResourceAsStream does return null and the anchor is the only resolver — but click-nodeps and openam-core sit side by side in WEB-INF/lib, so both anchor classes share one webapp classloader. Class.getResourceAsStream strips the / and delegates to it.
• :1573 is the reachable one, and its names are relative: deployResourcesOnClasspath hands deployFile entries like META-INF/resources/click/table.css, found by scanning the TCCL, so the first branch resolves them and the anchor is never consulted. deployControls contributes no names — click-controls.xml and extras-controls.xml both ship an empty <controls> element.

3 — ActionLink. Fixed. The branch was dead, though getCssSelector has no callers, so nothing observed the wrong selector.

7 — diagnostic. Fixed, and it was worse than stated: ConfigService.CONTEXT_NAME differs between fork and upstream, so the old text pointed operators at a servlet that sets a different attribute key entirely.

5 and 6 — not taken. Correct readings, but autoPostRedirect and deployFileList have zero callers. Same for the escaping and Content-Length bugs — agreed they are upstream-inherited and belong in their own issue.

Still open, found while verifying the above — logging rather than fixing here: PropertyUtils at ContainerUtils.java:272; the shadowing HtmlStringBuffer imports; and Form.java:3061, where addStatefulFields tests against upstream Container and so never recurses, silently dropping nested field state in saveState/restoreState. All confined to the fork's Form, which has no OpenAM consumers.

Build-time guard. Agreed, and this exchange is the argument for it — careful manual inspection produced an allowlist that was wrong on three of four entries. It will be handled as part of #1110.

Verification is bytecode plus the probe above, since the fork has no tests: javap -c -p over the three touched classes now reports no reference to upstream ClickUtils, LogService, Button, Label, FieldSet or ActionLink.