Datepicker: Make sure text option are text, shorten HTML strings #1953

mgol · 2021-04-28T20:23:30Z

Instead of using enormous HTML strings, various elements are now constructed
using jQuery APIs. This makes it more obvious user-provided data is used
correctly.

Fixes #15284

fnagel · 2021-05-04T09:17:37Z

ui/widgets/datepicker.js

-			" title='" + prevText + "'><span class='ui-icon ui-icon-circle-triangle-" + ( isRTL ? "e" : "w" ) + "'>" + prevText + "</span></a>" :
-			( hideIfNoPrevNext ? "" : "<a class='ui-datepicker-prev ui-corner-all ui-state-disabled' title='" + prevText + "'><span class='ui-icon ui-icon-circle-triangle-" + ( isRTL ? "e" : "w" ) + "'>" + prevText + "</span></a>" ) );
+		if ( this._canAdjustMonth( inst, -1, drawYear, drawMonth ) ) {
+			prev = $( "<a></a>" )


"<a>" should be enough or is there a reason you added the closing tag here?

The string I used is natively compliant with both HTML & XHTML, although for the single tag case it doesn't matter much as jQuery uses a shortcut that just does document.createElement then. I can change it to "<a>".

Using <a> would be more consistent with what we have in other (more modern, UI widget factory using) widgets.

@fnagel Although it's safe to do a single unclosed tag, an unclosed tag with attributes or several unclosed tags in a string can be an issue. It's been hard to explain these subtle differences to people so closing tags is a good practice.

I guess another option would be <a /> which jQuery also shortcuts to document.createElement? Not sure if that's better or not.

@dmethvin Sadly, the linked news does not provide a recommendation what is the best practice. Looks like best thing to do is using closed tags, right?

I would suggest to keep this consistent with all other widgets (using single, unclosed tags if no attributes are used) and then do a PR changing this to single closed tags.

The code deciding on whether to use the shortcut or not is here:
https://github.com/jquery/jquery/blob/a70274632dc19ff4a64d7bb7657a2cc647ff38b9/src/core/parseHTML.js#L35-L43
If the regex matches, the simple route is chosen. As Dave mention, just adding an attribute will break it, though, which makes $("<div>") work fine in XHTML mode but $("<div a='b'>") break. It's safer to always close tags for this reason. $("<div a='b' />") would work, though - in HTML mode the slash is ignored and in XHTML mode all tags can be self-closing. Because /> is generally just > in HTML mode, though, the fact that it works is a bit magical. But maybe that's fine.

I'm fine with making the change here as advised and modifying all single tags separately in a subsequent PR.

fnagel · 2021-05-04T09:18:19Z

ui/widgets/datepicker.js

-			" title='" + nextText + "'><span class='ui-icon ui-icon-circle-triangle-" + ( isRTL ? "w" : "e" ) + "'>" + nextText + "</span></a>" :
-			( hideIfNoPrevNext ? "" : "<a class='ui-datepicker-next ui-corner-all ui-state-disabled' title='" + nextText + "'><span class='ui-icon ui-icon-circle-triangle-" + ( isRTL ? "w" : "e" ) + "'>" + nextText + "</span></a>" ) );
+		if ( this._canAdjustMonth( inst, +1, drawYear, drawMonth ) ) {
+			next = $( "<a></a>" )


See comment above.

fnagel · 2021-05-04T09:18:31Z

ui/widgets/datepicker.js

+		} else if ( hideIfNoPrevNext ) {
+			prev = "";
+		} else {
+			prev = $( "<a></a>" )


See comment above.

fnagel · 2021-05-04T09:18:38Z

ui/widgets/datepicker.js

+		} else if ( hideIfNoPrevNext ) {
+			next = "";
+		} else {
+			next = $( "<a></a>" )


See comment above.

fnagel · 2021-05-04T09:22:39Z

ui/widgets/datepicker.js

-			">" + currentText + "</button>" : "" ) + ( isRTL ? "" : controls ) + "</div>" : "";
+		controls = "";
+		if ( !inst.inline ) {
+			controls = $( "<button></button>" )


Same here: is there a reason you added the closing tag?

fnagel · 2021-05-04T09:34:49Z

ui/widgets/datepicker.js

+						title: buttonText
+					} );
+			} else {
+				inst.trigger = $( "<button type='button'></button>" )


Closing tag not needed.

fnagel · 2021-05-04T10:20:19Z

ui/widgets/datepicker.js

@@ -240,7 +240,9 @@ $.extend( Datepicker.prototype, {
 			inst.append.remove();
 		}
 		if ( appendText ) {
-			inst.append = $( "<span class='" + this._appendClass + "'>" + appendText + "</span>" );
+			inst.append = $( "<span></span>" )
+				.attr( "class", this._appendClass )


Why not using addClass here?

fnagel · 2021-05-04T10:23:17Z

ui/widgets/datepicker.js

+				} )
+				.append(
+					$( "<span>" )
+						.attr( "class", "ui-icon ui-icon-circle-triangle-" +


Why not using addClass here?

fnagel · 2021-05-04T10:23:26Z

ui/widgets/datepicker.js

+				} )
+				.append(
+					$( "<span>" )
+						.attr( "class", "ui-icon ui-icon-circle-triangle-" +


Why not using addClass here?

fnagel · 2021-05-04T10:23:41Z

ui/widgets/datepicker.js

+				} )
+				.append(
+					$( "<span>" )
+						.attr( "class", "ui-icon ui-icon-circle-triangle-" +


Why not using addClass here?

Instead of using enormous HTML strings, various elements are now constructed using jQuery APIs. This makes it more obvious user-provided data is used correctly. Fixes #15284

fnagel

+1 by reading

mgol added this to the 1.13 milestone Apr 28, 2021

mgol requested a review from fnagel April 28, 2021 20:23

mgol self-assigned this Apr 28, 2021

fnagel requested changes May 4, 2021

View reviewed changes

Datepicker: Make sure text option are text, shorten HTML strings

7cd8b25

Instead of using enormous HTML strings, various elements are now constructed using jQuery APIs. This makes it more obvious user-provided data is used correctly. Fixes #15284

mgol force-pushed the datepicker-text-options branch from 5a69022 to 7cd8b25 Compare May 4, 2021 11:33

mgol requested a review from fnagel May 7, 2021 22:58

fixup! Datepicker: Make sure text option are text, shorten HTML strings

463654d

mgol force-pushed the datepicker-text-options branch from 49ccf42 to 463654d Compare May 7, 2021 23:00

fnagel approved these changes May 10, 2021

View reviewed changes

mgol merged commit afe20b7 into jquery:main May 11, 2021

mgol deleted the datepicker-text-options branch May 11, 2021 22:59

romulogp mentioned this pull request Jun 24, 2022

Datepicker: Various *Text options no longer accept HTML input [SO-2932013] philips-emr/jquery-ui#3

Merged

AASMACMX mentioned this pull request Feb 26, 2023

CVE-2021-41183 @ Npm-jquery-ui-1.10.4 CxDemoInABoxRepos/Java-Webgoat#250

Open

AASMACMX mentioned this pull request Apr 26, 2023

CVE-2021-41183 @ Npm-jquery-ui-1.10.4 CxDemoInABoxRepos/Java-Webgoat#517

Open

AASMACMX mentioned this pull request Jul 20, 2023

CVE-2021-41183 @ Npm-jquery-ui-1.10.4 CxDemoInABoxRepos/Java-Webgoat#813

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datepicker: Make sure text option are text, shorten HTML strings #1953

Datepicker: Make sure text option are text, shorten HTML strings #1953

mgol commented Apr 28, 2021 •

edited

fnagel May 4, 2021

mgol May 4, 2021

fnagel May 4, 2021

dmethvin May 4, 2021

fnagel May 4, 2021

mgol May 4, 2021 •

edited

fnagel May 4, 2021

fnagel May 4, 2021

fnagel May 4, 2021

fnagel May 4, 2021

fnagel May 4, 2021

fnagel May 4, 2021

fnagel May 4, 2021

fnagel May 4, 2021

fnagel May 4, 2021

fnagel left a comment

Datepicker: Make sure text option are text, shorten HTML strings #1953

Datepicker: Make sure text option are text, shorten HTML strings #1953

Conversation

mgol commented Apr 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol May 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnagel left a comment

Choose a reason for hiding this comment

mgol commented Apr 28, 2021 •

edited

mgol May 4, 2021 •

edited