Bug 78614 - Set Referrer header for media downloads
Summary: Set Referrer header for media downloads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-14 09:54 PST by Eric Carlson
Modified: 2012-02-22 10:59 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (11.13 KB, patch)
2012-02-16 14:18 PST, Eric Carlson
ap: review+
Details | Formatted Diff | Diff
Updated patch. (12.33 KB, patch)
2012-02-21 12:07 PST, Eric Carlson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2012-02-14 09:54:10 PST
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.
Comment 1 Eric Carlson 2012-02-16 14:18:32 PST
Created attachment 127441 [details]
Proposed patch
Comment 2 Adam Barth 2012-02-16 15:17:26 PST
Comment on attachment 127441 [details]
Proposed patch

SecurityPolicy / referrer stuff looks good to me.  The ObjC part mystifies me.
Comment 3 Alexey Proskuryakov 2012-02-16 16:48:31 PST
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.
Comment 4 Eric Carlson 2012-02-17 11:12:02 PST
(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.
Comment 5 Alexey Proskuryakov 2012-02-20 15:22:33 PST
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/
Comment 6 Eric Carlson 2012-02-21 12:07:46 PST
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.
Comment 7 Eric Carlson 2012-02-21 13:08:49 PST
 http://trac.webkit.org/changeset/108387
Comment 8 James Robinson 2012-02-21 16:55:30 PST
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?
Comment 9 Eric Carlson 2012-02-22 07:46:40 PST
(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.
Comment 10 James Robinson 2012-02-22 10:59:14 PST
OK, thank you for investigating and filing that bug.