Bug 78614 – Set Referrer header for media downloads

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

WebKit should be able to set the Referer header sent when a media engine downloads an audio or video file, so servers can know the address of the webpage that links to it. This is important for logging and simple security purposes.

[Eric Carlson]

Created attachment 127441 [details]
Proposed patch

[Adam Barth]

Comment on attachment 127441 [details]
Proposed patch

SecurityPolicy / referrer stuff looks good to me.  The ObjC part mystifies me.

[Alexey Proskuryakov]

Comment on attachment 127441 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127441&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:3827
> +    return SecurityPolicy::generateReferrerHeader(document()->referrerPolicy(), KURL(ParsedURLString, url), frame->loader()->outgoingReferrer());

I tried to follow the code, but couldn't find what guarantees that the url argument is a result of calling string() on a valid KURL. This is the only case when ParsedURLString-style constructor can be used.

[Eric Carlson]

(In reply to comment #3)
> (From update of attachment 127441 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127441&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3827
> > +    return SecurityPolicy::generateReferrerHeader(document()->referrerPolicy(), KURL(ParsedURLString, url), frame->loader()->outgoingReferrer());
> 
> I tried to follow the code, but couldn't find what guarantees that the url argument is a result of calling string() on a valid KURL. This is the only case when ParsedURLString-style constructor can be used.

HTMLMediaElement::loadResource passes a valid KURL to MediaPlayer::load,  MediaPlayer::loadWithNextMediaEngine passes url.string() to the media engine.

[Alexey Proskuryakov]

Comment on attachment 127441 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127441&action=review

Shouldn't the test have custom results on some platforms now? Or is it already skipped there?

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Please mention that a test was modified to cover this.

> Source/WebCore/html/HTMLMediaElement.cpp:3825
> +        return emptyString();

Probably doesn't matter here, but I'd have returned a null string here, signaling that we don't have a value.

>>> Source/WebCore/html/HTMLMediaElement.cpp:3827
>>> +    return SecurityPolicy::generateReferrerHeader(document()->referrerPolicy(), KURL(ParsedURLString, url), frame->loader()->outgoingReferrer());
>> 
>> I tried to follow the code, but couldn't find what guarantees that the url argument is a result of calling string() on a valid KURL. This is the only case when ParsedURLString-style constructor can be used.
> 
> HTMLMediaElement::loadResource passes a valid KURL to MediaPlayer::load,  MediaPlayer::loadWithNextMediaEngine passes url.string() to the media engine.

OK. I wish compiler could enforce this (there was some talk about adding ParsedURLString class for tis purpose).

> Source/WebCore/platform/graphics/MediaPlayer.cpp:939
> +    if (m_mediaPlayerClient)
> +        return m_mediaPlayerClient->mediaPlayerReferrer(url);
> +    return emptyString();

Null string here, too, and we usually prefer early return to nesting normal code path.

> Source/WebCore/platform/graphics/MediaPlayer.h:171
> +    virtual String mediaPlayerReferrer(const String&) const { return emptyString(); }

Null string again (what are the subclasses that don't need to implement?)

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:265
> +    if (!referrer.isEmpty()) {

This check should remain isEmpty even if you change other places to make null strings. I'm not really sure why, but it seems like common pattern.

> LayoutTests/ChangeLog:9
> +            anything bug video-referer.html.

s/bug/but/

[Eric Carlson]

Created attachment 128016 [details]
Updated patch.

HTMLMediaElement already knows the current url so I simplified the logic by removing the url parameter from the referrer methods. Realized that the layout test did not need to use two cgis, so consolidated their functionality and removed one.

[Eric Carlson]

 http://trac.webkit.org/changeset/108387

[James Robinson]

This test seems to fail on the chromium bots:


CONSOLE MESSAGE: line 30: Uncaught TypeError: Cannot read property 'code' of null
EVENT(error)
EVENT(canplay)
Tests that the media player will send the relevant referer when requesting the media file.


is this error expected?

[Eric Carlson]

(In reply to comment #8)
> This test seems to fail on the chromium bots:
> 
> 
> CONSOLE MESSAGE: line 30: Uncaught TypeError: Cannot read property 'code' of null
> EVENT(error)
> EVENT(canplay)
> Tests that the media player will send the relevant referer when requesting the media file.
> 
> 
> is this error expected?

No, the error is not expected. The old test "succeeded" if the referrer header had any value at all, now it fails unless the correct value is passed so maybe the Chromium media engine does not send the correct referrer? I am not set up to debug the failure so I wrote up https://bugs.webkit.org/show_bug.cgi?id=79239 and skipped the test.

[James Robinson]

OK, thank you for investigating and filing that bug.