Bug 73184 - Out-of-band text tracks must load with the default origin behaviour set to fail.
Summary: Out-of-band text tracks must load with the default origin behaviour set to fail.
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: InRadar
Depends on:
Blocks: 43668
  Show dependency treegraph
 
Reported: 2011-11-27 20:41 PST by Eric Carlson
Modified: 2011-12-05 07:41 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (26.37 KB, patch)
2011-11-29 11:43 PST, Eric Carlson
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (27.86 KB, patch)
2011-11-30 10:27 PST, Eric Carlson
sam: 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 2011-11-27 20:41:30 PST
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#sourcing-out-of-band-text-tracks says:

If URL is not the empty string, and its origin is the same as the media element's Document's origin, then fetch URL, from the media element's Document's origin, with the force same-origin flag set.
Comment 1 Radar WebKit Bug Importer 2011-11-27 20:41:50 PST
<rdar://problem/10489317>
Comment 2 Eric Carlson 2011-11-29 11:42:51 PST
Actually, the current version of the spec mentions CORS:

If URL is not the empty string, perform a potentially CORS-enabled fetch of URL, with the mode being the state of the media element's crossorigin content attribute, the origin being the origin of the media element's Document, and the default origin behaviour set to fail.

The resource obtained in this fashion, if any, contains the text track data. If any data is obtained, it is by definition CORS-same-origin (cross-origin resources that are not suitably CORS-enabled do not get this far).
Comment 3 Eric Carlson 2011-11-29 11:43:35 PST
Created attachment 117011 [details]
Proposed patch
Comment 4 Sam Weinig 2011-11-29 13:08:22 PST
Comment on attachment 117011 [details]
Proposed patch

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

Most cached resources do there cross origin checks in CachedResourceLoader::canRequest() (e.g. XSLTStyleSheet), can we do it for TextTracks there too?

> Source/WebCore/html/HTMLTrackElement.cpp:243
> +    if (!document()->contentSecurityPolicy()->allowMediaFromSource(url)) {
> +        DEFINE_STATIC_LOCAL(String, consoleMessage, ("Cross-origin image load denied by Content Security Policy."));
> +        document()->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, consoleMessage);
> +        LOG(Media, "HTMLTrackElement::canLoadUrl(%s) -> rejected by Content Security Policy", urlForLogging(url).utf8().data());
> +        return false;

CSP denying the load is not the same
Comment 5 Eric Carlson 2011-11-29 15:28:37 PST
(In reply to comment #4)
> (From update of attachment 117011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117011&action=review
> 
> Most cached resources do there cross origin checks in CachedResourceLoader::canRequest() (e.g. XSLTStyleSheet), can we do it for TextTracks there too?
> 
I modeled this after ImageLoader because it was the only other element I found that used the element's "crossorigin" attribute. It does the check in ImageLoader::updateFromElement before it asks the cached resource loader to load the image.

In order to move the check down into CachedResourceLoader::canRequest I will have to change CachedResourceLoader::requestResource so it has the 'crossorgin' attribute value.
Comment 6 WebKit Review Bot 2011-11-30 05:44:20 PST
Comment on attachment 117011 [details]
Proposed patch

Attachment 117011 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10697165

New failing tests:
media/track/track-add-track.html
http/tests/security/text-track-crossorigin.html
Comment 7 Eric Carlson 2011-11-30 10:27:30 PST
Created attachment 117216 [details]
Updated patch

Updated the test results and fixed comment logged when CSP denies a load.
Comment 8 Eric Carlson 2011-12-05 07:41:25 PST
http://trac.webkit.org/changeset/101999