Skip to content

Fixes to resolve unsafe warnings in WWAs #1537

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

Closed
wants to merge 5 commits into from

Conversation

jonathansampson
Copy link
Contributor

Setting the innerHTML property in an unsafe manner raises issues in
Windows Web Applications. Strings being passed into innerHTML cannot
include name the attribute. Creating elements, and settings attributes,
has far better cross-environment compliance.

Historical context:

Setting the innerHTML property in an unsafe manner raises issues in
Windows Web Applications. Strings being passed into innerHTML cannot
include name the attribute. Creating elements, and settings attributes,
has far better cross-environment compliance.
@mgol
Copy link
Member

mgol commented Mar 11, 2014

Thanks for your contribution! This will need comments like "Support: Windows Web Apps" (did we name it differently in other parts of the code base?) to justify this madness so that someone doesn't remove it accidentally.

It's bad we don't have a way to automatically test it. Do we have contacts in MS we could signal it's really important to us? cc @dmethvin

option.setAttribute("selected", "");

select.appendChild(option);
div.appendChild(select);
Copy link
Member

Choose a reason for hiding this comment

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

Sizzle changes must be addressed in Sizzle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmethvin dmethvin added this to the 1.11.1/2.1.1 milestone Mar 19, 2014
@dmethvin
Copy link
Member

Ugh, I was wondering why this cropped up since we were passing in WWA at one point. It was changed in 6d2c5c7. @timmywil do you recall if this was causing a test fail for 5.1? Or was it just an optimization? If it's just the latter I can land something like what @jonathansampson is proposing and add a comment about not using .innerHTML.

@dmethvin
Copy link
Member

Oh also I'm wondering if we even need this specific part of the feature test, per the comment from @mzgol in http://bugs.jquery.com/ticket/11217 . This patch seems to predate our // Support comments at the very least.

@timmywil
Copy link
Member

@dmethvin I believe we were getting false positives if setAttribute was used to set checked, but I'm not 100%. Let's just double check that the support test actually fails in 5.1.

@dmethvin dmethvin closed this in 85af4e6 Mar 20, 2014
mgol added a commit to mgol/jquery that referenced this pull request Jul 28, 2015
mgol added a commit to mgol/jquery that referenced this pull request Jul 28, 2015
mgol added a commit to mgol/jquery that referenced this pull request Jul 28, 2015
mgol added a commit to mgol/jquery that referenced this pull request Aug 16, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants