This is an automated email from the ASF dual-hosted git repository. dashorst pushed a commit to branch wicket-8.x in repository https://gitbox.apache.org/repos/asf/wicket.git
commit b4fa931c64794c9a262d32dc84a5cf04210f1a0b 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 also writes 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]> --- .../src/main/java/org/apache/wicket/Component.java | 74 +++++++++- .../java/org/apache/wicket/IMarkupIdGenerator.java | 6 + .../html/form/upload/MultiFileUploadField.java | 4 +- .../apache/wicket/ComponentIdCharactersTest.java | 155 +++++++++++++++++++++ .../html/form/upload/MultiFileUploadFieldTest.java | 33 +++++ 5 files changed, 267 insertions(+), 5 deletions(-) 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 1fb6f65733..5ca9a1ed4e 100644 --- a/wicket-core/src/main/java/org/apache/wicket/Component.java +++ b/wicket-core/src/main/java/org/apache/wicket/Component.java @@ -398,6 +398,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} @@ -760,6 +766,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() { @@ -769,7 +778,7 @@ public abstract class Component String id = tag.getAttribute("id"); if (Strings.isEmpty(id) == false) { - return id.trim(); + return checkMarkupIdFromMarkup(id.trim()); } } @@ -1476,7 +1485,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) @@ -2795,7 +2804,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 */ @@ -2805,6 +2815,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) @@ -2849,7 +2864,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) @@ -4261,6 +4278,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 252b39cea2..85794b577b 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.fileupload.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-core/src/test/java/org/apache/wicket/ComponentIdCharactersTest.java b/wicket-core/src/test/java/org/apache/wicket/ComponentIdCharactersTest.java new file mode 100644 index 0000000000..f6ca8f3510 --- /dev/null +++ b/wicket-core/src/test/java/org/apache/wicket/ComponentIdCharactersTest.java @@ -0,0 +1,155 @@ +/* + * 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 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.Test; + +/** + * Tests the characters a component id and a markup id may contain. + */ +public 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 b", "a b" }; + + /** 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über1"; + + /** */ + @Test + public void componentIdMayContainOtherCharacters() + { + assertEquals(VALID_ID, new WebComponent(VALID_ID).getId()); + } + + /** */ + @Test + public void componentIdWithDisallowedCharactersIsRejected() + { + for (final String id : INVALID_IDS) + { + assertRejected(WicketRuntimeException.class, () -> new WebComponent(id), id); + } + assertRejected(WicketRuntimeException.class, () -> new WebComponent("a:b"), "a:b"); + assertRejected(WicketRuntimeException.class, () -> new WebComponent("a~b"), "a~b"); + } + + /** */ + @Test + public void markupIdMayContainOtherCharacters() + { + WebComponent component = new WebComponent("component"); + component.setMarkupId(VALID_ID); + + assertEquals(VALID_ID, component.getMarkupId()); + } + + /** */ + @Test + public void markupIdWithDisallowedCharactersIsRejected() + { + final WebComponent component = new WebComponent("component"); + for (final String id : INVALID_IDS) + { + assertRejected(IllegalArgumentException.class, () -> component.setMarkupId(id), id); + assertRejected(IllegalArgumentException.class, () -> component.setMarkupIdImpl(id), id); + } + } + + /** */ + @Test + public void markupIdFromTheMarkupIsChecked() + { + tester.startPage(new MarkupIdPage("a-b_c.d1 e&f")); + assertTrue(tester.getLastResponseAsString().contains("id=\"a-b_c.d1 e&f\"")); + + Throwable e = assertRejected(WicketRuntimeException.class, + () -> tester.startPage(new MarkupIdPage("a'b")), "a'b"); + assertCauseMentions(e, "The id attribute 'a'b'"); + } + + private static Throwable assertRejected(Class<? extends Throwable> expected, Runnable action, + String id) + { + try + { + action.run(); + } + catch (Throwable e) + { + assertTrue(id + ": " + e, expected.isInstance(e)); + return e; + } + fail(id + " was not rejected"); + return null; + } + + 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; + } + } + fail("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/src/test/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadFieldTest.java b/wicket-core/src/test/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadFieldTest.java index 139f41b3c6..94ad2896fe 100644 --- a/wicket-core/src/test/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadFieldTest.java +++ b/wicket-core/src/test/java/org/apache/wicket/markup/html/form/upload/MultiFileUploadFieldTest.java @@ -19,13 +19,16 @@ package org.apache.wicket.markup.html.form.upload; 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; @@ -104,6 +107,36 @@ public class MultiFileUploadFieldTest extends WicketTestCase tester.assertContains("disabled=\"disabled\""); } + /** + * The localized text of the delete button is written into a JavaScript string literal. + */ + @Test + public 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, response.contains("'l\\'\\\\item'")); + } + private static class TestPage extends WebPage implements IMarkupResourceStreamProvider { public TestPage(final ListModel<FileUpload> model)
