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 e629876c593ed52cf83939189acfab78c0a43b6b Author: Emond Papegaaij <[email protected]> AuthorDate: Fri Oct 2 13:36:07 2026 +0200 Restrict the characters a component id and a markup id may contain Wicket writes a component's markup id into the JavaScript it generates, and its component id as part of an input name, inside string literals that a quote or a backslash in the id would end. Rather than escaping every place that writes one, the ids are checked where they enter: a component id when the component is constructed, a markup id in setMarkupIdImpl, which setMarkupId calls, and the id attribute in the markup when it is read. A generated markup id is built from the component id; IMarkupIdGenerator now states which characters a custom generator may use. Quotes, backslashes, brackets, ';' and control characters are rejected; all other characters are still allowed. MultiFileUploadField and TrapFocusBehavior also write a text from the resource bundles into such a literal; it is now escaped with JavaScriptUtils#escapeQuotes. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- .../apache/wicket/ComponentIdCharactersTest.java | 138 +++++++++++++++++++++ .../html/form/upload/MultiFileUploadFieldTest.java | 34 +++++ .../src/main/java/org/apache/wicket/Component.java | 74 ++++++++++- .../java/org/apache/wicket/IMarkupIdGenerator.java | 6 + .../html/form/upload/MultiFileUploadField.java | 4 +- .../ajax/markup/html/modal/TrapFocusBehavior.java | 4 +- .../markup/html/modal/TrapFocusBehaviorTest.java | 86 +++++++++++++ .../markup/html/repeater/AjaxListPanelTest.java | 33 ++++- 8 files changed, 369 insertions(+), 10 deletions(-) diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/ComponentIdCharactersTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/ComponentIdCharactersTest.java new file mode 100644 index 0000000000..500359cf2e --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/ComponentIdCharactersTest.java @@ -0,0 +1,138 @@ +/* + * 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; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.apache.wicket.markup.IMarkupCacheKeyProvider; +import org.apache.wicket.markup.IMarkupResourceStreamProvider; +import org.apache.wicket.markup.html.WebComponent; +import org.apache.wicket.markup.html.WebPage; +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; + +/** + * Tests the characters a component id and a markup id may contain. + */ +class ComponentIdCharactersTest extends WicketTestCase +{ + private static final String[] INVALID_IDS = { "a'b", "a\"b", "a`b", "a\\b", "a;b", "a(b", + "a)b", "a[b", "a]b", "a{b", "a}b", "a<b", "a>b", "a\nb", "a\rb", "a\tb", "a\u0000b", + "a\u2028b", "a\u2029b" }; + + /** Characters that cannot end a JavaScript string literal are still allowed */ + private static final String VALID_ID = "a b&c=d/e+f,g.h-i_j*k!l\u00fcber1"; + + @Test + void componentIdMayContainOtherCharacters() + { + assertEquals(VALID_ID, new WebComponent(VALID_ID).getId()); + } + + @Test + void componentIdWithDisallowedCharactersIsRejected() + { + for (String id : INVALID_IDS) + { + assertThrows(WicketRuntimeException.class, () -> new WebComponent(id), id); + } + assertThrows(WicketRuntimeException.class, () -> new WebComponent("a:b")); + assertThrows(WicketRuntimeException.class, () -> new WebComponent("a~b")); + } + + @Test + void markupIdMayContainOtherCharacters() + { + WebComponent component = new WebComponent("component"); + component.setMarkupId(VALID_ID); + + assertEquals(VALID_ID, component.getMarkupId()); + } + + @Test + void markupIdWithDisallowedCharactersIsRejected() + { + WebComponent component = new WebComponent("component"); + for (String id : INVALID_IDS) + { + assertThrows(IllegalArgumentException.class, () -> component.setMarkupId(id), id); + assertThrows(IllegalArgumentException.class, () -> component.setMarkupIdImpl(id), id); + } + } + + @Test + void markupIdFromTheMarkupIsChecked() + { + tester.startPage(new MarkupIdPage("a-b_c.d1 e&f")); + assertTrue(tester.getLastResponseAsString().contains("id=\"a-b_c.d1 e&f\"")); + + WicketRuntimeException e = assertThrows(WicketRuntimeException.class, + () -> tester.startPage(new MarkupIdPage("a'b"))); + assertCauseMentions(e, "The id attribute 'a'b'"); + } + + private static void assertCauseMentions(Throwable e, String text) + { + for (Throwable cause = e; cause != null; cause = cause.getCause()) + { + if (cause.getMessage() != null && cause.getMessage().contains(text)) + { + return; + } + } + assertTrue(false, "no cause mentions " + text + ": " + e); + } + + /** + * A page whose markup gives its component an id attribute. + */ + public static class MarkupIdPage extends WebPage + implements + IMarkupResourceStreamProvider, + IMarkupCacheKeyProvider + { + private static final long serialVersionUID = 1L; + + private final String markupId; + + MarkupIdPage(String markupId) + { + this.markupId = markupId; + add(new WebComponent("component").setOutputMarkupId(true)); + } + + @Override + public IResourceStream getMarkupResourceStream(MarkupContainer container, + Class<?> containerClass) + { + return new StringResourceStream( + "<html><body><span wicket:id=\"component\" id=\"" + markupId + + "\"></span></body></html>"); + } + + @Override + public String getCacheKey(MarkupContainer container, Class<?> containerClass) + { + // the markup differs per instance + return null; + } + } +} diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadFieldTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadFieldTest.java index 94d4f062bd..93d5c06526 100644 --- a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadFieldTest.java +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadFieldTest.java @@ -17,18 +17,22 @@ package org.apache.wicket.markup.html.form.upload; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import java.io.IOException; import java.util.ArrayList; import java.util.List; +import java.util.Locale; import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.wicket.Component; import org.apache.wicket.MarkupContainer; import org.apache.wicket.markup.IMarkupResourceStreamProvider; import org.apache.wicket.markup.html.WebPage; import org.apache.wicket.markup.html.form.Form; import org.apache.wicket.model.util.ListModel; +import org.apache.wicket.resource.loader.IStringResourceLoader; import org.apache.wicket.util.file.File; import org.apache.wicket.util.io.IOUtils; import org.apache.wicket.util.resource.IResourceStream; @@ -108,6 +112,36 @@ class MultiFileUploadFieldTest extends WicketTestCase tester.assertContains("disabled=\"disabled\""); } + /** + * The localized text of the delete button is written into a JavaScript string literal. + */ + @Test + void deleteMessageIsEscapedForJavaScript() + { + tester.getApplication().getResourceSettings().getStringResourceLoaders().add(0, + new IStringResourceLoader() + { + @Override + public String loadStringResource(Class<?> clazz, String key, Locale locale, + String style, String variation) + { + return "org.apache.wicket.mfu.delete".equals(key) ? "l'\\item" : null; + } + + @Override + public String loadStringResource(Component component, String key, + Locale locale, String style, String variation) + { + return loadStringResource((Class<?>)null, key, locale, style, variation); + } + }); + + tester.startPage(new TestPage(new ListModel<>(new ArrayList<FileUpload>()))); + + String response = tester.getLastResponseAsString(); + assertTrue(response.contains("'l\\'\\\\item'"), response); + } + private static class TestPage extends WebPage implements IMarkupResourceStreamProvider { TestPage(final ListModel<FileUpload> model) diff --git a/wicket-core/src/main/java/org/apache/wicket/Component.java b/wicket-core/src/main/java/org/apache/wicket/Component.java index 75736dd2d1..326544557a 100644 --- a/wicket-core/src/main/java/org/apache/wicket/Component.java +++ b/wicket-core/src/main/java/org/apache/wicket/Component.java @@ -400,6 +400,12 @@ public abstract class Component */ private static final String MARKUP_ID_ATTR_NAME = "id"; + /** Characters a component id or a markup id must not contain, see {@link #isValidId(String)} */ + private static final String INVALID_ID_CHARS = "'\"`\\()[]{}<>;"; + + private static final String INVALID_ID_MESSAGE = + "must not contain quotes, backslashes, brackets, ';' or control characters."; + /** * Meta data key for line precise error logging for the moment of addition. Made package private * for access in {@link MarkupContainer} and {@link Page} @@ -762,6 +768,9 @@ public abstract class Component /** * @return The 'id' attribute from the associated markup tag + * @throws WicketRuntimeException + * if the id contains a quote, a backslash, a bracket, {@code ';'} or a control + * character */ public final String getMarkupIdFromMarkup() { @@ -771,7 +780,7 @@ public abstract class Component String id = tag.getAttribute("id"); if (Strings.isEmpty(id) == false) { - return id.trim(); + return checkMarkupIdFromMarkup(id.trim()); } } @@ -1480,7 +1489,7 @@ public abstract class Component * @param createIfDoesNotExist * When there is no existing markup id, determines whether it should be generated or * whether <code>null</code> should be returned. - * + * * @return markup id of the component */ public String getMarkupId(boolean createIfDoesNotExist) @@ -2806,7 +2815,8 @@ public abstract class Component } /** - * Set markup ID, which must be String or Integer + * Set markup ID, which must be String or Integer. A String must not contain quotes, + * backslashes, brackets, {@code ';'} or control characters. * * @param markupId */ @@ -2816,6 +2826,11 @@ public abstract class Component { throw new IllegalArgumentException("markupId must be String or Integer"); } + if (markupId instanceof String && !isValidId((String)markupId)) + { + throw new IllegalArgumentException( + "The markup id '" + markupId + "' " + INVALID_ID_MESSAGE); + } setOutputMarkupId(true); if (markupId instanceof Integer) @@ -2860,7 +2875,9 @@ public abstract class Component * @see #getMarkupId() * * @param markupId - * markup id value or null to clear any previous user defined value + * markup id value or null to clear any previous user defined value. It must not + * contain quotes, backslashes, brackets, {@code ';'} or control characters, because + * Wicket writes it into the JavaScript it generates. * @return this for chaining */ public Component setMarkupId(String markupId) @@ -4274,6 +4291,55 @@ public abstract class Component { throw new WicketRuntimeException("The component ID must not contain ':' or '~' chars."); } + + if ((id != null) && !isValidId(id)) + { + throw new WicketRuntimeException("The component ID '" + id + "' " + INVALID_ID_MESSAGE); + } + } + + /** + * Checks the characters of a component id or a markup id. Wicket writes both into the + * JavaScript it generates, the markup id as it is and the component id as part of an input name, + * so characters that can end a JavaScript string literal or statement are not allowed: quotes, + * backslashes, brackets, {@code ';'} and control characters, line breaks included. + * + * @param id + * the id to check + * @return {@code true} if no character of the id is disallowed + */ + private static boolean isValidId(final String id) + { + for (int i = 0; i < id.length(); i++) + { + char c = id.charAt(i); + if (INVALID_ID_CHARS.indexOf(c) != -1 || Character.isISOControl(c) || c == '\u2028' || + c == '\u2029') + { + return false; + } + } + return true; + } + + /** + * Checks the characters of the id attribute in the markup of this component. + * + * @param markupId + * the id attribute + * @return the id attribute + * @throws WicketRuntimeException + * if the id attribute contains a character that is not allowed + * @see #isValidId(String) + */ + private String checkMarkupIdFromMarkup(final String markupId) + { + if (!isValidId(markupId)) + { + throw new WicketRuntimeException("The id attribute '" + markupId + + "' in the markup of " + this + " " + INVALID_ID_MESSAGE); + } + return markupId; } /** diff --git a/wicket-core/src/main/java/org/apache/wicket/IMarkupIdGenerator.java b/wicket-core/src/main/java/org/apache/wicket/IMarkupIdGenerator.java index f1cbf0f93c..da72b3aa9f 100644 --- a/wicket-core/src/main/java/org/apache/wicket/IMarkupIdGenerator.java +++ b/wicket-core/src/main/java/org/apache/wicket/IMarkupIdGenerator.java @@ -24,6 +24,12 @@ public interface IMarkupIdGenerator { /** * Generates markup id for the given component + * <p> + * Wicket writes the markup id into the JavaScript it generates, without escaping it. A + * generated markup id must therefore not contain quotes, backslashes, brackets, {@code ';'} or + * control characters. An id the component already has, from + * {@link Component#setMarkupId(String)} or from the id attribute in its markup, has been + * checked already. * * @param component * The component for which to generate a markup id diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadField.java b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadField.java index a8c278bcfc..6bbc670144 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadField.java +++ b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadField.java @@ -24,6 +24,7 @@ import java.util.Map; import java.util.Map.Entry; import org.apache.commons.fileupload2.core.FileItem; +import org.apache.wicket.core.util.string.JavaScriptUtils; import org.apache.wicket.markup.ComponentTag; import org.apache.wicket.markup.head.IHeaderResponse; import org.apache.wicket.markup.head.JavaScriptHeaderItem; @@ -237,7 +238,8 @@ public class MultiFileUploadField extends FormComponentPanel<Collection<FileUplo response.render(JavaScriptHeaderItem.forReference(JS)); response.render(OnDomReadyHeaderItem.forScript("new MultiSelector('" + getInputName() + "', document.getElementById('" + container.getMarkupId() + "'), " + max + ", " + - useMultipleAttr + ", '" + getString("org.apache.wicket.mfu.delete") + + useMultipleAttr + ", '" + + JavaScriptUtils.escapeQuotes(getString("org.apache.wicket.mfu.delete")) + "').addElement(document.getElementById('" + upload.getMarkupId() + "'));")); } diff --git a/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/TrapFocusBehavior.java b/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/TrapFocusBehavior.java index 07211193a2..776e85c0e2 100644 --- a/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/TrapFocusBehavior.java +++ b/wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/TrapFocusBehavior.java @@ -19,6 +19,7 @@ package org.apache.wicket.extensions.ajax.markup.html.modal; import org.apache.wicket.Component; import org.apache.wicket.behavior.Behavior; import org.apache.wicket.core.util.string.CssUtils; +import org.apache.wicket.core.util.string.JavaScriptUtils; import org.apache.wicket.markup.head.IHeaderResponse; import org.apache.wicket.markup.head.JavaScriptHeaderItem; import org.apache.wicket.markup.head.OnDomReadyHeaderItem; @@ -51,7 +52,8 @@ public class TrapFocusBehavior extends Behavior String styleClass = component.getString(CSS_CURRENT_KEY, null, "current-focus-trap"); - CharSequence script = String.format("Wicket.trapFocus('%s', '%s');", component.getMarkupId(), styleClass); + CharSequence script = String.format("Wicket.trapFocus('%s', '%s');", component.getMarkupId(), + JavaScriptUtils.escapeQuotes(styleClass)); response.render(new PriorityHeaderItem(OnDomReadyHeaderItem.forScript(script))); } diff --git a/wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/modal/TrapFocusBehaviorTest.java b/wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/modal/TrapFocusBehaviorTest.java new file mode 100644 index 0000000000..1fbe6650f6 --- /dev/null +++ b/wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/modal/TrapFocusBehaviorTest.java @@ -0,0 +1,86 @@ +/* + * 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.modal; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Locale; + +import org.apache.wicket.Component; +import org.apache.wicket.MarkupContainer; +import org.apache.wicket.markup.IMarkupResourceStreamProvider; +import org.apache.wicket.markup.html.WebMarkupContainer; +import org.apache.wicket.markup.html.WebPage; +import org.apache.wicket.resource.loader.IStringResourceLoader; +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; + +/** + * Tests for {@link TrapFocusBehavior}. + */ +class TrapFocusBehaviorTest extends WicketTestCase +{ + /** + * The style class from the resource bundle is written into a JavaScript string literal. + */ + @Test + void styleClassIsEscapedForJavaScript() + { + tester.getApplication().getResourceSettings().getStringResourceLoaders().add(0, + new IStringResourceLoader() + { + @Override + public String loadStringResource(Class<?> clazz, String key, Locale locale, + String style, String variation) + { + return TrapFocusBehavior.CSS_CURRENT_KEY.equals(key) ? "a'\\b" : null; + } + + @Override + public String loadStringResource(Component component, String key, + Locale locale, String style, String variation) + { + return loadStringResource((Class<?>)null, key, locale, style, variation); + } + }); + + tester.startPage(new TestPage()); + + String response = tester.getLastResponseAsString(); + assertTrue(response.contains("'a\\'\\\\b');"), response); + } + + private static class TestPage extends WebPage implements IMarkupResourceStreamProvider + { + private static final long serialVersionUID = 1L; + + TestPage() + { + add(new WebMarkupContainer("container").add(new TrapFocusBehavior())); + } + + @Override + public IResourceStream getMarkupResourceStream(MarkupContainer container, + Class<?> containerClass) + { + return new StringResourceStream( + "<html><head></head><body><div wicket:id=\"container\"></div></body></html>"); + } + } +} diff --git a/wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/repeater/AjaxListPanelTest.java b/wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/repeater/AjaxListPanelTest.java index a8cbabf17a..0d80163fb9 100644 --- a/wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/repeater/AjaxListPanelTest.java +++ b/wicket-extensions/src/test/java/org/apache/wicket/extensions/ajax/markup/html/repeater/AjaxListPanelTest.java @@ -18,9 +18,13 @@ package org.apache.wicket.extensions.ajax.markup.html.repeater; import static org.junit.jupiter.api.Assertions.assertTrue; +import org.apache.wicket.MarkupContainer; import org.apache.wicket.ajax.AjaxEventBehavior; import org.apache.wicket.ajax.AjaxRequestTarget; +import org.apache.wicket.markup.IMarkupResourceStreamProvider; import org.apache.wicket.markup.html.basic.Label; +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; @@ -70,16 +74,13 @@ class AjaxListPanelTest extends WicketTestCase @Test void appendedMarkupStaysInsideItsJavaScriptStringLiteral() { - final String backslashBeforeQuote = "a\\';b"; - - final AjaxListPanel list = new AjaxListPanel("list"); + final AjaxListPanel list = new BackslashBeforeQuotePanel("list"); AjaxEventBehavior behavior = new AjaxEventBehavior("click") { @Override protected void onEvent(AjaxRequestTarget target) { Label label = new Label(list.newChildId()); - label.setMarkupId(backslashBeforeQuote); list.append(label, target); } }; @@ -92,4 +93,28 @@ class AjaxListPanelTest extends WicketTestCase String response = tester.getLastResponseAsString(); assertTrue(response.contains("a\\\\';b"), "no markup was appended"); } + + /** + * A list panel whose repeater tag carries a backslash before a quote, which ends up in the + * appended markup. + */ + private static class BackslashBeforeQuotePanel extends AjaxListPanel + implements + IMarkupResourceStreamProvider + { + private static final long serialVersionUID = 1L; + + BackslashBeforeQuotePanel(String id) + { + super(id); + } + + @Override + public IResourceStream getMarkupResourceStream(MarkupContainer container, + Class<?> containerClass) + { + return new StringResourceStream("<wicket:panel><div wicket:id=\"container\">" + + "<div wicket:id=\"repeater\" title=\"a\\';b\"></div></div></wicket:panel>"); + } + } }
