diff --git a/.gitattributes b/.gitattributes index fe89a3ba5dd..3d1bc142acb 100644 --- a/.gitattributes +++ b/.gitattributes @@ -2742,7 +2742,6 @@ study/test/src/org/labkey/test/tests/study/AssayTest.java -text study/test/src/org/labkey/test/tests/study/CohortTest.java -text study/test/src/org/labkey/test/tests/study/ExtraKeyStudyTest.java -text study/test/src/org/labkey/test/tests/study/QuerySnapshotTest.java -text -study/test/src/org/labkey/test/tests/study/SpecimenReplaceTest.java -text study/test/src/org/labkey/test/tests/study/StudyCohortExportTest.java -text study/test/src/org/labkey/test/tests/study/StudyContinuousTest.java -text study/test/src/org/labkey/test/tests/study/StudyDateBasedTest.java -text diff --git a/api/src/org/labkey/api/security/AuthFilter.java b/api/src/org/labkey/api/security/AuthFilter.java index 94d5aa87fef..972843fac55 100644 --- a/api/src/org/labkey/api/security/AuthFilter.java +++ b/api/src/org/labkey/api/security/AuthFilter.java @@ -40,6 +40,7 @@ import org.labkey.api.util.GUID; import org.labkey.api.util.HttpUtil; import org.labkey.api.util.HttpsUtil; +import org.labkey.api.view.BadRequestException; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewServlet; @@ -189,6 +190,11 @@ else if (!AppProps.getInstance().isDevMode()) resp.sendError(HttpServletResponse.SC_BAD_REQUEST, uee.getMessage()); return; } + catch (BadRequestException bre) + { + resp.sendError(bre.getStatus(), bre.getMessage()); + return; + } catch (UnauthorizedException ue) { ExceptionUtil.handleException(req, resp, ue, ue.getMessage(), false); diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index e0238f36858..c0db6f86104 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -44,6 +44,8 @@ import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.permissions.CanSeeAuditLogPermission; import org.labkey.api.audit.provider.GroupAuditProvider; +import org.labkey.api.cache.CacheManager; +import org.labkey.api.cache.Throttle; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.CoreSchema; @@ -111,6 +113,7 @@ import org.labkey.api.util.emailTemplate.UserOriginatedEmailTemplate; import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; +import org.labkey.api.view.BadRequestException; import org.labkey.api.view.HasHttpRequest; import org.labkey.api.view.HttpView; import org.labkey.api.view.NotFoundException; @@ -176,6 +179,8 @@ public class SecurityManager public static final String TRANSFORM_SESSION_ID = "LabKeyTransformSessionId"; // issue 19748 /** GH Issue 1489: gates acceptance of the deprecated TRANSFORM_SESSION_ID cookie; default off */ public static final String FEATUREFLAG_ALLOW_TRANSFORM_SESSION_ID = "AllowTransformSessionIdAuth"; + public static final String FEATURE_FLAG_ALLOW_APIKEY_PARAMETER = "AllowApiKeyParameter"; + public static final String FEATURE_FLAG_ALLOW_APIKEY_PARAMETER_DESCRIPTION = "Allow authentication via 'apikey' URL parameter"; public static final String API_KEY = "apikey"; public static final String USER_ID_KEY = User.class.getName() + "$userId"; @@ -419,7 +424,13 @@ public void userAccountDisabled(User user) } } - private record Credentials(String username, String password) {} + private record Credentials(String username, String password, boolean shouldSetSessionCookie) + { + Credentials(String username, String password) + { + this(username, password, true); + } + } private static @Nullable Credentials getBasicCredentials(HttpServletRequest request) { @@ -568,12 +579,16 @@ public record AuthenticationAttempt(User user, HttpServletRequest request) {} String sessionId = PageFlowUtil.getCookieValue(request.getCookies(), JSESSIONID, null); if (!session.getId().equals(sessionId)) { - Cookie sessionCookie = new Cookie(JSESSIONID, session.getId()); - sessionCookie.setPath("/"); - sessionCookie.setHttpOnly(true); - if (AppProps.getInstance().isSSLRequired() || request.isSecure()) - sessionCookie.setSecure(true); - response.addCookie(sessionCookie); + // A URL can be planted in a victim's browser, so a URL parameter key must not set the session cookie + if (basicCredentials.shouldSetSessionCookie()) + { + Cookie sessionCookie = new Cookie(JSESSIONID, session.getId()); + sessionCookie.setPath("/"); + sessionCookie.setHttpOnly(true); + if (AppProps.getInstance().isSSLRequired() || request.isSecure()) + sessionCookie.setSecure(true); + response.addCookie(sessionCookie); + } request = new SessionReplacingRequest(request, session); } } @@ -670,8 +685,8 @@ public record AuthenticationAttempt(User user, HttpServletRequest request) {} /** * Determine if an API key is present, checking the "apikey" header first, then the deprecated * "LabKeyTransformSessionId" cookie (gated behind {@link #FEATUREFLAG_ALLOW_TRANSFORM_SESSION_ID}), and finally - * the "LabKeyTransformSessionId" GET parameter (supported permanently, since SSRS can't be made to use the header - * or a cookie). Return the credentials if an API key is present via any of these; otherwise return null. + * the "apikey" GET parameter (supported since SSRS can't be made to use a header, but only if the optional feature + * flag is enabled). Return the credentials if an API key is present via any of these; otherwise return null. * @param request Current request * @return First API key found or null if an apikey is not present. */ @@ -680,6 +695,7 @@ public record AuthenticationAttempt(User user, HttpServletRequest request) {} // Passing via the "apikey" HTTP header is our preferred approach and used by most LabKey client API // implementations String apiKey = request.getHeader(API_KEY); + boolean shouldSetSessionCookie = true; if (null == apiKey) { @@ -710,24 +726,54 @@ public record AuthenticationAttempt(User user, HttpServletRequest request) {} } else { - // Continue to support "LabKeyTransformSessionId" as a GET parameter, to support authentication through - // SSRS which can't be made to use BasicAuth, pass cookies, or other HTTP headers. Do not use - // request.getParameter() since that will consume the POST body, #32711. + // Continue to support "apikey" as a GET parameter only if the optional feature flag is enabled. This + // supports authentication through SSRS, which can't be made to use BasicAuth, pass cookies, or use HTTP + // headers. + Map params; try { - Map params = PageFlowUtil.mapFromQueryString(request.getQueryString()); - apiKey = params.get(TRANSFORM_SESSION_ID); + // Do not use request.getParameter() since that will consume the POST body, #32711. + params = PageFlowUtil.mapFromQueryString(request.getQueryString()); } catch (IllegalArgumentException e) { + // URLDecoder throws on malformed escapes; AuthFilter maps this to a 400 throw new UnsupportedEncodingException(e.getMessage()); } + + String apiKeyParameter = params.get(API_KEY); + + if (apiKeyParameter != null) + { + if (AppProps.getInstance().isOptionalFeatureEnabled(FEATURE_FLAG_ALLOW_APIKEY_PARAMETER)) + { + apiKey = apiKeyParameter; + shouldSetSessionCookie = false; + } + else + { + API_KEY_PARAMETER_WARNING_THROTTLE.execute("Rejected \"" + API_KEY + "\" parameter; " + + "enable the \"" + FEATURE_FLAG_ALLOW_APIKEY_PARAMETER_DESCRIPTION + "\" optional feature " + + "flag or authenticate via a different approach."); + } + } + else if (params.get(TRANSFORM_SESSION_ID) != null) + { + String message = "Rejected \"" + TRANSFORM_SESSION_ID + "\" parameter because it's no longer " + + "supported. Enable the \"" + FEATURE_FLAG_ALLOW_APIKEY_PARAMETER_DESCRIPTION + "\" optional " + + "feature flag and use the \"" + API_KEY + "\" parameter instead."; + API_KEY_PARAMETER_WARNING_THROTTLE.execute(message); + throw new BadRequestException(message); + } } } - return null != apiKey ? new Credentials(API_KEY, apiKey) : null; + return null != apiKey ? new Credentials(API_KEY, apiKey, shouldSetSessionCookie) : null; } + // Unauthenticated callers can trigger these warnings on every request + private static final Throttle API_KEY_PARAMETER_WARNING_THROTTLE = new Throttle<>("apikey parameter warnings", 10, CacheManager.HOUR, AUTH_LOG::warn); + public static final int SECONDS_PER_DAY = 60*60*24; public static abstract class TransformSession implements Closeable diff --git a/assay/src/org/labkey/assay/AssayModule.java b/assay/src/org/labkey/assay/AssayModule.java index 062512e22d9..8605523d7bf 100644 --- a/assay/src/org/labkey/assay/AssayModule.java +++ b/assay/src/org/labkey/assay/AssayModule.java @@ -119,6 +119,7 @@ import static org.labkey.api.assay.transform.DataTransformService.LEGACY_SESSION_COOKIE_NAME_REPLACEMENT; import static org.labkey.api.assay.transform.DataTransformService.LEGACY_SESSION_ID_REPLACEMENT; +import static org.labkey.api.assay.transform.DataTransformService.R_SESSIONID_REPLACEMENT; public class AssayModule extends SpringModule { @@ -184,6 +185,8 @@ protected void init() ParamReplacementSvc.get().registerDeprecated(LEGACY_SESSION_COOKIE_NAME_REPLACEMENT, ValidationException.SEVERITY.WARN, "Use '" + SecurityManager.API_KEY + "' instead"); ParamReplacementSvc.get().registerDeprecated(LEGACY_SESSION_ID_REPLACEMENT, ValidationException.SEVERITY.WARN, "Use '" + SecurityManager.API_KEY + "' instead"); + ParamReplacementSvc.get().registerDeprecated(R_SESSIONID_REPLACEMENT, ValidationException.SEVERITY.WARN, "Use '" + SecurityManager.API_KEY + "' instead"); + ParamReplacementSvc.get().registerDeprecated(SecurityManager.TRANSFORM_SESSION_ID, ValidationException.SEVERITY.WARN, "Use '" + SecurityManager.API_KEY + "' instead"); RoleManager.registerRole(new AssayDesignerRole()); diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index a96ce5ad774..7c7a1846424 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -373,6 +373,7 @@ import java.util.stream.Stream; import static org.labkey.api.mcp.McpService.VECTOR_SCHEMA; +import static org.labkey.api.security.SecurityManager.FEATURE_FLAG_ALLOW_APIKEY_PARAMETER_DESCRIPTION; import static org.labkey.api.settings.StashedStartupProperties.homeProjectFolderType; import static org.labkey.api.settings.StashedStartupProperties.homeProjectResetPermissions; import static org.labkey.api.settings.StashedStartupProperties.homeProjectWebparts; @@ -543,6 +544,11 @@ public QuerySchema createSchema(DefaultSchema schema, Module module) "Allow script authentication via legacy substitution parameters", "Allows pipeline/transform scripts to authenticate via legacy approaches ('LabKeyTransformSessionId', 'rLabkeySessionId', 'httpSessionId', and 'sessionCookieName' substitution parameters) instead of 'apikey' header authentication. This option will be removed in a future release of LabKey Server.", false, false, FeatureType.Deprecated)); + OptionalFeatureService.get().addFeatureFlag(new OptionalFeatureFlag(SecurityManager.FEATURE_FLAG_ALLOW_APIKEY_PARAMETER, + FEATURE_FLAG_ALLOW_APIKEY_PARAMETER_DESCRIPTION, + "Allows tools such as SSRS to authenticate by providing an API key via an 'apikey' parameter. Providing " + + "a credential via a URL parameter is not generally recommended, but in some cases this is the only option.", + false, false, FeatureType.Optional)); OptionalFeatureService.get().addExperimentalFeatureFlag(PageTemplate.EXPERIMENTAL_SHORT_CIRCUIT_ROBOTS, "Short-circuit robots", "Save resources by not rendering pages marked as 'noindex' for robots. This is experimental as not all robots are search engines.",