Skip to content

LTS-only - fix(core): use appropriate inert document strategy for Firefox & Safari #22077

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

LTS-only - fix(core): use appropriate inert document strategy for Firefox & Safari #22077

wants to merge 5 commits into from

Conversation

petebacondarwin
Copy link
Contributor

Both Firefox and Safari are vulnerable to XSS if we use an inert document
created via document.implementation.createHTMLDocument().

Now we check for those vulnerabilities and then use a DOMParser or XHR
strategy if needed.

Further the platform-server has its own library for parsing HTML, so we
sniff for that (by checking whether DOMParser exists) and fall back to
the standard strategy.

Thanks to @cure53 for the heads up on this issue.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

Other information

This is the LTS version of #17019

@petebacondarwin petebacondarwin added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours freq1: low security Issues that generally impact framework or application security area: security Issues related to built-in security features, such as HTML sanitation target: lts This PR is targeting a version currently in long-term support labels Feb 7, 2018
@petebacondarwin petebacondarwin mentioned this pull request Feb 7, 2018
Closed
@IgorMinar IgorMinar changed the title fix(core): use appropriate inert document strategy for Firefox & Safari LTS-only - fix(core): use appropriate inert document strategy for Firefox & Safari Feb 9, 2018
@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 9, 2018
@ngbot
Copy link

ngbot bot commented Feb 9, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "continuous-integration/travis-ci/pr" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@IgorMinar
Copy link
Contributor

uh. oh. I'm afraid that if we want to merge this we'll need several of @gkalpak's CI stability changes as well - at minimum the chrome/chrome-driver changes that prevent chrome flakes and crashes.

@IgorMinar IgorMinar added state: blocked and removed action: merge The PR is ready for merge by the caretaker labels Feb 9, 2018
@gkalpak
Copy link
Member

gkalpak commented Feb 9, 2018

I'll try backporting the changes. It shouldn't be too bad (since it is just infrastructure changes).

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@gkalpak gkalpak force-pushed the sanitize-inert-doc-lts branch from 976cfa4 to 9dc047b Compare February 11, 2018 17:52
gkalpak and others added 4 commits February 11, 2018 21:18
@gkalpak
build: pin ChromeDriver version (angular#20940)
1d9024e 
Since our version of Chromium is also pinned, a new ChromeDriver (that
drops support for our Chromium version) can cause random (and unrelated
to the corresponding changes) errors on CI.
This commit pins the version of ChromeDriver and it should now be
manually upgraded to a vrsion that is compatible with th currently used
Chromium version.

PR Close angular#20940
@gkalpak
ci: downgrade Chromium to a version that does not cause flakes
e9f1d44 
There seems to be some issue that causes Chrome/ChromeDriver to
unexpectedly reload during the aio e2e tests, causing flakes. It is not
clear what exactly is causing the reloading, but to the best of my
knowledge it is something inside Chrome or ChromeDriver.

Pinning Chrome to r494239 (between 62.0.3185.0 and 62.0.3186.0) fixes
the flakes.

Fixes angular#20159
@gkalpak
ci: use sudo: false on Travis (angular#21641)
0dacf6d 
Related to angular#21422.

PR Close angular#21641
@petebacondarwin @gkalpak
fix(core): use appropriate inert document strategy for Firefox & Safari
53045c7 
Both Firefox and Safari are vulnerable to XSS if we use an inert document
created via `document.implementation.createHTMLDocument()`.

Now we check for those vulnerabilities and then use a DOMParser or XHR
strategy if needed.

Further the platform-server has its own library for parsing HTML, so we
sniff for that (by checking whether DOMParser exists) and fall back to
the standard strategy.

Thanks to @cure53 for the heads up on this issue.
@gkalpak gkalpak force-pushed the sanitize-inert-doc-lts branch from 9dc047b to 53045c7 Compare February 11, 2018 19:18
@gkalpak
Copy link
Member

gkalpak commented Feb 11, 2018

CI is green now 🎉

@IgorMinar IgorMinar removed the request for review from mhevery February 13, 2018 07:10
@IgorMinar
Copy link
Contributor

the instructions at https://g3doc.corp.google.com/javascript/angular/g3doc/sync.md#syncing-tap-status-to-github don't work for me, so I can't force push the g3 status for this pr.

@mhevery can you please try to force push the status? since this PR targets the lts branch, the g3 check is pointless.

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Feb 13, 2018
@IgorMinar
Copy link
Contributor

wrt the error, I'm getting error: unknown option--raw-output'(that is after I had to installnpm install -g jq` and some other packages.

mhevery pushed a commit that referenced this pull request Feb 13, 2018
@petebacondarwin @mhevery
fix(core): use appropriate inert document strategy for Firefox & Safa…
2c5cf19 
…ri (#22077)

Both Firefox and Safari are vulnerable to XSS if we use an inert document
created via `document.implementation.createHTMLDocument()`.

Now we check for those vulnerabilities and then use a DOMParser or XHR
strategy if needed.

Further the platform-server has its own library for parsing HTML, so we
sniff for that (by checking whether DOMParser exists) and fall back to
the standard strategy.

Thanks to @cure53 for the heads up on this issue.
@mhevery
Copy link
Contributor

mhevery commented Feb 13, 2018

merged as 2c5cf19

@mhevery mhevery closed this Feb 13, 2018
@petebacondarwin petebacondarwin deleted the sanitize-inert-doc-lts branch February 25, 2018 10:00
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@IgorMinar IgorMinar IgorMinar approved these changes

Assignees

@mhevery mhevery

Labels
action: merge The PR is ready for merge by the caretaker area: security Issues related to built-in security features, such as HTML sanitation cla: yes effort1: hours freq1: low merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note security Issues that generally impact framework or application security target: lts This PR is targeting a version currently in long-term support type: bug/fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@petebacondarwin @IgorMinar @gkalpak @googlebot @mhevery @filipesilva