This is an automated email from the ASF dual-hosted git repository. dashorst pushed a commit to branch wicket-10.x in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 4c1d83d306302f5b609e12b015d341f48f72cd2c Author: Emond Papegaaij <[email protected]> AuthorDate: Thu Sep 24 19:03:50 2026 +0200 Judge a resumed destination on the origin that chose it RestartResponseAtInterceptPageException remembers the url of the request it interrupts, and continueToOriginalDestination() returns the visitor to it once they have signed in. That second request is a redirect from inside the application, so it is same-origin whichever origin chose the url, and ResourceIsolationRequestCycleListener allows it on information that is entirely correct about a request that is entirely the wrong one. Another origin can therefore have a listener of its choosing run once in a session that has just authenticated - a Link.onClick(), or a Form.onSubmit() carrying the field values it put in the query string, since the whole url is remembered. The two cannot be told apart at that point: a signed out visitor clicking a link on a protected page, signing in and being returned to it produces the very same same-origin GET of a remembered listener url. So record where the interrupted request came from, ask the policies about that origin as well as about the request making the call, and keep the stricter of the two answers. Record it on the request rather than at a throw site. Anything can throw the exception - a page constructor, a listener, the render of a page - and only the exception knows when a destination is being remembered, so no particular stack can be relied on to be there when it happens. onBeginRequest runs before resolveRequestHandler(), so what it attaches is in place ahead of all of them, and InterceptData.set() copies it whoever throws. Keep the carrying of it generic. RestartResponseAtInterceptPageException holds a MetaDataEntry[] it never looks into, moved by setInterceptMetaData() from the request cycle into the remembered destination, and back out by the request mapper that already recognises the resume, before that mapper clears the data. Which properties matter is the listener's business: recordOrigin() takes the Sec-Fetch-Site, -Mode and -Dest headers FetchMetadataResourceIsolationPolicy reads, the Origin and Referer OriginResourceIsolationPolicy reads, and the method, and is protected so that a custom policy can record what it consults. Not every header - Cookie and Authorization have no business in session state. The order in which the listener obtains the targeted page and consults its policies is left alone. The interrupted request invokes no listener: it renders the login page, which any origin may ask for. It is the resume that has to be judged, and the resume is what this changes. An interrupted request that carried no origin headers resumes into unknownOutcomeAction, which defaults to ABORT - the same treatment such a client already gets when it invokes a listener directly, and setUnknownOutcomeAction relaxes both together. Where nothing was recorded, because the listener was not registered, was disabled, or did not check that request, the resume is judged exactly as before. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../wicket/protocol/http/intercept/HomePage.html | 3 + .../wicket/protocol/http/intercept/HomePage.java | 27 ++ .../http/intercept/InterceptedSession.java | 54 +++ .../wicket/protocol/http/intercept/LoginPage.html | 5 + .../wicket/protocol/http/intercept/LoginPage.java | 48 +++ .../intercept/ResumedDestinationIsolationTest.java | 424 +++++++++++++++++++++ .../protocol/http/intercept/SecureFormPage.html | 5 + .../protocol/http/intercept/SecureFormPage.java | 54 +++ .../protocol/http/intercept/SecureLinkPage.html | 5 + .../protocol/http/intercept/SecureLinkPage.java | 49 +++ .../protocol/http/intercept/SecureRenderPage.html | 3 + .../protocol/http/intercept/SecureRenderPage.java | 27 ++ .../RestartResponseAtInterceptPageException.java | 70 +++- .../ResourceIsolationRequestCycleListener.java | 216 ++++++++++- .../src/main/asciidoc/security/security_5.adoc | 2 + 15 files changed, 976 insertions(+), 16 deletions(-) diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/HomePage.html b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/HomePage.html new file mode 100644 index 0000000000..33413bf938 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/HomePage.html @@ -0,0 +1,3 @@ +<html xmlns:wicket> +<body>home</body> +</html> diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/HomePage.java b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/HomePage.java new file mode 100644 index 0000000000..35d2b80c15 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/HomePage.java @@ -0,0 +1,27 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.wicket.protocol.http.intercept; + +import org.apache.wicket.markup.html.WebPage; + +/** + * Where an unprotected visitor lands. + */ +public class HomePage extends WebPage +{ + private static final long serialVersionUID = 1L; +} diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/InterceptedSession.java b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/InterceptedSession.java new file mode 100644 index 0000000000..aa0a780334 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/InterceptedSession.java @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.wicket.protocol.http.intercept; + +import org.apache.wicket.protocol.http.WebSession; +import org.apache.wicket.request.Request; + +/** + * Session that knows whether the visitor signed in. + */ +public class InterceptedSession extends WebSession +{ + private static final long serialVersionUID = 1L; + + private boolean signedIn; + + /** + * @param request + */ + public InterceptedSession(Request request) + { + super(request); + } + + /** + * @return whether the visitor signed in + */ + public boolean isSignedIn() + { + return signedIn; + } + + /** + * Signs the visitor in. + */ + public void signIn() + { + signedIn = true; + } +} diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/LoginPage.html b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/LoginPage.html new file mode 100644 index 0000000000..4ac8426be7 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/LoginPage.html @@ -0,0 +1,5 @@ +<html xmlns:wicket> +<body> + <form wicket:id="form"><input type="submit" value="sign in"/></form> +</body> +</html> diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/LoginPage.java b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/LoginPage.java new file mode 100644 index 0000000000..70b02089d3 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/LoginPage.java @@ -0,0 +1,48 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.wicket.protocol.http.intercept; + +import org.apache.wicket.markup.html.WebPage; +import org.apache.wicket.markup.html.form.StatelessForm; + +/** + * Signs the visitor in and returns them to where they were going, the way + * {@code UsernamePasswordPanel} does. + */ +public class LoginPage extends WebPage +{ + private static final long serialVersionUID = 1L; + + /** + * Construct. + */ + public LoginPage() + { + add(new StatelessForm<Void>("form") + { + private static final long serialVersionUID = 1L; + + @Override + protected void onSubmit() + { + ((InterceptedSession)getSession()).signIn(); + continueToOriginalDestination(); + setResponsePage(getApplication().getHomePage()); + } + }); + } +} diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/ResumedDestinationIsolationTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/ResumedDestinationIsolationTest.java new file mode 100644 index 0000000000..71688ddbc0 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/ResumedDestinationIsolationTest.java @@ -0,0 +1,424 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.wicket.protocol.http.intercept; + +import static jakarta.servlet.http.HttpServletResponse.SC_FORBIDDEN; +import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.CROSS_SITE; +import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.DEST_DOCUMENT; +import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.DEST_EMBED; +import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.MODE_NAVIGATE; +import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.NONE; +import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.SAME_ORIGIN; +import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.SEC_FETCH_DEST_HEADER; +import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.SEC_FETCH_MODE_HEADER; +import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.SEC_FETCH_SITE_HEADER; +import static java.util.Arrays.asList; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import jakarta.servlet.http.HttpServletRequest; + +import org.apache.wicket.Page; +import org.apache.wicket.RestartResponseAtInterceptPageException; +import org.apache.wicket.Session; +import org.apache.wicket.authorization.IAuthorizationStrategy; +import org.apache.wicket.core.request.handler.IPageRequestHandler; +import org.apache.wicket.mock.MockApplication; +import org.apache.wicket.protocol.http.IResourceIsolationPolicy.ResourceIsolationOutcome; +import org.apache.wicket.protocol.http.OriginResourceIsolationPolicy; +import org.apache.wicket.protocol.http.ResourceIsolationRequestCycleListener; +import org.apache.wicket.protocol.http.ResourceIsolationRequestCycleListener.CsrfAction; +import org.apache.wicket.protocol.http.WebApplication; +import org.apache.wicket.request.IRequestHandler; +import org.apache.wicket.request.Request; +import org.apache.wicket.request.Response; +import org.apache.wicket.request.http.WebRequest; +import org.apache.wicket.request.component.IRequestableComponent; +import org.apache.wicket.util.tester.WicketTestCase; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * A request that resumes a destination remembered by + * {@link RestartResponseAtInterceptPageException} carries the origin that resumed it - this + * application's own, because signing in redirects within it - and not the origin that chose the + * URL. {@link ResourceIsolationRequestCycleListener} therefore judges such a request on what was + * recorded of the request that was interrupted. + */ +class ResumedDestinationIsolationTest extends WicketTestCase +{ + private static final Set<Class<? extends Page>> PROTECTED = new HashSet<>( + Arrays.asList(SecureLinkPage.class, SecureFormPage.class, SecureRenderPage.class)); + + private ResourceIsolationRequestCycleListener listener; + + @Override + protected WebApplication newApplication() + { + return new MockApplication() + { + @Override + public Class<? extends Page> getHomePage() + { + return HomePage.class; + } + + @Override + public Session newSession(Request request, Response response) + { + return new InterceptedSession(request); + } + + @Override + protected void init() + { + super.init(); + + mountPage("secure", SecureLinkPage.class); + mountPage("secureform", SecureFormPage.class); + mountPage("secureRender", SecureRenderPage.class); + mountPage("login", LoginPage.class); + + // the pattern IAuthorizationStrategy documents, and the one both + // AuthenticatedWebApplication and SimplePageAuthorizationStrategy implement + getSecuritySettings().setAuthorizationStrategy( + new IAuthorizationStrategy.AllowAllAuthorizationStrategy() + { + @Override + public <T extends IRequestableComponent> boolean isInstantiationAuthorized( + Class<T> componentClass) + { + if (PROTECTED.contains(componentClass) && + ((InterceptedSession)Session.get()).isSignedIn() == false) + { + throw new RestartResponseAtInterceptPageException(LoginPage.class); + } + return true; + } + }); + } + }; + } + + @BeforeEach + void before() + { + SecureLinkPage.invoked = false; + SecureFormPage.submitted = null; + withListener(new ResourceIsolationRequestCycleListener()); + } + + /** + * Another origin navigates a signed out visitor to a listener URL. The listener is not invoked - + * the page is refused and the login page renders instead - but the URL is remembered, and after + * the visitor signs in the resume must not run what that origin chose. + */ + @Test + void aListenerChosenByAnotherOriginIsNotInvokedWhenItsDestinationIsResumed() + { + navigate(CROSS_SITE, "secure?-1.-link"); + tester.assertRenderedPage(LoginPage.class); + + resume(signIn()); + + assertRefused(); + assertFalse(SecureLinkPage.invoked, "the listener another origin chose was invoked"); + } + + /** + * The same flow, started by the visitor themselves, resumes as it always has. + */ + @Test + void aListenerTheVisitorChoseIsInvokedWhenItsDestinationIsResumed() + { + navigate(SAME_ORIGIN, "secure?-1.-link"); + tester.assertRenderedPage(LoginPage.class); + + resume(signIn()); + + tester.assertRenderedPage(SecureLinkPage.class); + assertTrue(SecureLinkPage.invoked); + } + + /** + * A URL the visitor typed or bookmarked reaches the listener too. + */ + @Test + void aListenerOnABrowserInitiatedUrlIsInvokedWhenItsDestinationIsResumed() + { + navigate(NONE, "secure?-1.-link"); + tester.assertRenderedPage(LoginPage.class); + + resume(signIn()); + + tester.assertRenderedPage(SecureLinkPage.class); + assertTrue(SecureLinkPage.invoked); + } + + /** + * The whole URL is remembered, its query string included, so a resumed form submission runs with + * the field values the interrupted request carried. A plain top-level navigation is enough to + * choose them; no form post is involved. + */ + @Test + void aFormChosenByAnotherOriginIsNotSubmittedWhenItsDestinationIsResumed() + { + navigate(CROSS_SITE, "secureform?-1.-form&amount=CHOSEN-ELSEWHERE"); + tester.assertRenderedPage(LoginPage.class); + + resume(signIn()); + + assertRefused(); + assertNull(SecureFormPage.submitted, "the form another origin chose was submitted"); + } + + /** + * The counterpart, which is what makes the previous test worth having: the same resume, started + * by the visitor themselves, is not refused. + * <p> + * The form is not submitted either, but for a reason of its own: the url carries no page + * identifier, and a listener on a stateful page is not invoked on an instance created for the + * request that carries it, so the page is rendered instead. + */ + @Test + void aFormTheVisitorChoseIsNotRefusedWhenItsDestinationIsResumed() + { + navigate(SAME_ORIGIN, "secureform?-1.-form&amount=42"); + tester.assertRenderedPage(LoginPage.class); + + resume(signIn()); + + tester.assertRenderedPage(SecureFormPage.class); + assertNull(SecureFormPage.submitted); + } + + /** + * Nothing is known about where the interrupted request came from, so the resume is settled by + * {@link ResourceIsolationRequestCycleListener#setUnknownOutcomeAction(CsrfAction)} - which is + * how such a client is already treated when it invokes a listener directly. + */ + @Test + void aResumeIsRefusedWhenNothingWasKnownAboutTheOriginThatChoseIt() + { + navigate(null, "secure?-1.-link"); + tester.assertRenderedPage(LoginPage.class); + + resume(signIn()); + + assertRefused(); + assertFalse(SecureLinkPage.invoked); + } + + /** + * @see #aResumeIsRefusedWhenNothingWasKnownAboutTheOriginThatChoseIt() + */ + @Test + void aResumeIsAllowedWhenAnUnknownOriginIsConfiguredToBeAllowed() + { + listener.setUnknownOutcomeAction(CsrfAction.ALLOW); + + navigate(null, "secure?-1.-link"); + tester.assertRenderedPage(LoginPage.class); + + resume(signIn()); + + tester.assertRenderedPage(SecureLinkPage.class); + assertTrue(SecureLinkPage.invoked); + } + + /** + * A page may be rendered from anywhere, so a resumed render stays allowed even where the + * listener is extended to police renders. The recorded mode, destination and method are what + * let the policy see that the interrupted request was a top level navigation. + */ + @Test + void aRenderChosenByAnotherOriginIsStillAllowedWhenItsDestinationIsResumed() + { + withRendersChecked(); + + navigate(CROSS_SITE, "secureRender"); + tester.assertRenderedPage(LoginPage.class); + + resume(signIn()); + + tester.assertRenderedPage(SecureRenderPage.class); + } + + /** + * The policies are asked about the request that chose the url, so they see its headers and its + * method, and not those of the request that resumes - which is always a same-origin GET + * navigation, whatever the interrupted request was. + */ + @Test + void theResumeIsJudgedOnWhatTheInterruptedRequestCarried() + { + List<String> asked = new ArrayList<>(); + withListener(new ResourceIsolationRequestCycleListener((request, page) -> { + asked.add(request.getHeader(SEC_FETCH_SITE_HEADER) + " " + + request.getHeader(SEC_FETCH_MODE_HEADER) + " " + + request.getHeader(SEC_FETCH_DEST_HEADER) + " " + request.getMethod()); + return ResourceIsolationOutcome.ALLOWED; + })); + + navigate(CROSS_SITE, MODE_NAVIGATE, DEST_EMBED, "POST", "secure?-1.-link"); + tester.assertRenderedPage(LoginPage.class); + + String destination = signIn(); + asked.clear(); + resume(destination); + + assertEquals(asList(SAME_ORIGIN + " " + MODE_NAVIGATE + " " + DEST_DOCUMENT + " GET", + CROSS_SITE + " " + MODE_NAVIGATE + " " + DEST_EMBED + " POST"), asked, + "the policies were not asked about the request that chose the url"); + } + + /** + * A client that sends no Fetch Metadata is judged by {@link OriginResourceIsolationPolicy}, so + * the {@code Origin} of the request that chose the url has to be recorded too. Unknown origins + * are allowed here, so that the recorded one is what decides. + */ + @Test + void theOriginHeaderIsRecordedForAClientThatSendsNoFetchMetadata() + { + listener.setUnknownOutcomeAction(CsrfAction.ALLOW); + + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://another.example.com";); + navigate(null, "secure?-1.-link"); + tester.assertRenderedPage(LoginPage.class); + + resume(signIn()); + + assertRefused(); + assertFalse(SecureLinkPage.invoked); + } + + /** + * What is recorded is the listener's to decide, so a policy reading something else can still be + * asked about the request that chose the URL. + */ + @Test + void recordOriginDecidesWhatTheResumeIsJudgedOn() + { + withListener(new ResourceIsolationRequestCycleListener( + (request, page) -> "elsewhere".equals(request.getHeader("X-From")) + ? ResourceIsolationOutcome.DISALLOWED + : ResourceIsolationOutcome.ALLOWED) + { + @Override + protected Map<String, String> recordOrigin(HttpServletRequest request) + { + Map<String, String> origin = super.recordOrigin(request); + origin.put("X-From", request.getHeader("X-From")); + return origin; + } + }); + + tester.addRequestHeader("X-From", "elsewhere"); + navigate(null, "secure?-1.-link"); + tester.assertRenderedPage(LoginPage.class); + + // neither the sign in nor the resume carries X-From + resume(signIn()); + + assertRefused(); + assertFalse(SecureLinkPage.invoked); + } + + /** Extends the listener to police renders as well, which it does not do by default. */ + private void withRendersChecked() + { + withListener(new ResourceIsolationRequestCycleListener() + { + @Override + protected boolean isChecked(IRequestHandler handler) + { + return handler instanceof IPageRequestHandler; + } + }); + } + + private void withListener(ResourceIsolationRequestCycleListener replacement) + { + if (listener != null) + { + tester.getApplication().getRequestCycleListeners().remove(listener); + } + listener = replacement; + tester.getApplication().getRequestCycleListeners().add(listener); + } + + /** + * A top level navigation from {@code site}, or from a client that sends no origin headers at + * all when {@code site} is {@code null}. + */ + private void navigate(String site, String url) + { + navigate(site, MODE_NAVIGATE, DEST_DOCUMENT, "GET", url); + } + + private void navigate(String site, String mode, String dest, String method, String url) + { + if (site != null) + { + tester.addRequestHeader(SEC_FETCH_SITE_HEADER, site); + tester.addRequestHeader(SEC_FETCH_MODE_HEADER, mode); + tester.addRequestHeader(SEC_FETCH_DEST_HEADER, dest); + } + tester.getRequest().setMethod(method); + tester.executeUrl(url); + } + + /** + * Signs in and returns the destination the sign in redirects to, without following it: the + * tester builds a fresh request for a redirect and carries no headers over, so the resume has + * to be driven the way a browser drives it. + */ + private String signIn() + { + tester.addRequestHeader(SEC_FETCH_SITE_HEADER, SAME_ORIGIN); + tester.setFollowRedirects(false); + tester.newFormTester("form").submit(); + tester.setFollowRedirects(true); + + String destination = tester.getLastResponse().getRedirectLocation(); + assertNotNull(destination, "signing in did not resume a destination"); + return destination; + } + + /** A browser turns the redirect that follows the sign in into a same origin navigation. */ + private void resume(String destination) + { + navigate(SAME_ORIGIN, destination); + } + + private void assertRefused() + { + assertEquals(SC_FORBIDDEN, tester.getLastResponse().getStatus()); + assertEquals(ResourceIsolationRequestCycleListener.ERROR_MESSAGE, + tester.getLastResponse().getErrorMessage()); + } +} diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureFormPage.html b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureFormPage.html new file mode 100644 index 0000000000..2f360214ec --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureFormPage.html @@ -0,0 +1,5 @@ +<html xmlns:wicket> +<body> + <form wicket:id="form"><input wicket:id="amount" type="text"/></form> +</body> +</html> diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureFormPage.java b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureFormPage.java new file mode 100644 index 0000000000..2a9bd52b7a --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureFormPage.java @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.wicket.protocol.http.intercept; + +import org.apache.wicket.markup.html.WebPage; +import org.apache.wicket.markup.html.form.Form; +import org.apache.wicket.markup.html.form.TextField; +import org.apache.wicket.model.Model; + +/** + * A protected page carrying an ordinary form, whose listener URL takes its field value from the + * query string when the request is a GET. + */ +public class SecureFormPage extends WebPage +{ + private static final long serialVersionUID = 1L; + + /** The value {@code onSubmit()} saw, or {@code null} when it did not run. */ + public static String submitted; + + /** + * Construct. + */ + public SecureFormPage() + { + Model<String> amount = Model.of(); + Form<Void> form = new Form<>("form") + { + private static final long serialVersionUID = 1L; + + @Override + protected void onSubmit() + { + submitted = amount.getObject(); + } + }; + form.add(new TextField<>("amount", amount)); + add(form); + } +} diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureLinkPage.html b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureLinkPage.html new file mode 100644 index 0000000000..2a5a0a1604 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureLinkPage.html @@ -0,0 +1,5 @@ +<html xmlns:wicket> +<body> + <a wicket:id="link">a listener on a protected page</a> +</body> +</html> diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureLinkPage.java b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureLinkPage.java new file mode 100644 index 0000000000..f207d3a754 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureLinkPage.java @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.wicket.protocol.http.intercept; + +import org.apache.wicket.markup.html.WebPage; +import org.apache.wicket.markup.html.link.StatelessLink; + +/** + * A protected page whose listener is reachable on a URL without a page identifier, because the page + * is stateless. + */ +public class SecureLinkPage extends WebPage +{ + private static final long serialVersionUID = 1L; + + /** Whether the link's listener ran. */ + public static boolean invoked; + + /** + * Construct. + */ + public SecureLinkPage() + { + add(new StatelessLink<Void>("link") + { + private static final long serialVersionUID = 1L; + + @Override + public void onClick() + { + invoked = true; + } + }); + } +} diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureRenderPage.html b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureRenderPage.html new file mode 100644 index 0000000000..b13b3983af --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureRenderPage.html @@ -0,0 +1,3 @@ +<html xmlns:wicket> +<body>a protected page</body> +</html> diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureRenderPage.java b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureRenderPage.java new file mode 100644 index 0000000000..5e20839c82 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/intercept/SecureRenderPage.java @@ -0,0 +1,27 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.wicket.protocol.http.intercept; + +import org.apache.wicket.markup.html.WebPage; + +/** + * A protected page that is only ever rendered. + */ +public class SecureRenderPage extends WebPage +{ + private static final long serialVersionUID = 1L; +} diff --git a/wicket-core/src/main/java/org/apache/wicket/RestartResponseAtInterceptPageException.java b/wicket-core/src/main/java/org/apache/wicket/RestartResponseAtInterceptPageException.java index c34fcb15be..3a1ef4142e 100644 --- a/wicket-core/src/main/java/org/apache/wicket/RestartResponseAtInterceptPageException.java +++ b/wicket-core/src/main/java/org/apache/wicket/RestartResponseAtInterceptPageException.java @@ -112,6 +112,66 @@ public class RestartResponseAtInterceptPageException extends ResetResponseExcept return postParameters; } + /** + * Attaches data to the request that is being handled, to be remembered along with the + * destination should this request be intercepted. The request that later resumes that + * destination reads it back with {@link #getInterceptMetaData(MetaDataKey)}. + * <p> + * A request is not intercepted where it is interrupted: any code can throw this exception - a + * page constructor, a listener, the render of a page - and one request can be intercepted more + * than once, each interception replacing what the previous one remembered. Data attached here + * travels with the destination whichever of those happens, which data collected at a particular + * throw site cannot. + * <p> + * The data is remembered in the session, so it has to be serializable, and it is handed to every + * request that resumes the destination, so it should be immutable. + * + * @param <T> + * the type of the data + * @param key + * the key to attach the data under + * @param data + * the data, or {@code null} to remove what was attached under {@code key} + */ + public static <T> void setInterceptMetaData(MetaDataKey<T> key, T data) + { + RequestCycle cycle = RequestCycle.get(); + if (cycle != null) + { + cycle.setMetaData(pendingMetaDataKey, + key.set(cycle.getMetaData(pendingMetaDataKey), data)); + } + } + + /** + * The data that {@link #setInterceptMetaData(MetaDataKey, Object)} attached to the request whose + * destination the current request resumes. + * + * @param <T> + * the type of the data + * @param key + * the key the data was attached under + * @return the data, or {@code null} when the current request resumes no destination, or when + * nothing was attached under {@code key} + */ + public static <T> T getInterceptMetaData(MetaDataKey<T> key) + { + RequestCycle cycle = RequestCycle.get(); + return cycle == null ? null : key.get(cycle.getMetaData(resumedMetaDataKey)); + } + + /** What is attached to the request being handled, in case it is intercepted. */ + private static final MetaDataKey<MetaDataEntry<?>[]> pendingMetaDataKey = new MetaDataKey<>() + { + private static final long serialVersionUID = 1L; + }; + + /** What was attached to the request whose destination the current request resumes. */ + private static final MetaDataKey<MetaDataEntry<?>[]> resumedMetaDataKey = new MetaDataKey<>() + { + private static final long serialVersionUID = 1L; + }; + /** * INTERNAL CLASS, DO NOT USE * @@ -123,6 +183,7 @@ public class RestartResponseAtInterceptPageException extends ResetResponseExcept private Url originalUrl; private Map<String, List<StringValue>> postParameters; + private MetaDataEntry<?>[] metaData; public Url getOriginalUrl() { @@ -139,7 +200,9 @@ public class RestartResponseAtInterceptPageException extends ResetResponseExcept Session session = Session.get(); session.bind(); InterceptData data = new InterceptData(); - Request request = RequestCycle.get().getRequest(); + RequestCycle cycle = RequestCycle.get(); + Request request = cycle.getRequest(); + data.metaData = cycle.getMetaData(pendingMetaDataKey); data.originalUrl = request.getOriginalUrl(); Iterator<QueryParameter> itor = data.originalUrl.getQueryParameters().iterator(); while (itor.hasNext()) @@ -236,6 +299,11 @@ public class RestartResponseAtInterceptPageException extends ResetResponseExcept parameters.setParameterValues(s, data.postParameters.get(s)); } } + RequestCycle cycle = RequestCycle.get(); + if (cycle != null) + { + cycle.setMetaData(resumedMetaDataKey, data.metaData); + } InterceptData.clear(); } return null; diff --git a/wicket-core/src/main/java/org/apache/wicket/protocol/http/ResourceIsolationRequestCycleListener.java b/wicket-core/src/main/java/org/apache/wicket/protocol/http/ResourceIsolationRequestCycleListener.java index 4ad3792870..070403e454 100644 --- a/wicket-core/src/main/java/org/apache/wicket/protocol/http/ResourceIsolationRequestCycleListener.java +++ b/wicket-core/src/main/java/org/apache/wicket/protocol/http/ResourceIsolationRequestCycleListener.java @@ -20,13 +20,22 @@ import static java.util.Arrays.asList; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; +import java.util.Enumeration; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.TreeMap; +import java.util.TreeSet; import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletRequestWrapper; import jakarta.servlet.http.HttpServletResponse; +import org.apache.wicket.MetaDataKey; +import org.apache.wicket.RestartResponseAtInterceptPageException; import org.apache.wicket.RestartResponseException; import org.apache.wicket.core.request.handler.IPageRequestHandler; import org.apache.wicket.core.request.handler.RenderPageRequestHandler; @@ -37,6 +46,7 @@ import org.apache.wicket.request.IRequestHandlerDelegate; import org.apache.wicket.request.component.IRequestablePage; import org.apache.wicket.request.cycle.IRequestCycleListener; import org.apache.wicket.request.cycle.RequestCycle; +import org.apache.wicket.request.http.WebRequest; import org.apache.wicket.request.http.WebResponse; import org.apache.wicket.request.http.flow.AbortWithHttpErrorCodeException; import org.apache.wicket.util.lang.Classes; @@ -155,6 +165,25 @@ public class ResourceIsolationRequestCycleListener implements IRequestCycleListe private final List<IResourceIsolationPolicy> resourceIsolationPolicies = new ArrayList<>(); + /** + * The key {@link #recordOrigin(HttpServletRequest)} records the request method under. Not a + * valid header name, so it cannot collide with one. + */ + protected static final String RECORDED_METHOD = ":method"; + + /** The headers {@link #recordOrigin(HttpServletRequest)} records by default. */ + private static final List<String> RECORDED_HEADERS = asList( + FetchMetadataResourceIsolationPolicy.SEC_FETCH_SITE_HEADER, + FetchMetadataResourceIsolationPolicy.SEC_FETCH_MODE_HEADER, + FetchMetadataResourceIsolationPolicy.SEC_FETCH_DEST_HEADER, WebRequest.HEADER_ORIGIN, + WebRequest.HEADER_REFERER); + + /** What {@link #recordOrigin(HttpServletRequest)} recorded, carried with a destination. */ + private static final MetaDataKey<Map<String, String>> ORIGIN = new MetaDataKey<>() + { + private static final long serialVersionUID = 1L; + }; + /** * Create a new listener with the given policies. If no policies are given, * {@link FetchMetadataResourceIsolationPolicy} and {@link OriginResourceIsolationPolicy} will @@ -244,6 +273,44 @@ public class ResourceIsolationRequestCycleListener implements IRequestCycleListe .getContainerRequest(); log.debug("Processing request to: {}", containerRequest.getPathInfo()); + + if (isEnabled()) + { + // Record where this request came from while that is still known. Should the application + // interrupt this request and remember its destination to be resumed after a sign in, + // the request that resumes it will have been redirected from inside this application + // and will say so: it carries the origin that resumed it, not the origin that chose the + // URL. This record is what lets that request be judged on the latter. + RestartResponseAtInterceptPageException.setInterceptMetaData(ORIGIN, + recordOrigin(containerRequest)); + } + } + + /** + * Records what the isolation policies need in order to tell where a request came from. Called + * for every request, and remembered with the destination whenever a request is interrupted and + * its destination kept to be resumed later. + * <p> + * Override to record what a custom {@link IResourceIsolationPolicy} reads. Record an entry for + * every header consulted, with a {@code null} value where the header was absent: a name the + * record does not mention at all is answered by the request that resumes the destination, which + * is never what a header deciding provenance should fall back to. Keys are matched ignoring + * case, and the record is held in the session, so its values have to be serializable. + * + * @param request + * the request being handled + * @return the recorded properties, keyed by header name, with the request method under + * {@link #RECORDED_METHOD} + */ + protected Map<String, String> recordOrigin(HttpServletRequest request) + { + Map<String, String> origin = new HashMap<>(); + for (String header : RECORDED_HEADERS) + { + origin.put(header, request.getHeader(header)); + } + origin.put(RECORDED_METHOD, request.getMethod()); + return origin; } /** @@ -344,23 +411,30 @@ public class ResourceIsolationRequestCycleListener implements IRequestCycleListe RequestType requestType = getRequestType(handler); - for (IResourceIsolationPolicy policy : resourceIsolationPolicies) + ResourceIsolationOutcome outcome = outcomeFor(containerRequest, targetedPage, + requestType); + + Map<String, String> origin = RestartResponseAtInterceptPageException + .getInterceptMetaData(ORIGIN); + if (origin != null) { - ResourceIsolationOutcome outcome = policy.isRequestAllowed(containerRequest, - targetedPage, requestType); - if (ResourceIsolationOutcome.DISALLOWED.equals(outcome)) - { - log.debug("Isolation policy {} has rejected a request to {}", - Classes.simpleName(policy.getClass()), pathInfo); - disallowedOutcomeAction.apply(this, containerRequest, targetedPage); - return; - } - else if (ResourceIsolationOutcome.ALLOWED.equals(outcome)) - { - return; - } + // This request resumes a destination that an earlier request chose. It carries the + // origin that resumed it - this application's own, because signing in redirects + // within it - so its own headers say nothing about where the URL came from. Judge + // the request that did choose it as well, and keep the stricter of the two. + outcome = stricter(outcome, + outcomeFor(new RecordedOriginRequest(containerRequest, origin), targetedPage, + requestType)); + } + + if (ResourceIsolationOutcome.DISALLOWED.equals(outcome)) + { + disallowedOutcomeAction.apply(this, containerRequest, targetedPage); + } + else if (ResourceIsolationOutcome.UNKNOWN.equals(outcome)) + { + unknownOutcomeAction.apply(this, containerRequest, targetedPage); } - unknownOutcomeAction.apply(this, containerRequest, targetedPage); } else { @@ -437,6 +511,47 @@ public class ResourceIsolationRequestCycleListener implements IRequestCycleListe throw new AbortWithHttpErrorCodeException(errorCode, errorMessage); } + /** + * The outcome the policies reach for {@code request}: the first policy that does not answer + * {@link ResourceIsolationOutcome#UNKNOWN} decides. + */ + private ResourceIsolationOutcome outcomeFor(HttpServletRequest request, + IRequestablePage targetedPage, RequestType requestType) + { + for (IResourceIsolationPolicy policy : resourceIsolationPolicies) + { + ResourceIsolationOutcome outcome = policy.isRequestAllowed(request, targetedPage, + requestType); + if (ResourceIsolationOutcome.DISALLOWED.equals(outcome)) + { + log.debug("Isolation policy {} has rejected a request to {}", + Classes.simpleName(policy.getClass()), request.getPathInfo()); + return outcome; + } + else if (ResourceIsolationOutcome.ALLOWED.equals(outcome)) + { + return outcome; + } + } + return ResourceIsolationOutcome.UNKNOWN; + } + + private static ResourceIsolationOutcome stricter(ResourceIsolationOutcome one, + ResourceIsolationOutcome other) + { + if (ResourceIsolationOutcome.DISALLOWED.equals(one) || + ResourceIsolationOutcome.DISALLOWED.equals(other)) + { + return ResourceIsolationOutcome.DISALLOWED; + } + if (ResourceIsolationOutcome.UNKNOWN.equals(one) || + ResourceIsolationOutcome.UNKNOWN.equals(other)) + { + return ResourceIsolationOutcome.UNKNOWN; + } + return ResourceIsolationOutcome.ALLOWED; + } + private static IRequestHandler unwrap(IRequestHandler handler) { while (handler instanceof IRequestHandlerDelegate) @@ -445,4 +560,75 @@ public class ResourceIsolationRequestCycleListener implements IRequestCycleListe } return handler; } + + /** + * Presents the request that was interrupted, so that the policies judge the origin that chose a + * resumed URL rather than the origin that resumed it. + * <p> + * Whatever the record mentions is answered from the record alone, its absences included: a + * resumed request always carries this application's own origin, so falling through to it would + * hand back the very answer this class exists to avoid. Everything else is answered by the + * resuming request, whose target scheme, host and port are the same ones the interrupted + * request was compared against. + */ + private static class RecordedOriginRequest extends HttpServletRequestWrapper + { + private final Map<String, String> origin; + + private RecordedOriginRequest(HttpServletRequest request, Map<String, String> origin) + { + super(request); + + // header names are matched ignoring case, whatever recordOrigin() returned + this.origin = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + this.origin.putAll(origin); + } + + @Override + public String getMethod() + { + return origin.containsKey(RECORDED_METHOD) ? origin.get(RECORDED_METHOD) + : super.getMethod(); + } + + @Override + public String getHeader(String name) + { + return origin.containsKey(name) ? origin.get(name) : super.getHeader(name); + } + + @Override + public Enumeration<String> getHeaders(String name) + { + if (origin.containsKey(name)) + { + String value = origin.get(name); + return Collections.enumeration( + value == null ? Collections.emptyList() : Collections.singletonList(value)); + } + return super.getHeaders(name); + } + + @Override + public Enumeration<String> getHeaderNames() + { + Set<String> names = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); + for (Enumeration<String> live = super.getHeaderNames(); live.hasMoreElements();) + { + String name = live.nextElement(); + if (origin.containsKey(name) == false) + { + names.add(name); + } + } + for (Map.Entry<String, String> recorded : origin.entrySet()) + { + if (recorded.getValue() != null && RECORDED_METHOD.equals(recorded.getKey()) == false) + { + names.add(recorded.getKey()); + } + } + return Collections.enumeration(names); + } + } } diff --git a/wicket-user-guide/src/main/asciidoc/security/security_5.adoc b/wicket-user-guide/src/main/asciidoc/security/security_5.adoc index ecab4acb02..93f824b6d2 100644 --- a/wicket-user-guide/src/main/asciidoc/security/security_5.adoc +++ b/wicket-user-guide/src/main/asciidoc/security/security_5.adoc @@ -65,6 +65,8 @@ Because a _same-site_ request is cross-origin, it is treated as one: by default Note that this decision rests entirely with the Fetch Metadata policy: __OriginResourceIsolationPolicy__ is only consulted for requests that carry no Fetch Metadata headers at all, which no current browser does. +A request that is interrupted and resumed later is judged on where it originally came from. An application that refuses an unauthenticated visitor by throwing _RestartResponseAtInterceptPageException_ has the url that visitor asked for remembered, and _continueToOriginalDestination()_ returns them to it once they have signed in. That second request is a redirect from inside our own application, so it says _same-origin_ no matter which origin chose the url in the first place. The listener [...] + If every origin on our site is trusted - for instance when all of our subdomains are part of the same application - we can allow sibling origins to invoke listeners as well with _setSameSiteAllowed(true)_: [source,java]
