Closed Bug 1426234 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: DOM: Security, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files)

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?
Flags: needinfo?(ckerschb)
Flags: needinfo?(amarchesini)
Flags: needinfo?(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.
Flags: needinfo?(francois)
Is SRI expected to work on xml-stylesheet PIs?  If so, how is it supposed to work?
(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.
(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?
Flags: needinfo?(ckerschb) → needinfo?(francois)
Actually, I might just remove all this code in bug 1426432.
Depends on: 1426432
Flags: needinfo?(francois)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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
Attachment #8938133 - Flags: review?(nika)
No longer depends on: 1426432
Duplicate of this bug: 1426432
Attachment #8938132 - Flags: review?(nika) → review+
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.
Attachment #8938133 - Flags: review?(nika) → review+
Attachment #8938134 - Flags: review?(nika) → review+
> 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!
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/fca3912d1077
https://hg.mozilla.org/mozilla-central/rev/2569ef6ea526
https://hg.mozilla.org/mozilla-central/rev/55bd77a4e5bc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.