I think this might be an issue with CodexHTMLForm. Looking into it.

In T394396#10826597, @dmaza wrote:

I think this might be an issue with CodexHTMLForm. Looking into it.

Relevant code block? That's the only place I'm seeing ipbsubmit set right now, and then it's immediately passed to setSubmitTextMsg, which CodexHTMLForm does not override. So we'd maybe want to change setSubmitTextMsg to escape messages (or maybe have some default arg option) or introduce a function like setSubmitEscapedMsg.

Just to note, I did test with the old Special:Block and didn't see the submit message causing this problem.

In T394396#10827176, @Dreamy_Jazz wrote:

Just to note, I did test with the old Special:Block and didn't see the submit message causing this problem.

That's correct. because the form displayFormat is ooui when codex is disabled. If you change the form displayFormat to always be codex here on SpecialBlock you will get the xss regardless, even if the new Vue special page is being not loaded.

In T394396#10827775, @dmaza wrote:

In T394396#10827176, @Dreamy_Jazz wrote:

Just to note, I did test with the old Special:Block and didn't see the submit message causing this problem.

That's correct. because the form displayFormat is ooui when codex is disabled. If you change the form displayFormat to always be codex here on SpecialBlock you will get the xss regardless, even if the new Vue special page is being not loaded.

Interesting. Hopefully that helps you narrow the issue down.

Proposed fix

In T394396#10831178, @dmaza wrote:

Proposed fix

T394396.patch1 KBDownload

+1 from me!

In T394396#10831178, @dmaza wrote:

Proposed fix

T394396.patch1 KBDownload

Feedback (not tested locally yet):

• Inside the getInputCodex function, can the buttonLabel logic be wrapped in the escape function in one line instead, like this:

$buttonLabel = htmlspecialchars( $this->buttonLabel ?: $this->getDefault() );

• Then in buildCodexComponent, no change is needed:

$buttonHtml = Html::rawElement(
			'button', $attribs, $buttonLabel
		);

In T394396#10831318, @lwatson wrote:

In T394396#10831178, @dmaza wrote:

Proposed fix

T394396.patch1 KBDownload

Feedback (not tested locally yet):

• Inside the getInputCodex function, can the buttonLabel logic be wrapped in the escape function in one line instead, like this:

$buttonLabel = htmlspecialchars( $this->buttonLabel ?: $this->getDefault() );

• Then in buildCodexComponent, no change is needed:

$buttonHtml = Html::rawElement(
			'button', $attribs, $buttonLabel
		);

Thank you for your review. Unfortunately buildCodexComponent also gets called from CodexHTMLForm.php so we have to escape the label in that context and not before. What you are suggesting wouldn't fix this particular issue, unless I'm overlooking something.

Another alternative would be to use Html::element instead of rawElement and remove the call to htmlspecialchars

@dmaza - I think your patch in T394396#10831178 is fine. Generally, it's a best practice to escape any data as close to the relevant data sink as possible, as you've done. This typically makes the code easier to read and audit, especially for anyone who isn't immediately familiar with said code. We should try to get this security-deployed during this Monday's window (2025-05-19).

I don't understand the almost duplicate lines of code in both functions. Would it be safe to wrap the logic once in each function to ensure the values are escaped? So maybe this:

• getInputCodex

$buttonLabel = htmlspecialchars( $this->buttonLabel ?: $this->getDefault() );

• buildCodexComponent

$buttonHtml = Html::rawElement(
            'button', $attribs, htmlspecialchars( $buttonLabel )
);

I tested this locally and added ?uselang=x-xss&usecodex=true to the URL, and noticed no popup alert. What else should I look for? What does "success" look like?

In T394396#10831542, @lwatson wrote:

I don't understand the almost duplicate lines of code in both functions. Would it be safe to wrap the logic once in each function to ensure the values are escaped? So maybe this:

• getInputCodex

$buttonLabel = htmlspecialchars( $this->buttonLabel ?: $this->getDefault() );

• buildCodexComponent

$buttonHtml = Html::rawElement(
            'button', $attribs, htmlspecialchars( $buttonLabel )
);

I tested this locally and added ?uselang=x-xss&usecodex=true to the URL, and noticed no popup alert. What else should I look for? What does "success" look like?

@lwatson I don't think the lines in @dmaza's patch are duplicative - my understanding is that the htmlspecialchars call needed to be moved from the getInputCodex() method to buildCodexComponent() because some things call buildCodexComponent() directly (like CodexHTMLForm), so the escaping needs to happen there. Your suggested code for getInputCodex would only work when that's not the case and getInputCodex() is called. We also want to make sure we're only calling htmlspecialchars once.

@dmaza the code LGTM

@AnneT Thanks

@lwatson What Anne Said

In T394396#10831412, @sbassett wrote:

@dmaza - I think your patch in T394396#10831178 is fine. Generally, it's a best practice to escape any data as close to the relevant data sink as possible, as you've done. This typically makes the code easier to read and audit, especially for anyone who isn't immediately familiar with said code. We should try to get this security-deployed during this Monday's window (2025-05-19).

@sbassett I'm not typically around during that window. Do you need me or someone else to be around to test it after deployment?

Thank you both @dmaza and @AnneT! I noticed my mistake was misreading the diff file.

In T394396#10834821, @dmaza wrote:

@sbassett I'm not typically around during that window. Do you need me or someone else to be around to test it after deployment?

It would be nice, yes. For a patch like this, it probably isn't essential though, unless there are reasonable concerns about it breaking the UI in some unintended way.

Deployed to Wikimedia production.

Change #1193170 had a related patch set uploaded (by Reedy; author: Dmaza):

[mediawiki/core@REL1_43] SECURITY: Escape rawElement $content

https://gerrit.wikimedia.org/r/1193170

I think this is a duplicate of T402313 which has a separate fix that does not double escape messages.

As I mentioned on gerrit already, as far as I can see, $this->buttonLabel seems to either be parsed (L63), escaped (L69) or intentionally raw HTML (L72). It shouldn't be escaped in buildCodexComponent. Instead, it should be marked as exec_html and escaped by the caller, which https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1193175 (the fix for T402313) does already.

Change #1193170 merged by jenkins-bot:

[mediawiki/core@REL1_43] SECURITY: Escape rawElement $content

https://gerrit.wikimedia.org/r/1193170

Change #1193194 had a related patch set uploaded (by Reedy; author: Dmaza):

[mediawiki/core@REL1_44] SECURITY: Escape rawElement $content

https://gerrit.wikimedia.org/r/1193194

Change #1193194 merged by jenkins-bot:

[mediawiki/core@REL1_44] SECURITY: Escape rawElement $content

https://gerrit.wikimedia.org/r/1193194

Change #1193216 had a related patch set uploaded (by Reedy; author: Dmaza):

[mediawiki/core@master] SECURITY: Escape rawElement $content

https://gerrit.wikimedia.org/r/1193216

Change #1193216 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: Escape rawElement $content

https://gerrit.wikimedia.org/r/1193216

In T394396#11238391, @SomeRandomDeveloper wrote:

As I mentioned on gerrit already, as far as I can see, $this->buttonLabel seems to either be parsed (L63), escaped (L69) or intentionally raw HTML (L72). It shouldn't be escaped in buildCodexComponent. Instead, it should be marked as exec_html and escaped by the caller, which https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1193175 (the fix for T402313) does already.

I also came here to say basically this. The current fix doesn't seem like the correct approach to me.

@Reedy - should we open this task back up to address the double-escape or just file a new bug? This should just need a revert and an explanation in the commit msg, correct?

Change #1193501 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] Revert "SECURITY: Escape rawElement $content"

https://gerrit.wikimedia.org/r/1193501

Change #1193507 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@REL1_44] Revert "SECURITY: Escape rawElement $content"

https://gerrit.wikimedia.org/r/1193507

Change #1193508 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@REL1_43] Revert "SECURITY: Escape rawElement $content"

https://gerrit.wikimedia.org/r/1193508

Change #1193501 merged by jenkins-bot:

[mediawiki/core@master] Revert "SECURITY: Escape rawElement $content"

https://gerrit.wikimedia.org/r/1193501

Change #1193508 merged by jenkins-bot:

[mediawiki/core@REL1_43] Revert "SECURITY: Escape rawElement $content"

https://gerrit.wikimedia.org/r/1193508

Change #1193507 merged by jenkins-bot:

[mediawiki/core@REL1_44] Revert "SECURITY: Escape rawElement $content"

https://gerrit.wikimedia.org/r/1193507