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 ddb0325f20e0c243161727296f241765ac0bbfdf Author: Emond Papegaaij <[email protected]> AuthorDate: Wed Aug 19 22:25:56 2026 +0200 Escape the body of the default option of a single select choice getDefaultChoice wrote getNullValidDisplayValue() and getNullKeyDisplayValue() into the body of the option as they came, while renderValue escapes the body of every other option in the same select according to escapeModelStrings. Both methods are protected, so what they hand back is not necessarily the plain text the default implementation reads from a bundle; a subclass that shows the label of the field instead returns the label model. The value is escaped where the option is written, and through escapeOptionHtml, so the whole select follows one rule and a choice that gave itself special escaping of its options gets it for the default option too. It is deliberately not escaped inside the two display value methods. A subclass may take what super returns and compare it against a resource it looks up itself, to tell whether the value is still the general one; escaping there would compare an escaped value against an unescaped one and quietly stop matching. The shipped bundles hold nullValid= and null=Choose One, both plain text, so nothing changes for them. An application whose own nullValid or null entry holds markup will see it escaped, and can still turn escaping off. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../form/AbstractSingleSelectChoiceEscapeTest.java | 191 +++++++++++++++++++++ .../html/form/AbstractSingleSelectChoice.java | 21 ++- 2 files changed, 210 insertions(+), 2 deletions(-) diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/AbstractSingleSelectChoiceEscapeTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/AbstractSingleSelectChoiceEscapeTest.java new file mode 100644 index 0000000000..e8f0565659 --- /dev/null +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/AbstractSingleSelectChoiceEscapeTest.java @@ -0,0 +1,191 @@ +/* + * 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; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Arrays; + +import org.apache.wicket.MarkupContainer; +import org.apache.wicket.markup.IMarkupResourceStreamProvider; +import org.apache.wicket.markup.html.WebPage; +import org.apache.wicket.model.Model; +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; + +/** + * The body of the default ("null") option of a single select choice is escaped the same way the body + * of every other option is. The display value is looked up by an overridable method, so it is not + * necessarily the plain text from a resource bundle that the default implementation returns. + */ +class AbstractSingleSelectChoiceEscapeTest extends WicketTestCase +{ + private static final String MARKUP = "<script>x=1</script>"; + + private static final String ESCAPED = "<script>x=1</script>"; + + /** + * A choice whose null display values come from somewhere other than a bundle, as an application + * override does when it shows the field's label instead of the generic text. + */ + private static class NullDisplayValueChoice extends DropDownChoice<String> + { + private static final long serialVersionUID = 1L; + + private final String displayValue; + + /** What the hooks handed back, to show escaping did not happen inside them. */ + private String observedDisplayValue; + + private NullDisplayValueChoice(String id, String displayValue) + { + super(id, Model.of((String)null), Arrays.asList("A", "B")); + this.displayValue = displayValue; + } + + @Override + protected String getNullValidDisplayValue() + { + observedDisplayValue = displayValue; + return displayValue; + } + + @Override + protected String getNullKeyDisplayValue() + { + observedDisplayValue = displayValue; + return displayValue; + } + + @Override + protected CharSequence escapeOptionHtml(String value) + { + return "[" + super.escapeOptionHtml(value) + "]"; + } + } + + private static class ChoicePage extends WebPage implements IMarkupResourceStreamProvider + { + private static final long serialVersionUID = 1L; + + private final NullDisplayValueChoice choice; + + private ChoicePage(String displayValue, boolean nullValid) + { + Form<Void> form = new Form<>("form"); + add(form); + choice = new NullDisplayValueChoice("dropdown", displayValue); + choice.setNullValid(nullValid); + form.add(choice); + } + + @Override + public IResourceStream getMarkupResourceStream(MarkupContainer container, + Class<?> containerClass) + { + return new StringResourceStream( + "<html><body><form wicket:id=\"form\"><select wicket:id=\"dropdown\"></select>" + + "</form></body></html>"); + } + } + + /** {@code nullValid} is true, so the "nullValid" display value is rendered. */ + @Test + void nullValidDisplayValueIsEscaped() + { + tester.startPage(new ChoicePage(MARKUP, true)); + + String response = tester.getLastResponseAsString(); + assertTrue(response.contains(ESCAPED), "the default option body should be escaped"); + assertFalse(response.contains(MARKUP), + "the default option body should not reach the markup as markup"); + } + + /** {@code nullValid} is false and nothing is selected, so the "null" display value is used. */ + @Test + void nullKeyDisplayValueIsEscaped() + { + tester.startPage(new ChoicePage(MARKUP, false)); + + String response = tester.getLastResponseAsString(); + assertTrue(response.contains(ESCAPED), "the default option body should be escaped"); + assertFalse(response.contains(MARKUP), + "the default option body should not reach the markup as markup"); + } + + /** + * The escaping follows escapeModelStrings, like the other options, so an application that means + * to put markup in the default option can still do so. + */ + @Test + void defaultOptionIsNotEscapedWhenEscapeModelStringsIsFalse() + { + ChoicePage page = new ChoicePage(MARKUP, true); + page.choice.setEscapeModelStrings(false); + tester.startPage(page); + + assertTrue(tester.getLastResponseAsString().contains(MARKUP), + "escaping is off, so the default option body should be written as is"); + } + + /** + * The escape goes through {@code escapeOptionHtml}, so an application that customised the + * escaping of its options gets it applied to the default option too. + */ + @Test + void escapeOptionHtmlIsUsedForTheDefaultOption() + { + tester.startPage(new ChoicePage(MARKUP, true)); + + // the marker the override adds has to show up in the body of the default option itself. + // Asserting that the method was called says nothing, it is called for every other option + assertTrue( + tester.getLastResponseAsString().contains("value=\"\">[" + ESCAPED + "]</option>"), + "the default option should be escaped through escapeOptionHtml"); + } + + /** + * Escaping happens where the option is written, not inside the display value methods. An + * override that compares what super returns against a resource it looks up itself would stop + * matching if those methods escaped. + */ + @Test + void displayValueHooksAreNotEscapedThemselves() + { + ChoicePage page = new ChoicePage(MARKUP, true); + tester.startPage(page); + + assertEquals(MARKUP, page.choice.observedDisplayValue, + "the display value hook should see and return unescaped text"); + assertTrue(tester.getLastResponseAsString().contains(ESCAPED), + "while the rendered option body is escaped"); + } + + /** Plain text, which is what the shipped bundles hold, renders unchanged. */ + @Test + void plainDisplayValueRendersUnchanged() + { + tester.startPage(new ChoicePage("Choose one", true)); + + assertTrue(tester.getLastResponseAsString().contains("value=\"\">[Choose one]</option>"), + "plain text should pass through the escaping unchanged"); + } +} diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/AbstractSingleSelectChoice.java b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/AbstractSingleSelectChoice.java index d12f6d96f1..d2bee6c813 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/AbstractSingleSelectChoice.java +++ b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/AbstractSingleSelectChoice.java @@ -289,6 +289,10 @@ public abstract class AbstractSingleSelectChoice<T> extends AbstractChoice<T, T> * * Otherwise no additional default choice will be returned. * + * The body of the option is escaped according to {@link #getEscapeModelStrings()}, the same way + * the body of every other option is. An override that builds an option itself is responsible for + * escaping what it puts in the body. + * * @see #getNullValidKey() * @see #getNullKey() * @see org.apache.wicket.markup.html.form.AbstractChoice#getDefaultChoice(String) @@ -315,7 +319,7 @@ public abstract class AbstractSingleSelectChoice<T> extends AbstractChoice<T, T> } // Add body of option tag - buffer.append(" value=\"\">").append(option).append("</option>"); + buffer.append(" value=\"\">").append(escapeOptionBody(option)).append("</option>"); return buffer; } else @@ -325,12 +329,25 @@ public abstract class AbstractSingleSelectChoice<T> extends AbstractChoice<T, T> { // Force the user to pick a non-null value String option = getNullKeyDisplayValue(); - return "\n<option selected=\"selected\" value=\"\">" + option + "</option>"; + return "\n<option selected=\"selected\" value=\"\">" + escapeOptionBody(option) + + "</option>"; } } return ""; } + /** + * Escapes the body of the default option, the way + * {@code AbstractChoice#renderValue} escapes the body of every other option. The value + * is escaped here rather than in {@link #getNullValidDisplayValue()} and + * {@link #getNullKeyDisplayValue()} because those return a display value, which an override may + * compare against a resource it looks up itself. + */ + private CharSequence escapeOptionBody(String option) + { + return getEscapeModelStrings() ? escapeOptionHtml(option) : option; + } + /** * Returns the display value for the null value. The default behavior is to look the value up by * using the key from <code>getNullValidKey()</code>.
