getInertBodyElement returns null, crashing in _sanitizeHtml #39834

jsgoupil · 2020-11-25T02:32:46Z

🐞 bug report

Affected Package

The issue is caused by package @angular/core

Is this a regression?

No

Description

With IVY, the following line

parsedHtml = inertBodyElement!.innerHTML;

angular/packages/core/src/sanitization/html_sanitizer.ts

Line 267 in 3e1e5a1

parsedHtml = inertBodyElement!.innerHTML;

causes this error

null is not an object (evaluating 'n.innerHTML')

I have real trouble to repro this consistently; and it happens only on a specific browser without access to the console (I'm not a mac developer).

But the getInertBodyElement CAN return null within a try/catch, but nothing is reported. The line inertBodyElement bypasses the nullability with !.
And it happens ONLY with this browser:

Mobile Safari UI/WKWebView
Version 14.1, 14.0, 13.7, 13.6, 12.4
To get that specific browser, you get it via an iPad app with a UIWebView

This is getting quite specific, but it happens a lot.
Why in the first place there is no check for this nullability?

🔬 Minimal Reproduction

The data being passed in is
<span class="quick-books-status"><img src="/assets/images/quick-books-pending.svg" title="QuickBooks Pending" alt="" /></span>

but I have doubts that this is the main issues.

🔥 Exception or Error




null is not an object (evaluating 'n.innerHTML')

🌍 Your Environment

Angular Version: 11.0.0




Angular CLI: 11.0.1
Node: 14.13.1
OS: win32 x64

Angular: 11.0.0
... animations, common, compiler, compiler-cli, core, forms
... language-service, localize, platform-browser
... platform-browser-dynamic, router
Ivy Workspace: Yes

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1100.1
@angular-devkit/build-angular   0.1100.1
@angular-devkit/core            11.0.1
@angular-devkit/schematics      11.0.1
@angular/cdk                    10.2.7
@angular/cli                    11.0.1
@angular/material               10.2.7
@schematics/angular             11.0.1
@schematics/update              0.1100.1
rxjs                            6.6.3
typescript                      4.0.5

Anything else relevant?

Yes. It's a browser served in a UIWebView on an iPad; which results in Safari.

Example of a user-agent affected

Mozilla/5.0 (iPad; CPU OS 14_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Mobile/15E148

The text was updated successfully, but these errors were encountered:

petebacondarwin · 2020-11-25T11:55:16Z

This is interesting. There are two InertBodyHelper implementations that are used to get the inertBodyElement.
This is the selection: isDOMParserAvailable() ? new DOMParserHelper() : new InertDocumentHelper(defaultDoc);.

Note that InertDocumentHelper never returns null for inertBodyElement, so it must be that in this case Angular is using the DOMParserHelper, which can indeed return null. But it only does this if the DOMParser crashed for some reason.

One thing you could do to help diagnose what is going on is to remove the catch block inside the DOMParserHelper.getInertBodyElement() method. This will allow the original error to surface that might tell us why Safari mobile does not work in this case.

jsgoupil · 2020-11-25T17:34:40Z

null is not an object (evaluating 't.removeChild')

Not as helpful as you wished I bet.

petebacondarwin · 2020-11-25T17:47:15Z

Well it's a step forward... I expect the error is now throwing on this line:

angular/packages/core/src/sanitization/inert_body.ts

Line 44 in 8fd25d9

body.removeChild(body.firstChild!);

jsgoupil · 2020-11-25T20:49:52Z

To save you from a bit of debugging.. I tried this, but with no luck, it works "fine".
I tried to stress the DOMParser, but didn't help.

<script>
document.write(navigator.userAgent);
document.write('<br />');
document.write(new Date());
document.write('<br />');
var html = '<span class="quick-books-status"><img src="/assets/images/quick-books-pending.svg" title="QuickBooks Pending" alt="" /></span>';
html = '<body><remove></remove>' + html;

var domParser = new window.DOMParser();
document.write('domParser: ');
document.write(domParser);
document.write('<br />');

var parsed = domParser.parseFromString(html, 'text/html');
document.write('Parsed: ');
document.write(parsed);
document.write('<br />');

var body = parsed.body;
document.write('body: ');
document.write(body);
document.write('<br />');

document.write('parsed.firstChild: ');
document.write(parsed.firstChild);
document.write('<br />');

document.write('parsed.firstChild.children ->');
Array.from(parsed.firstChild.children).map(c => document.write(c));

setTimeout(() => {
var a = document.getElementById('a');
for (let x = 0; x < 1000; x++) {

  setTimeout(() => { 
    var b = new window.DOMParser().parseFromString(html, 'text/html').body;
    if (b) {
        a.innerHTML = a.innerHTML + '<br />' + x + 'OK';
    } else {
        a.innerHTML = a.innerHTML + '<br />' + x + 'ERROR';
    }
  }, 1);
}

}, 100);
</script>
<div id="a"></div>
<a href="javascript:window.location.reload();">Reload</a>

petebacondarwin · 2020-11-25T20:59:44Z

Hmm, we need to dig/debug a bit further. I assume that if you remove our DOMParserHelper from the mix you don't get the error?

jsgoupil · 2020-11-25T21:04:02Z

To be frank, this is a production app and I am modifying minified code right now. And not all my traffic is coming from an iPad.
So that's really playing with fire for a prod app.
I can try but traffic is dying down right now and tomorrow is holiday, so I won't get much traffic until Monday.

petebacondarwin · 2020-11-25T21:08:33Z

OK, no stress. I don't have access to an iPad to test myself right now. Sorry.

jsgoupil · 2020-11-25T21:19:45Z

Alright, I found the code... not simple. Pushed to production and we will see. I had to make sure the "fallback" works with most browser first.

Thanks.

jsgoupil · 2020-12-04T20:14:06Z

I confirm that when I was using the other implementation, I was getting no error whatsoever about this.

petebacondarwin · 2020-12-04T20:36:45Z

Hmm, so it looks like for Mobile Safari we cannot use the DOMParser..., right?
We could probably handle this by adding a try-catch and then reverting to the other strategy if the DOMParser throws.
Does this sound right to you?

jsgoupil · 2020-12-04T20:49:24Z

I think it's a good alternative, but I'm not a IVY/compiler coder :)
It's also unfortunate that we are not able to repro this in a small piece of code. Maybe there is something about memory overloaded? It's really hard to tell why this is happening in the first place.
My app is pretty massive, so it could be a reason?

Do you guys have any reason why this would throw in the first place (other than the one we haven't found the exact cause here).

Looking at the DOMParser in WebKit. (If this is the correct code being used)
https://github.com/WebKit/webkit/blob/817c46e152af795d735678386db68805d0aa505e/Source/WebCore/xml/DOMParser.cpp#L41

The document cannot be null, but the reach of ".body" returned null.
I'm not a C++ developer so I can't really find out why this is happening.

petebacondarwin · 2020-12-07T08:56:18Z

One thing, can I ask @jsgoupil, did this suddenly start happening, i.e. after an upgrade to Angular, or has it always been doing this? The reason I ask is that this commit landed in 11.0.0 and it might be causing the DOMParser.parseFromString() to be receiving something it cannot handle.

jsgoupil · 2020-12-07T15:55:33Z

That will be tough to call. The answer is a maybe it happened before but I have doubts.

I had some recurring errors before upgrading to 11 so I was not paying good attention to my analytics. And when I upgraded to Angular 11, I also upgraded the analytics version; so the information is not matchy matchy.

This is what it looks like today with Angular 11

I have nothing with this similitude other than this one, but the text does not seem to be related at all.

Luckily, I still have the JavaScript for this, but I believe it got caught with the callstack of RXJS, so the information is not pertinent.
Notice it was happening only on the same set of browsers.
And that error went away with Angular 11.

But, sorry I can't commit to a conclusion here.

petebacondarwin · 2020-12-07T17:44:54Z

I note this issue seems similar: DylanPiercey/set-dom#23

at times (quite rarely, but prominent when using set-dom a lot), DOMParser.parseFromString will return a document, but doc.body is not available until the next tick, even for a simple markup (like <div></div>).

jsgoupil · 2020-12-07T20:17:59Z

What you propose seems what they have done on that other repo. (to fallback if the body is not present)
So it sounds legitimate. Quite the strange bug indeed!

petebacondarwin · 2020-12-09T16:36:35Z

@koto and @cure53 - have you come across this issue in https://github.com/cure53/DOMPurify? I can't see any mention of it. But I do see that you fall back on the inertDocument approach if the doc or documentElement is not truthy. See https://github.com/cure53/DOMPurify/blob/5c78ec59a99fd3d7c73dbc72602a9396e42fbefc/src/purify.js#L370-L383
Is this to deal with this issue?

cure53 · 2020-12-09T16:42:09Z

We have seen many issues with Safari but not this one it seems.

In some browsers, notably a mobile version of webkit on iPad, the result of calling `DOMParser.parseFromString()` returns a document whose `body` property is null until the next tick of the browser. Since this is of no use to us for sanitization, we now fall back to the "inert document" strategy for this case. Fixes angular#39834

petebacondarwin · 2020-12-14T16:30:47Z

I created a PR that should fall back in this scenario... #40107

In some browsers, notably a mobile version of webkit on iPad, the result of calling `DOMParser.parseFromString()` returns a document whose `body` property is null until the next tick of the browser. Since this is of no use to us for sanitization, we now fall back to the "inert document" strategy for this case. Fixes angular#39834

In some browsers, notably a mobile version of webkit on iPad, the result of calling `DOMParser.parseFromString()` returns a document whose `body` property is null until the next tick of the browser. Since this is of no use to us for sanitization, we now fall back to the "inert document" strategy for this case. Fixes #39834 PR Close #40107

angular-automatic-lock-bot · 2021-02-06T16:33:59Z

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.}

AndrewKushnir added browser: safari area: core Issues related to the framework runtime labels Nov 25, 2020

ngbot bot modified the milestone: needsTriage Nov 25, 2020

petebacondarwin added the needs: clarification This issue needs additional clarification from the reporter before the team can investigate. label Nov 25, 2020

ngbot bot modified the milestones: needsTriage, Backlog Nov 25, 2020

petebacondarwin added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs: clarification This issue needs additional clarification from the reporter before the team can investigate. labels Nov 25, 2020

petebacondarwin mentioned this issue Dec 14, 2020

fix(core): ensure sanitizer works if DOMParser return null body #40107

Closed

josephperrott closed this as completed in 8ebac24 Jan 6, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

getInertBodyElement returns null, crashing in _sanitizeHtml #39834

getInertBodyElement returns null, crashing in _sanitizeHtml #39834

jsgoupil commented Nov 25, 2020 •

edited

petebacondarwin commented Nov 25, 2020

jsgoupil commented Nov 25, 2020

petebacondarwin commented Nov 25, 2020

jsgoupil commented Nov 25, 2020

petebacondarwin commented Nov 25, 2020

jsgoupil commented Nov 25, 2020

petebacondarwin commented Nov 25, 2020

jsgoupil commented Nov 25, 2020

jsgoupil commented Dec 4, 2020

petebacondarwin commented Dec 4, 2020

jsgoupil commented Dec 4, 2020

petebacondarwin commented Dec 7, 2020

jsgoupil commented Dec 7, 2020

petebacondarwin commented Dec 7, 2020 •

edited

jsgoupil commented Dec 7, 2020

petebacondarwin commented Dec 9, 2020 •

edited

cure53 commented Dec 9, 2020

petebacondarwin commented Dec 14, 2020

angular-automatic-lock-bot bot commented Feb 6, 2021

getInertBodyElement returns null, crashing in _sanitizeHtml #39834

getInertBodyElement returns null, crashing in _sanitizeHtml #39834

Comments

jsgoupil commented Nov 25, 2020 • edited

🐞 bug report

Affected Package

Is this a regression?

Description

🔬 Minimal Reproduction

🔥 Exception or Error

🌍 Your Environment

petebacondarwin commented Nov 25, 2020

jsgoupil commented Nov 25, 2020

petebacondarwin commented Nov 25, 2020

jsgoupil commented Nov 25, 2020

petebacondarwin commented Nov 25, 2020

jsgoupil commented Nov 25, 2020

petebacondarwin commented Nov 25, 2020

jsgoupil commented Nov 25, 2020

jsgoupil commented Dec 4, 2020

petebacondarwin commented Dec 4, 2020

jsgoupil commented Dec 4, 2020

petebacondarwin commented Dec 7, 2020

jsgoupil commented Dec 7, 2020

petebacondarwin commented Dec 7, 2020 • edited

jsgoupil commented Dec 7, 2020

petebacondarwin commented Dec 9, 2020 • edited

cure53 commented Dec 9, 2020

petebacondarwin commented Dec 14, 2020

angular-automatic-lock-bot bot commented Feb 6, 2021

jsgoupil commented Nov 25, 2020 •

edited

petebacondarwin commented Dec 7, 2020 •

edited

petebacondarwin commented Dec 9, 2020 •

edited