You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by pa...@apache.org on 2026/08/19 12:02:39 UTC
(wicket) branch wicket-10.x updated (4211a1cf49 -> 1597d6348e)
This is an automated email from the ASF dual-hosted git repository.
papegaaij pushed a change to branch wicket-10.x
in repository https://gitbox.apache.org/repos/asf/wicket.git
from 4211a1cf49 restrict SourcesPage to resources of the page's own package (#1523)
new 600e3b732a Validate locale/style/variation decoded from resource URLs
new 71f86141a6 Enforce the configured upload limits on the Servlet Part fallback
new 00ded8b78a Distinguish page renders from listener invocations in resource isolation
new 9edff354b6 Respect escapeModelStrings in the editable labels and their editors
new 1597d6348e Clarify two constants in the resource URL and upload limit tests
The 5 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../upload/FileUploadServletPartLimitsTest.java | 461 +++++++++++++++++++++
.../ResourceIsolationRequestCycleListenerTest.java | 107 ++++-
.../ResourceUrlAttributeValidationTest.java | 223 ++++++++++
.../apache/wicket/resource/ResourceUtilTest.java | 67 +++
.../resource/locator/ResourceNameIterator.java | 9 +-
.../http/FetchMetadataResourceIsolationPolicy.java | 84 +++-
.../protocol/http/IResourceIsolationPolicy.java | 50 ++-
.../ResourceIsolationRequestCycleListener.java | 26 +-
.../servlet/MultipartServletWebRequestImpl.java | 74 +++-
.../org/apache/wicket/resource/ResourceUtil.java | 71 +++-
.../ajax/markup/html/AjaxEditableChoiceLabel.java | 25 +-
.../ajax/markup/html/AjaxEditableLabel.java | 7 +
.../markup/html/AjaxEditableMultiLineLabel.java | 7 +
.../AjaxEditableLabelEscapeModelStringsTest.java | 246 +++++++++++
.../src/main/asciidoc/security/security_5.adoc | 26 ++
15 files changed, 1441 insertions(+), 42 deletions(-)
create mode 100644 wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/FileUploadServletPartLimitsTest.java
create mode 100644 wicket-core-tests/src/test/java/org/apache/wicket/request/resource/ResourceUrlAttributeValidationTest.java
create mode 100644 wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableLabelEscapeModelStringsTest.java
(wicket) 04/05: Respect escapeModelStrings in the editable labels and their editors
Posted by pa...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
papegaaij pushed a commit to branch wicket-10.x
in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 9edff354b6751f85953f93ec15bc9df452d92604
Author: Emond Papegaaij <pa...@apache.org>
AuthorDate: Tue Aug 18 14:59:01 2026 +0200
Respect escapeModelStrings in the editable labels and their editors
The label of AjaxEditableChoiceLabel starts from
getDefaultModelObjectAsString(), which escapes according to escapeModelStrings,
and then replaces that value with the one the IChoiceRenderer returns. That
value was written to the markup as it came, both where it is used directly and
where it is first run through a converter, so a choice whose display value
contains markup was rendered as markup. The same value in the dropdown was
escaped, because AbstractChoice#renderValue escapes what the renderer returns.
The escaping is applied to the value taken from the renderer only. The value it
replaces has already been escaped by getDefaultModelObjectAsString(), so
escaping where the body is replaced instead would escape that one a second time;
choiceLabelWithoutRendererEscapesExactlyOnce covers it.
escapeModelStrings had no effect on any of the three editable labels to begin
with. It is read by the label component the panel renders with, not by the
panel, and nothing carried it across, so calling setEscapeModelStrings on an
AjaxEditableLabel, an AjaxEditableMultiLineLabel or an AjaxEditableChoiceLabel
changed nothing. Component#getEscapeModelStrings is final and cannot be
delegated, so each label component now reads the setting of the panel in
onConfigure, beside the getConverter delegation already there.
The editor of AjaxEditableChoiceLabel reads it the same way. The choices it
renders are display values just like the label's, and escaping one but not the
other shows the same value in two different ways depending on whether the label
is in edit mode.
The text editors of the other two are left as they are. TextField puts the value
in an attribute, which is escaped when the tag is written whatever the setting
says, so it makes no difference there. TextArea writes it into the element body,
where the escaping is what makes the value round-trip: an unescaped body ends
the textarea early at a </textarea> the value happens to contain.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../ajax/markup/html/AjaxEditableChoiceLabel.java | 25 ++-
.../ajax/markup/html/AjaxEditableLabel.java | 7 +
.../markup/html/AjaxEditableMultiLineLabel.java | 7 +
.../AjaxEditableLabelEscapeModelStringsTest.java | 246 +++++++++++++++++++++
4 files changed, 283 insertions(+), 2 deletions(-)
diff --git a/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableChoiceLabel.java b/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableChoiceLabel.java
index bb26487c26..8ac6d85d90 100644
--- a/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableChoiceLabel.java
+++ b/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableChoiceLabel.java
@@ -195,6 +195,13 @@ public class AjaxEditableChoiceLabel<T> extends AjaxEditableLabel<T>
AjaxEditableChoiceLabel.this.onModelChanging();
}
+ @Override
+ protected void onConfigure()
+ {
+ super.onConfigure();
+ setEscapeModelStrings(AjaxEditableChoiceLabel.this.getEscapeModelStrings());
+ }
+
};
editor.setOutputMarkupId(true);
@@ -255,6 +262,13 @@ public class AjaxEditableChoiceLabel<T> extends AjaxEditableLabel<T>
return AjaxEditableChoiceLabel.this.getConverter(type);
}
+ @Override
+ protected void onConfigure()
+ {
+ super.onConfigure();
+ setEscapeModelStrings(AjaxEditableChoiceLabel.this.getEscapeModelStrings());
+ }
+
/**
* {@inheritDoc}
*/
@@ -269,15 +283,22 @@ public class AjaxEditableChoiceLabel<T> extends AjaxEditableLabel<T>
Object displayObject = renderer.getDisplayValue(getModelObject());
Class<?> objectClass = (displayObject == null ? null : displayObject.getClass());
+ String rendered = null;
if ((objectClass != null) && (objectClass != String.class))
{
@SuppressWarnings("rawtypes")
final IConverter converter = getConverter(objectClass);
- displayValue = converter.convertToString(displayObject, getLocale());
+ rendered = converter.convertToString(displayObject, getLocale());
}
else if (displayObject != null)
{
- displayValue = displayObject.toString();
+ rendered = displayObject.toString();
+ }
+
+ if (rendered != null)
+ {
+ displayValue = getEscapeModelStrings()
+ ? Strings.escapeMarkup(rendered).toString() : rendered;
}
}
diff --git a/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableLabel.java b/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableLabel.java
index 61ce8032cb..d061ac4413 100644
--- a/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableLabel.java
+++ b/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableLabel.java
@@ -389,6 +389,13 @@ public class AjaxEditableLabel<T> extends Panel implements IGenericComponent<T,
return AjaxEditableLabel.this.getConverter(type);
}
+ @Override
+ protected void onConfigure()
+ {
+ super.onConfigure();
+ setEscapeModelStrings(AjaxEditableLabel.this.getEscapeModelStrings());
+ }
+
@Override
public void onComponentTagBody(final MarkupStream markupStream,
final ComponentTag openTag)
diff --git a/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableMultiLineLabel.java b/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableMultiLineLabel.java
index db8c2a4066..f38883d44e 100644
--- a/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableMultiLineLabel.java
+++ b/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableMultiLineLabel.java
@@ -93,6 +93,13 @@ public class AjaxEditableMultiLineLabel<T> extends AjaxEditableLabel<T>
return AjaxEditableMultiLineLabel.this.getConverter(type);
}
+ @Override
+ protected void onConfigure()
+ {
+ super.onConfigure();
+ setEscapeModelStrings(AjaxEditableMultiLineLabel.this.getEscapeModelStrings());
+ }
+
@Override
public void onComponentTagBody(final MarkupStream markupStream,
final ComponentTag openTag)
diff --git a/wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableLabelEscapeModelStringsTest.java b/wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableLabelEscapeModelStringsTest.java
new file mode 100644
index 0000000000..02ea828168
--- /dev/null
+++ b/wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/AjaxEditableLabelEscapeModelStringsTest.java
@@ -0,0 +1,246 @@
+/*
+ * 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.extensions.ajax.markup.html;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Arrays;
+import java.util.List;
+
+import org.apache.wicket.Component;
+import org.apache.wicket.behavior.AbstractAjaxBehavior;
+import org.apache.wicket.markup.html.form.ChoiceRenderer;
+import org.apache.wicket.markup.html.form.IChoiceRenderer;
+import org.apache.wicket.model.IModel;
+import org.apache.wicket.model.Model;
+import org.apache.wicket.util.tester.WicketTestCase;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests that the editable labels escape their model value by default, and that
+ * {@link Component#setEscapeModelStrings(boolean)} on the editable label itself is honoured
+ * rather than the setting of the label component it renders with.
+ */
+class AjaxEditableLabelEscapeModelStringsTest extends WicketTestCase
+{
+ private static final String MARKUP = "<img src=x onerror=alert(1)>";
+
+ private static final String ESCAPED = "<img src=x onerror=alert(1)>";
+
+ private static final String DOUBLE_ESCAPED = "&lt;img";
+
+ private static final List<String> CHOICES = Arrays.asList(MARKUP, "other");
+
+ /**
+ * A display value that is not a String, so that it is rendered through an
+ * {@link org.apache.wicket.util.convert.IConverter} rather than by {@code toString()} directly.
+ */
+ private static class DisplayValue
+ {
+ @Override
+ public String toString()
+ {
+ return MARKUP;
+ }
+ }
+
+ private String render(Component component)
+ {
+ tester.startComponentInPage(component);
+ return tester.getLastResponse().getDocument();
+ }
+
+ /**
+ * Renders the editable label, then puts it in edit mode so that the response holds the markup
+ * of the editor instead of the label.
+ */
+ private String renderEditor(AjaxEditableLabel<?> label)
+ {
+ tester.startComponentInPage(label);
+ AbstractAjaxBehavior labelBehavior = (AbstractAjaxBehavior)label.get("label")
+ .getBehaviors()
+ .get(0);
+ tester.executeBehavior(labelBehavior);
+ return tester.getLastResponse().getDocument();
+ }
+
+ @Test
+ void choiceLabelWithRendererEscapesByDefault()
+ {
+ String document = render(new AjaxEditableChoiceLabel<>("label", Model.of(MARKUP), CHOICES,
+ new ChoiceRenderer<String>()));
+
+ assertTrue(document.contains(ESCAPED), document);
+ assertFalse(document.contains(MARKUP), document);
+ }
+
+ @Test
+ void choiceLabelWithRendererDoesNotEscapeWhenTurnedOff()
+ {
+ AjaxEditableChoiceLabel<String> label = new AjaxEditableChoiceLabel<>("label",
+ Model.of(MARKUP), CHOICES, new ChoiceRenderer<String>());
+ label.setEscapeModelStrings(false);
+
+ assertTrue(render(label).contains(MARKUP));
+ }
+
+ /**
+ * A renderer returning a non-String display value is rendered through a converter, which does
+ * not escape either.
+ */
+ @Test
+ void choiceLabelWithConvertedDisplayValueEscapesByDefault()
+ {
+ IChoiceRenderer<String> renderer = new ChoiceRenderer<String>()
+ {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ public Object getDisplayValue(String object)
+ {
+ return new DisplayValue();
+ }
+ };
+
+ String document = render(
+ new AjaxEditableChoiceLabel<>("label", Model.of("value"), CHOICES, renderer));
+
+ assertTrue(document.contains(ESCAPED), document);
+ assertFalse(document.contains(MARKUP), document);
+ }
+
+ /**
+ * Without a renderer the value is escaped by {@code getDefaultModelObjectAsString()}, so it
+ * must not be escaped a second time.
+ */
+ @Test
+ void choiceLabelWithoutRendererEscapesExactlyOnce()
+ {
+ String document = render(
+ new AjaxEditableChoiceLabel<>("label", Model.of(MARKUP), CHOICES));
+
+ assertTrue(document.contains(ESCAPED), document);
+ assertFalse(document.contains(DOUBLE_ESCAPED), document);
+ }
+
+ @Test
+ void choiceLabelWithoutRendererDoesNotEscapeWhenTurnedOff()
+ {
+ AjaxEditableChoiceLabel<String> label = new AjaxEditableChoiceLabel<>("label",
+ Model.of(MARKUP), CHOICES);
+ label.setEscapeModelStrings(false);
+
+ assertTrue(render(label).contains(MARKUP));
+ }
+
+ @Test
+ void labelEscapesByDefault()
+ {
+ String document = render(new AjaxEditableLabel<>("label", Model.of(MARKUP)));
+
+ assertTrue(document.contains(ESCAPED), document);
+ assertFalse(document.contains(MARKUP), document);
+ }
+
+ @Test
+ void labelDoesNotEscapeWhenTurnedOff()
+ {
+ AjaxEditableLabel<String> label = new AjaxEditableLabel<>("label", Model.of(MARKUP));
+ label.setEscapeModelStrings(false);
+
+ assertTrue(render(label).contains(MARKUP));
+ }
+
+ @Test
+ void multiLineLabelEscapesByDefault()
+ {
+ String document = render(new AjaxEditableMultiLineLabel<>("label", Model.of(MARKUP)));
+
+ assertTrue(document.contains(ESCAPED), document);
+ assertFalse(document.contains(MARKUP), document);
+ }
+
+ @Test
+ void multiLineLabelDoesNotEscapeWhenTurnedOff()
+ {
+ AjaxEditableMultiLineLabel<String> label = new AjaxEditableMultiLineLabel<>("label",
+ Model.of(MARKUP));
+ label.setEscapeModelStrings(false);
+
+ assertTrue(render(label).contains(MARKUP));
+ }
+
+ /**
+ * The setting has to be picked up even when it is changed after the label component has been
+ * created.
+ */
+ @Test
+ void settingIsHonouredWhenChangedAfterLabelWasCreated()
+ {
+ IModel<String> model = Model.of(MARKUP);
+ AjaxEditableChoiceLabel<String> label = new AjaxEditableChoiceLabel<>("label", model,
+ CHOICES, new ChoiceRenderer<String>());
+ // forces the internal label component to be created up front
+ label.setRequired(true);
+ label.setEscapeModelStrings(false);
+
+ assertTrue(render(label).contains(MARKUP));
+ }
+
+ /**
+ * The choices rendered by the editor are display values just like the label, so they follow
+ * the same setting. Escaping one but not the other would show the same value in two different
+ * ways depending on whether the label is in edit mode.
+ */
+ @Test
+ void choiceEditorEscapesOptionsByDefault()
+ {
+ String document = renderEditor(new AjaxEditableChoiceLabel<>("label", Model.of(MARKUP),
+ CHOICES, new ChoiceRenderer<String>()));
+
+ assertTrue(document.contains(ESCAPED), document);
+ assertFalse(document.contains(MARKUP), document);
+ }
+
+ @Test
+ void choiceEditorDoesNotEscapeOptionsWhenTurnedOff()
+ {
+ AjaxEditableChoiceLabel<String> label = new AjaxEditableChoiceLabel<>("label",
+ Model.of(MARKUP), CHOICES, new ChoiceRenderer<String>());
+ label.setEscapeModelStrings(false);
+
+ String document = renderEditor(label);
+
+ assertTrue(document.contains(MARKUP), document);
+ assertFalse(document.contains(ESCAPED), document);
+ }
+
+ /**
+ * The text editors are deliberately not covered by the setting: they carry the value as text
+ * to be edited, and a textarea body that is not escaped does not round-trip the value.
+ */
+ @Test
+ void multiLineEditorEscapesEvenWhenTurnedOff()
+ {
+ AjaxEditableMultiLineLabel<String> label = new AjaxEditableMultiLineLabel<>("label",
+ Model.of(MARKUP));
+ label.setEscapeModelStrings(false);
+
+ assertTrue(renderEditor(label).contains(ESCAPED));
+ }
+}
(wicket) 03/05: Distinguish page renders from listener invocations in resource isolation
Posted by pa...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
papegaaij pushed a commit to branch wicket-10.x
in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 00ded8b78a4895e25f1ecffe68d43b639abfecfd
Author: Emond Papegaaij <em...@topicus.nl>
AuthorDate: Thu Aug 6 15:47:59 2026 +0200
Distinguish page renders from listener invocations in resource isolation
A resource isolation policy could not tell what a request was going to do with
the targeted page, because it only received the page itself. That distinction is
what the Fetch Metadata policy needs: a page may legitimately be rendered as the
result of a top-level navigation from another site, but a listener on that page
must not be invoked from another site. Wicket invokes listeners through ordinary
GET navigations, so the headers alone do not separate the two.
IResourceIsolationPolicy gains a RequestType and an isRequestAllowed overload
that receives it. The existing two-argument method remains the single abstract
method, so the interface stays functional and policies written against it keep
working: the new overload has a default implementation that delegates to it.
ResourceIsolationRequestCycleListener derives the type from the resolved
handler through the new getRequestType(), which subclasses can override.
FetchMetadataResourceIsolationPolicy allows another origin to render a page
through a simple top-level navigation but not to invoke a listener. Its
two-argument method applies the listener rules, which are the stricter of the
two, so a caller that cannot say what the request does gets the safe answer.
Sec-Fetch-Site: same-site means the request comes from the same registrable
domain and scheme but a different origin, so by default it does not allow a
listener invocation. Applications that trust every origin on their site can
allow it with setSameSiteAllowed(true). Renders are unaffected: a page remains
reachable by a top-level navigation from a sibling origin or from another site.
OriginResourceIsolationPolicy is unchanged and stays in the default chain as
the fallback for requests that carry no Fetch Metadata headers at all.
The user guide chapter on resource isolation describes what the policy allows,
what same-site means, and how to allow sibling origins with
setSameSiteAllowed(true).
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../ResourceIsolationRequestCycleListenerTest.java | 107 ++++++++++++++++++++-
.../http/FetchMetadataResourceIsolationPolicy.java | 84 +++++++++++++++-
.../protocol/http/IResourceIsolationPolicy.java | 50 +++++++++-
.../ResourceIsolationRequestCycleListener.java | 26 ++++-
.../src/main/asciidoc/security/security_5.adoc | 26 +++++
5 files changed, 281 insertions(+), 12 deletions(-)
diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/ResourceIsolationRequestCycleListenerTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/ResourceIsolationRequestCycleListenerTest.java
index d4bc1c26b6..1d3f987321 100644
--- a/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/ResourceIsolationRequestCycleListenerTest.java
+++ b/wicket-core-tests/src/test/java/org/apache/wicket/protocol/http/ResourceIsolationRequestCycleListenerTest.java
@@ -22,6 +22,8 @@ import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPoli
import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.DEST_DOCUMENT;
import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.MODE_NAVIGATE;
import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.MODE_NO_CORS;
+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.SAME_SITE;
import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.SEC_FETCH_DEST_HEADER;
import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.SEC_FETCH_MODE_HEADER;
@@ -29,7 +31,9 @@ import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPoli
import static org.apache.wicket.protocol.http.FetchMetadataResourceIsolationPolicy.VARY_HEADER;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import org.apache.wicket.protocol.http.IResourceIsolationPolicy.RequestType;
import org.apache.wicket.protocol.http.IResourceIsolationPolicy.ResourceIsolationOutcome;
+import org.apache.wicket.request.component.IRequestablePage;
import org.apache.wicket.util.tester.WicketTestCase;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@@ -112,17 +116,114 @@ public class ResourceIsolationRequestCycleListenerTest extends WicketTestCase
}
/**
- * Tests whether a top level navigation request is allowed by FM checks
+ * A top-level navigation from another site may render a page, but not invoke a listener on it.
*/
@Test
- void topLevelNavigationAllowedFM()
+ void topLevelNavigationListenerAborted()
{
tester.addRequestHeader(SEC_FETCH_SITE_HEADER, CROSS_SITE);
tester.addRequestHeader(SEC_FETCH_MODE_HEADER, MODE_NAVIGATE);
+ assertRequestAborted();
+ }
+
+ /**
+ * The other half of {@link #topLevelNavigationListenerAborted()}: the same request may still
+ * render the page, so that pages remain linkable from elsewhere.
+ */
+ @Test
+ void topLevelNavigationMayRenderButNotInvokeAListener()
+ {
+ // set on the request itself: addRequestHeader() only stages headers for the next request
+ tester.getRequest().setHeader(SEC_FETCH_SITE_HEADER, CROSS_SITE);
+ tester.getRequest().setHeader(SEC_FETCH_MODE_HEADER, MODE_NAVIGATE);
+ // a simple top-level navigation is a GET; the mock request defaults to POST
+ tester.getRequest().setMethod("GET");
+
+ FetchMetadataResourceIsolationPolicy policy = new FetchMetadataResourceIsolationPolicy();
+ IRequestablePage page = tester.getLastRenderedPage();
+
+ assertEquals(ResourceIsolationOutcome.ALLOWED,
+ policy.isRequestAllowed(tester.getRequest(), page, RequestType.RENDER));
+ assertEquals(ResourceIsolationOutcome.DISALLOWED,
+ policy.isRequestAllowed(tester.getRequest(), page, RequestType.LISTENER));
+ }
+
+ /**
+ * A sibling origin on the same site is a different origin, so it may not invoke a listener.
+ */
+ @Test
+ void sameSiteListenerAborted()
+ {
+ tester.addRequestHeader(SEC_FETCH_SITE_HEADER, SAME_SITE);
+
+ assertRequestAborted();
+ }
+
+ /**
+ * ... unless the application declares every origin on the site to be trusted.
+ */
+ @Test
+ void sameSiteListenerAllowedWhenConfigured()
+ {
+ withCustomListener(new ResourceIsolationRequestCycleListener(
+ new FetchMetadataResourceIsolationPolicy().setSameSiteAllowed(true),
+ new OriginResourceIsolationPolicy()));
+
+ tester.addRequestHeader(SEC_FETCH_SITE_HEADER, SAME_SITE);
+
+ assertRequestAccepted();
+ }
+
+ /**
+ * A same-site request may render a page regardless of the setting, as a top-level navigation.
+ */
+ @Test
+ void sameSiteMayRenderRegardlessOfTheSetting()
+ {
+ tester.getRequest().setHeader(SEC_FETCH_SITE_HEADER, SAME_SITE);
+ tester.getRequest().setHeader(SEC_FETCH_MODE_HEADER, MODE_NAVIGATE);
+ tester.getRequest().setMethod("GET");
+
+ assertEquals(ResourceIsolationOutcome.ALLOWED,
+ new FetchMetadataResourceIsolationPolicy().isRequestAllowed(tester.getRequest(),
+ tester.getLastRenderedPage(), RequestType.RENDER));
+ }
+
+ /**
+ * Requests from the page itself are allowed, which is the normal case.
+ */
+ @Test
+ void sameOriginListenerAccepted()
+ {
+ tester.addRequestHeader(SEC_FETCH_SITE_HEADER, SAME_ORIGIN);
+
assertRequestAccepted();
}
+ /**
+ * {@code none} means there was no initiating document - a typed URL or a bookmark - which
+ * another document cannot cause, so it is allowed.
+ */
+ @Test
+ void browserInitiatedListenerAccepted()
+ {
+ tester.addRequestHeader(SEC_FETCH_SITE_HEADER, NONE);
+
+ assertRequestAccepted();
+ }
+
+ /**
+ * Without Fetch Metadata headers the policy has nothing to judge on and the next policy decides.
+ */
+ @Test
+ void missingFetchMetadataIsUnknown()
+ {
+ assertEquals(ResourceIsolationOutcome.UNKNOWN,
+ new FetchMetadataResourceIsolationPolicy().isRequestAllowed(tester.getRequest(),
+ tester.getLastRenderedPage(), RequestType.LISTENER));
+ }
+
/**
* Tests that a POST is not a simple top-level navigation request and is blocked
*/
@@ -165,7 +266,7 @@ public class ResourceIsolationRequestCycleListenerTest extends WicketTestCase
@Test
void varyHeaderSetWhenFetchMetadataAcceptsRequest()
{
- tester.addRequestHeader(SEC_FETCH_SITE_HEADER, SAME_SITE);
+ tester.addRequestHeader(SEC_FETCH_SITE_HEADER, SAME_ORIGIN);
tester.setFollowRedirects(false);
assertRequestAccepted();
diff --git a/wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataResourceIsolationPolicy.java b/wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataResourceIsolationPolicy.java
index fc67e0e5ae..4d06a607e2 100644
--- a/wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataResourceIsolationPolicy.java
+++ b/wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataResourceIsolationPolicy.java
@@ -25,6 +25,24 @@ import org.apache.wicket.util.string.Strings;
/**
* Default resource isolation policy used in {@link ResourceIsolationRequestCycleListener},
* based on <a href="https://web.dev/fetch-metadata/">https://web.dev/fetch-metadata/</a>.
+ * <p>
+ * The policy decides on the {@code Sec-Fetch-*} headers, which a browser sets itself and which
+ * cannot be set or removed by page content, and it distinguishes what the request does with the
+ * targeted page:
+ * <ul>
+ * <li>{@code same-origin} and {@code none} are allowed. {@code none} means there was no initiating
+ * document at all - a typed URL or a bookmark - which another document cannot cause.</li>
+ * <li>{@code same-site} is a <em>different</em> origin on the same site, so by default it may not
+ * invoke a listener. See {@link #setSameSiteAllowed(boolean)}.</li>
+ * <li>Anything else from another origin may still {@link RequestType#RENDER} a page through a simple
+ * top-level navigation, so that pages can be linked to from elsewhere, but may never invoke a
+ * {@link RequestType#LISTENER}.</li>
+ * <li>When the request carries no {@code Sec-Fetch-Site} header at all the outcome is
+ * {@link ResourceIsolationOutcome#UNKNOWN} and the next policy decides.</li>
+ * </ul>
+ * Wicket invokes listeners through ordinary GET navigations, so the headers alone do not say whether
+ * a request merely renders a page or performs an action on it. The {@link RequestType} the listener
+ * passes in is what separates the two.
*
* @see <a href="https://web.dev/fetch-metadata/">https://web.dev/fetch-metadata/</a>
*
@@ -56,9 +74,22 @@ public class FetchMetadataResourceIsolationPolicy implements IResourceIsolationP
private static final String VARY_HEADER_VALUE = SEC_FETCH_DEST_HEADER + ", "
+ SEC_FETCH_SITE_HEADER + ", " + SEC_FETCH_MODE_HEADER;
+ private boolean sameSiteAllowed = false;
+
+ /**
+ * Called when the type of the request is not known. Applies the rules for a
+ * {@link RequestType#LISTENER}, which are the stricter of the two.
+ */
@Override
public ResourceIsolationOutcome isRequestAllowed(HttpServletRequest request,
IRequestablePage targetPage)
+ {
+ return isRequestAllowed(request, targetPage, RequestType.LISTENER);
+ }
+
+ @Override
+ public ResourceIsolationOutcome isRequestAllowed(HttpServletRequest request,
+ IRequestablePage targetPage, RequestType requestType)
{
// request made by a legacy browser with no support for Fetch Metadata
String site = request.getHeader(SEC_FETCH_SITE_HEADER);
@@ -66,19 +97,62 @@ public class FetchMetadataResourceIsolationPolicy implements IResourceIsolationP
{
return ResourceIsolationOutcome.UNKNOWN;
}
-
- // Allow same-site and browser-initiated requests
- if (SAME_ORIGIN.equals(site) || SAME_SITE.equals(site) || NONE.equals(site))
+
+ // Allow same-origin and browser-initiated requests. A browser cannot be made to report
+ // 'none' by another document, so it is not a forgeable value.
+ if (SAME_ORIGIN.equals(site) || NONE.equals(site))
+ {
+ return ResourceIsolationOutcome.ALLOWED;
+ }
+
+ // Allow requests from a sibling origin on the same site only when configured to do so
+ if (SAME_SITE.equals(site) && sameSiteAllowed)
{
return ResourceIsolationOutcome.ALLOWED;
}
- // Allow simple top-level navigations except <object> and <embed>
- return isAllowedTopLevelNavigation(request)
+ // The request comes from another origin. Rendering a page is allowed for a simple top-level
+ // navigation, except <object> and <embed>, so that the page can still be linked to from
+ // elsewhere. Invoking a listener from another origin is not allowed.
+ return requestType == RequestType.RENDER && isAllowedTopLevelNavigation(request)
? ResourceIsolationOutcome.ALLOWED
: ResourceIsolationOutcome.DISALLOWED;
}
+ /**
+ * Sets whether requests from a different origin on the same site are allowed to invoke a
+ * listener. {@code Sec-Fetch-Site: same-site} means the request comes from the same registrable
+ * domain and scheme but a <em>different</em> origin, such as another subdomain or another port.
+ * <p>
+ * This is {@code false} by default, so a sibling origin cannot invoke a listener. Enable it when
+ * every origin on the site is trusted, for instance when the sibling subdomains are all part of
+ * the same application. Note that a hostile sibling - through a subdomain takeover, delegated
+ * user content, or an XSS elsewhere on the site - can then perform actions in the context of an
+ * authenticated user, because the browser sends the session cookie on a same-site request.
+ * <p>
+ * This setting does not affect renders: a page can always be reached by a simple top-level
+ * navigation, from a sibling origin or from another site entirely.
+ *
+ * @param sameSiteAllowed
+ * {@code true} to trust every origin on the same site
+ * @return {@code this} object for chaining
+ */
+ public FetchMetadataResourceIsolationPolicy setSameSiteAllowed(boolean sameSiteAllowed)
+ {
+ this.sameSiteAllowed = sameSiteAllowed;
+ return this;
+ }
+
+ /**
+ * @return whether requests from a different origin on the same site may invoke a listener,
+ * {@code false} by default
+ * @see #setSameSiteAllowed(boolean)
+ */
+ public boolean isSameSiteAllowed()
+ {
+ return sameSiteAllowed;
+ }
+
private boolean isAllowedTopLevelNavigation(HttpServletRequest request)
{
String mode = request.getHeader(SEC_FETCH_MODE_HEADER);
diff --git a/wicket-core/src/main/java/org/apache/wicket/protocol/http/IResourceIsolationPolicy.java b/wicket-core/src/main/java/org/apache/wicket/protocol/http/IResourceIsolationPolicy.java
index fb5de31218..11cb61bc2e 100644
--- a/wicket-core/src/main/java/org/apache/wicket/protocol/http/IResourceIsolationPolicy.java
+++ b/wicket-core/src/main/java/org/apache/wicket/protocol/http/IResourceIsolationPolicy.java
@@ -51,8 +51,32 @@ public interface IResourceIsolationPolicy
}
/**
- * Is the given request allowed.
- *
+ * What the request is going to do with the targeted page. A policy usually has to treat these
+ * differently: a page may legitimately be rendered as the result of a top-level navigation from
+ * another site, but a listener on that page must never be invoked from another site.
+ *
+ * @author papegaaij
+ *
+ * @see IResourceIsolationPolicy#isRequestAllowed(jakarta.servlet.http.HttpServletRequest,
+ * org.apache.wicket.request.component.IRequestablePage, RequestType)
+ */
+ public enum RequestType
+ {
+ /** The request renders the targeted page. */
+ RENDER,
+ /** The request invokes a listener on the targeted page, such as a link or a form submit. */
+ LISTENER
+ }
+
+ /**
+ * Is the given request allowed. Implement {@link #isRequestAllowed(HttpServletRequest,
+ * IRequestablePage, RequestType)} instead when the outcome depends on what the request does with
+ * the page, which is normally the case.
+ * <p>
+ * Implementations of this method are called for both renders and listener invocations without
+ * being able to tell them apart, so they must apply the rules that are safe for a listener
+ * invocation.
+ *
* @param request
* request
* @param targetPage
@@ -62,6 +86,28 @@ public interface IResourceIsolationPolicy
ResourceIsolationOutcome isRequestAllowed(HttpServletRequest request,
IRequestablePage targetPage);
+ /**
+ * Is the given request allowed, given what it is going to do with the targeted page.
+ * <p>
+ * This is the method {@link ResourceIsolationRequestCycleListener} calls. The default
+ * implementation ignores {@code requestType} and delegates to
+ * {@link #isRequestAllowed(HttpServletRequest, IRequestablePage)}, so that policies written
+ * against the two-argument method keep working unchanged.
+ *
+ * @param request
+ * request
+ * @param targetPage
+ * targeted page
+ * @param requestType
+ * what the request does with {@code targetPage}
+ * @return outcome, must not be <code>null</code>
+ */
+ default ResourceIsolationOutcome isRequestAllowed(HttpServletRequest request,
+ IRequestablePage targetPage, RequestType requestType)
+ {
+ return isRequestAllowed(request, targetPage);
+ }
+
/**
* Set possible response headers.
*
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 5858b38fe2..4ad3792870 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
@@ -30,6 +30,7 @@ import jakarta.servlet.http.HttpServletResponse;
import org.apache.wicket.RestartResponseException;
import org.apache.wicket.core.request.handler.IPageRequestHandler;
import org.apache.wicket.core.request.handler.RenderPageRequestHandler;
+import org.apache.wicket.protocol.http.IResourceIsolationPolicy.RequestType;
import org.apache.wicket.protocol.http.IResourceIsolationPolicy.ResourceIsolationOutcome;
import org.apache.wicket.request.IRequestHandler;
import org.apache.wicket.request.IRequestHandlerDelegate;
@@ -284,6 +285,25 @@ public class ResourceIsolationRequestCycleListener implements IRequestCycleListe
&& !(handler instanceof RenderPageRequestHandler);
}
+ /**
+ * Determines what the request does with the targeted page, so that the policies can distinguish
+ * a render - which may legitimately be the result of a top-level navigation from another site -
+ * from a listener invocation, which must never come from another site.
+ * <p>
+ * Note that {@link #isChecked(IRequestHandler)} excludes renders by default, so policies only
+ * see {@link RequestType#RENDER} when that method is overridden to extend resource isolation to
+ * page renders.
+ *
+ * @param handler
+ * the handler that is currently processing
+ * @return the type of request
+ */
+ protected RequestType getRequestType(IRequestHandler handler)
+ {
+ return handler instanceof RenderPageRequestHandler ? RequestType.RENDER
+ : RequestType.LISTENER;
+ }
+
@Override
public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
{
@@ -322,10 +342,12 @@ public class ResourceIsolationRequestCycleListener implements IRequestCycleListe
return;
}
+ RequestType requestType = getRequestType(handler);
+
for (IResourceIsolationPolicy policy : resourceIsolationPolicies)
{
- ResourceIsolationOutcome outcome = policy
- .isRequestAllowed(containerRequest, targetedPage);
+ ResourceIsolationOutcome outcome = policy.isRequestAllowed(containerRequest,
+ targetedPage, requestType);
if (ResourceIsolationOutcome.DISALLOWED.equals(outcome))
{
log.debug("Isolation policy {} has rejected a request to {}",
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 8225ea6d3d..ecab4acb02 100644
--- a/wicket-user-guide/src/main/asciidoc/security/security_5.adoc
+++ b/wicket-user-guide/src/main/asciidoc/security/security_5.adoc
@@ -57,6 +57,32 @@ For example:
}
----
+The _Sec-Fetch-Site_ header tells __FetchMetadataResourceIsolationPolicy__ where a request came from. A value of _same-origin_ means it was made from the very same scheme, host and port. _cross-site_ means it came from a different site altogether. _same-site_ sits in between: the same registrable domain and scheme, but a *different* origin - another subdomain, or another port. A _same-site_ request is therefore always cross-origin.
+
+The policy allows _same-origin_ requests, and browser-initiated ones (_none_, such as a typed url or a bookmark). For a request from another origin it looks at what that request does with the page: rendering is allowed for a simple top-level navigation, so our pages stay linkable from anywhere, while invoking a listener is not.
+
+Because a _same-site_ request is cross-origin, it is treated as one: by default a sibling origin such as _other.example.com_ cannot execute _Link.onClick()_ or submit a form on _app.example.com_, even though the browser does send our session cookie along with such a request.
+
+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.
+
+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]
+----
+ @Override
+ protected void init() {
+ super.init();
+ getRequestCycleListeners().add(
+ new ResourceIsolationRequestCycleListener(
+ new FetchMetadataResourceIsolationPolicy().setSameSiteAllowed(true),
+ new OriginResourceIsolationPolicy()
+ ));
+ // ...
+ }
+----
+
+WARNING: Enabling this trusts every origin on our site. A hostile sibling origin - obtained through a subdomain takeover, through delegated user content, or through an XSS somewhere else on the site - can then perform actions on behalf of an authenticated user.
+
_ResourceIsolationRequestCycleListener_ is not an alternative to _CryptoMapper_! Both of them could be used separately or in tandem to prevent CSRF attacks depending on the application requirements.
NOTE: In the next chapter we will cover unit testing with Wicket. If your application is protected with _ResourceIsolationRequestCycleListener_ you have to properly set request header _"sec-fetch-site"_ to make you unit tests pass. In <<testing.adoc#_setting_request_headers,paragraph 23.1.10>> you will learn how to do it.
(wicket) 01/05: Validate locale/style/variation decoded from resource URLs
Posted by pa...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
papegaaij pushed a commit to branch wicket-10.x
in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 600e3b732a5b2c9b16feb8865435763ef245e4f0
Author: Emond Papegaaij <em...@topicus.nl>
AuthorDate: Mon Aug 3 16:55:07 2026 +0200
Validate locale/style/variation decoded from resource URLs
The locale, style and variation decoded from a resource URL each become a single
component of the resource lookup path, which ResourceNameIterator builds as
<path>_<variation>_<style>_<locale>.<extension>. They were used as decoded, so a
value containing a path separator contributed more than one component and the
lookup resolved in a different directory than the resource it belongs to.
None of the three can legitimately contain a path separator, so they are now
rejected where they are decoded, in
ResourceUtil#decodeResourceReferenceAttributes: the locale as the raw string
before parseLocale, and the style and variation after
unescapeAttributesSeparator, so that a separator written as '~' is caught after
it has been restored to '-'. A rejected value is dropped and logged, leaving the
request to resolve the resource under its remaining attributes rather than fail.
ResourceUtil#rejectPathSeparators is public and applied a second time in the
ResourceNameIterator constructor, so that callers which do not come from a URL -
an application deriving a style from a request parameter, say - are covered where
the attributes reach the path.
The locale needed covering as well as the style and variation: parseLocale passes
its argument to Locale.of, which does not validate it, and
LocaleResourceNameIterator then appends Locale#toString() to the path. A value
placed entirely in the locale reached the path the same way, limited to lowercase
because parseLocale lowercases its input.
A style or variation legitimately containing '/' or '\' no longer round-trips.
Such a value never resolved to the resource it named, so the change is intended.
Co-Authored-By: Claude Opus 5 <no...@anthropic.com>
---
.../ResourceUrlAttributeValidationTest.java | 225 +++++++++++++++++++++
.../apache/wicket/resource/ResourceUtilTest.java | 67 ++++++
.../resource/locator/ResourceNameIterator.java | 9 +-
.../org/apache/wicket/resource/ResourceUtil.java | 71 ++++++-
4 files changed, 365 insertions(+), 7 deletions(-)
diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/request/resource/ResourceUrlAttributeValidationTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/request/resource/ResourceUrlAttributeValidationTest.java
new file mode 100644
index 0000000000..5543281659
--- /dev/null
+++ b/wicket-core-tests/src/test/java/org/apache/wicket/request/resource/ResourceUrlAttributeValidationTest.java
@@ -0,0 +1,225 @@
+/*
+ * 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.request.resource;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.wicket.markup.html.WebPage;
+import org.apache.wicket.util.file.IResourceFinder;
+import org.apache.wicket.util.resource.IResourceStream;
+import org.apache.wicket.util.resource.StringResourceStream;
+import org.apache.wicket.util.tester.WicketTestCase;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Verifies that the locale, style and variation decoded from a resource URL cannot change the
+ * directory a resource is resolved in.
+ * <p>
+ * The resource stream locator appends these three attributes to the path it asks every
+ * {@link IResourceFinder} for, as {@code <path>_<variation>_<style>_<locale>.<extension>}. A value
+ * containing a path separator would therefore contribute more than one path component, and the
+ * lookup would resolve somewhere other than the package the resource belongs to. No legitimate
+ * locale, style or variation contains one, so {@code ResourceUtil#decodeResourceReferenceAttributes}
+ * drops such a value.
+ * <p>
+ * These tests replace the application's resource finders with a recording one, so that the paths
+ * Wicket asks for can be asserted directly, independently of how a servlet container resolves them.
+ * {@link NormalizingFinder} additionally collapses {@code ..} the way a container does when
+ * resolving a path. {@code MockServletContext} does not: it resolves through
+ * {@code new File(root, name)}, which will not walk through a {@code PublicPage_..} component that
+ * does not exist, so a test relying on the mock context would not exercise this at all.
+ */
+public class ResourceUrlAttributeValidationTest extends WicketTestCase
+{
+ /** Served by {@link NormalizingFinder} for a path that resolved outside the package. */
+ private static final String OUTSIDE_CONTENT = "content-from-another-location";
+
+ /** Where the style below points, relative to the root the finder resolves against. */
+ private static final String OUTSIDE_PREFIX = "other-package/";
+
+ private static final String OUTSIDE_NAME = OUTSIDE_PREFIX + "Config";
+
+ private static final String PACKAGE_PREFIX =
+ ResourceUrlAttributeValidationTest.class.getPackageName().replace('.', '/') + "/";
+
+ /**
+ * How many {@code ../} the style needs to reach the root the finder resolves against. The style
+ * is appended as {@code <package>/<name>_<style>}, so its first {@code ..} is glued onto
+ * {@code PublicPage_} and forms a literal path component rather than a parent reference - hence
+ * one extra, plus one to consume that component itself.
+ */
+ private static final int PARENT_STEPS =
+ ResourceUrlAttributeValidationTest.class.getPackageName().split("\\.").length + 2;
+
+ /**
+ * The style is carried in the first query parameter's NAME, with an empty value. The {@code -} is
+ * the attribute separator, so a {@code -} within the style itself has to be written as
+ * {@code ~}: {@code ResourceUtil#unescapeAttributesSeparator} turns {@code (\w)~(\w)} back into
+ * {@code -} after the split. Keeping one here means the validation is also shown to happen after
+ * that restoration rather than before it.
+ */
+ private static final String STYLE_WITH_SEPARATORS =
+ "en-" + "..%2F".repeat(PARENT_STEPS) + OUTSIDE_NAME.replace("-", "~").replace("/", "%2F");
+
+ private static final String URL = "wicket/resource/" +
+ ResourceUrlAttributeValidationTest.class.getName() + "/PublicPage.html?" +
+ STYLE_WITH_SEPARATORS;
+
+ /** Referenced by the resource URL above; it does not need to exist as a file. */
+ public static class PublicPage extends WebPage
+ {
+ }
+
+ /** Records every path the locator asks for. */
+ private static class RecordingFinder implements IResourceFinder
+ {
+ final List<String> asked = new ArrayList<>();
+
+ @Override
+ public IResourceStream find(Class<?> clazz, String pathname)
+ {
+ asked.add(pathname);
+ return found(pathname);
+ }
+
+ IResourceStream found(String pathname)
+ {
+ return null;
+ }
+ }
+
+ /**
+ * Collapses {@code ..} in the path string the way a servlet container does when resolving it, and
+ * returns content for a path that ends up under {@link #OUTSIDE_PREFIX} - that is, for a lookup
+ * that left the package it started in.
+ */
+ private static class NormalizingFinder extends RecordingFinder
+ {
+ @Override
+ IResourceStream found(String pathname)
+ {
+ return normalize(pathname).startsWith(OUTSIDE_PREFIX)
+ ? new StringResourceStream(OUTSIDE_CONTENT)
+ : null;
+ }
+
+ private static String normalize(String path)
+ {
+ List<String> segments = new ArrayList<>();
+ for (String segment : path.split("/", -1))
+ {
+ if (segment.isEmpty() || ".".equals(segment))
+ {
+ continue;
+ }
+ if ("..".equals(segment))
+ {
+ if (segments.isEmpty() == false)
+ {
+ segments.remove(segments.size() - 1);
+ }
+ continue;
+ }
+ segments.add(segment);
+ }
+ return String.join("/", segments);
+ }
+ }
+
+ private <T extends RecordingFinder> T install(T finder)
+ {
+ List<IResourceFinder> finders = tester.getApplication()
+ .getResourceSettings()
+ .getResourceFinders();
+ finders.clear();
+ finders.add(finder);
+ return finder;
+ }
+
+ /**
+ * A rejected style is dropped rather than failing the request, so the resource is still located
+ * under its unstyled name.
+ */
+ @Test
+ void unstyledResourceIsStillLocatedWhenStyleIsRejected()
+ {
+ RecordingFinder finder = install(new RecordingFinder());
+
+ tester.executeUrl(URL);
+
+ assertTrue(finder.asked.contains(PACKAGE_PREFIX + "PublicPage.html"),
+ "the unstyled resource should still be looked up; asked: " + finder.asked);
+ }
+
+ /**
+ * Every path the locator asks for stays within the package, for separators carried by the style.
+ */
+ @Test
+ void pathFromStyleStaysWithinPackage()
+ {
+ RecordingFinder finder = install(new RecordingFinder());
+
+ tester.executeUrl(URL);
+
+ assertFalse(finder.asked.stream().anyMatch(path -> path.contains("..")),
+ "no path should contain a parent reference; asked: " + finder.asked);
+ assertTrue(finder.asked.stream().allMatch(path -> path.startsWith(PACKAGE_PREFIX)),
+ "every path should start with " + PACKAGE_PREFIX + "; asked: " + finder.asked);
+ }
+
+ /**
+ * As {@link #pathFromStyleStaysWithinPackage()}, for the locale: {@code ResourceUtil#parseLocale}
+ * hands its input to {@code Locale.of}, which does not validate it, and
+ * {@code LocaleResourceNameIterator} then appends {@code Locale#toString()} to the path. The
+ * locale may contain neither {@code -} (the attribute separator) nor {@code _} (the locale
+ * separator), and is lowercased by {@code parseLocale}.
+ */
+ @Test
+ void pathFromLocaleStaysWithinPackage()
+ {
+ RecordingFinder finder = install(new RecordingFinder());
+
+ tester.executeUrl("wicket/resource/" +
+ ResourceUrlAttributeValidationTest.class.getName() + "/PublicPage.html?" +
+ "..%2F".repeat(PARENT_STEPS) + "otherpackage%2Fconfig");
+
+ assertFalse(finder.asked.stream().anyMatch(path -> path.contains("..")),
+ "no path should contain a parent reference; asked: " + finder.asked);
+ assertTrue(finder.asked.stream().allMatch(path -> path.startsWith(PACKAGE_PREFIX)),
+ "every path should start with " + PACKAGE_PREFIX + "; asked: " + finder.asked);
+ }
+
+ /**
+ * On a finder that collapses {@code ..} the way a container does, a path built from a style
+ * carrying separators would resolve outside the package. Nothing found there may be returned to
+ * the client.
+ */
+ @Test
+ void resourceResolvingOutsideThePackageIsNotServed()
+ {
+ install(new NormalizingFinder());
+
+ tester.executeUrl(URL);
+
+ assertFalse(tester.getLastResponseAsString().contains(OUTSIDE_CONTENT),
+ "a resource resolved outside the package must not be served");
+ }
+}
diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/resource/ResourceUtilTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/resource/ResourceUtilTest.java
index 92b84bac25..a56c396948 100644
--- a/wicket-core-tests/src/test/java/org/apache/wicket/resource/ResourceUtilTest.java
+++ b/wicket-core-tests/src/test/java/org/apache/wicket/resource/ResourceUtilTest.java
@@ -61,6 +61,73 @@ class ResourceUtilTest
assertNull(attributes.getVariation());
}
+ @Test
+ void rejectPathSeparators() throws Exception
+ {
+ assertEquals("style", ResourceUtil.rejectPathSeparators("style", "style"));
+ assertEquals("my-style", ResourceUtil.rejectPathSeparators("my-style", "style"));
+ assertEquals("", ResourceUtil.rejectPathSeparators("", "style"));
+ assertNull(ResourceUtil.rejectPathSeparators(null, "style"));
+
+ assertNull(ResourceUtil.rejectPathSeparators("a/b", "style"));
+ assertNull(ResourceUtil.rejectPathSeparators("a\\b", "style"));
+ assertNull(ResourceUtil.rejectPathSeparators("..", "style"));
+ assertNull(ResourceUtil.rejectPathSeparators("../../etc", "style"));
+ assertNull(ResourceUtil.rejectPathSeparators("a\0b", "style"));
+ assertNull(ResourceUtil.rejectPathSeparators("\0", "style"));
+ }
+
+ @Test
+ void rejectPathSeparatorsForLocale() throws Exception
+ {
+ assertEquals(Locale.UK, ResourceUtil.rejectPathSeparators(Locale.UK));
+ assertNull(ResourceUtil.rejectPathSeparators((Locale)null));
+
+ assertNull(ResourceUtil.rejectPathSeparators(new Locale("../../etc")));
+ assertNull(ResourceUtil.rejectPathSeparators(new Locale("a/b")));
+ }
+
+ /**
+ * A locale, style or variation carrying a path separator is dropped: each becomes a single
+ * component of the resource lookup path, so a separator would make the lookup resolve in a
+ * different directory than the resource it belongs to.
+ */
+ @Test
+ void decodeResourceReferenceAttributesRejectsPathSeparators() throws Exception
+ {
+ for (String value : new String[] { "../../etc", "..\\..\\etc", "a/b", "a\\b", "..",
+ "a\0b" })
+ {
+ UrlAttributes attributes = ResourceUtil.decodeResourceReferenceAttributes(value);
+ assertNull(attributes.getLocale(), "locale should be dropped for '" + value + "'");
+
+ attributes = ResourceUtil.decodeResourceReferenceAttributes("en-" + value);
+ assertEquals(Locale.ENGLISH, attributes.getLocale());
+ assertNull(attributes.getStyle(), "style should be dropped for '" + value + "'");
+
+ attributes = ResourceUtil.decodeResourceReferenceAttributes("en-style-" + value);
+ assertEquals(Locale.ENGLISH, attributes.getLocale());
+ assertEquals("style", attributes.getStyle());
+ assertNull(attributes.getVariation(),
+ "variation should be dropped for '" + value + "'");
+ }
+ }
+
+ /**
+ * The separator check runs after {@code ~} has been restored to {@code -}, and must not disturb
+ * that restoration.
+ */
+ @Test
+ void decodeResourceReferenceAttributesKeepsEscapedSeparator() throws Exception
+ {
+ UrlAttributes attributes =
+ ResourceUtil.decodeResourceReferenceAttributes("en-my~style-my~variation");
+
+ assertEquals(Locale.ENGLISH, attributes.getLocale());
+ assertEquals("my-style", attributes.getStyle());
+ assertEquals("my-variation", attributes.getVariation());
+ }
+
@Test
void decodeResourceReferenceAttributesWithUrl() throws Exception
{
diff --git a/wicket-core/src/main/java/org/apache/wicket/core/util/resource/locator/ResourceNameIterator.java b/wicket-core/src/main/java/org/apache/wicket/core/util/resource/locator/ResourceNameIterator.java
index a659047d61..f89c959a3a 100644
--- a/wicket-core/src/main/java/org/apache/wicket/core/util/resource/locator/ResourceNameIterator.java
+++ b/wicket-core/src/main/java/org/apache/wicket/core/util/resource/locator/ResourceNameIterator.java
@@ -20,6 +20,7 @@ import java.util.Arrays;
import java.util.Locale;
import org.apache.wicket.WicketRuntimeException;
+import org.apache.wicket.resource.ResourceUtil;
import org.apache.wicket.util.string.Strings;
/**
@@ -90,7 +91,9 @@ public class ResourceNameIterator implements IResourceNameIterator
public ResourceNameIterator(final String path, final String style, final String variation,
final Locale locale, final Iterable<String> extensions, final boolean strict)
{
- this.locale = locale;
+ // the style, variation and locale each become a single component of the paths built below, so
+ // a value carrying a path separator would resolve in a different directory than the resource
+ this.locale = ResourceUtil.rejectPathSeparators(locale);
boolean noext = extensions == null || !extensions.iterator().hasNext();
@@ -106,7 +109,9 @@ public class ResourceNameIterator implements IResourceNameIterator
this.path = path;
}
- styleIterator = newStyleAndVariationResourceNameIterator(style, variation);
+ styleIterator = newStyleAndVariationResourceNameIterator(
+ ResourceUtil.rejectPathSeparators(style, "style"),
+ ResourceUtil.rejectPathSeparators(variation, "variation"));
this.strict = strict;
}
diff --git a/wicket-core/src/main/java/org/apache/wicket/resource/ResourceUtil.java b/wicket-core/src/main/java/org/apache/wicket/resource/ResourceUtil.java
index 28ed7e2562..851ebde007 100644
--- a/wicket-core/src/main/java/org/apache/wicket/resource/ResourceUtil.java
+++ b/wicket-core/src/main/java/org/apache/wicket/resource/ResourceUtil.java
@@ -30,17 +30,74 @@ import org.apache.wicket.util.lang.Args;
import org.apache.wicket.util.resource.IResourceStream;
import org.apache.wicket.util.resource.ResourceStreamNotFoundException;
import org.apache.wicket.util.string.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Utilities for resources.
- *
+ *
* @author Jeremy Thomerson
*/
public class ResourceUtil
{
+ private static final Logger log = LoggerFactory.getLogger(ResourceUtil.class);
private static final Pattern ESCAPED_ATTRIBUTE_PATTERN = Pattern.compile("(\\w)~(\\w)");
+ /**
+ * Rejects a resource attribute that cannot be used as a single path component.
+ * <p>
+ * The locale, style and variation are appended to the resource lookup path by
+ * {@link org.apache.wicket.core.util.resource.locator.ResourceNameIterator}, as
+ * {@code <path>_<variation>_<style>_<locale>}. A value containing a path separator would
+ * therefore contribute more than one component and the lookup would resolve in a different
+ * directory than the resource it belongs to. None of the three can legitimately contain one.
+ *
+ * @param attribute
+ * the attribute, may be {@code null}
+ * @param attributeName
+ * the attribute's name, used for logging
+ * @return the attribute, or {@code null} if it contains {@code /}, {@code \}, {@code ..} or a
+ * NUL character
+ */
+ public static String rejectPathSeparators(final String attribute, final String attributeName)
+ {
+ if (attribute == null)
+ {
+ return null;
+ }
+
+ if (attribute.contains("/") || attribute.contains("\\") || attribute.contains("..") ||
+ attribute.contains("\0"))
+ {
+ log.warn("Ignoring the {} because it contains a path separator or NUL: {}",
+ attributeName, attribute);
+
+ return null;
+ }
+
+ return attribute;
+ }
+
+ /**
+ * Rejects a locale whose {@link Locale#toString()} cannot be used as a single path component.
+ *
+ * @param locale
+ * the locale, may be {@code null}
+ * @return the locale, or {@code null} if its string representation contains a path separator
+ *
+ * @see #rejectPathSeparators(String, String)
+ */
+ public static Locale rejectPathSeparators(final Locale locale)
+ {
+ if (locale == null || rejectPathSeparators(locale.toString(), "locale") != null)
+ {
+ return locale;
+ }
+
+ return null;
+ }
+
/**
* Reads resource reference attributes (style, locale, variation) encoded in the given string.
*
@@ -59,15 +116,19 @@ public class ResourceUtil
if (Strings.isEmpty(encodedAttributes) == false)
{
String split[] = Strings.split(encodedAttributes, '-');
- locale = parseLocale(split[0]);
+ locale = parseLocale(rejectPathSeparators(split[0], "locale"));
if (split.length == 2)
{
- style = Strings.defaultIfEmpty(unescapeAttributesSeparator(split[1]), null);
+ style = rejectPathSeparators(
+ Strings.defaultIfEmpty(unescapeAttributesSeparator(split[1]), null), "style");
}
else if (split.length == 3)
{
- style = Strings.defaultIfEmpty(unescapeAttributesSeparator(split[1]), null);
- variation = Strings.defaultIfEmpty(unescapeAttributesSeparator(split[2]), null);
+ style = rejectPathSeparators(
+ Strings.defaultIfEmpty(unescapeAttributesSeparator(split[1]), null), "style");
+ variation = rejectPathSeparators(
+ Strings.defaultIfEmpty(unescapeAttributesSeparator(split[2]), null),
+ "variation");
}
}
return new ResourceReference.UrlAttributes(locale, style, variation);
(wicket) 02/05: Enforce the configured upload limits on the Servlet Part fallback
Posted by pa...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
papegaaij pushed a commit to branch wicket-10.x
in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 71f86141a60f8f6a2ffe10d175d0f30ac7e2d8e9
Author: Emond Papegaaij <em...@topicus.nl>
AuthorDate: Mon Aug 3 17:20:15 2026 +0200
Enforce the configured upload limits on the Servlet Part fallback
MultipartServletWebRequestImpl#parseFileParts() parses with commons-fileupload,
which newFileUpload() configures from the form's maxSize, fileMaxSize and
fileCountMax. When that parse yields no items it falls back to
readServlet3Parts(), which wrapped whatever HttpServletRequest#getParts()
returned in a ServletPartFileItem and applied none of those limits. Nothing
downstream re-checks - Form#handleMultiPart() only reacts to a
FileUploadException raised during parsing - so a quiet return left the limits
inert.
That fallback is not an error path. It is the normal one wherever something else
has already consumed the request body: a @MultipartConfig servlet, Spring Boot's
multipart resolver, or any filter calling getParameter() on a multipart request,
which is why it was added for WICKET-5924. It is also the only path while
uploadProgressUpdatesEnabled is left at its default of false.
The two per-item limits, fileMaxSize and fileCountMax, are the ones this
affected. Both are consulted per parsed item, and the fallback is taken
precisely when there are no items, so neither was ever applied to the parts the
container had already produced. An upload could therefore exceed the configured
size or count, up to whatever the component that parsed the request allowed.
The aggregate maxSize was applied on this path already.
FileItemInputIteratorImpl#init() compares Content-Length against sizeMax before
reading the body, and that header survives the body having been consumed, so a
request declaring an oversized length is rejected on the commons-fileupload path
and never reaches the fallback; confirmed by reading the library.
declaredContentLengthOverMaxSizeIsRejectedBeforeTheFallback pins it down,
because the other maxSize tests here would otherwise suggest the limit was not
applied at all: they only exercise the fallback because MockHttpServletRequest
reports the length of the near-empty body it builds for a parts-only request.
readServlet3Parts() now applies all three limits as it builds the item list,
mirroring commons-fileupload's own semantics so that both paths accept and
reject the same requests: fileMaxSize against every part, form fields included,
as FileItemInputImpl has no form-field guard; fileCountMax rejecting at
count == max, with a negative value meaning unlimited; and maxSize against a
running total of the part sizes. The aggregate check is for completeness rather
than a fix for anything observed - it can only matter for a request that
declares no length at all, which is conceivable through chunked transfer
encoding but has not been demonstrated to reach the fallback with parts
attached.
The checks read only Part#getSize(), so they complete before any part is opened.
That ordering is deliberate rather than incidental: parseFileParts() calls
getString() on every form field, which pulls the whole part into a single heap
array through IOUtils#toByteArray, so a check running after the parts had been
collected would allocate exactly what it was meant to refuse - for any part
arriving without a Content-Type, which is what ServletPartFileItem treats as a
form field. The test asserts the part's stream is never opened, so a later
reordering fails the build rather than quietly reintroducing it.
The exceptions are the types the upload entry points dispatch on, so the
existing feedback messages are unchanged: FileUploadByteCountLimitException for
a single file, FileUploadFileCountLimitException for the count, and
FileUploadSizeException for the total. Both Form#onFileUploadException() and
AbstractFileUploadResource#getFileUploadExceptionKey() map those three to the
UPLOAD_* resource keys; AjaxFileDropBehavior routes any FileUploadException to
onError(). Note the constructors do not agree on argument order - byte count
takes (actual, permitted), size takes (permitted, actual).
All three entry points that accept uploads this way - Form#handleMultiPart(),
AbstractFileUploadResource behind FileUploadToResourceField, and
AjaxFileDropBehavior#onEvent() - set the limits and then call parseFileParts(),
so enforcing in readServlet3Parts() covers each of them in one place.
Also removes the per-file loop in newMultipartWebRequest(). It could not fire on
the Form path: Form#handleMultiPart() reaches
ServletWebRequest#newMultipartWebRequest(), which constructs a fresh instance,
and even when the override was reached, files was empty and fileMaxSize unset,
both being set afterwards. Its FIXME asked why the sum of the file sizes is
never compared against maxSize; it is, by commons-fileupload, against the whole
request stream.
The tests assert the corrected behaviour. Five of the eleven fail without this
change. The other six - the commons-fileupload control, the Content-Length
behaviour above, delivery of files and of form fields within the limits, the
count boundary, and the unconfigured defaults of Bytes.MAX / null / -1 - hold
either way, and guard against the enforcement rejecting uploads it should not.
Reported and originally fixed by GitHub: @deprrous.
Co-Authored-By: Claude Opus 5 <no...@anthropic.com>
---
.../upload/FileUploadServletPartLimitsTest.java | 460 +++++++++++++++++++++
.../servlet/MultipartServletWebRequestImpl.java | 74 +++-
2 files changed, 513 insertions(+), 21 deletions(-)
diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/FileUploadServletPartLimitsTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/FileUploadServletPartLimitsTest.java
new file mode 100644
index 0000000000..7016a7ba8a
--- /dev/null
+++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/FileUploadServletPartLimitsTest.java
@@ -0,0 +1,460 @@
+/*
+ * 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.markup.html.form.upload;
+
+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.assertThrows;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+import java.util.List;
+
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletRequestWrapper;
+import jakarta.servlet.http.Part;
+
+import org.apache.commons.fileupload2.core.FileUploadException;
+import org.apache.commons.fileupload2.core.FileUploadSizeException;
+import org.apache.wicket.markup.html.form.Form;
+import org.apache.wicket.protocol.http.mock.MockHttpServletRequest;
+import org.apache.wicket.protocol.http.servlet.MultipartServletWebRequestImpl;
+import org.apache.wicket.request.resource.AbstractResource;
+import org.apache.wicket.util.lang.Bytes;
+import org.apache.wicket.util.tester.WicketTestCase;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Asserts that the upload limits a {@link Form} configures are enforced when
+ * {@code MultipartServletWebRequestImpl#parseFileParts()} falls back to the Servlet multipart API.
+ * <p>
+ * {@code parseFileParts()} first parses with commons-fileupload, which {@code newFileUpload()}
+ * configures from those limits. If that yields no items it falls back to
+ * {@code readServlet3Parts()}, which wraps whatever {@code HttpServletRequest#getParts()} returns in
+ * a {@code ServletPartFileItem}. The container has already parsed those parts, so the limits handed
+ * to commons-fileupload never had a chance to apply to them and {@code readServlet3Parts()} has to
+ * apply {@code maxSize}, {@code fileMaxSize} and {@code fileCountMax} itself. Nothing downstream
+ * re-checks: {@code Form#handleMultiPart()} only reacts to a {@link FileUploadException} raised
+ * during parsing, so a quiet return on this path means the limits are simply gone.
+ * <p>
+ * The fallback is not an error path. It exists for deployments where something else has already
+ * consumed the request body - a {@code @MultipartConfig} servlet, Spring Boot's multipart resolver,
+ * or any filter touching {@code request.getParameter()} on a multipart request - which is why it was
+ * added for <a href="https://issues.apache.org/jira/browse/WICKET-5924">WICKET-5924</a>. There it is
+ * the normal path, and it is the only path while {@code uploadProgressUpdatesEnabled} is left at its
+ * default of {@code false}.
+ * <p>
+ * Two properties of the fallback make the enforcement worth pinning down in detail. Rejection has to
+ * happen before a part is read, because {@code parseFileParts()} materialises every form field with
+ * {@code ServletPartFileItem#getString()}, which pulls the whole part into a single heap array
+ * through {@code IOUtils#toByteArray} - a check running after the parts had been collected would
+ * allocate exactly what it was meant to refuse. See
+ * {@link #oversizedFormFieldPartIsRejectedBeforeItIsBuffered()}. And {@code maxSize} was already
+ * applied to this path: commons-fileupload compares {@code Content-Length} against it before reading
+ * anything, so a request declaring an oversized length is rejected before the fallback is taken - see
+ * {@link #declaredContentLengthOverMaxSizeIsRejectedBeforeTheFallback()}. The aggregate check
+ * {@code readServlet3Parts()} performs is for completeness, not a fix for something observed.
+ * <p>
+ * {@code maxPartHeaderSize}, which {@code newFileUpload()} also configures, is deliberately not
+ * covered here: on this path the container has already parsed the part headers under its own rules,
+ * so Wicket's value was never going to apply to them.
+ * <p>
+ * These tests drive {@code MultipartServletWebRequestImpl} in the same order
+ * {@code Form#handleMultiPart()} does, rather than going through a page, so that nothing but the
+ * limit handling is under test. {@link #limitsAreEnforcedOnTheCommonsFileUploadPath()} is the
+ * control: it shows the same limit honoured when commons-fileupload does the parsing, which keeps
+ * the suite honest about which of the two paths each other test exercises.
+ */
+class FileUploadServletPartLimitsTest extends WicketTestCase
+{
+ /** Comfortably over every limit configured below. */
+ private static final int OVERSIZED = 10_000;
+
+ /**
+ * The size of the form field part. Large enough that buffering it would be unmistakably deliberate
+ * rather than a rounding error, small enough to be harmless in a test.
+ */
+ private static final int LARGE = 1 << 20;
+
+ private static final Bytes LIMIT = Bytes.bytes(100);
+
+ /** Comfortably inside {@link #LIMIT}, for the tests that assert an upload is let through. */
+ private static final int WITHIN_LIMIT = 10;
+
+ private static final String FIELD = "upload";
+
+ /**
+ * Registers {@code parts} on a multipart request that carries no body of its own, so that
+ * commons-fileupload finds nothing and the Servlet fallback is taken - the same situation as a
+ * container which has already parsed the upload.
+ */
+ private MockHttpServletRequest requestWithParts(Part... parts)
+ {
+ MockHttpServletRequest request = tester.getRequest();
+ request.setUseMultiPartContentType(true);
+ for (int i = 0; i < parts.length; i++)
+ {
+ request.setPart(FIELD + i, parts[i]);
+ }
+ return request;
+ }
+
+ /**
+ * Parses exactly as {@code Form#handleMultiPart()} does: construct with {@code maxSize}, then set
+ * the per-file and count limits, then parse.
+ */
+ private MultipartServletWebRequestImpl parse(HttpServletRequest request, Bytes maxSize,
+ Bytes fileMaxSize, Long fileCountMax) throws FileUploadException
+ {
+ MultipartServletWebRequestImpl multipart =
+ new MultipartServletWebRequestImpl(request, "", maxSize, "uploadId");
+ multipart.setFileMaxSize(fileMaxSize);
+ if (fileCountMax != null)
+ {
+ multipart.setFileCountMax(fileCountMax);
+ }
+ multipart.parseFileParts();
+ return multipart;
+ }
+
+ /**
+ * {@link MockHttpServletRequest#getContentLength()} reports the length of the body it builds, which
+ * for a request carrying only pre-parsed parts is a few dozen bytes. A real request declares the
+ * length of the upload it carried, so that has to be substituted to see what happens on the
+ * commons-fileupload path.
+ */
+ private HttpServletRequest withDeclaredContentLength(MockHttpServletRequest request,
+ int contentLength)
+ {
+ return new HttpServletRequestWrapper(request)
+ {
+ @Override
+ public int getContentLength()
+ {
+ return contentLength;
+ }
+
+ @Override
+ public long getContentLengthLong()
+ {
+ return contentLength;
+ }
+ };
+ }
+
+ /**
+ * Records where {@code maxSize} is really enforced when the container has already parsed the body.
+ * commons-fileupload compares the declared {@code Content-Length} against {@code maxSize} before it
+ * reads anything, and that header survives the body having been consumed, so an oversized
+ * <em>declared</em> length is rejected on the commons-fileupload path and the fallback is never
+ * reached. This holds independently of the enforcement below - it is commons-fileupload doing the
+ * work - and is asserted so that the division of labour between the two is not lost.
+ * <p>
+ * So the aggregate limit was not what went unapplied here; {@code fileMaxSize} and
+ * {@code fileCountMax} were, being per-item checks that are consulted only when there are items,
+ * and the fallback is taken precisely when there are none. The other {@code maxSize} tests here
+ * exercise the aggregate check {@code readServlet3Parts()} performs regardless. That check can only
+ * matter for a request declaring no length at all - conceivable through chunked transfer encoding,
+ * but not demonstrated against a real container - so it is kept for completeness rather than as a
+ * fix for anything observed.
+ */
+ @Test
+ void declaredContentLengthOverMaxSizeIsRejectedBeforeTheFallback()
+ {
+ MockHttpServletRequest request = requestWithParts(new SizedPart(FIELD, OVERSIZED));
+
+ FileUploadSizeException e = assertThrows(FileUploadSizeException.class,
+ () -> parse(withDeclaredContentLength(request, OVERSIZED), LIMIT, null, null),
+ "a declared Content-Length over maxSize must be rejected");
+
+ // pins it to the request-size check rather than any other parse failure
+ assertEquals(LIMIT.bytes(), e.getPermitted(), "permitted size should be maxSize");
+ assertEquals(OVERSIZED, e.getActualSize(), "actual size should be the declared length");
+ }
+
+ /**
+ * A part far larger than {@code fileMaxSize}.
+ */
+ @Test
+ void fileMaxSizeIsEnforcedOnServletPartFallback()
+ {
+ MockHttpServletRequest request = requestWithParts(new SizedPart(FIELD, OVERSIZED));
+
+ assertThrows(FileUploadException.class,
+ () -> parse(request, Bytes.MAX, LIMIT, null),
+ "a part exceeding fileMaxSize must be rejected");
+ }
+
+ /**
+ * A single part far larger than the total {@code maxSize}, on a request that does not declare a
+ * length large enough for commons-fileupload to have rejected it first. Covers the aggregate check
+ * kept for completeness, not something that went unapplied - see
+ * {@link #declaredContentLengthOverMaxSizeIsRejectedBeforeTheFallback()}.
+ */
+ @Test
+ void maxSizeIsEnforcedOnServletPartFallback()
+ {
+ MockHttpServletRequest request = requestWithParts(new SizedPart(FIELD, OVERSIZED));
+
+ assertThrows(FileUploadException.class,
+ () -> parse(request, LIMIT, null, null),
+ "an upload exceeding the form's maxSize must be rejected");
+ }
+
+ /**
+ * More parts than {@code fileCountMax} allows.
+ */
+ @Test
+ void fileCountMaxIsEnforcedOnServletPartFallback()
+ {
+ MockHttpServletRequest request = requestWithParts(new SizedPart(FIELD, 1),
+ new SizedPart(FIELD, 1), new SizedPart(FIELD, 1));
+
+ assertThrows(FileUploadException.class,
+ () -> parse(request, Bytes.MAX, null, 1L),
+ "more parts than fileCountMax must be rejected");
+ }
+
+ /**
+ * Parts each within {@code fileMaxSize} but together over {@code maxSize}. Neither trips the
+ * per-part check, so only the running total catches this. For completeness, as above.
+ */
+ @Test
+ void aggregateSizeOfPartsIsEnforcedAgainstMaxSize()
+ {
+ int half = (int)LIMIT.bytes() - 20;
+ MockHttpServletRequest request =
+ requestWithParts(new SizedPart(FIELD, half), new SizedPart(FIELD, half));
+
+ assertThrows(FileUploadException.class,
+ () -> parse(request, LIMIT, Bytes.bytes(half), null),
+ "parts summing to more than maxSize must be rejected");
+ }
+
+ /**
+ * An oversized form field part - one with no {@code Content-Type} - must be rejected without being
+ * read, since {@code parseFileParts()} would otherwise buffer it whole. The part fails the test if
+ * its stream is opened at all, which is what makes this a guard against the checks being reordered
+ * after the parts have been collected rather than just another rejection test.
+ */
+ @Test
+ void oversizedFormFieldPartIsRejectedBeforeItIsBuffered()
+ {
+ SizedPart part = new SizedPart(FIELD, LARGE, null);
+ MockHttpServletRequest request = requestWithParts(part);
+
+ assertThrows(FileUploadException.class,
+ () -> parse(request, LIMIT, null, null),
+ "a form field part exceeding maxSize must be rejected");
+
+ assertFalse(part.wasRead(), "the part must be rejected before its content is read");
+ }
+
+ /**
+ * The fallback still delivers what it should. This is also the assurance that the tests above
+ * exercise the fallback at all rather than failing for an unrelated reason: the same construction
+ * that is rejected when oversized arrives as an uploaded file when it fits.
+ */
+ @Test
+ void fallbackDeliversPartsWithinTheLimits() throws Exception
+ {
+ MockHttpServletRequest request = requestWithParts(new SizedPart(FIELD, WITHIN_LIMIT));
+
+ MultipartServletWebRequestImpl multipart = parse(request, LIMIT, LIMIT, 1L);
+
+ List<org.apache.commons.fileupload2.core.FileItem> files = multipart.getFile(FIELD);
+ assertNotNull(files, "the part should have come through the fallback");
+ assertEquals(1, files.size(), "exactly one part was uploaded");
+ assertEquals(WITHIN_LIMIT, files.get(0).getSize(), "the part should arrive intact");
+ }
+
+ /**
+ * A form field part within the limits still becomes a request parameter, so the enforcement has not
+ * cost the fallback its handling of form fields.
+ */
+ @Test
+ void fallbackDeliversFormFieldPartsWithinTheLimits() throws Exception
+ {
+ MockHttpServletRequest request =
+ requestWithParts(new SizedPart(FIELD, WITHIN_LIMIT, null));
+
+ MultipartServletWebRequestImpl multipart = parse(request, LIMIT, LIMIT, null);
+
+ assertEquals(WITHIN_LIMIT,
+ multipart.getPostParameters().getParameterValue(FIELD).toString().length(),
+ "the form field should arrive intact");
+ }
+
+ /**
+ * Exactly {@code fileCountMax} parts are allowed through; it is the part after the limit that is
+ * refused, as in {@code AbstractFileUpload}.
+ */
+ @Test
+ void exactlyFileCountMaxPartsAreAccepted() throws Exception
+ {
+ MockHttpServletRequest request = requestWithParts(new SizedPart(FIELD, WITHIN_LIMIT),
+ new SizedPart(FIELD, WITHIN_LIMIT), new SizedPart(FIELD, WITHIN_LIMIT));
+
+ MultipartServletWebRequestImpl multipart = parse(request, Bytes.MAX, null, 3L);
+
+ assertEquals(3, multipart.getFile(FIELD).size(), "all three parts are within fileCountMax");
+ }
+
+ /**
+ * An application that configures no limits keeps accepting everything: {@code maxSize} defaults to
+ * {@link Bytes#MAX}, {@code fileMaxSize} to {@code null} and {@code fileCountMax} to {@code -1}.
+ * Guards against the enforcement breaking deployments that never asked for a limit.
+ */
+ @Test
+ void nothingConfiguredAcceptsLargeAndNumerousParts() throws Exception
+ {
+ MockHttpServletRequest request = requestWithParts(new SizedPart(FIELD, OVERSIZED),
+ new SizedPart(FIELD, OVERSIZED), new SizedPart(FIELD, OVERSIZED));
+
+ MultipartServletWebRequestImpl multipart = parse(request, Bytes.MAX, null, null);
+
+ assertEquals(3, multipart.getFile(FIELD).size(), "no limit was configured");
+ }
+
+ /**
+ * The control: with a real multipart body, commons-fileupload does the parsing and the very same
+ * limit is honoured. Holds with or without the enforcement on the fallback, which is what makes it
+ * a control.
+ */
+ @Test
+ void limitsAreEnforcedOnTheCommonsFileUploadPath() throws Exception
+ {
+ MockHttpServletRequest request = tester.getRequest();
+ request.setUseMultiPartContentType(true);
+ request.addFile(FIELD, oversizedFile(), "text/plain");
+
+ assertThrows(FileUploadException.class,
+ () -> parse(request, Bytes.MAX, LIMIT, null),
+ "commons-fileupload should still enforce fileMaxSize");
+ }
+
+ private org.apache.wicket.util.file.File oversizedFile() throws IOException
+ {
+ java.io.File file = java.io.File.createTempFile("wicket-upload-limits", ".txt");
+ file.deleteOnExit();
+ Files.write(file.toPath(), new byte[OVERSIZED]);
+ return new org.apache.wicket.util.file.File(file);
+ }
+
+ /** A {@link Part} reporting an arbitrary size, as a container-parsed upload would. */
+ private static class SizedPart implements Part
+ {
+ private final LinkedHashMap<String, List<String>> headers = new LinkedHashMap<>();
+ private final String fieldName;
+ private final String contentType;
+ private final byte[] data;
+ private boolean read;
+
+ private SizedPart(String fieldName, int size)
+ {
+ this(fieldName, size, "text/plain");
+ }
+
+ /**
+ * @param contentType
+ * {@code null} for a part {@code ServletPartFileItem} will treat as a form field
+ */
+ private SizedPart(String fieldName, int size, String contentType)
+ {
+ this.fieldName = fieldName;
+ this.contentType = contentType;
+ this.data = new byte[size];
+ // printable ASCII, so that the decoded length is the part's length in any encoding
+ Arrays.fill(this.data, (byte)'a');
+ headers.put(AbstractResource.CONTENT_DISPOSITION_HEADER_NAME,
+ Arrays.asList(contentType == null
+ ? "form-data;name=" + fieldName
+ : "attachment;filename=oversized.txt"));
+ }
+
+ @Override
+ public InputStream getInputStream()
+ {
+ read = true;
+ return new ByteArrayInputStream(data);
+ }
+
+ /** Whether anything asked for this part's content, rather than only its metadata. */
+ boolean wasRead()
+ {
+ return read;
+ }
+
+ @Override
+ public String getContentType()
+ {
+ return contentType;
+ }
+
+ @Override
+ public String getName()
+ {
+ return fieldName;
+ }
+
+ @Override
+ public String getSubmittedFileName()
+ {
+ return "oversized.txt";
+ }
+
+ @Override
+ public long getSize()
+ {
+ return data.length;
+ }
+
+ @Override
+ public void write(String fileName)
+ {
+ }
+
+ @Override
+ public void delete()
+ {
+ }
+
+ @Override
+ public String getHeader(String name)
+ {
+ return headers.containsKey(name) ? headers.get(name).get(0) : null;
+ }
+
+ @Override
+ public Collection<String> getHeaders(String name)
+ {
+ return headers.get(name);
+ }
+
+ @Override
+ public Collection<String> getHeaderNames()
+ {
+ return headers.keySet();
+ }
+ }
+}
diff --git a/wicket-core/src/main/java/org/apache/wicket/protocol/http/servlet/MultipartServletWebRequestImpl.java b/wicket-core/src/main/java/org/apache/wicket/protocol/http/servlet/MultipartServletWebRequestImpl.java
index 9485550b96..1e8bbc54d4 100644
--- a/wicket-core/src/main/java/org/apache/wicket/protocol/http/servlet/MultipartServletWebRequestImpl.java
+++ b/wicket-core/src/main/java/org/apache/wicket/protocol/http/servlet/MultipartServletWebRequestImpl.java
@@ -33,6 +33,8 @@ import org.apache.commons.fileupload2.core.FileItem;
import org.apache.commons.fileupload2.core.FileItemFactory;
import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException;
import org.apache.commons.fileupload2.core.FileUploadException;
+import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException;
+import org.apache.commons.fileupload2.core.FileUploadSizeException;
import org.apache.commons.fileupload2.core.DiskFileItemFactory;
import org.apache.commons.fileupload2.jakarta.servlet5.JakartaServletFileUpload;
import org.apache.commons.fileupload2.jakarta.servlet5.JakartaServletRequestContext;
@@ -241,11 +243,25 @@ public class MultipartServletWebRequestImpl extends MultipartServletWebRequest
*
* <strong>Note</strong>: By using Servlet 3.0 APIs the application won't be able to use
* upload progress updates.
+ * <p>
+ * The container has already parsed these parts, so the limits {@link #newFileUpload(String)}
+ * hands to commons-fileupload never had a chance to apply to them. They are applied here instead,
+ * with the same semantics and the same exception types, so that a request arriving along this path
+ * is accepted or rejected exactly as it would have been had commons-fileupload done the parsing.
+ * The checks use {@link Part#getSize()} and so complete before any part is read, which matters
+ * because {@link #parseFileParts()} materialises every form field in memory.
+ * <p>
+ * The per-file and file-count limits are the ones this path did not apply before. The aggregate
+ * {@code maxSize} is also checked, though commons-fileupload has normally rejected an oversized
+ * request already by comparing {@code Content-Length} against it before reading the body; the
+ * check below covers a request that declares no length at all.
*
* @param request
* The http request with the upload data
* @return A list of {@link FileItem}s
* @throws FileUploadException
+ * if the parts exceed the maximum upload size, the maximum size of a single file, or
+ * the maximum number of files
*/
private List<FileItem> readServlet3Parts(HttpServletRequest request) throws FileUploadException
{
@@ -255,9 +271,42 @@ public class MultipartServletWebRequestImpl extends MultipartServletWebRequest
Collection<Part> parts = request.getParts();
if (parts != null)
{
+ final long fileCountMax = getFileCountMax();
+ final Bytes fileMaxSize = getFileMaxSize();
+ final long maxSize = getMaxSize().bytes();
+ long totalSize = 0;
+
for (Part part : parts)
{
+ // a negative fileCountMax means unlimited, as in AbstractFileUpload
+ if (fileCountMax >= 0 && itemsFromParts.size() >= fileCountMax)
+ {
+ throw new FileUploadFileCountLimitException(
+ String.format("Request '%s' failed: Maximum file count %,d exceeded.",
+ AbstractFileUpload.MULTIPART_FORM_DATA, fileCountMax),
+ fileCountMax, itemsFromParts.size());
+ }
+
FileItem fileItem = new ServletPartFileItem(part);
+ long size = fileItem.getSize();
+
+ // commons-fileupload applies this to every part, form fields included
+ if (fileMaxSize != null && size > fileMaxSize.bytes())
+ {
+ throw new FileUploadByteCountLimitException(
+ String.format("The field %s exceeds its maximum permitted size of %s bytes.",
+ fileItem.getFieldName(), fileMaxSize.bytes()),
+ size, fileMaxSize.bytes(), fileItem.getName(), fileItem.getFieldName());
+ }
+
+ totalSize += size;
+ if (totalSize > maxSize)
+ {
+ throw new FileUploadSizeException(String.format(
+ "the request was rejected because its size (%s) exceeds the configured maximum (%s)",
+ totalSize, maxSize), maxSize, totalSize);
+ }
+
itemsFromParts.add(fileItem);
}
}
@@ -478,27 +527,10 @@ public class MultipartServletWebRequestImpl extends MultipartServletWebRequest
public MultipartServletWebRequest newMultipartWebRequest(Bytes maxSize, String upload)
throws FileUploadException
{
- // FIXME mgrigorov: Why these checks are made here ?!
- // Why they are not done also at org.apache.wicket.protocol.http.servlet.MultipartServletWebRequestImpl.newMultipartWebRequest(org.apache.wicket.util.lang.Bytes, java.lang.String, org.apache.wicket.util.upload.FileItemFactory)() ?
- // Why there is no check that the summary of all files' sizes is less than the set maxSize ?
- // Setting a breakpoint here never breaks with the standard upload examples.
-
- Bytes fileMaxSize = getFileMaxSize();
- for (Map.Entry<String, List<FileItem>> entry : files.entrySet())
- {
- List<FileItem> fileItems = entry.getValue();
- for (FileItem fileItem : fileItems)
- {
- if (fileMaxSize != null && fileItem.getSize() > fileMaxSize.bytes())
- {
- String fieldName = entry.getKey();
- FileUploadException fslex = new FileUploadByteCountLimitException("The field '" +
- fieldName + "' exceeds its maximum permitted size of '" +
- maxSize + "' characters.", fileItem.getSize(), fileMaxSize.bytes(), fileItem.getName(), fieldName);
- throw fslex;
- }
- }
- }
+ // This request is already multipart, so there is nothing to create. The limits are enforced by
+ // parseFileParts(), on both the commons-fileupload and the Servlet parts path, which is after
+ // this method runs: Form#handleMultiPart() sets fileMaxSize and fileCountMax and only then
+ // parses.
return this;
}
(wicket) 05/05: Clarify two constants in the resource URL and upload limit tests
Posted by pa...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
papegaaij pushed a commit to branch wicket-10.x
in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 1597d6348e6c120e9ffbbdabc7aa52e6f2c8cd27
Author: Emond Papegaaij <pa...@apache.org>
AuthorDate: Wed Aug 19 13:36:51 2026 +0200
Clarify two constants in the resource URL and upload limit tests
PARENT_STEPS carried four lines explaining why the count is what it is.
One per sub-package, plus one for the style and one for the component,
says the same thing. The value is unchanged: the expression counts
package components, which is one more than the number of dots.
OVERSIZED said it was over every limit configured below it, which reads
oddly next to LARGE, a megabyte and a hundred times its size. LARGE is a
part size rather than a limit, so each now says which it is.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../markup/html/form/upload/FileUploadServletPartLimitsTest.java | 7 ++++---
.../request/resource/ResourceUrlAttributeValidationTest.java | 6 ++----
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/FileUploadServletPartLimitsTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/FileUploadServletPartLimitsTest.java
index 7016a7ba8a..f444dc5a3a 100644
--- a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/FileUploadServletPartLimitsTest.java
+++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/FileUploadServletPartLimitsTest.java
@@ -87,12 +87,13 @@ import org.junit.jupiter.api.Test;
*/
class FileUploadServletPartLimitsTest extends WicketTestCase
{
- /** Comfortably over every limit configured below. */
+ /** Comfortably over {@link #LIMIT}, the only limit these tests configure. */
private static final int OVERSIZED = 10_000;
/**
- * The size of the form field part. Large enough that buffering it would be unmistakably deliberate
- * rather than a rounding error, small enough to be harmless in a test.
+ * The size of the form field part. Not a limit, but a size at which buffering the part would be
+ * unmistakably deliberate rather than a rounding error; still small enough to be harmless in a
+ * test.
*/
private static final int LARGE = 1 << 20;
diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/request/resource/ResourceUrlAttributeValidationTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/request/resource/ResourceUrlAttributeValidationTest.java
index 5543281659..1e77e14b72 100644
--- a/wicket-core-tests/src/test/java/org/apache/wicket/request/resource/ResourceUrlAttributeValidationTest.java
+++ b/wicket-core-tests/src/test/java/org/apache/wicket/request/resource/ResourceUrlAttributeValidationTest.java
@@ -61,10 +61,8 @@ public class ResourceUrlAttributeValidationTest extends WicketTestCase
ResourceUrlAttributeValidationTest.class.getPackageName().replace('.', '/') + "/";
/**
- * How many {@code ../} the style needs to reach the root the finder resolves against. The style
- * is appended as {@code <package>/<name>_<style>}, so its first {@code ..} is glued onto
- * {@code PublicPage_} and forms a literal path component rather than a parent reference - hence
- * one extra, plus one to consume that component itself.
+ * For every sub-package we need to move up one directory with {@code ../}, plus one for the
+ * style and one for the component.
*/
private static final int PARENT_STEPS =
ResourceUrlAttributeValidationTest.class.getPackageName().split("\\.").length + 2;