This is an automated email from the ASF dual-hosted git repository.
ashishvijaywargiya pushed a commit to branch release24.09
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/release24.09 by this push:
new 503b3cb4e5 Improved externalLoginKey handling for cross-application
navigation (…#1710) (#1713)
503b3cb4e5 is described below
commit 503b3cb4e5ff97f1bccf1e1d1298895ead46de8b
Author: Ashish Vijaywargiya <[email protected]>
AuthorDate: Mon Aug 24 14:21:26 2026 +0530
Improved externalLoginKey handling for cross-application navigation
(…#1710) (#1713)
- 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.
Thank you Krishna Uprit for the contribution.
Cherry-picked from trunk commit
5f919b428ba43eff02e6be1712de30880f0bfc34, excluding test-only changes.
Co-authored-by: Krishna Uprit <[email protected]>
Co-authored-by: Krishnauprit18 <[email protected]>
---
.../webapp/control/ExternalLoginKeysManager.java | 147 +++++++++++++++------
.../java/org/apache/ofbiz/widget/WidgetWorker.java | 25 +++-
.../widget/renderer/html/HtmlTreeRenderer.java | 5 +-
3 files changed, 133 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 0abeaeca45..010f8ad227 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/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
index d13529a272..2209a4f360 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
@@ -47,6 +47,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;
@@ -54,8 +55,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) {
@@ -85,8 +101,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 b6645c8d6a..f6f4878708 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 {
