This is an automated email from the ASF dual-hosted git repository. dashorst pushed a commit to branch wicket-10.x in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 37d482aac8a1c2febff0ce104d03a612756cd6f7 Author: Emond Papegaaij <[email protected]> AuthorDate: Thu Sep 24 19:52:38 2026 +0200 Ask the package resource guard about the resource that was located The style, the variation and the locale are not part of the path handed to the resource stream locator: it appends them to the file name itself, before the extension, so the resource it opens is not always the one whose path the guard was asked about. A name that no class matches is accepted, and an appended attribute can complete it into the name of a component's markup template. Rebuild the path of the resource the locator returned, from the style, the variation and the locale it records on the stream, and ask the guard about that path as well. PackageResource.getResourceStream(), which decides whether a package resource exists at all and which the resource URL sanitizer and the resource reference registry both go through, did not consult the guard at all. Ask it there too, but only about a path the attributes have completed into a different one: a path the guard denies on its own account is denied when the resource is served anyway, and reporting such a resource as absent would change which resource references exist. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../wicket/markup/html/PackageResourceTest.java | 152 +++++++++++++++++ .../apache/wicket/markup/html/ReportPanel_nl.html | 3 + .../apache/wicket/markup/html/ReportPanel_nl.java | 33 ++++ .../apache/wicket/markup/html/ReportPanel_v2.html | 3 + .../apache/wicket/markup/html/ReportPanel_v2.java | 35 ++++ .../wicket/markup/html/ReportPanel_v2_ja.html | 3 + .../wicket/markup/html/nonComponent_snippet.html | 1 + .../wicket/request/resource/PackageResource.java | 187 ++++++++++++++++++++- 8 files changed, 410 insertions(+), 7 deletions(-) diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/PackageResourceTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/PackageResourceTest.java index 0a0c00f4f0..1203c71442 100644 --- a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/PackageResourceTest.java +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/PackageResourceTest.java @@ -25,11 +25,14 @@ import org.apache.wicket.request.resource.JavaScriptPackageResource; import org.apache.wicket.request.resource.PackageResource; import org.apache.wicket.request.resource.PackageResourceReference; import org.apache.wicket.request.resource.ResourceReference; +import org.apache.wicket.util.io.IOUtils; import org.apache.wicket.util.lang.Packages; import org.apache.wicket.util.tester.WicketTestCase; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import java.io.IOException; +import java.io.InputStream; import java.util.Locale; import static org.assertj.core.api.Assertions.assertThat; @@ -43,6 +46,15 @@ import static org.junit.jupiter.api.Assertions.*; */ public class PackageResourceTest extends WicketTestCase { + /** The package the ReportPanel fixtures live in. */ + private static final String PACKAGE = "/org/apache/wicket/markup/html/"; + + /** Only in {@code ReportPanel_v2.html}. */ + private static final String STYLED_MARKUP = "markup-of-report-panel-v2"; + + /** Only in {@code ReportPanel_v2_ja.html}. */ + private static final String LOCALIZED_MARKUP = "localized-markup-of-report-panel-v2"; + /** mock application object */ private WebApplication application; @@ -249,4 +261,144 @@ public class PackageResourceTest extends WicketTestCase PackageResource.PackageResourceBlockedException.class); } + /** + * The premise of every ReportPanel test below, asserted rather than assumed. + * <p> + * They all ask for {@code ReportPanel.html}, a name the guard accepts because nothing under + * that base name is a class, and rely on the style, the variation or the locale completing it + * into a name that is. Were a {@code ReportPanel} class ever added to this package, the guard + * would refuse the requested name outright, every one of those tests would keep passing on + * that older check alone, and the completed name they exist to cover would go untested with + * nothing to show it. + */ + @Test + void reportPanelFixturesAreShapedAsTheTestsBelowAssume() throws IOException + { + assertNull(getClass().getResource(PACKAGE + "ReportPanel.class"), + "a ReportPanel class makes the guard refuse ReportPanel.html on the requested name, " + + "so the tests below would no longer cover the name the attributes complete"); + assertNull(getClass().getResource(PACKAGE + "ReportPanel.html"), + "ReportPanel.html must not exist, or the locator would resolve it instead of the " + + "name the attributes complete"); + + // the classes whose markup the attributes reach, and which the guard has to find + assertNotNull(getClass().getResource(PACKAGE + "ReportPanel_v2.class")); + assertNotNull(getClass().getResource(PACKAGE + "ReportPanel_nl.class")); + + // their markup, carrying the markers the request-cycle tests look for in the response + assertThat(markupOf("ReportPanel_v2.html")).contains(STYLED_MARKUP); + assertThat(markupOf("ReportPanel_v2_ja.html")).contains(LOCALIZED_MARKUP); + + // and the static html that has to stay servable, with no class under any prefix of it + assertNotNull(getClass().getResource(PACKAGE + "nonComponent_snippet.html")); + assertNull(getClass().getResource(PACKAGE + "nonComponent_snippet.class")); + assertNull(getClass().getResource(PACKAGE + "nonComponent.class")); + } + + private String markupOf(String name) throws IOException + { + try (InputStream markup = getClass().getResourceAsStream(PACKAGE + name)) + { + assertNotNull(markup, name + " is missing from the test classpath"); + + return IOUtils.toString(markup, "UTF-8"); + } + } + + /** + * The style, the variation and the locale are appended to the name before the extension, so a + * name that is not a markup file itself can be completed into one. There is no + * {@code ReportPanel} class, so the name that is asked for is accepted, while + * {@link ReportPanel_v2} and {@link ReportPanel_nl} are components whose markup must not be + * served. + */ + @Test + void dontGetResourceStreamWhenStyleCompletesTheName() + { + PackageResource resource = new PackageResourceReference(PackageResourceTest.class, + "ReportPanel.html", null, "v2", null).getResource(); + assertThatThrownBy(resource::getResourceStream).isInstanceOf( + PackageResource.PackageResourceBlockedException.class); + } + + @Test + void dontGetResourceStreamWhenVariationCompletesTheName() + { + PackageResource resource = new PackageResourceReference(PackageResourceTest.class, + "ReportPanel.html", null, null, "v2").getResource(); + assertThatThrownBy(resource::getResourceStream).isInstanceOf( + PackageResource.PackageResourceBlockedException.class); + } + + @Test + void dontGetResourceStreamWhenLocaleCompletesTheName() + { + PackageResource resource = new PackageResourceReference(PackageResourceTest.class, + "ReportPanel.html", new Locale("nl"), null, null).getResource(); + assertThatThrownBy(resource::getResourceStream).isInstanceOf( + PackageResource.PackageResourceBlockedException.class); + } + + /** + * A style and a locale together, reaching the localized markup of the same component: + * {@code ReportPanel.html} plus the style {@code v2} and the locale {@code ja} completes into + * {@code ReportPanel_v2_ja.html}, which is {@link ReportPanel_v2}'s Japanese template. The + * guard's underscore loop has to walk back two suffixes to find the class. + */ + @Test + void dontGetResourceStreamWhenStyleAndLocaleCompleteTheName() + { + PackageResource resource = new PackageResourceReference(PackageResourceTest.class, + "ReportPanel.html", new Locale("ja"), "v2", null).getResource(); + assertThatThrownBy(resource::getResourceStream).isInstanceOf( + PackageResource.PackageResourceBlockedException.class); + } + + @Test + void componentMarkupIsNotServedThroughAStyledAndLocalizedUrl() + { + tester.executeUrl("wicket/resource/" + PackageResourceTest.class.getName() + + "/ReportPanel.html?ja-v2"); + + assertThat(tester.getLastResponseAsString()) + .doesNotContain(LOCALIZED_MARKUP); + } + + /** + * A resource that a style completes into a component's markup does not exist as a package + * resource, so no resource reference is created for it either. + */ + @Test + void dontExistWhenStyleCompletesTheName() + { + assertFalse(PackageResource.exists(PackageResourceTest.class, "ReportPanel.html", null, + "v2", null)); + } + + /** + * The whole request cycle, with the style in the name of the first query parameter, the way a + * client sends it. + */ + @Test + void componentMarkupIsNotServedThroughAStyledUrl() + { + tester.executeUrl("wicket/resource/" + PackageResourceTest.class.getName() + + "/ReportPanel.html?-v2"); + + assertThat(tester.getLastResponseAsString()) + .doesNotContain(STYLED_MARKUP); + } + + /** + * An underscore in the name of an html package resource is not by itself a reason to refuse + * it: only a name for which a class exists is a markup template. + */ + @Test + void getResourceStreamForStaticMarkupWithAnUnderscore() + { + PackageResource resource = new PackageResourceReference(PackageResourceTest.class, + "nonComponent_snippet.html").getResource(); + assertThat(resource.getResourceStream()).isNotNull(); + } + } diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_nl.html b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_nl.html new file mode 100644 index 0000000000..1bd5f73f61 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_nl.html @@ -0,0 +1,3 @@ +<wicket:panel> + <span>markup-of-report-panel-nl</span> +</wicket:panel> diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_nl.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_nl.java new file mode 100644 index 0000000000..1a9059e614 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_nl.java @@ -0,0 +1,33 @@ +/* + * 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; + +import org.apache.wicket.markup.html.panel.Panel; + +/** + * The same as {@link ReportPanel_v2}, for the locale rather than the style: its markup template + * must not be reachable by asking for {@code ReportPanel.html} with the locale {@code nl}. + */ +public class ReportPanel_nl extends Panel +{ + private static final long serialVersionUID = 1L; + + public ReportPanel_nl(String id) + { + super(id); + } +} diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_v2.html b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_v2.html new file mode 100644 index 0000000000..4eb394a292 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_v2.html @@ -0,0 +1,3 @@ +<wicket:panel> + <span>markup-of-report-panel-v2</span> +</wicket:panel> diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_v2.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_v2.java new file mode 100644 index 0000000000..77dd2494e1 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_v2.java @@ -0,0 +1,35 @@ +/* + * 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; + +import org.apache.wicket.markup.html.panel.Panel; + +/** + * A component whose simple name contains an underscore. Its markup template must not be reachable + * as a package resource, not even through a style or a variation that completes + * {@code ReportPanel.html} into {@code ReportPanel_v2.html}. There is deliberately no + * {@code ReportPanel} class, so the base name is not a markup file itself. + */ +public class ReportPanel_v2 extends Panel +{ + private static final long serialVersionUID = 1L; + + public ReportPanel_v2(String id) + { + super(id); + } +} diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_v2_ja.html b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_v2_ja.html new file mode 100644 index 0000000000..e6717ee757 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/ReportPanel_v2_ja.html @@ -0,0 +1,3 @@ +<wicket:panel> + <span>localized-markup-of-report-panel-v2</span> +</wicket:panel> diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/nonComponent_snippet.html b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/nonComponent_snippet.html new file mode 100644 index 0000000000..a00eaabbfc --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/nonComponent_snippet.html @@ -0,0 +1 @@ +<span>a static html package resource whose name contains an underscore</span> diff --git a/wicket-core/src/main/java/org/apache/wicket/request/resource/PackageResource.java b/wicket-core/src/main/java/org/apache/wicket/request/resource/PackageResource.java index f50d8880d2..13da1b1c9f 100644 --- a/wicket-core/src/main/java/org/apache/wicket/request/resource/PackageResource.java +++ b/wicket-core/src/main/java/org/apache/wicket/request/resource/PackageResource.java @@ -40,6 +40,7 @@ import org.apache.wicket.util.resource.IFixedLocationResourceStream; import org.apache.wicket.util.resource.IResourceStream; import org.apache.wicket.util.resource.ResourceStreamNotFoundException; import org.apache.wicket.util.resource.ResourceStreamWrapper; +import org.apache.wicket.util.resource.ResourceUtils; import org.apache.wicket.util.string.Strings; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -270,11 +271,9 @@ public class PackageResource extends AbstractResource implements IStaticCacheabl String currentStyle = getCurrentStyle(); Locale currentLocale = getCurrentLocale(); - IResourceStream packageResource = Application.get() - .getResourceSettings() - .getResourceStreamLocator() - .locate(scope, absolutePath, currentStyle, variation, currentLocale, null, false); - + IResourceStream packageResource = getResourceStream(scope, name, currentLocale, + currentStyle, variation, true); + // if resource stream can not be found do not cache if (packageResource != null) { @@ -570,6 +569,17 @@ public class PackageResource extends AbstractResource implements IStaticCacheabl if (resourceStream != null) { + String locatedPath = locatedPath(absolutePath, resourceStream); + + // absolutePath was accepted above, so denying the located path is the mismatch that + // #acceptsLocatedPath(String, IResourceStream) tests on the existence path + if (!locatedPath.equals(absolutePath) && !accept(locatedPath)) + { + throw new PackageResourceBlockedException("Access denied to (static) package resource " + + absolutePath + ", which the style, variation and locale completed into " + + locatedPath + ". See IPackageResourceGuard"); + } + resourceStream = new ProcessingResourceStream(resourceStream); } return resourceStream; @@ -637,6 +647,11 @@ public class PackageResource extends AbstractResource implements IStaticCacheabl * * By default IPackageResourceGuard is used to check the permissions but the resource itself can * also make the check. + * <p> + * An override is only consulted while the resource is being served. The static + * {@link #getResourceStream(Class, String, Locale, String, String, boolean)}, which decides + * whether a package resource exists at all, asks the configured IPackageResourceGuard + * directly, and only about a path the style, the variation or the locale has completed. * * @param path * resource path @@ -687,19 +702,177 @@ public class PackageResource extends AbstractResource implements IStaticCacheabl return getResourceStream(scope, path, locale, style, variation, true) != null; } + /** + * Locates the resource stream for the given set of criteria, or {@code null} when there is no + * package resource that may be served for them. + * + * @param scope + * This argument will be used to get the class loader for loading the package + * resource, and to determine what package it is in + * @param path + * The path to the resource + * @param locale + * The locale of the resource + * @param style + * The style of the resource (see {@link org.apache.wicket.Session}) + * @param variation + * The component's variation (of the style) + * @param updateCache + * Whether the resource stream locator may cache the result + * @return the resource stream, or {@code null} if there is none, or if the style, the + * variation and the locale completed the path into a resource the configured + * {@link IPackageResourceGuard} denies + */ public static IResourceStream getResourceStream(final Class<?> scope, final String path, final Locale locale, final String style, final String variation, final boolean updateCache) { String absolutePath = Packages.absolutePath(scope, path); IResourceStreamLocator resourceStreamLocator = Application.get().getResourceSettings() .getResourceStreamLocator(); + IResourceStream resourceStream; if (resourceStreamLocator instanceof CachingResourceStreamLocator cache) { - return cache.locate(scope, absolutePath, style, variation, locale, null, false, updateCache); + resourceStream = cache.locate(scope, absolutePath, style, variation, locale, null, false, updateCache); } else { - return resourceStreamLocator.locate(scope, absolutePath, style, variation, locale, null, false); + resourceStream = resourceStreamLocator.locate(scope, absolutePath, style, variation, locale, null, false); + } + + if (resourceStream != null && !acceptsLocatedPath(absolutePath, resourceStream)) + { + return null; + } + + return resourceStream; + } + + /** + * Whether the resource that was located may be served, given that the path the locator was + * asked about may be. + * <p> + * A path the guard denies on its own account is not this method's concern: it is denied when + * the resource is served, whichever answer is given here, and reporting it as absent would + * change which resource references exist. What this method is for is the path the style, the + * variation and the locale have completed into a different one, which the guard was never + * asked about. + * <p> + * Not to be confused with {@link #accept(String)}, which asks the guard about a single given + * path. The serving path applies this same rule through that method: there + * {@code absolutePath} has been accepted already, so denying the located path on its own is + * the same mismatch. + * + * @param absolutePath + * the path the locator was asked for + * @param stream + * the stream the locator returned + * @return {@code false} only if the guard accepts {@code absolutePath} but denies the resource + * that was located + */ + private static boolean acceptsLocatedPath(final String absolutePath, final IResourceStream stream) + { + String locatedPath = locatedPath(absolutePath, stream); + + if (locatedPath.equals(absolutePath)) + { + return true; + } + + IPackageResourceGuard guard = Application.get() + .getResourceSettings() + .getPackageResourceGuard(); + + return !guard.accept(absolutePath) || guard.accept(locatedPath); + } + + /** + * Rebuilds the path of the resource a resource stream locator has located. + * <p> + * The style, variation and locale are not part of the path the locator is given: it appends + * them itself, as {@code <path>_<variation>_<style>_<locale>} before the extension, so the + * resource it opens is not the resource the path it was given names. The located stream carries + * the three values that were used, and + * {@link org.apache.wicket.resource.ResourceUtil#rejectPathSeparators(String, String)} + * guarantees that none of them can contribute a path component of its own, so only the file + * name can differ. + * + * @param absolutePath + * the path the locator was asked for + * @param stream + * the stream the locator returned + * @return the path of the located resource, or {@code absolutePath} itself when the locator + * appended nothing to it + */ + private static String locatedPath(final String absolutePath, final IResourceStream stream) + { + final String variation = stream.getVariation(); + final String style = stream.getStyle(); + final Locale locale = stream.getLocale(); + + // A locale in the file name itself replaces the one the locator was given, and + // ResourceUtils only recognizes one that an underscore introduces. So a file name without + // an underscore, located with none of the three attributes, is the path itself. + final boolean mayCarryLocale = + absolutePath.indexOf('_', absolutePath.lastIndexOf('/') + 1) != -1; + + if (variation == null && style == null && locale == null && !mayCarryLocale) + { + return absolutePath; + } + + String path = absolutePath; + if (mayCarryLocale) + { + ResourceUtils.PathLocale pathLocale = ResourceUtils.getLocaleFromFilename(absolutePath); + if (pathLocale != null && pathLocale.locale != null) + { + path = pathLocale.path; + } + } + + final String localeName = locale != null ? locale.toString() : null; + + // the extension is split off and re-appended where the locator splits and re-appends it + final int ixExtension = path.lastIndexOf('.'); + final int ixInsert = ixExtension != -1 ? ixExtension : path.length(); + + final StringBuilder locatedPath = new StringBuilder(path.length() + + appendedLength(variation) + appendedLength(style) + appendedLength(localeName)); + + locatedPath.append(path, 0, ixInsert); + append(locatedPath, variation); + append(locatedPath, style); + append(locatedPath, localeName); + locatedPath.append(path, ixInsert, path.length()); + + return locatedPath.toString(); + } + + /** + * @param attribute + * a style, variation or locale, may be {@code null} + * @return the number of characters {@link #append(StringBuilder, String)} adds for it + */ + private static int appendedLength(final String attribute) + { + return attribute != null ? attribute.length() + 1 : 0; + } + + /** + * Appends an attribute preceded by an underscore, the way + * {@link org.apache.wicket.core.util.resource.locator.ResourceNameIterator} does, and nothing + * at all when the attribute is {@code null}. + * + * @param path + * the path being built + * @param attribute + * a style, variation or locale, may be {@code null} + */ + private static void append(final StringBuilder path, final String attribute) + { + if (attribute != null) + { + path.append('_').append(attribute); } }
