github-advanced-security[bot] commented on code in PR #2907: URL: https://github.com/apache/shiro/pull/2907#discussion_r4225662264
########## support/jakarta-ee/src/main/java/org/apache/shiro/ee/filters/FormResubmitResponse.java: ########## @@ -0,0 +1,142 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.shiro.ee.filters; + +import static org.apache.shiro.ee.filters.FormResubmitSupport.HttpHeaderConstants.LOCATION; +import jakarta.servlet.http.Cookie; +import jakarta.servlet.http.HttpServletResponse; +import jakarta.servlet.http.HttpServletResponseWrapper; +import java.io.IOException; +import java.lang.invoke.MethodHandles; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Consumer; +import lombok.Getter; +import lombok.Setter; +import lombok.SneakyThrows; +import lombok.experimental.Delegate; +import org.omnifaces.servlet.BufferedHttpServletResponse; + +/** + * Captures a replay's status, headers, cookies and body instead of committing them to the browser. + * Nothing reaches the browser response until {@link #applyTo} is called for a successful replay, + * so view-state GETs, failed attempts and Faces error handling (which resets the response) + * cannot disturb what the login request has already written, such as session cookies. + */ +final class FormResubmitResponse extends BufferedHttpServletResponse { + private final List<Consumer<HttpServletResponse>> deferred = new ArrayList<>(); + private final @Delegate(types = Captured.class) HttpServletResponse recorder; Review Comment: ## CodeQL / Failure to use secure cookies Cookie is added to response without the 'secure' flag being set. [Show more details](https://github.com/apache/shiro/security/code-scanning/57) ########## support/jakarta-ee/src/test/java/org/apache/shiro/ee/filters/FormResubmitSupportCookiesTest.java: ########## @@ -0,0 +1,104 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.shiro.ee.filters; + +import jakarta.servlet.ServletContext; +import jakarta.servlet.http.Cookie; +import jakarta.servlet.http.HttpServletResponse; + +import static org.apache.shiro.ee.filters.FormResubmitSupport.SHIRO_FORM_DATA_KEY; +import static org.apache.shiro.ee.filters.FormResubmitSupportCookies.addCookie; +import static org.apache.shiro.ee.filters.FormResubmitSupportCookies.cookieName; +import static org.apache.shiro.ee.filters.FormResubmitSupportCookies.deleteCookie; +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; + +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.mockito.junit.jupiter.MockitoExtension; + +/** + * Secure saved-form cookies can't be planted by another host, since they are host-prefixed + */ +@ExtendWith(MockitoExtension.class) +class FormResubmitSupportCookiesTest { + private static final String SECURE_COOKIES = "org.apache.shiro.form-resubmit.secure-cookies"; + @Mock + private ServletContext servletContext; + @Mock + private HttpServletResponse response; + + private Cookie added(boolean secure, String contextPath, int maxAge) { + when(servletContext.getAttribute(SECURE_COOKIES)).thenReturn(secure); + when(servletContext.getContextPath()).thenReturn(contextPath); + if (maxAge == 0) { + deleteCookie(response, servletContext, SHIRO_FORM_DATA_KEY); + } else { + addCookie(response, servletContext, SHIRO_FORM_DATA_KEY, "value", maxAge, true); + } + var cookie = ArgumentCaptor.forClass(Cookie.class); + verify(response).addCookie(cookie.capture()); Review Comment: ## CodeQL / Failure to use secure cookies Cookie is added to response without the 'secure' flag being set. [Show more details](https://github.com/apache/shiro/security/code-scanning/58) ########## support/jakarta-ee/src/main/java/org/apache/shiro/ee/filters/FormResubmitSupportCookies.java: ########## @@ -46,41 +34,45 @@ @NoArgsConstructor(access = AccessLevel.PRIVATE) @SuppressWarnings("HideUtilityClassConstructor") public class FormResubmitSupportCookies { - static final String DONT_ADD_ANY_MORE_COOKIES = "org.apache.shiro.no-more-cookies"; + private static final String HOST_PREFIX = "__Host-"; - static void addCookie(@NonNull HttpServletResponse response, ServletContext servletContext, - @NonNull String cookieName, @NonNull String cookieValue, int maxAge, boolean httpOnly) { - var cookie = new Cookie(cookieName, cookieValue); - cookie.setPath(servletContext.getContextPath()); + /** + * With secure cookies, the {@code __Host-} prefix makes browsers refuse the cookie unless this very host + * set it over HTTPS, so it can't be planted from another subdomain or over plain HTTP. + * The prefix requires {@code Path=/}, hence the context path moves into the name, URL-encoded + * (cookie-name safe) to keep co-hosted applications apart. + */ + static String cookieName(ServletContext servletContext, @NonNull String baseName) { + return isFormResubmitSecureCookies(servletContext) + ? HOST_PREFIX + baseName + URLEncoder.encode(servletContext.getContextPath(), StandardCharsets.UTF_8) + : baseName; + } + + private static Cookie newCookie(ServletContext servletContext, String baseName, String value, int maxAge) { + var cookie = new Cookie(cookieName(servletContext, baseName), value); + boolean secure = isFormResubmitSecureCookies(servletContext); + cookie.setPath(secure ? "/" : servletContext.getContextPath()); + cookie.setSecure(secure); cookie.setMaxAge(maxAge); - cookie.setHttpOnly(httpOnly); - if (EnvironmentLoaderListener.isFormResubmitSecureCookies(servletContext)) { - cookie.setSecure(true); - } - response.addCookie(cookie); + return cookie; } + /** + * @param cookieName base name, see {@link #cookieName} + */ static void addCookie(@NonNull HttpServletResponse response, ServletContext servletContext, - @NonNull String cookieName, @NonNull HttpCookie inputCookie) { - var cookie = new Cookie(cookieName, inputCookie.getValue()); - cookie.setPath(inputCookie.getPath() != null ? inputCookie.getPath() : servletContext.getContextPath()); - cookie.setMaxAge(Math.toIntExact(inputCookie.getMaxAge())); - cookie.setHttpOnly(inputCookie.isHttpOnly()); - if (EnvironmentLoaderListener.isFormResubmitSecureCookies(servletContext)) { - cookie.setSecure(true); - } + @NonNull String cookieName, @NonNull String cookieValue, int maxAge, boolean httpOnly) { + var cookie = newCookie(servletContext, cookieName, cookieValue, maxAge); + cookie.setHttpOnly(httpOnly); response.addCookie(cookie); } + /** + * @param cookieName base name, see {@link #cookieName} + */ static void deleteCookie(@NonNull HttpServletResponse response, ServletContext servletContext, @NonNull String cookieName) { - var cookieToDelete = new Cookie(cookieName, "tbd"); - cookieToDelete.setPath(servletContext.getContextPath()); - cookieToDelete.setMaxAge(0); - if (EnvironmentLoaderListener.isFormResubmitSecureCookies(servletContext)) { - cookieToDelete.setSecure(true); - } - response.addCookie(cookieToDelete); + response.addCookie(newCookie(servletContext, cookieName, "tbd", 0)); Review Comment: ## CodeQL / Sensitive cookies without the HttpOnly response header set [This sensitive cookie](1) doesn't have the HttpOnly flag set. [This sensitive cookie](2) doesn't have the HttpOnly flag set. [This sensitive cookie](3) doesn't have the HttpOnly flag set. [Show more details](https://github.com/apache/shiro/security/code-scanning/55) ########## support/jakarta-ee/src/main/java/org/apache/shiro/ee/filters/FormResubmitSupportCookies.java: ########## @@ -46,41 +34,45 @@ @NoArgsConstructor(access = AccessLevel.PRIVATE) @SuppressWarnings("HideUtilityClassConstructor") public class FormResubmitSupportCookies { - static final String DONT_ADD_ANY_MORE_COOKIES = "org.apache.shiro.no-more-cookies"; + private static final String HOST_PREFIX = "__Host-"; - static void addCookie(@NonNull HttpServletResponse response, ServletContext servletContext, - @NonNull String cookieName, @NonNull String cookieValue, int maxAge, boolean httpOnly) { - var cookie = new Cookie(cookieName, cookieValue); - cookie.setPath(servletContext.getContextPath()); + /** + * With secure cookies, the {@code __Host-} prefix makes browsers refuse the cookie unless this very host + * set it over HTTPS, so it can't be planted from another subdomain or over plain HTTP. + * The prefix requires {@code Path=/}, hence the context path moves into the name, URL-encoded + * (cookie-name safe) to keep co-hosted applications apart. + */ + static String cookieName(ServletContext servletContext, @NonNull String baseName) { + return isFormResubmitSecureCookies(servletContext) + ? HOST_PREFIX + baseName + URLEncoder.encode(servletContext.getContextPath(), StandardCharsets.UTF_8) + : baseName; + } + + private static Cookie newCookie(ServletContext servletContext, String baseName, String value, int maxAge) { + var cookie = new Cookie(cookieName(servletContext, baseName), value); + boolean secure = isFormResubmitSecureCookies(servletContext); + cookie.setPath(secure ? "/" : servletContext.getContextPath()); + cookie.setSecure(secure); cookie.setMaxAge(maxAge); - cookie.setHttpOnly(httpOnly); - if (EnvironmentLoaderListener.isFormResubmitSecureCookies(servletContext)) { - cookie.setSecure(true); - } - response.addCookie(cookie); + return cookie; } + /** + * @param cookieName base name, see {@link #cookieName} + */ static void addCookie(@NonNull HttpServletResponse response, ServletContext servletContext, - @NonNull String cookieName, @NonNull HttpCookie inputCookie) { - var cookie = new Cookie(cookieName, inputCookie.getValue()); - cookie.setPath(inputCookie.getPath() != null ? inputCookie.getPath() : servletContext.getContextPath()); - cookie.setMaxAge(Math.toIntExact(inputCookie.getMaxAge())); - cookie.setHttpOnly(inputCookie.isHttpOnly()); - if (EnvironmentLoaderListener.isFormResubmitSecureCookies(servletContext)) { - cookie.setSecure(true); - } + @NonNull String cookieName, @NonNull String cookieValue, int maxAge, boolean httpOnly) { + var cookie = newCookie(servletContext, cookieName, cookieValue, maxAge); + cookie.setHttpOnly(httpOnly); response.addCookie(cookie); } + /** + * @param cookieName base name, see {@link #cookieName} + */ static void deleteCookie(@NonNull HttpServletResponse response, ServletContext servletContext, @NonNull String cookieName) { - var cookieToDelete = new Cookie(cookieName, "tbd"); - cookieToDelete.setPath(servletContext.getContextPath()); - cookieToDelete.setMaxAge(0); - if (EnvironmentLoaderListener.isFormResubmitSecureCookies(servletContext)) { - cookieToDelete.setSecure(true); - } - response.addCookie(cookieToDelete); + response.addCookie(newCookie(servletContext, cookieName, "tbd", 0)); Review Comment: ## CodeQL / Failure to use secure cookies Cookie is added to response without the 'secure' flag being set. [Show more details](https://github.com/apache/shiro/security/code-scanning/56) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
