From 893579c88c98b8dfbc6612ff7c2e1b3ac024e6f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20Micha=C5=82owski?= Date: Sat, 5 Nov 2016 17:59:56 +0100 Subject: [PATCH 1/3] Add origin header to ajax requests in BaseWicketTester --- .../http/CsrfPreventionRequestCycleListener.java | 5 +++-- .../apache/wicket/util/tester/BaseWicketTester.java | 11 +++++++++++ .../org/apache/wicket/request/http/WebRequest.java | 4 ++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java b/wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java index 814e90da253..8f96efe4e53 100644 --- a/wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java +++ b/wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java @@ -33,6 +33,7 @@ import org.apache.wicket.request.cycle.AbstractRequestCycleListener; 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.flow.AbortWithHttpErrorCodeException; import org.apache.wicket.util.lang.Checks; import org.apache.wicket.util.string.Strings; @@ -383,10 +384,10 @@ public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler */ protected String getSourceUri(HttpServletRequest containerRequest) { - String sourceUri = containerRequest.getHeader("Origin"); + String sourceUri = containerRequest.getHeader(WebRequest.HEADER_ORIGIN); if (Strings.isEmpty(sourceUri)) { - sourceUri = containerRequest.getHeader("Referer"); + sourceUri = containerRequest.getHeader(WebRequest.HEADER_REFERER); } return normalizeUri(sourceUri); } diff --git a/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java b/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java index 2f972e90e94..10c99aea8f8 100644 --- a/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java +++ b/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java @@ -1174,6 +1174,7 @@ public void executeBehavior(final AbstractAjaxBehavior behavior) Charset.forName(request.getCharacterEncoding())); transform(url); request.setUrl(url); + request.addHeader(WebRequest.HEADER_ORIGIN, createOriginHeader()); request.addHeader(WebRequest.HEADER_AJAX_BASE_URL, url.toString()); request.addHeader(WebRequest.HEADER_AJAX, "true"); @@ -1195,6 +1196,16 @@ public void executeBehavior(final AbstractAjaxBehavior behavior) processRequest(); } + + /** + * Build value to Origin header based on RequestCycle Url + * + * @return Origin header + */ + protected String createOriginHeader(){ + Url url = RequestCycle.get().getRequest().getUrl(); + return url.getProtocol() + "://" +url.getHost() + ":" + url.getPort(); + } /** * diff --git a/wicket-request/src/main/java/org/apache/wicket/request/http/WebRequest.java b/wicket-request/src/main/java/org/apache/wicket/request/http/WebRequest.java index fbadee23336..1ac1596f99b 100644 --- a/wicket-request/src/main/java/org/apache/wicket/request/http/WebRequest.java +++ b/wicket-request/src/main/java/org/apache/wicket/request/http/WebRequest.java @@ -47,6 +47,10 @@ public abstract class WebRequest extends Request public static final String HEADER_AJAX_BASE_URL = "Wicket-Ajax-BaseURL"; /** anti-cache query parameter added by Wicket.Ajax.Request at its URL */ public static final String PARAM_AJAX_REQUEST_ANTI_CACHE = "_"; + /** {@code Origin} http header */ + public static final String HEADER_ORIGIN = "Origin"; + /** {@code Referer} http header */ + public static final String HEADER_REFERER = "Referer"; /** * @return request cookies From 09d39f90d24baf450f4997bc718ba54f815e58a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20Micha=C5=82owski?= Date: Sun, 6 Nov 2016 14:46:50 +0100 Subject: [PATCH 2/3] use WebRequest.HEADER_ORIGIN constant instead "Origin" --- ...srfPreventionRequestCycleListenerTest.java | 45 ++++++++++--------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/wicket-core/src/test/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListenerTest.java b/wicket-core/src/test/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListenerTest.java index 3db62abaff3..9c0c0d86d08 100644 --- a/wicket-core/src/test/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListenerTest.java +++ b/wicket-core/src/test/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListenerTest.java @@ -24,6 +24,7 @@ import org.apache.wicket.protocol.http.CsrfPreventionRequestCycleListener.CsrfAction; import org.apache.wicket.request.IRequestHandler; import org.apache.wicket.request.component.IRequestablePage; +import org.apache.wicket.request.http.WebRequest; import org.apache.wicket.util.tester.WicketTestCase; import org.junit.Before; import org.junit.Test; @@ -50,7 +51,7 @@ public void startWithFirstPageRender() // Rendering a page is allowed, regardless of Origin (this allows external links into your // website to function) - tester.addRequestHeader("Origin", "https://google.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "https://google.com/"); tester.startPage(FirstPage.class); tester.assertRenderedPage(FirstPage.class); @@ -72,7 +73,7 @@ public void disabledListenerDoesntCheckAnything() public void disabledListenerDoesntCheckMismatchedOrigin() { csrfEnabled = false; - tester.addRequestHeader("Origin", "http://malicioussite.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://malicioussite.com/"); tester.clickLink("link"); assertOriginsNotChecked(); tester.assertRenderedPage(SecondPage.class); @@ -110,7 +111,7 @@ public void withoutOriginAborted() public void matchingOriginsAllowed() { csrfListener.setConflictingOriginAction(CsrfAction.ALLOW); - tester.addRequestHeader("Origin", "http://localhost/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://localhost/"); tester.clickLink("link"); @@ -123,7 +124,7 @@ public void matchingOriginsAllowed() public void conflictingOriginsAllowed() { csrfListener.setConflictingOriginAction(CsrfAction.ALLOW); - tester.addRequestHeader("Origin", "http://example.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://example.com/"); tester.clickLink("link"); @@ -135,7 +136,7 @@ public void conflictingOriginsAllowed() @Test public void conflictingOriginsSuppressed() { - tester.addRequestHeader("Origin", "http://example.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://example.com/"); csrfListener.setConflictingOriginAction(CsrfAction.SUPPRESS); tester.clickLink("link"); @@ -148,7 +149,7 @@ public void conflictingOriginsSuppressed() @Test public void conflictingOriginsAborted() { - tester.addRequestHeader("Origin", "http://example.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://example.com/"); tester.clickLink("link"); @@ -162,7 +163,7 @@ public void conflictingOriginsAbortedWith401Unauhorized() setErrorCode(401); setErrorMessage("NOT AUTHORIZED"); - tester.addRequestHeader("Origin", "http://example.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://example.com/"); csrfListener.setNoOriginAction(CsrfAction.ABORT); tester.clickLink("link"); @@ -176,7 +177,7 @@ public void conflictingButWhitelistedOriginAllowed() { csrfListener.setConflictingOriginAction(CsrfAction.ALLOW); csrfListener.addAcceptedOrigin("example.com"); - tester.addRequestHeader("Origin", "http://example.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://example.com/"); tester.clickLink("link"); @@ -191,7 +192,7 @@ public void conflictingButWhitelistedSubdomainOriginAllowed() csrfListener.addAcceptedOrigin("example.com"); csrfListener.setConflictingOriginAction(CsrfAction.ALLOW); - tester.addRequestHeader("Origin", "http://foo.example.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://foo.example.com/"); tester.clickLink("link"); @@ -206,7 +207,7 @@ public void conflictingButWhitelistedSubdomainOriginAllowed() @Test public void conflictingOriginPageNotCheckedAllowed() { - tester.addRequestHeader("Origin", "http://example.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://example.com/"); csrfListener.setConflictingOriginAction(CsrfAction.ABORT); // disable the check for this page @@ -236,7 +237,7 @@ public void run() setSuppressHandler(thirdPageRedirect); csrfListener.setConflictingOriginAction(CsrfAction.SUPPRESS); - tester.addRequestHeader("Origin", "http://example.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://example.com/"); tester.clickLink("link"); @@ -262,7 +263,7 @@ public void run() setAllowHandler(thirdPageRedirect); csrfListener.setConflictingOriginAction(CsrfAction.ALLOW); - tester.addRequestHeader("Origin", "http://example.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://example.com/"); tester.clickLink("link"); @@ -287,7 +288,7 @@ public void run() }; setAbortHandler(thirdPageRedirect); - tester.addRequestHeader("Origin", "http://example.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://example.com/"); csrfListener.setConflictingOriginAction(CsrfAction.ABORT); tester.clickLink("link"); @@ -305,7 +306,7 @@ public void run() @Test public void differentPortOriginAborted() { - tester.addRequestHeader("Origin", "http://localhost:8080"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://localhost:8080"); csrfListener.setConflictingOriginAction(CsrfAction.ABORT); tester.clickLink("link"); @@ -317,7 +318,7 @@ public void differentPortOriginAborted() @Test public void differentSchemeOriginAborted() { - tester.addRequestHeader("Origin", "https://localhost"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "https://localhost"); csrfListener.setConflictingOriginAction(CsrfAction.ABORT); tester.clickLink("link"); @@ -329,7 +330,7 @@ public void differentSchemeOriginAborted() @Test public void longerOriginAllowed() { - tester.addRequestHeader("Origin", "http://localhost/supercalifragilisticexpialidocious"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://localhost/supercalifragilisticexpialidocious"); csrfListener.setConflictingOriginAction(CsrfAction.ABORT); tester.clickLink("link"); @@ -345,14 +346,14 @@ public void simulatedCsrfAttackThroughAjaxIsPrevented() csrfListener.setConflictingOriginAction(CsrfAction.ABORT); // first render a page in the user's session - tester.addRequestHeader("Origin", "http://localhost"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://localhost"); tester.startPage(ThirdPage.class); assertOriginsNotChecked(); tester.assertRenderedPage(ThirdPage.class); // then click on a link from another external page - tester.addRequestHeader("Origin", "http://attacker.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://attacker.com/"); tester.clickLink("link", true); assertConflictingOriginsRequestAborted(); @@ -365,14 +366,14 @@ public void simulatedCsrfAttackIsSuppressed() csrfListener.setConflictingOriginAction(CsrfAction.SUPPRESS); // first render a page in the user's session - tester.addRequestHeader("Origin", "http://localhost"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://localhost"); tester.startPage(ThirdPage.class); assertOriginsNotChecked(); tester.assertRenderedPage(ThirdPage.class); // then click on a link from another external page - tester.addRequestHeader("Origin", "http://attacker.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://attacker.com/"); tester.clickLink("link", true); assertConflictingOriginsRequestSuppressed(); @@ -386,14 +387,14 @@ public void simulatedCsrfAttackOnFormIsSuppressed() csrfListener.setConflictingOriginAction(CsrfAction.SUPPRESS); // first render a page in the user's session - tester.addRequestHeader("Origin", "http://localhost"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://localhost"); tester.startPage(ThirdPage.class); assertOriginsNotChecked(); tester.assertRenderedPage(ThirdPage.class); // then click on a link from another external page - tester.addRequestHeader("Origin", "http://attacker.com/"); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, "http://attacker.com/"); tester.submitForm("form"); assertConflictingOriginsRequestSuppressed(); From fe11a60ea1272a71f22445356a26955f6a906a04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20Micha=C5=82owski?= Date: Sun, 6 Nov 2016 15:33:04 +0100 Subject: [PATCH 3/3] CsrfPreventionRequestCycleListenerTest fix withoutOrigin test cases --- .../http/CsrfPreventionRequestCycleListenerTest.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/wicket-core/src/test/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListenerTest.java b/wicket-core/src/test/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListenerTest.java index 9c0c0d86d08..9882bd69150 100644 --- a/wicket-core/src/test/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListenerTest.java +++ b/wicket-core/src/test/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListenerTest.java @@ -83,8 +83,10 @@ public void disabledListenerDoesntCheckMismatchedOrigin() @Test public void withoutOriginAllowed() { + csrfListener.setNoOriginAction(CsrfAction.ALLOW); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, null); tester.clickLink("link"); - assertConflictingOriginsRequestAborted(); + assertConflictingOriginsRequestAllowed(); } /** Tests the alternative action of suppressing a request without Origin header */ @@ -92,6 +94,7 @@ public void withoutOriginAllowed() public void withoutOriginSuppressed() { csrfListener.setNoOriginAction(CsrfAction.SUPPRESS); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, null); tester.clickLink("link"); tester.assertRenderedPage(FirstPage.class); assertConflictingOriginsRequestSuppressed(); @@ -102,6 +105,7 @@ public void withoutOriginSuppressed() public void withoutOriginAborted() { csrfListener.setNoOriginAction(CsrfAction.ABORT); + tester.addRequestHeader(WebRequest.HEADER_ORIGIN, null); tester.clickLink("link"); assertConflictingOriginsRequestAborted(); }