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.

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

Fixes #17516 - Update jquery to 2.2.4 to fix XSS #4065

Merged
merged 1 commit into from Dec 21, 2016

Conversation

dLobatog
Copy link
Member

Affected versions of the package (< 1.12) are vulnerable to Cross-site Scripting (XSS) attacks when a cross-domain ajax request is performed without the dataType option causing text/javascript responses to be executed.

jquery/jquery#2432 for more information

@mention-bot
Copy link

@dLobatog, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tbrisker, @gailsteiger and @domcleal to be potential reviewers.

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • commit message for b1a7f7a is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@@ -44,7 +44,7 @@
"events": "^1.1.1",
"flux": "^2.1.1",
"ipaddr.js": "~1.2.0",
"jquery": "~1.11.0",
"jquery": "~1.12.0",
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be 1.13?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no 1.13, 1.12 is the one that contains the fix - it was a typo

@dLobatog
Copy link
Member Author

Repushed with the right commit title (it said 1.13 instead of 1.12)

@dLobatog dLobatog changed the title Fixes #17516 - Update jquery to 1.13 to fix XSS Fixes #17516 - Update jquery to 1.12 to fix XSS Nov 29, 2016
@tbrisker
Copy link
Member

@dLobatog please rebase, tests are failing due to #4064

@dLobatog
Copy link
Member Author

Rebased!

@tbrisker
Copy link
Member

tests are failing. Looks like this upgrade broke something.

@tbrisker
Copy link
Member

@dLobatog application.js line 66 should be changed to

$('a[href="#'+this.id+'"]').addClass("tab-error");

to fix the failing test. Care to upgrade jQuery to 2.x while you are at it? It is compatible with 1.x except it dropped IE<9 support. I am looking into upgrading to 3.x which does introduce some breaking changes.

@dLobatog
Copy link
Member Author

Updated , including jquery 2.1 and the application.js fix, thanks @tbrisker

@@ -39,6 +39,6 @@ describe('initInheritedRoles', () => {
$('.dropdown-menu li a').last().click();
expect($('.btn').text()).toContain('First');
expect($('.list-group li[data-id="1"]').is(':visible')).toBeTruthy();
expect($('.list-group li[data-id="2"]').is(':visible')).toBeFalsy();
expect($('.list-group li[data-id="2"]').css('display')).toEqual('none');
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like :visible meaning changed on 2.1, so I made it more specific

@@ -44,7 +44,7 @@
"events": "^1.1.1",
"flux": "^2.1.1",
"ipaddr.js": "~1.2.0",
"jquery": "~1.11.0",
"jquery": "~2.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Latest version is 2.2.4, any reason not to use that?

Affected versions of the package (< 1.12) are vulnerable to Cross-site
Scripting (XSS) attacks when a cross-domain ajax request is performed
without the dataType option causing text/javascript responses to be
executed.

jquery/jquery#2432 for more information
@dLobatog dLobatog changed the title Fixes #17516 - Update jquery to 1.12 to fix XSS Fixes #17516 - Update jquery to 2.2.4 to fix XSS Dec 21, 2016
@dLobatog
Copy link
Member Author

@tbrisker I think I saw some breakages on tests with 2.2.4 but it seems fine now. Updated to 2.2.4

@tbrisker tbrisker merged commit 9356b0b into theforeman:develop Dec 21, 2016
@tbrisker
Copy link
Member

Works just fine, thanks @dLobatog!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants