Add support for noreferrer link relation

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

Implement <a rel="noreferrer"> as currently specified: http://www.whatwg.org/specs/web-apps/current-work/#link-type-noreferrer

Currently, HTMLAnchorElement ignores the rel attribute, so that will need to be added as well.

[Nate Chapin]

Created attachment 39091 [details]
First pass

[Alexey Proskuryakov]

Comment on attachment 39091 [details]
First pass

+        * loader/FrameLoaderTypes.h: Add enum for referrer policy
+        (WebCore::):

It's best to edit or just remove the second line - prepare-Changelog is not perfect, so its output is just a starting point or a human, and this is just garbage.

+    , m_linkRelations()

A default constructor will be called anyway, there is no need to invoke it explicitly.

+    ClassNames m_linkRelations;

This data is already stored in attribute map, I don't think you need any data members added to HTMLAnchorElement class.

Instead, it should expose a shouldSendReferrer() method that will in turn call getAttribute().

+    if (referrerPolicy == NoReferrer || !argsReferrer.isEmpty())
         referrer = argsReferrer;

If I'm reading it correctly, this code depends on argsReferrer being empty if referrerPolicy is NoReferrer. Please add an assertion to this effect.

Also, can this result in an empty Referer header being sent with some or all network back-ends? Do we want this, or do we want to omit it completely? Formally, servers should not make any distinction between those, so we probably want to save a few bytes sent over the wire.

+    , m_suppressOpenerInNewFrame(false)

The ChangeLog should promptly mention this (probably before per-function logs), because this is not entirely expected.

Could you add a test case for normal navigation that just replaces top frame content, and another for navigation in a subframe? Also, what about navigating an existing subframe? The spec seems to be silent about this, which is strange:

<iframe name="a" src="a.html"></iframe>
<a href="b.html" target="a" rel="noreferrer">link</a>

+    if (referrerPolicy == NoReferrer) {
+        m_suppressOpenerInNewFrame = true;

Does it need to be reset to false otherwise? I'm not 100% sure about the role of FrameLoader, but I think it is re-used for future navigations.

I think that this (two consecutive navigations with different noreferrer values) should be tested, too.

HTML5 says that AREA elements also support noreferrer. Will this be added in a followup patch?

+    newEvent.initMouseEvent("click", false, false, window, 0, 10, 10, 10, 10, false, false, false, false, 0, target);

Will a simple click() work? I have some doubts whether a synthetic mouse event should be able to do this - we already block synthetic keyboard events in default handlers.

+    // Prevent from being cached.
+    header("Cache-Control: no-cache, private, max-age=0");

Counter-intuitively, the directive to prevent caching is "no-store", not "no-cache". The latter only affects intermediate caches.

[Nate Chapin]

Clarifications and a couple questions inline...

(In reply to comment #2)
> (From update of attachment 39091 [details])
> +        * loader/FrameLoaderTypes.h: Add enum for referrer policy
> +        (WebCore::):
> 
> It's best to edit or just remove the second line - prepare-Changelog is not
> perfect, so its output is just a starting point or a human, and this is just
> garbage.
> 
> +    , m_linkRelations()
> 
> A default constructor will be called anyway, there is no need to invoke it
> explicitly.
> 
> +    ClassNames m_linkRelations;
> 
> This data is already stored in attribute map, I don't think you need any data
> members added to HTMLAnchorElement class.
> 
> Instead, it should expose a shouldSendReferrer() method that will in turn call
> getAttribute().
> 
> +    if (referrerPolicy == NoReferrer || !argsReferrer.isEmpty())
>          referrer = argsReferrer;
> 
> If I'm reading it correctly, this code depends on argsReferrer being empty if
> referrerPolicy is NoReferrer. Please add an assertion to this effect.

I think that's a safe assumption, but I'm not absolutely certain.  Regardless, I'll move the NoReferrer check down next to the FrameLoader::shouldSendReferrer() check later in the function, which is probably where it belongs anyway.

> 
> Also, can this result in an empty Referer header being sent with some or all
> network back-ends? Do we want this, or do we want to omit it completely?
> Formally, servers should not make any distinction between those, so we probably
> want to save a few bytes sent over the wire.
I was assuming that identical behavior to the FrameLoader::shouldSendReferrer() usages was fine, but I don't know enough about the varioius network backends to say for certain.

> 
> +    , m_suppressOpenerInNewFrame(false)
> 
> The ChangeLog should promptly mention this (probably before per-function logs),
> because this is not entirely expected.
> 
> Could you add a test case for normal navigation that just replaces top frame
> content, and another for navigation in a subframe? Also, what about navigating
> an existing subframe? The spec seems to be silent about this, which is strange:

Will do.  I think I'll end up with the following cases:
* Simple noreferrer  navigation
* Subframe noreferrer navigation
* noreferrer navigation follow by referrer navigation
* noreferrer in new window (also verifying window.opener is empty)

Is that sufficient?  Do I need to check window.opener in all those cases, or is it sufficient to just demonstrate it's proper behavior in the new window case?

> 
> <iframe name="a" src="a.html"></iframe>
> <a href="b.html" target="a" rel="noreferrer">link</a>
> 
> +    if (referrerPolicy == NoReferrer) {
> +        m_suppressOpenerInNewFrame = true;
> 
> Does it need to be reset to false otherwise? I'm not 100% sure about the role
> of FrameLoader, but I think it is re-used for future navigations.
> 
> I think that this (two consecutive navigations with different noreferrer
> values) should be tested, too.

My thought was that urlSelected would be the only place this gets set to true (since that's the place HTMLAnchorElement::defaultEventHandler() calls into FrameLoader), and that it would then set it to false when completed. I don't know if that's the right answer, but it seemed to handle the future navigation case.  

> 
> HTML5 says that AREA elements also support noreferrer. Will this be added in a
> followup patch?

HTMLAreaElement inherits from HTMLAnchorElement and uses its defaultEventHandler, so this should Just Work.  I've manually verified that an <area> with rel="noreferrer" doesn't send a referrer and follows the same code paths as an <a>.  Do you want a layout test for this as well?

> 
> +    newEvent.initMouseEvent("click", false, false, window, 0, 10, 10, 10, 10,
> false, false, false, false, 0, target);
> 
> Will a simple click() work? I have some doubts whether a synthetic mouse event
> should be able to do this - we already block synthetic keyboard events in
> default handlers.

It worked on my local machine, and this is basically a copy/paste from other layout tests I've written, so I think it's fine.

> 
> +    // Prevent from being cached.
> +    header("Cache-Control: no-cache, private, max-age=0");
> 
> Counter-intuitively, the directive to prevent caching is "no-store", not
> "no-cache". The latter only affects intermediate caches.

[Alexey Proskuryakov]

> Is that sufficient?  Do I need to check window.opener in all those cases, or is
> it sufficient to just demonstrate it's proper behavior in the new window case?

It's definitely best to test as much as practical.


> My thought was that urlSelected would be the only place this gets set to true
> (since that's the place HTMLAnchorElement::defaultEventHandler() calls into
> FrameLoader), and that it would then set it to false when completed. I don't
> know if that's the right answer, but it seemed to handle the future navigation
> case.  

So, m_suppressOpenerInNewFrame is supposed to always be false when entering this function? Could you add an ASSERT to this effect?

With an assertion and a test, it would be just fine.

> HTMLAreaElement inherits from HTMLAnchorElement and uses its
> defaultEventHandler, so this should Just Work.  I've manually verified that an
> <area> with rel="noreferrer" doesn't send a referrer and follows the same code
> paths as an <a>.  Do you want a layout test for this as well?

Yes, we definitely need at least one test fro AREA. Probably no need to repeat all tests we have for anchor.

> > Will a simple click() work? I have some doubts whether a synthetic mouse event
> > should be able to do this - we already block synthetic keyboard events in
> > default handlers.
> 
> It worked on my local machine, and this is basically a copy/paste from other
> layout tests I've written, so I think it's fine.

Sorry, I was not clear enough. What I'm saying is that we might consider changing this in the future, so it would be easier if we didn't have to change this test if we do. Not a big deal either way.

[Nate Chapin]

> Sorry, I was not clear enough. What I'm saying is that we might consider
> changing this in the future, so it would be easier if we didn't have to change
> this test if we do. Not a big deal either way.

It appears I can either test window.opener or I can make use of eventSender, but not both.

The issue is that my attempts to set window.opener manually and have it persist through page load have failed (If there's a trick I need to be using, I'd love to know...).  Therefore, I have to open a new window to get window.opener set, and new windows opened in the test shell apparently don't have eventSender attached to them.

[Nate Chapin]

Created attachment 39463 [details]
Attempt #2

[Alexey Proskuryakov]

Comment on attachment 39463 [details]
Attempt #2

+    return !ClassNames(getAttribute(relAttr), document()->inCompatMode()).contains("noreferrer");

I don't think we should be using ClassNames on something that isn't a list of class names.

> So, m_suppressOpenerInNewFrame is supposed to always be false when entering
> this function? Could you add an ASSERT to this effect?

Is there a reason not to add this ASSERT?

+    if (!m_suppressOpenerInNewFrame)
+        mainFrame->loader()->setOpener(frame.get());

One thing I didn't ask about originally - what makes us believe that it's ok not to set the internal opener attribute? One example that makes me curious is: can we programmatically navigate a frame with a different origin, but whose opener is same origin? The tests are in FrameLoader::shouldAllowNavigation(), and they rely on opener() value.

Tentatively, I'd say yes, but the patch appears to prevent this.

[Nate Chapin]

(In reply to comment #7)
> (From update of attachment 39463 [details])
> +    return !ClassNames(getAttribute(relAttr),
> document()->inCompatMode()).contains("noreferrer");
> 
> I don't think we should be using ClassNames on something that isn't a list of
> class names.

My understanding is that ClassNames was originally more generically named, and that it was given its current name because its only use was to store and access space separated class names (http://trac.webkit.org/changeset/28722).  The behavior of these class strings (as far as I and everyone that I've asked can tell) precisely matches the behvaior of space separated lists in HTML5 (http://www.whatwg.org/specs/web-apps/current-work/#space-separated-tokens).  I think using this class is the right answer and the question is primarily when and how it gets renamed back to something more generic :-)

Sorry for not explaining sooner.  Does that seem reasonable?

> 
> > So, m_suppressOpenerInNewFrame is supposed to always be false when entering
> > this function? Could you add an ASSERT to this effect?
> 
> Is there a reason not to add this ASSERT?

Doh, sorry.  I lost that suggestion somehow.  Will do.

> 
> +    if (!m_suppressOpenerInNewFrame)
> +        mainFrame->loader()->setOpener(frame.get());
> 
> One thing I didn't ask about originally - what makes us believe that it's ok
> not to set the internal opener attribute? One example that makes me curious is:
> can we programmatically navigate a frame with a different origin, but whose
> opener is same origin? The tests are in FrameLoader::shouldAllowNavigation(),
> and they rely on opener() value.
> 
> Tentatively, I'd say yes, but the patch appears to prevent this.

I'll look into it.

[Alexey Proskuryakov]

Comment on attachment 39463 [details]
Attempt #2

> Sorry for not explaining sooner.  Does that seem reasonable?

Probably - it's the first time I see this class, so I do not have a strong opinion. Does rel attribute value parsing depend on compatMode?

It also seems quite wasteful to temporarily construct a class with a vector of AtomicStrings just to check if there's a value in the space-separated list.

However, if we decide to reuse the class, it needs to be renamed back.

Marking r-, as the patch will be updated, at least per your above comment.

[Nate Chapin]

(In reply to comment #9)
> (From update of attachment 39463 [details])
> > Sorry for not explaining sooner.  Does that seem reasonable?
> 
> Probably - it's the first time I see this class, so I do not have a strong
> opinion. Does rel attribute value parsing depend on compatMode?
> 
> It also seems quite wasteful to temporarily construct a class with a vector of
> AtomicStrings just to check if there's a value in the space-separated list.

In my first revision of the patch, I wasn't temporarily constructing the class, it was a member variable of HTMLAnchorElement.  My thought at the time was that, as more of HTML5 gets implemented, the other link relations will be added, and that this was a decent way of storing those attributes (and also consistent with how ClassNames is used the couple of other places it appears).

> 
> However, if we decide to reuse the class, it needs to be renamed back.

If I do end up using it, do you have a preference whether I rename ClassNames before or after this patch gets completed and committed?

> 
> Marking r-, as the patch will be updated, at least per your above comment.

[Alexey Proskuryakov]

> In my first revision of the patch, I wasn't temporarily constructing the class,
> it was a member variable of HTMLAnchorElement.  My thought at the time was
> that, as more of HTML5 gets implemented, the other link relations will be
> added, and that this was a decent way of storing those attributes (and also

One significant difference between class names and relation names is that the former can be arbitrary, but the latter come from a predefined set.

> If I do end up using it, do you have a preference whether I rename ClassNames
> before or after this patch gets completed and committed?

I don't have a particularly strong opinion on this matter.

[Nate Chapin]

(In reply to comment #11)
> > In my first revision of the patch, I wasn't temporarily constructing the class,
> > it was a member variable of HTMLAnchorElement.  My thought at the time was
> > that, as more of HTML5 gets implemented, the other link relations will be
> > added, and that this was a decent way of storing those attributes (and also
> 
> One significant difference between class names and relation names is that the
> former can be arbitrary, but the latter come from a predefined set.
> 
> > If I do end up using it, do you have a preference whether I rename ClassNames
> > before or after this patch gets completed and committed?
> 
> I don't have a particularly strong opinion on this matter.

You make a good point, I hadn't thought about the fact that relations are a predefined set.  Perhaps we should parse once in HTMLAnchorElement::parseMappedAttribute() and store bools or a bitmask?  It might look a bit silly when only one of the link relations is implemented though.

[Nate Chapin]

Created attachment 41062 [details]
Attempt #3

Requested ASSERT added.

Changed implementation of rel attribute in HTMLAnchorElement.  Link relations are stored as a bitmask (only one bit is used at the moment, but the definitions are there and ready to be uncommented as more are implemented).  HTMLAnchorElement::setRel() parses a rel string using ClassNames and initializes the bitmask.  hasRel() returns whether a given link relation is set based on the bitmask.  I'll plan on renaming ClassNames in a later patch.

> One thing I didn't ask about originally - what makes us believe that it's ok
> not to set the internal opener attribute? One example that makes me curious is:
> can we programmatically navigate a frame with a different origin, but whose
> opener is same origin? The tests are in FrameLoader::shouldAllowNavigation(),
> and they rely on opener() value.
> 
> Tentatively, I'd say yes, but the patch appears to prevent this.

My interpretation of what the spec says about rel="noreferrer" is that it is essentially a request by the current page for a total severing of any connection between it and a destination page.  As such, unless I'm misinterpreting your question, I think we want to just not set the internal opener attribute to prevent any connection to the opener under any circumstances, even if it is same origin.

[Alexey Proskuryakov]

Comment on attachment 41062 [details]
Attempt #3

> +// const unsigned long RELATION_ALTERNATE   = 0x00000001;

We only use uppercase names for #defined constants - these should use normal interCaps style. But I'd actually suggest using an enum for these (enum members should user InterCaps with an initial capital letter).

> +    void setRel(const AtomicString&);

Why is this an atomic string? Rel values are arbitrary combinations of tokens, and you just parse it immediately anyway. Please use plain String.

> +    unsigned long m_linkRelations;

No need to take 8 bytes on 64 bit systems; I suggest using uint32_t.

r=me on a condition that you address these comments.

[Nate Chapin]

Landed: http://trac.webkit.org/changeset/49809
Fix for win and qt: http://trac.webkit.org/changeset/49812

Thanks for your help, ap!