This is an automated email from the ASF dual-hosted git repository.
ashishvijaywargiya pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push:
new 5f919b428b Improved externalLoginKey handling for cross-application
navigation (#1710)
5f919b428b is described below
commit 5f919b428ba43eff02e6be1712de30880f0bfc34
Author: Krishna Uprit <[email protected]>
AuthorDate: Mon Aug 24 11:24:30 2026 +0530
Improved externalLoginKey handling for cross-application navigation (#1710)
- External login keys are now single-use per webapp and time-limited,
instead of a reusable token stored indefinitely in a JVM-wide map.
- Key is no longer attached to inter-app link targets that carry a
scheme or a network-path authority (widget links and tree links).
- A failed base-permission/logout check is now treated as a no-op
instead of proceeding with a null userLogin.
- Key usage is tracked per webapp, so a key stays usable for the other
webapps it was issued for.
- Added unit test coverage for key reuse rejection and the inter-app
absolute-target check; fixed an NPE in the replay test caused by an
unstubbed getServletContext().
Thank you Krishna Uprit for your contribution.
---------
Co-authored-by: Ashish Vijaywargiya <[email protected]>
Co-authored-by: Krishnauprit18 <[email protected]>
---
.../webapp/control/ExternalLoginKeysManager.java | 147 +++++++++++++++------
.../control/ExternalLoginKeysManagerTests.java | 78 +++++++++++
.../java/org/apache/ofbiz/widget/WidgetWorker.java | 25 +++-
.../widget/renderer/html/HtmlTreeRenderer.java | 5 +-
.../org/apache/ofbiz/widget/WidgetWorkerTest.java | 41 ++++++
5 files changed, 252 insertions(+), 44 deletions(-)
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManager.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManager.java
index c3389144bd..46d64475b2 100644
---
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManager.java
+++
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManager.java
@@ -19,6 +19,7 @@
package org.apache.ofbiz.webapp.control;
import java.util.Map;
+import java.util.Set;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
@@ -41,12 +42,65 @@ import org.apache.ofbiz.webapp.WebAppUtil;
public class ExternalLoginKeysManager {
private static final String MODULE =
ExternalLoginKeysManager.class.getName();
private static final String EXTERNAL_LOGIN_KEY_ATTR = "externalLoginKey";
- // This Map is keyed by the randomly generated externalLoginKey and the
value is a UserLogin GenericValue object
- private static final Map<String, GenericValue> EXTERNAL_LOGIN_KEYS = new
ConcurrentHashMap<>();
+ // How long a minted key remains redeemable. Bounds the window during
which a leaked key
+ // (address bar, browser history, Referer header, proxy log) is worth
anything to an attacker.
+ private static final long EXTERNAL_LOGIN_KEY_TTL_MILLIS = 2 * 60 * 1000;
+ // This Map is keyed by the randomly generated externalLoginKey and the
value is the ticket
+ // describing who it authenticates as and until when. One render mints one
key and embeds it
+ // in every cross-webapp link on that page (see getExternalLoginKey's
request-attribute
+ // cache), so a ticket is redeemable once per distinct destination webapp
-- see
+ // ExternalLoginTicket#redeemFor -- rather than once globally: that keeps
the ordinary
+ // app-switcher workflow (open two different apps from the same page)
working while still
+ // rejecting a second redemption against the same webapp.
+ private static final Map<String, ExternalLoginTicket> EXTERNAL_LOGIN_KEYS
= new ConcurrentHashMap<>();
// This variable is set to empty so we know need to read from the
properties file.
private static String isExternalLoginKeyEnabled = "";
+ /**
+ * A minted external login key, bound to the UserLogin it authenticates
and to a deadline.
+ * Optionally bound to a target context path too, for mint sites that know
which webapp the
+ * key is destined for; existing mint sites do not, so that field is null
and the check is
+ * skipped for them.
+ */
+ private static final class ExternalLoginTicket {
+ private final GenericValue userLogin;
+ private final String targetContextPath;
+ private final long expiresAtMillis;
+ // Context paths this ticket has already been redeemed for. A single
render shares one
+ // key across every cross-webapp link it emits, so the same key
legitimately needs to
+ // authenticate into several different webapps; this set is what stops
it authenticating
+ // into the *same* webapp twice, which is the actual replay this
ticket must prevent.
+ private final Set<String> redeemedContextPaths =
ConcurrentHashMap.newKeySet();
+
+ ExternalLoginTicket(GenericValue userLogin, String targetContextPath) {
+ this.userLogin = userLogin;
+ this.targetContextPath = targetContextPath;
+ this.expiresAtMillis = System.currentTimeMillis() +
EXTERNAL_LOGIN_KEY_TTL_MILLIS;
+ }
+
+ GenericValue getUserLogin() {
+ return userLogin;
+ }
+
+ boolean isValidFor(HttpServletRequest request) {
+ if (System.currentTimeMillis() > expiresAtMillis) {
+ return false;
+ }
+ return targetContextPath == null ||
targetContextPath.equals(request.getServletContext().getContextPath());
+ }
+
+ /**
+ * Atomically marks this ticket as redeemed for the request's webapp.
+ * @param request the request presenting this ticket's key
+ * @return true the first time this webapp redeems this ticket, false
on any repeat
+ * (replay) for the same webapp
+ */
+ boolean redeemFor(HttpServletRequest request) {
+ return
redeemedContextPaths.add(request.getServletContext().getContextPath());
+ }
+ }
+
/**
* Gets (and creates if necessary) an authentication token to be used for
an external login parameter.
* When a new token is created, it is persisted in the web session and in
the web request and map entry keyed by the
@@ -81,7 +135,7 @@ public class ExternalLoginKeysManager {
request.setAttribute(EXTERNAL_LOGIN_KEY_ATTR, externalKey);
session.setAttribute(EXTERNAL_LOGIN_KEY_ATTR, externalKey);
- EXTERNAL_LOGIN_KEYS.put(externalKey, userLogin);
+ EXTERNAL_LOGIN_KEYS.put(externalKey, new
ExternalLoginTicket(userLogin, null));
return externalKey;
}
}
@@ -110,49 +164,62 @@ public class ExternalLoginKeysManager {
String externalKey = request.getParameter(EXTERNAL_LOGIN_KEY_ATTR);
if (externalKey == null) return "success";
- GenericValue userLogin = EXTERNAL_LOGIN_KEYS.get(externalKey);
- if (userLogin != null) {
- //to check it's the right tenant
- //in case username and password are the same in different tenants
- Delegator delegator = (Delegator)
request.getAttribute("delegator");
- String oldDelegatorName = delegator.getDelegatorName();
- if
(!oldDelegatorName.equals(userLogin.getDelegator().getDelegatorName())) {
- delegator =
DelegatorFactory.getDelegator(userLogin.getDelegator().getDelegatorName());
- LocalDispatcher dispatcher =
WebAppUtil.makeWebappDispatcher(request.getServletContext(), delegator);
- LoginWorker.setWebContextObjects(request, response, delegator,
dispatcher);
- }
- // found userLogin, do the external login...
-
- // if the user is already logged in and the login is different,
logout the other user
- HttpSession session = request.getSession();
- GenericValue currentUserLogin = (GenericValue)
session.getAttribute("userLogin");
- if (currentUserLogin != null) {
- if
(currentUserLogin.getString("userLoginId").equals(userLogin.getString("userLoginId")))
{
- // same user, just make sure the autoUserLogin is set to
the same and that the client cookie has the correct userLoginId
- LoginWorker.autoLoginSet(request, response);
- // Same for the SecuredLoginId cookie
- LoginWorker.createSecuredLoginIdCookie(request, response);
- return "success";
- }
-
- // logout the current user and login the new user...
- LoginWorker.logout(request, response);
- // ignore the return value; even if the operation failed we
want to set the new UserLogin
- }
+ // Look up without removing: the same key is shared across every
cross-webapp link one
+ // render emits, so it must stay valid for whichever *other*
destination webapps the
+ // user still hasn't visited yet. redeemFor(), below, is what actually
stops replay --
+ // it rejects a second redemption against a webapp this exact ticket
already logged into.
+ ExternalLoginTicket ticket = EXTERNAL_LOGIN_KEYS.get(externalKey);
+ if (ticket == null || !ticket.isValidFor(request) ||
!ticket.redeemFor(request)) {
+ Debug.logWarning("Could not find a valid, not-yet-redeemed
userLogin for external login key: " + externalKey, MODULE);
+ // make sure the autoUserLogin is set to the same and that the
client cookie has the correct userLoginId
+ LoginWorker.autoLoginSet(request, response);
+ return "success";
+ }
- // check userLogin base permission and if it is enabled
- request.getSession().setAttribute("userLogin", userLogin);
- userLogin = LoginWorker.checkLogout(request, response);
+ GenericValue userLogin = ticket.getUserLogin();
+ //to check it's the right tenant
+ //in case username and password are the same in different tenants
+ Delegator delegator = (Delegator) request.getAttribute("delegator");
+ String oldDelegatorName = delegator.getDelegatorName();
+ if
(!oldDelegatorName.equals(userLogin.getDelegator().getDelegatorName())) {
+ delegator =
DelegatorFactory.getDelegator(userLogin.getDelegator().getDelegatorName());
+ LocalDispatcher dispatcher =
WebAppUtil.makeWebappDispatcher(request.getServletContext(), delegator);
+ LoginWorker.setWebContextObjects(request, response, delegator,
dispatcher);
+ }
+ // found userLogin, do the external login...
- LoginWorker.doBasicLogin(userLogin, request, response);
+ // if the user is already logged in and the login is different, logout
the other user
+ HttpSession session = request.getSession();
+ GenericValue currentUserLogin = (GenericValue)
session.getAttribute("userLogin");
+ if (currentUserLogin != null) {
+ if
(currentUserLogin.getString("userLoginId").equals(userLogin.getString("userLoginId")))
{
+ // same user, just make sure the autoUserLogin is set to the
same and that the client cookie has the correct userLoginId
+ LoginWorker.autoLoginSet(request, response);
+ // Same for the SecuredLoginId cookie
+ LoginWorker.createSecuredLoginIdCookie(request, response);
+ return "success";
+ }
- // Create a secured cookie with the correct userLoginId
- LoginWorker.createSecuredLoginIdCookie(request, response);
+ // logout the current user and login the new user...
+ LoginWorker.logout(request, response);
+ // ignore the return value; even if the operation failed we want
to set the new UserLogin
+ }
- } else {
- Debug.logWarning("Could not find userLogin for external login key:
" + externalKey, MODULE);
+ // check userLogin base permission and if it is enabled
+ request.getSession().setAttribute("userLogin", userLogin);
+ userLogin = LoginWorker.checkLogout(request, response);
+ if (userLogin == null) {
+ // base permission check failed or the account is flagged
logged-out; checkLogout
+ // already tore the session login down, so stop here rather than
dereference null.
+ LoginWorker.autoLoginSet(request, response);
+ return "success";
}
+ LoginWorker.doBasicLogin(userLogin, request, response);
+
+ // Create a secured cookie with the correct userLoginId
+ LoginWorker.createSecuredLoginIdCookie(request, response);
+
// make sure the autoUserLogin is set to the same and that the client
cookie has the correct userLoginId
LoginWorker.autoLoginSet(request, response);
return "success";
diff --git
a/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManagerTests.java
b/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManagerTests.java
index c5c3c7b1f9..7976bcb669 100644
---
a/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManagerTests.java
+++
b/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/ExternalLoginKeysManagerTests.java
@@ -20,15 +20,23 @@ package org.apache.ofbiz.webapp.control;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import jakarta.servlet.ServletContext;
import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
import jakarta.servlet.http.HttpSession;
+import org.apache.ofbiz.entity.Delegator;
import org.apache.ofbiz.entity.GenericValue;
import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
public class ExternalLoginKeysManagerTests {
@Test
@@ -79,4 +87,74 @@ public class ExternalLoginKeysManagerTests {
verify(request).setAttribute("externalLoginKey", externalLoginKey);
verify(session).setAttribute("externalLoginKey", externalLoginKey);
}
+
+ @Test
+ public void checkExternalLoginKeyIgnoresUnknownKeyWithoutLoggingAnyoneIn()
{
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ HttpServletResponse response = mock(HttpServletResponse.class);
+
when(request.getParameter("externalLoginKey")).thenReturn("ELunknown-key-not-in-map");
+
+ try (MockedStatic<LoginWorker> loginWorker =
mockStatic(LoginWorker.class)) {
+ String result =
ExternalLoginKeysManager.checkExternalLoginKey(request, response);
+
+ assertEquals("success", result);
+ loginWorker.verify(() -> LoginWorker.checkLogout(any(), any()),
never());
+ loginWorker.verify(() -> LoginWorker.doBasicLogin(any(), any(),
any()), never());
+ loginWorker.verify(() -> LoginWorker.autoLoginSet(request,
response));
+ }
+ }
+
+ @Test
+ public void checkExternalLoginKeyConsumesTheKeySoItCannotBeReplayed() {
+ // Mint a key the way ScreenRenderer/RequestHandler do.
+ Delegator delegator = mock(Delegator.class);
+ when(delegator.getDelegatorName()).thenReturn("default");
+ GenericValue userLogin = mock(GenericValue.class);
+ when(userLogin.getDelegator()).thenReturn(delegator);
+ when(userLogin.getString("userLoginId")).thenReturn("demoadmin");
+
+ HttpServletRequest mintRequest = mock(HttpServletRequest.class);
+ HttpSession mintSession = mock(HttpSession.class);
+ when(mintRequest.getSession()).thenReturn(mintSession);
+ when(mintRequest.getAttribute("userLogin")).thenReturn(userLogin);
+ String key = ExternalLoginKeysManager.getExternalLoginKey(mintRequest);
+
+ // Both redemptions target the same webapp, so the second one is the
actual replay case.
+ ServletContext servletContext = mock(ServletContext.class);
+ when(servletContext.getContextPath()).thenReturn("/partymgr");
+
+ try (MockedStatic<LoginWorker> loginWorker =
mockStatic(LoginWorker.class)) {
+ loginWorker.when(() -> LoginWorker.checkLogout(any(),
any())).thenReturn(userLogin);
+
+ // First redemption: a cookie-less client presents the freshly
minted key.
+ HttpServletRequest firstUse = mock(HttpServletRequest.class);
+ HttpServletResponse firstResponse =
mock(HttpServletResponse.class);
+ HttpSession firstSession = mock(HttpSession.class);
+ when(firstUse.getParameter("externalLoginKey")).thenReturn(key);
+ when(firstUse.getAttribute("delegator")).thenReturn(delegator);
+ when(firstUse.getSession()).thenReturn(firstSession);
+ when(firstUse.getServletContext()).thenReturn(servletContext);
+
+ String firstResult =
ExternalLoginKeysManager.checkExternalLoginKey(firstUse, firstResponse);
+
+ assertEquals("success", firstResult);
+ loginWorker.verify(() -> LoginWorker.doBasicLogin(userLogin,
firstUse, firstResponse), times(1));
+
+ // Replay: a second client presents the very same key value,
against the same webapp.
+ HttpServletRequest replay = mock(HttpServletRequest.class);
+ HttpServletResponse replayResponse =
mock(HttpServletResponse.class);
+ HttpSession replaySession = mock(HttpSession.class);
+ when(replay.getParameter("externalLoginKey")).thenReturn(key);
+ when(replay.getAttribute("delegator")).thenReturn(delegator);
+ when(replay.getSession()).thenReturn(replaySession);
+ when(replay.getServletContext()).thenReturn(servletContext);
+
+ String replayResult =
ExternalLoginKeysManager.checkExternalLoginKey(replay, replayResponse);
+
+ assertEquals("success", replayResult);
+ // doBasicLogin was called exactly once overall: never for the
replay.
+ loginWorker.verify(() -> LoginWorker.doBasicLogin(any(), any(),
any()), times(1));
+ loginWorker.verify(() -> LoginWorker.autoLoginSet(replay,
replayResponse));
+ }
+ }
}
diff --git
a/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
index 43cd182bd9..e32037954d 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
@@ -48,6 +48,7 @@ import org.jsoup.parser.Tag;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.HashMap;
+import java.util.regex.Pattern;
import static org.apache.ofbiz.base.util.UtilValidate.isNotEmpty;
@@ -55,8 +56,23 @@ public final class WidgetWorker {
private static final String MODULE = WidgetWorker.class.getName();
+ // Matches a target that carries a scheme (e.g. "https:", "javascript:")
or is protocol-relative
+ // ("//host/..."), meaning it points off the current host rather than at
another control servlet.
+ private static final Pattern ABSOLUTE_TARGET =
Pattern.compile("^(//|[A-Za-z][A-Za-z0-9+.-]*:)");
+
private WidgetWorker() { }
+ /**
+ * Checks whether an inter-app link target is absolute (carries a scheme
or a network-path
+ * authority) rather than a relative control path. Used to keep the
externalLoginKey
+ * credential off targets that can point at an attacker-chosen host.
+ * @param target the widget-supplied target, already HTML-entity-decoded
+ * @return true if the target has a scheme or starts with {@code //}
+ */
+ public static boolean isAbsoluteTarget(String target) {
+ return target != null && ABSOLUTE_TARGET.matcher(target).find();
+ }
+
public static URI buildHyperlinkUri(String target, String targetType,
Map<String, String> parameterMap,
String prefix, boolean fullPath,
boolean secure, boolean encode,
HttpServletRequest request,
HttpServletResponse response) {
@@ -86,8 +102,13 @@ public final class WidgetWorker {
}
} else if ("inter-app".equals(targetType)) {
uriString = localRequestName;
- String externalLoginKey = (String)
request.getAttribute("externalLoginKey");
- additionalParameters.put("externalLoginKey", externalLoginKey);
+ // Never attach the credential to a target that can point at
another host: the
+ // decoded target is checked here, not the original entity-encoded
one, so an
+ // HTML-encoded scheme is caught too. See
WidgetWorker#isAbsoluteTarget.
+ if (!isAbsoluteTarget(localRequestName)) {
+ String externalLoginKey = (String)
request.getAttribute("externalLoginKey");
+ additionalParameters.put("externalLoginKey", externalLoginKey);
+ }
} else if ("content".equals(targetType)) {
uriString = getContentUrl(localRequestName, request);
} else {
diff --git
a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlTreeRenderer.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlTreeRenderer.java
index cd9910a042..341e157f85 100644
---
a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlTreeRenderer.java
+++
b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlTreeRenderer.java
@@ -249,9 +249,10 @@ public class HtmlTreeRenderer extends HtmlWidgetRenderer
implements TreeStringRe
newURL.append(target);
writer.append(newURL.toString());
} else if ("inter-app".equalsIgnoreCase(urlMode) && req != null) {
+ writer.append(target);
String externalLoginKey = (String)
req.getAttribute("externalLoginKey");
- if (UtilValidate.isNotEmpty(externalLoginKey)) {
- writer.append(target);
+ // Never attach the credential to a target that can point at
another host.
+ if (UtilValidate.isNotEmpty(externalLoginKey) &&
!WidgetWorker.isAbsoluteTarget(target)) {
if (target.contains("?")) {
writer.append("&externalLoginKey=");
} else {
diff --git
a/framework/widget/src/test/java/org/apache/ofbiz/widget/WidgetWorkerTest.java
b/framework/widget/src/test/java/org/apache/ofbiz/widget/WidgetWorkerTest.java
index 05dfef9615..73979c4c4e 100644
---
a/framework/widget/src/test/java/org/apache/ofbiz/widget/WidgetWorkerTest.java
+++
b/framework/widget/src/test/java/org/apache/ofbiz/widget/WidgetWorkerTest.java
@@ -29,11 +29,16 @@ import java.net.URI;
import java.util.HashMap;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasProperty;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.when;
public final class WidgetWorkerTest {
@@ -72,4 +77,40 @@ public final class WidgetWorkerTest {
assertThat(withEncodedSpaces, hasProperty("scheme",
equalTo("javascript")));
assertThat(withEncodedSpaces, hasProperty("schemeSpecificPart",
equalTo("set_value('system', 'system', '')")));
}
+
+ @Test
+ public void interAppTargetKeepsExternalLoginKeyForARelativeControlPath() {
+ HttpServletRequest request = mock(HttpServletRequest.class);
+
when(request.getAttribute("externalLoginKey")).thenReturn("ELsome-key");
+
+ final URI uri = WidgetWorker.buildHyperlinkUri(
+ "/marketing/control/EditMarketingCampaign", "inter-app", new
HashMap<>(), null,
+ false, true, true, request, null);
+
+ assertThat(uri.getQuery(),
containsString("externalLoginKey=ELsome-key"));
+ }
+
+ @Test
+ public void interAppTargetDropsExternalLoginKeyForAnAbsoluteUrl() {
+ HttpServletRequest request = mock(HttpServletRequest.class);
+
when(request.getAttribute("externalLoginKey")).thenReturn("ELsome-key");
+
+ final URI uri = WidgetWorker.buildHyperlinkUri(
+ "https://attacker.example/collect";, "inter-app", new
HashMap<>(), null,
+ false, true, true, request, null);
+
+ assertThat(uri.getQuery(), not(containsString("externalLoginKey")));
+ }
+
+ @Test
+ public void interAppTargetDropsExternalLoginKeyForAProtocolRelativeUrl() {
+ HttpServletRequest request = mock(HttpServletRequest.class);
+
when(request.getAttribute("externalLoginKey")).thenReturn("ELsome-key");
+
+ final URI uri = WidgetWorker.buildHyperlinkUri(
+ "//attacker.example/collect", "inter-app", new HashMap<>(),
null,
+ false, true, true, request, null);
+
+ assertThat(uri.getQuery(), nullValue());
+ }
}
