Tests: Add tests for recently fixed manipulation XSS issues #4685
Conversation
| assert.ok( window.xss.withArgs( currCounter ).notCalled, | ||
| "Insecure code wasn't executed, input: " + htmlString ); | ||
| done(); | ||
| }, 1000 ); |
There was a problem hiding this comment.
It would be nice to reduce this forced one-second frequency... what would you think about something like so?
div.appendTo( container );
div.html( htmlString );
container.append("<img src=x onerror=\"xssDone(counter)\">")There was a problem hiding this comment.
@gibson042 Now that I think about it... How do we know this onerror callback will be fired as the last one? 404 responses may not be cached AFAIK so there's a risk that one will fire before the other ones are done.
There was a problem hiding this comment.
Well, src="#" would guarantee that the invalid source is cached.
There was a problem hiding this comment.
That will resolve to the current page, though, so it won't be a 404.
Also, currently all the tests succeed even if cache is disabled in the browser DevTools. I'd like to keep that working.
There was a problem hiding this comment.
@gibson042 I'm thinking about ways to reduce this delay but if I don't find anything reliable that'd work with cache disabled, I think I'll just merge it, we can always apply improvements later.
There was a problem hiding this comment.
We don't need a 404, just need a response that isn't a successful image. And I don't see any cache concerns, but if you want to merge without the acceleration then I won't object.
There was a problem hiding this comment.
Good point about not needing 404s. I tested your suggestion, though, and - if I understood it correctly - it looks like I was right about race conditions. See the code at:
mgol@7cd593f
I reverted the two security fixes locally & all assertions should fail but it's pretty random for me; on each refresh different ones are failing for me. I tried the esmodules tests.
There was a problem hiding this comment.
An example test run below, I only run this single test. Note that the order of reported assertions is also pretty random.
There was a problem hiding this comment.
Yeah, raciness confirmed. I don't think that's fatal to the testing, but it would require restructuring. No worries.
ccadbc6 to
40cd040
Compare
40cd040 to
f9983f1
Compare
f9983f1 to
75429b4
Compare
iOS 8-12 parses `<noembed>` tags differently, executing this code. This is no different to native behavior on that OS, though, so just accept it. Ref jquerygh-4685
|
Looks like one of the added tests fails in iOS 8 - 12. A followup fix PR, PTAL: #4694. |
Summary
Add tests for recently fixed manipulation XSS issues.
Ref gh-4642
Ref gh-4647
Checklist
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com