Validate ID-FF forward targets, FilesRepo identity names and SAML1 POST target by vharseko · Pull Request #1128 · OpenIdentityPlatform/OpenAM

vharseko · 2026-09-14T10:16:00Z

Four independent hardenings from the CodeQL high triage, grouped because each is a few lines.

ID-FF forwards (`java/unvalidated-url-forward` #196, #197, #198)

FSSingleLogoutServlet (/liberty-logout, unauthenticated) and FSReturnLogoutServlet append the metaAlias request parameter to the /ProcessLogout/metaAlias dispatcher path unchecked, and FSUtils.forwardRequest forwards to any same-host path taken from goto/LRURL. A RequestDispatcher can reach /WEB-INF and does not run the web.xml filters, so metaAlias=/../../WEB-INF/web.xml served the file.

New ForwardPathValidator: no .. segments (path parameters stripped the way the container does), no backslashes or control characters, not under /WEB-INF or /META-INF. The container percent-decodes a dispatcher path once more before normalising it (Tomcat ≥ 11.0.3 / 10.1.35: decode, then normalize, with no rejection of an encoded ..; dispatchersUseEncodedPaths is on by default), so the validator decodes once itself — two ASCII hex digits, + untouched — and runs every check on both the raw and the decoded form; the container then collapses //, /./ and ;params before mapping, so the /WEB-INF//META-INF check reads the collapsed path (and its fragment-stripped form, which HttpServletRequest.getRequestDispatcher() maps). An escape that is malformed, decodes to a URL delimiter (%25, %3F, %23) or is not valid UTF-8 is refused: no in-app path of the product carries one. The logout servlets answer 400 for a rejected alias, and forwardRequest answers 400 when the target is not a plain in-app path (a rejected target is never a legitimate flow).

FilesRepo (`java/path-injection` #208, #209; `java/regex-injection` #164)

constructFile built new File(typeDir, name) straight from the identity name; with a Files data store configured an identity could be created, read or deleted outside the repository. Names containing path separators (both kinds, so a repository copied between platforms stays valid), control characters, . or .. (or empty/null) now raise IdRepoException(ILLEGAL_ARGUMENTS); constructFile does not log the rejected name, and the exception message is the fixed bundle text (201), so IdServicesImpl.isExists does not surface it either. (FilesRepo.authenticate logging the user name at message level is pre-existing and untouched.)

Behaviour change: FileRepoFileFilter compiled the search pattern as a regex after replacing only *. The pattern is now a glob — * is the only wildcard, everything else is literal (Pattern.quote()): a(b no longer throws, user.1 no longer matches userx1. A caller that relied on regex metacharacters in a Files data store search now matches them literally. Case folding is CASE_INSENSITIVE | UNICODE_CASE, keeping the previous toLowerCase() behaviour for non-ASCII names.

SAML 1.x POST profile (`java/xss` #120)

SAMLUtils.postToTarget wrote the target URL into FORM ACTION unescaped; postYN() validates host, port and path but not the query string. Escaped with StringEscapeUtils.escapeHtml4 (commons-lang3, already on the module classpath; a sanitizer CodeQL models).

OAuth module (`java/insecure-randomness` #173, #174)

The CSRF state token id (CTS key, also the NONCE_TOKEN_ID cookie) came from RandomStringUtils.randomAlphanumeric (java.util.Random). newCsrfStateTokenId() now draws from the module's SecureRandom. The state value itself was already UUID.randomUUID().

Review rounds

CodeQL on the first revision drew two new findings on the changed lines: the FORM ACTION escaper was not one CodeQL recognises (Bump antisamy from 1.6.5 to 1.6.7 #489) and the forward guard's redirect fallback reused the request URL (<opendj.version>4.4.13</opendj.version> #488). 76ef05a switches to escapeHtml4 and to a 400 response respectively; both threads are answered inline.
Round 2 (maximthomas): the guard validated the pre-decode string, so metaAlias=/%252e%252e/%252e%252e/WEB-INF/web.xml passed it and the container's own decode turned it into the traversal. 5cbbeb7 makes the validator decode-then-check (above), adds entry-point tests through FSSingleLogoutServlet and FSUtils.forwardRequest, restores Unicode case folding in the file filter, rejects control characters in identity names without logging them, and replaces raw NUL bytes in the test sources with "\0" escapes so the files diff as text. Backslash rejection on POSIX and the unconditional audit call in spAssertionConsumer.jsp are answered in the discussion.
Round 3 (maximthomas): the reserved-directory check read the decoded but uncollapsed path, so //WEB-INF/web.xml, /./WEB-INF/web.xml, /WEB-INF;x/web.xml and /%2e/WEB-INF/web.xml passed it. e415dc5 collapses the path first (above), refuses delimiter escapes and invalid UTF-8, adds FSReturnLogoutServletTest through the raw-URI alias fallback, and pins the collapsed forms in the validator and forwardRequest tests.

Tests

ForwardPathValidatorTest (74 cases: encoded, collapsed, fragment, delimiter-escape and overlong-UTF-8 forms included), FSSingleLogoutServletTest (5: forward for a clean alias, 400 and no dispatcher for literal, single- and double-encoded traversal), FSReturnLogoutServletTest (5, through the raw getRequestURI() alias fallback), FSUtilsForwardPathTest (13), SAMLUtilsTest, FilesRepoTest (16), OAuthTest +1. Full suites of openam-federation-library (141) and openam-core (1847) pass.

Closes CodeQL alerts #120, #164, #173, #174, #196, #197, #198, #208, #209.

…ST target - FSSingleLogoutServlet (/liberty-logout, unauthenticated) and FSReturnLogoutServlet appended the metaAlias request parameter to the "/ProcessLogout/metaAlias" dispatcher path unchecked, and FSUtils.forwardRequest forwarded to any same-host path taken from goto/LRURL. A RequestDispatcher can reach /WEB-INF and skips the web.xml filters. Reject aliases with path traversal (400) and redirect instead of forwarding when the target is not a plain in-app path (ForwardPathValidator). - FilesRepo built new File(typeDir, name) from the identity name; with a Files data store an identity could be created, read or deleted outside the repository. Names with path separators, NUL, "." or ".." now raise IdRepoException. The search filter quoted nothing but "*", so regex metacharacters were interpreted; literal parts are now Pattern.quote()d. - SAMLUtils.postToTarget wrote the target URL into FORM ACTION unescaped; postYN() checks host, port and path but not the query string. - The OAuth module's CSRF state token id (also the NONCE_TOKEN_ID cookie) came from RandomStringUtils.randomAlphanumeric (java.util.Random); use the module's SecureRandom. Closes CodeQL alerts OpenIdentityPlatform#120, OpenIdentityPlatform#164, OpenIdentityPlatform#173, OpenIdentityPlatform#174, OpenIdentityPlatform#196, OpenIdentityPlatform#197, OpenIdentityPlatform#198, OpenIdentityPlatform#208, OpenIdentityPlatform#209

…arget - FSUtils.forwardRequest: answer 400 instead of redirecting when the forward target is rejected; the redirect reused the request-supplied URL. - SAMLUtils.postToTarget: escape the FORM ACTION with StringEscapeUtils.escapeHtml4, which CodeQL recognises as a sanitizer.

CodeQL kept reporting java/xss on the escaped lines: it follows taint through XMLUtils.escapeSpecialCharacters' char-by-char loop. Use commons-lang3 StringEscapeUtils.escapeHtml4 instead, as OpenIdentityPlatform#1128 does for the SAML1 POST target; taint does not flow through it. The clientsdk sample war gains the commons-lang3 dependency (the shaded clientsdk jar does not bundle it), the policy demo drops its XMLUtils import.

maximthomas

praise: three of the four hardenings are correct, two of them pinned by tests.

SecureRandom now backs the OAuth state token — openam-authentication/openam-auth-oauth2/src/main/java/org/forgerock/openam/authentication/modules/oauth2/OAuth.java:109 (newCsrfStateTokenId) — with the emitted alphabet unchanged at [A-Za-z0-9]{32}.
SAML1 POST ACTION is HTML-attribute-escaped with escapeHtml4 — openam-federation/openam-federation-library/src/main/java/com/sun/identity/saml/common/SAMLUtils.java:748 — and SAMLUtilsTest asserts the escaped output.
FilesRepo identity-name validation rejects .. segments and is genuinely pinned: FilesRepoTest runs 12/13 red at BASE and green at HEAD, and constructFile is the real sink.
The ID-FF forward branch now returns sendError(400) instead of the former open redirect, matching the author's #488 note (revision a4b3473 to 76ef05a).

issue (blocking): the forward guard validates the pre-decode string; getRequestDispatcher decodes once more, so one extra encoding layer passes it.

openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java:65-83
callers: openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/services/logout/FSSingleLogoutServlet.java:117-150, .../logout/FSReturnLogoutServlet.java, .../common/FSUtils.java:359-376

containsTraversal scans the raw, still-encoded string for literal \, control chars and literal .. segments — no percent-decode. getRequestDispatcher then decodes (Tomcat 10.1, %2F/%5C = DECODE) and normalizes. Unauthenticated PoC via /liberty-logout: metaAlias=/%252e%252e/%252e%252e/WEB-INF/web.xml — getParameter decodes once, the validator sees %2e and passes, the dispatcher decodes again and /WEB-INF/web.xml is served with the REQUEST-only filters bypassed. A single-encoded variant reaches the same sink through the raw-getRequestURI fallback in FSServiceUtils.getMetaAlias. The same gap defeats the FSUtils.forwardRequest guard for goto / RelayState.

// Validate the string the container will actually resolve: decode + normalize first.
String decoded = path;
for (String prev = null; !decoded.equals(prev); ) {          // defends double-encoding
    prev = decoded;
    decoded = URLDecoder.decode(decoded, StandardCharsets.UTF_8);  // throws on a malformed %
}
decoded = decoded.replace('\\', '/');
// run the existing segment / WEB-INF / META-INF checks on `decoded`, not on the raw input

Or: treat the web.xml <dispatcher>FORWARD backstop (GHSA-5c3q) as the real control and keep this validator as defence-in-depth — URLDecoder is not byte-identical to Tomcat's UDecoder (it maps + to space), so a hand-rolled decoder is itself a divergence risk.

issue (blocking): the guard is tested only through its helper, with no encoded inputs, so both the bypass and a dropped call stay green.

openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java (all cases)

Every assertion calls isSafeForwardPath / isSafeMetaAlias directly — zero servlet/dispatcher references and no % in any datum. The suite is green at HEAD while the endpoint is fully bypassable (finding above), and a servlet that stops calling the guard would also stay green (entry-point-not-the-helper).

@DataProvider static Object[][] encodedTraversal() {
    return new Object[][] {
        {"/x/%2e%2e/%2e%2e/WEB-INF/web.xml"},   // single-encode — red today
        {"/x/%252e%252e/WEB-INF/web.xml"},       // double-encode — red today
        {"/x/..%2f..%2fWEB-INF/web.xml"},
    };
}
@Test(dataProvider = "encodedTraversal")
public void rejectsEncodedTraversal(String p) {
    assertThat(ForwardPathValidator.isSafeForwardPath(p)).isFalse();
}

Pin: also add an entry-point test through FSSingleLogoutServlet / FSUtils.forwardRequest asserting a 400 rather than a forward to /WEB-INF. Land both blocking fixes together.

issue (non-blocking): identity-name search folding is now ASCII-only, so a non-ASCII uppercase pattern silently misses.

openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java:1741

The old toLowerCase() folded Unicode; Pattern.CASE_INSENSITIVE alone is ASCII-only. javac-verified, raised by 4 sweep agents.

Pattern.compile(Pattern.quote(pattern), Pattern.CASE_INSENSITIVE | Pattern.UNICODE_CASE);

issue (non-blocking): \ is rejected in names on POSIX, where it is a legal filename char, so pre-existing backslash-named entries become unreadable.

openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java:1519-1521, :1093

An entry whose name contains a \ can no longer be read or deleted, and search() / getMembers over that type abort (the getAttributes call in the list loop at :1093 has no catch). Reject the platform separator (File.separatorChar) and .. segments rather than a hardcoded backslash.

issue (non-blocking): a malformed % in the path passes the un-decoding guard, then fails inconsistently downstream.

openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java:375-386, .../logout/FSSingleLogoutServlet.java:149-164

getRequestDispatcher throws on the bad escape; forwardRequest swallows it (blank 200), the servlet does not wrap it (500). Rejecting a malformed % in the guard (the decode above does this) yields a consistent 400.

issue (non-blocking): CR/LF is not rejected in names and the rejected name is logged verbatim — log-line forging where FilesRepo backs authenticate().

openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java:1519-1522

Reject \r / \n in the name, and log a sanitized value rather than the raw input at error level.

issue (non-blocking): a refused forward is still audited as success.

openam-server-only/src/main/webapp/saml2/jsp/spAssertionConsumer.jsp:224

auditForwardToLocalUserLogin() (emits AM_ACCESS_OUTCOME SUCCESSFUL) runs unconditionally after forwardRequest, so the new 400 path is recorded as a successful access. Pre-existing for the redirect arm; the 400 widens it. Gate the audit call on the forward's outcome.

issue (non-blocking): the two new test files contain raw NUL bytes, so git stores them as binary and GitHub shows no diff.

openam-core/src/test/java/com/sun/identity/idm/plugins/files/FilesRepoTest.java:48, openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java:51,83

Both files are unreviewable in the PR and editor-fragile. Use the `

…w follow-ups Review round 2 on OpenIdentityPlatform#1128: - ForwardPathValidator checked the raw string only, while the container percent-decodes a dispatcher path once more before normalising it (Tomcat >= 11.0.3 / 10.1.35: decode, then normalize, no rejection of an encoded ".."). metaAlias=/%252e%252e/%252e%252e/WEB-INF/web.xml passed the guard and reached /WEB-INF/web.xml. The validator now decodes once (two ASCII hex digits, "+" untouched), runs every check on both the raw and the decoded form, and refuses a malformed escape or one that survives the single decode. - Entry-point tests: FSSingleLogoutServletTest drives /liberty-logout and asserts 400 with no dispatcher for literal, single- and double-encoded aliases; FSUtilsForwardPathTest does the same for forwardRequest. - FilesRepo: identity names with any control character are refused (not only NUL) and the rejected name is no longer logged. FileRepoFileFilter folds case beyond ASCII again (UNICODE_CASE): accept() lowercases the file name with String.toLowerCase(), which the ASCII-only CASE_INSENSITIVE pattern did not match. - Test sources carry "\0" escapes instead of raw NUL bytes, so git diffs them as text.

vharseko · 2026-09-17T04:07:39Z

Round 2 addressed in 5cbbeb7.

Blocking: guard validated the pre-decode string — confirmed and fixed. Verified against the container we ship (tomcat:11-jre25): since 11.0.3 ("Refactor creation of RequestDispatcher instances so that the processing of the provided path is consistent with normal request processing", 10.1.35 on the 10.1 line) ApplicationContext.getRequestDispatcher does stripPathParams → URLDecode → normalize → map, with dispatchersUseEncodedPaths=true and encodedSolidusHandling/encodedReverseSolidusHandling=DECODE as context defaults; the former "Security check to catch attempts to encode /../ sequences" that returned null is gone. So metaAlias=/%252e%252e/%252e%252e/WEB-INF/web.xml did reach /WEB-INF/web.xml through the default servlet. ForwardPathValidator now decodes once itself — two ASCII hex digits only, + left alone, so it stays byte-for-byte with UDecoder on a path rather than with URLDecoder — and runs every check on both the raw and the decoded form. Rather than looping to a fixpoint, a value that still carries a % after the single decode is refused outright (no in-app path of the product has a second encoding layer), as is a malformed escape, which also gives the consistent 400 for the non-blocking %zz point instead of the swallowed IAE.

One detail of the finding does not hold on Tomcat and is noted only for the record: the raw-getRequestURI fallback in FSServiceUtils.getMetaAlias cannot deliver a working traversal there — /liberty-logout is an exact mapping (no path info), and for /ReturnLogout/* the request URI and the dispatcher path have the same depth, so an alias that climbs to /WEB-INF in the dispatcher climbs to /openam/WEB-INF in the request URI, which CoyoteAdapter normalises and StandardContextValve answers with 404 before the servlet runs. The guard covers both forms now regardless.

Blocking: tests only through the helper, no encoded inputs — ForwardPathValidatorTest now has 53 cases including single-, double- and mixed-encoded traversal, encoded WEB-INF, encoded NUL/CR/LF and malformed escapes, and both entry points are driven end to end: FSSingleLogoutServletTest calls doGet on the servlet with a mocked ServletContext and asserts sendError(400, …) with getRequestDispatcher never called for /../../WEB-INF/web.xml, /%2e%2e/%2e%2e/WEB-INF/web.xml, /%252e%252e/%252e%252e/WEB-INF/web.xml and /idp/..%2f..%2fWEB-INF/web.xml, plus the forward for a clean alias; FSUtilsForwardPathTest does the same for forwardRequest with a same-host target. Both were red before the fix (the encoded aliases reached the dispatcher). A servlet that drops the guard call goes red on the encoded rows.

Non-blocking

UNICODE_CASE — added; accept() still lowercases the file name with String.toLowerCase(), so without it ЖОР* no longer matched жора. Pinned by fileFilterFoldsCaseBeyondAscii.
Backslash on POSIX — keeping the rejection of both separators, deliberately. A backslash in an identity name has no legitimate use, and a Files repository is copied between hosts as a directory tree: a name that is one file on Linux becomes a traversal on Windows. An existing \-named entry can only have been created through the unvalidated path this PR closes.
CR/LF and the logged name — identity names now reject every control character (< 0x20, 0x7f), not only NUL, and the rejected name is not logged (type and length only). Pinned by the evil\r\nforged / tab\tname rows.
Malformed % — covered by the decode above: 400 from the guard, the container never sees it.
Audit after a refused forward in spAssertionConsumer.jsp — pre-existing, as you note; forwardRequest is void and swallows every outcome, so gating the audit means changing its signature and both JSP call sites. Out of scope here; happy to take it as a follow-up.
NUL bytes in the test sources — replaced with "\0" escapes; both files diff as text now.

Full suites: openam-federation-library 110/0, openam-core 1847/0.

maximthomas

praise: the round-1 gaps are genuinely addressed and the metaAlias sink is now sound.

ForwardPathValidator.resolve percent-decodes before validating, closing the round-1 pre-decode bypass.
The isSafeMetaAlias sink is airtight: climbing out of /ProcessLogout/metaAlias needs .. or \, and resolve refuses both in the raw and the decoded form, plus any residual %.
FilesRepo.FileRepoFileFilter no longer compiles caller input as a regex (Pattern.quote + *-only glob), removing a regex-injection / ReDoS surface.
New negative DataProvider cases pin the metaAlias traversal rejections.

issue (blocking): isSafeForwardPath runs its /WEB-INF deny-check on the decoded-but-un-normalized path.

openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java:59-61, :81-90

resolve decodes and rejects ../\/control/residual-%, but returns the string without collapsing //, folding /./, or stripping ;params from the value it returns (the ; strip lives only inside containsTraversal, for the .. test). The deny-check then reads that raw form, so //WEB-INF/web.xml, /./WEB-INF/web.xml, /WEB-INF;x/web.xml and /%2e/WEB-INF/web.xml all fail startsWith("/web-inf/") and pass — then Tomcat normalizes each to /WEB-INF/web.xml and DefaultServlet serves it. Delivery is unauthenticated: GET /openam/postLogin?metaAlias=<valid>&LRURL=http://<host:port>/openam//WEB-INF/web.xml.

private static String resolve(String value) {
    if (containsTraversal(value)) {
        return null;
    }
    String decoded = decodeOnce(value);
    if (decoded == null || decoded.indexOf('%') != -1 || containsTraversal(decoded)) {
        return null;
    }
    return normalize(decoded); // collapse //, drop "" and "." segments, strip ";param" per segment
}

Pin: isSafeForwardPath must return false for each of //WEB-INF/web.xml, /./WEB-INF/web.xml, /WEB-INF;x/web.xml, /%2e/WEB-INF/web.xml.

issue (non-blocking): the raw-URI getMetaAlias fallback is guarded but never exercised, and FSReturnLogoutServlet has no test at all.

openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/services/logout/FSReturnLogoutServlet.java:137-140, :150-156

Every traversingAliases row stubs request.getParameter("metaAlias"). But FSReturnLogoutServlet is prefix-mapped (/ReturnLogout/*), so the second source — FSServiceUtils.getMetaAlias(request), a raw getRequestURI() substring with zero decodes — is reachable and takes a different string into isSafeMetaAlias. The sink is sound today, so this is coverage, not a live bypass; but the load-bearing branch has no test.

// drive the fallback, not getParameter:
when(request.getParameter("metaAlias")).thenReturn(null);
when(request.getRequestURI()).thenReturn("/openam/ReturnLogout/metaAlias/idp/..%2f..%2fWEB-INF/web.xml");
// assert: sendError(SC_BAD_REQUEST) and verify(servletContext, never()).getRequestDispatcher(anyString());

issue (non-blocking): the identity name is still logged, so the "name not logged" mitigation holds only at the default error level.

openam-core/src/main/java/com/sun/identity/idm/server/IdServicesImpl.java:1103-1108, openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java:1396

isExists logs ide.getMessage() (which carries the queried name) at warning; FilesRepo logs the name at message. In a WAR, slf4j binds to openam-slf4j → Debug, so raising the level past error surfaces the raw name. Not a merge blocker, but the claim should be scoped to the default level.

issue (non-blocking): isExists returns a repo-order-dependent answer when one repository throws.

openam-core/src/main/java/com/sun/identity/idm/server/IdServicesImpl.java:1090-1111

lastException = null; // has good repo resets on every clean false, so a repo that throws IdRepoException is masked whenever a clean repo follows it, and re-raised when it is last. The new FilesRepo path makes this reachable. Pre-existing; decide the precedence (throw vs. answer false) explicitly rather than letting iteration order pick.

} catch (IdRepoException ide) {
    ...
    lastException = ide; // keep: don't let a later clean 'false' erase a real failure
}

note (non-blocking): the search filter changed semantics from regex to glob-literal — a real, useful, undocumented behaviour change.

openam-core/src/main/java/com/sun/identity/idm/plugins/files/FilesRepo.java:1740-1765

At BASE the filter did Pattern.compile(p.toLowerCase()) — caller input was a regex. At HEAD only * is a wildcard; the rest is Pattern.quoted. A caller that relied on regex metacharacters now matches them literally. Worth a changelog line and a pin.

// pin the new contract:
assertTrue(filterMatches("a*c", "aXc"));   // * still wildcards
assertFalse(filterMatches("a.c", "aXc"));  // '.' is now literal, not "any char"

suggestion (non-blocking): widen the validator's pinned inputs.

openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java, openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java

Add the blocking-issue normalization forms (//WEB-INF, /./WEB-INF, /%2e/WEB-INF, /WEB-INF;x/) and the untested edges #, %3f, DEL (0x7f), and an overlong-UTF-8 %C0%AE.

nitpick (non-blocking): a test mutates static FSUtils.deploymentURI.

openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java

Set-and-restore of a static is safe under sequential surefire but flakes under a forked/parallel runner sharing the JVM. Restore in try/finally (or @AfterMethod).

note (non-blocking): the WEB-INF/META-INF deny-set is wider than round-1's M2 — recorded as deliberate per the author; not re-argued.

Review round 3 on OpenIdentityPlatform#1128: - ForwardPathValidator ran the /WEB-INF check on the decoded but uncollapsed path, while the container collapses "//", "/./" and ";params" before mapping: "//WEB-INF/web.xml", "/./WEB-INF/web.xml", "/WEB-INF;x/web.xml" and "/%2e/WEB-INF/web.xml" passed and were served. resolve() now returns the collapsed path and the check reads that; the fragment-stripped form is checked too, since HttpServletRequest.getRequestDispatcher() drops "#..." before mapping. - An escape that decodes to a delimiter (%25, %3F, %23) or to invalid UTF-8 (overlong %C0%AE) is refused; the container would map the first literally and replace the second, neither of which is a path this code means to reach. - FSReturnLogoutServletTest drives the prefix-mapped servlet through the raw getRequestURI() alias fallback; FSUtilsForwardPathTest pins the collapsed forms and restores FSUtils.deploymentURI per test.

vharseko · 2026-09-17T07:15:26Z

Round 3 addressed in e415dc5.

Blocking: deny-check on the uncollapsed path — confirmed and fixed. resolve now returns the path as the container maps it: decoded once, then ;params stripped per segment and empty / . segments dropped, and isReserved reads that. //WEB-INF/web.xml, /./WEB-INF/web.xml, /WEB-INF;x/web.xml, /;/WEB-INF/web.xml, /%2e/WEB-INF/web.xml, /WEB-INF/ and /WEB-INF/./web.xml are pinned red-to-green in ForwardPathValidatorTest, and the first four again through FSUtils.forwardRequest in FSUtilsForwardPathTest (400, no dispatcher). Two neighbours closed while there: HttpServletRequest.getRequestDispatcher() drops #… before mapping, so the fragment-stripped form is checked as well (/WEB-INF#/x refused, /XUI/#login/ still forwards; a ServletContext dispatcher keeps the fragment, which the traversal checks on the whole string already cover); and an escape that decodes to a delimiter (%25, %3F, %23) or to invalid UTF-8 (%C0%AE, verified against UDecoder, which goes through baos.toString(charset) and so replaces rather than folds it to .) is refused outright rather than reasoned about.

Non-blocking

Raw-URI fallback / FSReturnLogoutServlet — FSReturnLogoutServletTest added: getParameter("metaAlias") returns null, the alias comes from getRequestURI(); literal, ..%2f, %2e%2e and %252e%252e forms get 400 with no dispatcher, and a clean alias gets past the guard (it then fails on the missing session with 500, not 400). Green at HEAD~1 as well, as you expected — coverage, not a bypass.
Name still logged — IdServicesImpl.isExists logs ide.getMessage(), but the message here is the fixed bundle text for 201 ("Illegal arguments: One or more required arguments is null or empty"), not the name; FilesRepo:1396 is the pre-existing authenticate username: debug line at message level, untouched by this PR. Claim scoped accordingly in the description.
isExists repo-order precedence — pre-existing and marked deliberate in the code (lastException = null; //has good repo: a repository that answers cleanly outranks one that failed). Changing that precedence is a behaviour decision for a separate PR; not touched here.
regex → glob — documented as a Behaviour change paragraph in the description; the contract is already pinned by fileFilterTreatsRegexMetaCharactersLiterally (user.1 does not match userx1, i.e. . is literal) and fileFilterStillExpandsWildcards (us*r.1 matches uSomeThingr.1).
Wider inputs — added: the collapsed forms above, # (both ways), %3f, %23, DEL raw and as %7f, %c0%ae. 74 validator cases now.
Static FSUtils.deploymentURI — set in @BeforeMethod, restored in @AfterMethod(alwaysRun = true). Surefire runs this module's TestNG suite sequentially and no other test reads the field; a parallel runner sharing the JVM would need a lock around it, which is out of scope for this PR.

Full suite: openam-federation-library 141/0 (openam-core untouched this round, 1847/0 at HEAD~1).

maximthomas

praise: Round 2's normalisation gap is closed on every form that was measured, and the fix is pinned.

With ForwardPathValidator.java reverted to 5cbbeb7 and everything else at head, 15/97 cases go red (10 in ForwardPathValidatorTest, 5 in FSUtilsForwardPathTest); head runs 97/97.
The per-caller dispatcher model is right: FSUtils.forwardRequest (:376) goes through Request.getRequestDispatcher, which cuts the fragment; both logout servlets go through ServletContext.getRequestDispatcher, which keeps it — checked against the Tomcat 10.1.41 bytecode.
FSReturnLogoutServletTest is real-path: init → doGetPost → FSServiceUtils.getMetaAlias substring → validator, with never().getRequestDispatcher on the servlet's own context.
No input makes resolve() throw (%, /a%, /a%2, /a%zz, "", ;, #, ?); decodeOnce refuses %25/%3F/%23 and non-UTF-8 runs instead of reasoning about them.
@BeforeMethod/@AfterMethod(alwaysRun = true) for FSUtils.deploymentURI, 0 NUL bytes, row counts 74/13/5/5 as stated.

issue (blocking): collapse() strips ;params after decoding and per decoded segment; Tomcat strips them on the raw string up to the next raw /, so an encoded slash inside a parameter re-opens the reserved-directory check.

openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java:99-103, :113-117

ApplicationContext.getRequestDispatcher runs RequestUtil.stripPathParams on the raw string — ; to the next raw / — and only then UDecoder.URLDecode and normalize. resolve() decodes first, so a %2F inside the parameter has already become a separator by the time collapse() cuts at ;. For resource = "/;%2Fjunk/WEB-INF/web.xml" (LRURL http://host/openam/;%252Fjunk/WEB-INF/web.xml on /postLogin, one getParameter decode) the validator sees /;/junk/WEB-INF/web.xml → /junk/WEB-INF/web.xml → safe; Tomcat sees //WEB-INF/web.xml → /WEB-INF/web.xml. Measured with the real tomcat-catalina 10.1.41 classes; same for /;x%2Fjunk/…, /;jsessionid=1%2Fa/…, /;%2Fa%2Fb/…. No test row combines ; with %2F, so the suite stays green.

Reproduce the container's order — strip on the raw string before decoding, then collapse:

private static String resolve(String value) {
    if (containsTraversal(value)) {
        return null;
    }
    // The container strips ";param" on the raw string, up to the next raw '/',
    // before it decodes - an encoded slash inside a parameter goes with it.
    String decoded = decodeOnce(stripPathParams(value));
    if (decoded == null || containsTraversal(decoded)) {
        return null;
    }
    return collapse(decoded);
}

private static String stripPathParams(String path) {
    StringBuilder stripped = new StringBuilder(path.length());
    int pos = 0;
    while (pos < path.length()) {
        int semicolon = path.indexOf(';', pos);
        if (semicolon == -1) {
            stripped.append(path, pos, path.length());
            break;
        }
        stripped.append(path, pos, semicolon);
        int slash = path.indexOf('/', semicolon);
        pos = slash == -1 ? path.length() : slash;
    }
    return stripped.toString();
}

Or: refuse ; outright in containsTraversal (c == ';'), the way %25/%3F/%23 are already refused — no forward target in the tree carries a path parameter. Either way, fix the Javadoc on resolve() and the comment in containsTraversal, which currently describe the code, not Tomcat.

Pin (red at head): {"/;%2Fjunk/WEB-INF/web.xml"}, {"/;jsessionid=1%2Fa/WEB-INF/web.xml"}, {"/;x%2Fjunk/WEB-INF/web.xml"} in unsafeForwardPaths, and "http://sp.example.com:8080/openam/;%2Fjunk/WEB-INF/web.xml" in FSUtilsForwardPathTest.

issue (non-blocking): The fragment-stripped form is checked for reserved directories only, not for traversal, so a trailing ..# segment passes.

openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java:68-69, :131-140

containsTraversal compares whole segments, so ..# is not ..; the #-cut form at :69 goes to isReserved(collapse(...)) only. /x/..# validates, Request.getRequestDispatcher cuts the fragment and forwards /x/.. → /; /..# normalises to null, getRequestDispatcher returns null, and the NPE inside forwardRequest's try becomes a blank 200. No route into /WEB-INF (only the last segment can be ..), but the forward lands where the validator did not look.

int fragment = uri.indexOf('#');
if (fragment != -1) {
    String withoutFragment = resolve(uri.substring(0, fragment));
    if (withoutFragment == null || isReserved(withoutFragment)) {
        return false;
    }
}

Pin: {"/x/..#"} and {"/..#"} in unsafeForwardPaths — both green today.

suggestion (non-blocking): Two of the six round-2 forms are not pinned: /;x/WEB-INF/web.xml only as the variant /;/WEB-INF/web.xml, /.;x/WEB-INF/web.xml nowhere.

openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java:76-96, openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java

Both are refused at head; the ;-on-a-.-segment branch of collapse() is the one with no row. FSUtilsForwardPathTest carries four of the six. Also, /WEB-INF/ and /WEB-INF/./web.xml are green with the round-2 validator (its startsWith("/web-inf/") already refused them), so "red-to-green" holds for five of the seven listed rows.

{"/;x/WEB-INF/web.xml"},
{"/.;x/WEB-INF/web.xml"},

Add the same two to FSUtilsForwardPathTest's traversing targets.

question (non-blocking): Is OAuth.java:256 the only java.util.Random-derived value that reaches CookieUtils.newCookie? If not, "Closes #173, #174" will not hold after merge.

openam-authentication/openam-auth-oauth2/src/main/java/org/forgerock/openam/authentication/modules/oauth2/OAuth.java:256

Both alerts (java/insecure-randomness) are located at openam-shared/src/main/java/com/sun/identity/shared/encode/CookieUtils.java:395 and :397, a file this PR does not touch; the PR fixes one source that flows into that sink. The PR-ref CodeQL analysis reports results_count 0 for every category, so nothing can be confirmed from the PR ref (#488/#489 do show state=fixed on the merge ref). If another caller feeds such a value, drop the two numbers from the description rather than let them close a still-open alert.

vharseko requested a review from maximthomas September 14, 2026 10:16

vharseko added java Pull requests that update java code security Security fix or hardening (CVE, GHSA, XSS/CSRF/SSRF) tests Test suite: coverage, fixtures, or test infrastructure saml SAML / SAML2 federation oauth2 OAuth2 / OpenID Connect labels Sep 14, 2026

github-advanced-security Bot found potential problems Sep 14, 2026

View reviewed changes

Comment thread ...deration/openam-federation-library/src/main/java/com/sun/identity/saml/common/SAMLUtils.java Fixed

Comment thread ...tion/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java Fixed

vharseko mentioned this pull request Sep 15, 2026

Escape reflected request parameters in the sample servlets #1129

Merged

maximthomas requested changes Sep 17, 2026

View reviewed changes

vharseko requested a review from maximthomas September 17, 2026 04:07

maximthomas requested changes Sep 17, 2026

View reviewed changes

vharseko requested a review from maximthomas September 17, 2026 07:15

maximthomas requested changes Sep 17, 2026

View reviewed changes

Navigation Menu

Uh oh!

Uh oh!

Validate ID-FF forward targets, FilesRepo identity names and SAML1 POST target - #1128

Validate ID-FF forward targets, FilesRepo identity names and SAML1 POST target#1128
vharseko wants to merge 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix/idff-forward-filesrepo-saml-oauth

vharseko commented Sep 14, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

maximthomas left a comment

Uh oh!

vharseko commented Sep 17, 2026

Uh oh!

maximthomas left a comment

Uh oh!

vharseko commented Sep 17, 2026

Uh oh!

maximthomas left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

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

Uh oh!

ID-FF forwards (java/unvalidated-url-forward #196, #197, #198)

FilesRepo (java/path-injection #208, #209; java/regex-injection #164)

SAML 1.x POST profile (java/xss #120)

OAuth module (java/insecure-randomness #173, #174)

Review rounds

Tests

Uh oh!

Uh oh!

Uh oh!

maximthomas left a comment

Choose a reason for hiding this comment

Uh oh!

vharseko commented Sep 17, 2026

Uh oh!

maximthomas left a comment

Choose a reason for hiding this comment

Uh oh!

vharseko commented Sep 17, 2026

Uh oh!

maximthomas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vharseko commented Sep 14, 2026 •

edited

Loading

ID-FF forwards (`java/unvalidated-url-forward` #196, #197, #198)

FilesRepo (`java/path-injection` #208, #209; `java/regex-injection` #164)

SAML 1.x POST profile (`java/xss` #120)

OAuth module (`java/insecure-randomness` #173, #174)