1426234 - sub-resource integrity code in nsContentSink::ProcessStyleLink makes no sense

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

The code looks like this:

  NS_ASSERTION(!aElement ||
               aElement->NodeType() == nsIDOMNode::PROCESSING_INSTRUCTION_NODE,
               "We only expect processing instructions here");

  nsAutoString integrity;
  if (aElement) {
    aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity, integrity);
  }
  if (!integrity.IsEmpty()) {

That last "if" will never test true, because GetAttr() on a PI node will never return anything nonempty.  What is this code trying to do, exactly?

Comment 1

[François Marier [:francois]]

I honestly can't remember why that's in there. It's quite possible it was never necessary/useful and that this `integrity` variable could just be replaced with an empty string.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Is SRI expected to work on xml-stylesheet PIs?  If so, how is it supposed to work?

Comment 3

[François Marier [:francois]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> Is SRI expected to work on xml-stylesheet PIs?  If so, how is it supposed to
> work?

I don't believe so. It only works on <link rel="stylesheet"> elements.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #0)
> That last "if" will never test true, because GetAttr() on a PI node will
> never return anything nonempty.  What is this code trying to do, exactly?

I traced through Bug 992096 to figure out why that's there, but I couldn't. The only mentiond of nsContentSink.cpp was in comment 67. Anyway, I suppose in that case we can just remove it, right? If so, can you remove it Francois?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Actually, I might just remove all this code in bug 1426432.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Fix preexisting bug where disabling prefetch/preload would turn off HTTP Link headers for stylesheets

MozReview-Commit-ID: 6FcW21tODIZ

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop calling nsContentSink::ProcessStyleLink for XML PIs

nsContentSink::ProcessStyleLink only does something useful for CSS
stylesheets.  But for those, nsXMLContentSink::HandleProcessingInstruction
already returns early before calling nsXMLContentSink::ProcessStyleLink.  So
the only way that nsContentSink::ProcessStyleLink can be reached for an XML PI
is already a no-op and there's no reason to make that call.

MozReview-Commit-ID: 5XPnvoY0T56

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Simplify ProcessStyleLink now that it's only called for Link headers

MozReview-Commit-ID: 2Gn3AsYwVmI

Comment 10

[Boris Zbarsky [:bzbarsky]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0ca1c37800c76186d942cdc158e38cd5075f6e5

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8938133 [details] [diff] [review]
part 2.  Stop calling nsContentSink::ProcessStyleLink for XML PIs

Review of attachment 8938133 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xml/nsXMLContentSink.cpp
@@ +690,5 @@
> +                                       const nsAString& aMedia,
> +                                       const nsAString& aReferrerPolicy,
> +                                       bool* aWasXSLT)
> +{
> +  NS_ConvertUTF16toUTF8 type(aType);

Why do we need to do this conversion here? It looks to me like TEXT_XSL and co. are just string literals, so we should be able to use `nsAString::LowerCaseEqualsLiteral` to just do a direct comparison without re-encoding.

We'll still have to encode it to call NS_CheckContentLoadPolicy, but we can do that later in the function.

@@ +710,5 @@
> +    // don't load alternate XSLT
> +    return NS_OK;
> +  }
> +  // LoadXSLStyleSheet needs a mDocShell.
> +  if (!mDocShell)

nit: since you're moving this code anyway, please add {} around the block.

::: dom/xml/nsXMLFragmentContentSink.cpp
@@ +361,3 @@
>  {
> +  // Our one caller doesn't care about whether we're XSLT.
> +  MOZ_ASSERT(!aWasXSLT);

nit: Please move the above comment into a MOZ_ASSERT explanation string.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> We'll still have to encode it to call NS_CheckContentLoadPolicy, but we can do that later in the function.

Right.  But I guess we might not reach that, good point.  I'll move it.

> nit: since you're moving this code anyway, please add {} around the block.

Will do.

> nit: Please move the above comment into a MOZ_ASSERT explanation string.

Nice catch!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fca3912d1077
part 1.  Fix preexisting bug where disabling prefetch/preload would turn off HTTP Link headers for stylesheets.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/2569ef6ea526
part 2.  Stop calling nsContentSink::ProcessStyleLink for XML PIs. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/55bd77a4e5bc
part 3.  Simplify ProcessStyleLink now that it's only called for Link headers.  r=mystor

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/fca3912d1077
https://hg.mozilla.org/mozilla-central/rev/2569ef6ea526
https://hg.mozilla.org/mozilla-central/rev/55bd77a4e5bc