Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tabs: Use CSS.escape for sanitizing selectors #2307

Merged
merged 1 commit into from
Oct 26, 2024
Merged

Tabs: Use CSS.escape for sanitizing selectors #2307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions tests/unit/tabs/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,37 @@ QUnit.test( "non-tab list items", function( assert ) {
"first actual tab is active" );
} );

QUnit.test( "ID escaping backslashes", function( assert ) {
assert.expect( 5 );

location.hash = "#fragment\b-2";

var element = $( "#tabs1" )
.find( "a[href='#fragment-2']" )
.attr( "href", "#fragment\b-2" )
.end()
.find( "#fragment-2" )
.attr( "id", "fragment\b-2" )
.end()
.tabs();
var tabs = element.find( ".ui-tabs-nav li" );
var anchors = tabs.find( ".ui-tabs-anchor" );
var panels = element.find( ".ui-tabs-panel" );

assert.strictEqual( element.tabs( "option", "active" ), 1,
"should set the active option" );

assert.strictEqual( anchors.length, 3, "should decorate all anchors" );
assert.strictEqual( panels.length, 3, "should decorate all panels" );

assert.strictEqual( panels.eq( 1 ).attr( "aria-labelledby" ), anchors.eq( 1 ).attr( "id" ),
"panel 2 aria-labelledby equals anchor 2 id" );
assert.strictEqual( tabs.eq( 1 ).attr( "aria-controls" ), "fragment\b-2",
"tab 2 aria-controls" );

location.hash = "";
} );

QUnit.test( "aria-controls", function( assert ) {
assert.expect( 7 );
var element = $( "#tabs1" ).tabs(),
Expand Down
3 changes: 1 addition & 2 deletions tests/unit/tabs/helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,7 @@ return $.extend( helper, {
var expected = $.makeArray( arguments ).slice( 2 ),
actual = tabs.find( ".ui-tabs-nav li" ).map( function() {
var tab = $( this ),
panel = $( $.ui.tabs.prototype._sanitizeSelector(
"#" + tab.attr( "aria-controls" ) ) ),
panel = $( "#" + CSS.escape( tab.attr( "aria-controls" ) ) ),
tabIsActive = tab.hasClass( "ui-state-active" ),
panelIsActive = panel.css( "display" ) !== "none";

Expand Down
16 changes: 6 additions & 10 deletions ui/widgets/tabs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,14 @@ $.widget( "ui.tabs", {
_initialActive: function() {
var active = this.options.active,
collapsible = this.options.collapsible,
locationHash = location.hash.substring( 1 );
locationHashDecoded = decodeURIComponent( location.hash.substring( 1 ) );

if ( active === null ) {

// check the fragment identifier in the URL
if ( locationHash ) {
if ( locationHashDecoded ) {
this.tabs.each( function( i, tab ) {
if ( $( tab ).attr( "aria-controls" ) === locationHash ) {
if ( $( tab ).attr( "aria-controls" ) === locationHashDecoded ) {
active = i;
return false;
}
Expand Down Expand Up @@ -312,10 +312,6 @@ $.widget( "ui.tabs", {
}
},

_sanitizeSelector: function( hash ) {
return hash ? hash.replace( /[!"$%&'()*+,.\/:;<=>?@\[\]\^`{|}~]/g, "\\$&" ) : "";
},
Comment on lines -315 to -317
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fnagel It's internal only so I thought we can just remove it. Or should we be conservative & only deprecate in 1.14? I don't know how jQuery UI historically approached removing private widget methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there's at least one case of $.fn._focus getting renamed to $.fn.focus between 1.10.1 & 1.10.2: 1.10.1...1.10.2

Copy link
Member

@fnagel fnagel Oct 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh that is in indeed a good question. Iirc Scott would not make a big thing of changing internal methods but I'm not really sure.

Why not just keep the $.fn._sanitizeSelector $.ui.tabs.prototype._sanitizeSelector until the next minor release but make it use CSS.escape?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just seen its not $.fn but $.ui.tabs so it's even more specific (as in Tabs widget only). Not sure if its worth keeping the old method around...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll keep it removed then. There's no universal way to sanitize the whole selector; the safe thing to do is to just escape the identifier part, especially when potentially coming from user input. Even if I changed the internal method to use CSS.escape, it would just handle the "# + id" case, which can be confusing and can still lead people to use it where it's not intended.


refresh: function() {
var options = this.options,
lis = this.tablist.children( ":has(a[href])" );
Expand Down Expand Up @@ -434,9 +430,9 @@ $.widget( "ui.tabs", {

// Inline tab
if ( that._isLocal( anchor ) ) {
selector = anchor.hash;
selector = decodeURIComponent( anchor.hash );
panelId = selector.substring( 1 );
panel = that.element.find( that._sanitizeSelector( selector ) );
panel = that.element.find( "#" + CSS.escape( panelId ) );

// remote tab
} else {
Expand Down Expand Up @@ -874,7 +870,7 @@ $.widget( "ui.tabs", {

_getPanelForTab: function( tab ) {
var id = $( tab ).attr( "aria-controls" );
return this.element.find( this._sanitizeSelector( "#" + id ) );
return this.element.find( "#" + CSS.escape( id ) );
}
} );

Expand Down
Loading