Skip to content

Commit

Permalink
Move Cors Taint check into DataSource, out of WMPI
Browse files Browse the repository at this point in the history
This removes a few more instances of |mb_data_source_|.

Bug: 1377053
Change-Id: I589aa72bab51481f7b9613f956da0f0fb57a4293
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3988658
Reviewed-by: Will Cassella <cassew@chromium.org>
Commit-Queue: Ted (Chromium) Meyer <tmathmeyer@chromium.org>
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067340}
  • Loading branch information
tm-chromium authored and Chromium LUCI CQ committed Nov 4, 2022
1 parent 29fa971 commit 7565e23
Show file tree
Hide file tree
Showing 10 changed files with 44 additions and 13 deletions.
8 changes: 8 additions & 0 deletions chrome/services/media_gallery_util/ipc_data_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,11 @@ bool IPCDataSource::PassedTimingAllowOriginCheck() {
// so that the mojo interface can be queried.
return false;
}

bool IPCDataSource::WouldTaintOrigin() {
// The mojo ipc channel doesn't support this yet, so cautiously return true,
// for now.
// TODO(crbug/1377053): Rework this method to be asynchronous, if possible,
// so that the mojo interface can be queried.
return true;
}
1 change: 1 addition & 0 deletions chrome/services/media_gallery_util/ipc_data_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class IPCDataSource : public media::DataSource {
bool IsStreaming() override;
void SetBitrate(int bitrate) override;
bool PassedTimingAllowOriginCheck() override;
bool WouldTaintOrigin() override;

private:
// Media data read helpers: must be run on the utility thread.
Expand Down
4 changes: 4 additions & 0 deletions media/base/data_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ class MEDIA_EXPORT DataSource {
// check.
virtual bool PassedTimingAllowOriginCheck() = 0;

// DataSource implementations that might make requests must ensure the value
// is accurate for cross origin resources.
virtual bool WouldTaintOrigin() = 0;

// Assume fully buffered by default.
virtual bool AssumeFullyBuffered() const;

Expand Down
5 changes: 5 additions & 0 deletions media/filters/file_data_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,9 @@ bool FileDataSource::PassedTimingAllowOriginCheck() {
return true;
}

bool FileDataSource::WouldTaintOrigin() {
// There are no HTTP responses, so this can safely return false.
return false;
}

} // namespace media
1 change: 1 addition & 0 deletions media/filters/file_data_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class MEDIA_EXPORT FileDataSource : public DataSource {
bool IsStreaming() override;
void SetBitrate(int bitrate) override;
bool PassedTimingAllowOriginCheck() final;
bool WouldTaintOrigin() final;

// Unit test helpers. Recreate the object if you want the default behaviour.
void force_read_errors_for_testing() { force_read_errors_ = true; }
Expand Down
5 changes: 5 additions & 0 deletions media/filters/memory_data_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,9 @@ bool MemoryDataSource::PassedTimingAllowOriginCheck() {
return true;
}

bool MemoryDataSource::WouldTaintOrigin() {
// There are no HTTP responses, so this can safely return false.
return false;
}

} // namespace media
1 change: 1 addition & 0 deletions media/filters/memory_data_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class MEDIA_EXPORT MemoryDataSource final : public DataSource {
bool IsStreaming() final;
void SetBitrate(int bitrate) final;
bool PassedTimingAllowOriginCheck() final;
bool WouldTaintOrigin() final;

private:
const std::string data_string_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,15 @@ bool MultiBufferDataSource::PassedTimingAllowOriginCheck() {
return url_data_->passed_timing_allow_origin_check();
}

bool MultiBufferDataSource::WouldTaintOrigin() {
// When the resource is redirected to another origin we think of it as
// tainted. This is actually not specified, and is under discussion.
// See https://github.com/whatwg/fetch/issues/737.
if (!HasSingleOrigin() && cors_mode() == UrlData::CORS_UNSPECIFIED)
return true;
return IsCorsCrossOrigin();
}

UrlData::CorsMode MultiBufferDataSource::cors_mode() const {
return url_data_->cors_mode();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ class PLATFORM_EXPORT MultiBufferDataSource : public media::DataSource {

bool PassedTimingAllowOriginCheck() override;

bool WouldTaintOrigin() override;

// Returns the CorsMode of the underlying UrlData.
UrlData::CorsMode cors_mode() const;

Expand Down
21 changes: 8 additions & 13 deletions third_party/blink/renderer/platform/media/web_media_player_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ void WebMediaPlayerImpl::DoLoad(LoadType load_type,
// See https://crbug.com/936566.
//
// The credentials mode also has repercussions in WouldTaintOrigin(), but we
// access what we need from `mb_data_source_`->cors_mode() directly, instead
// access what we need from `data_source_`->cors_mode() directly, instead
// of storing it here.
allow_media_player_renderer_credentials_ = cors_mode != kCorsModeAnonymous;
#endif // BUILDFLAG(IS_ANDROID)
Expand Down Expand Up @@ -1529,18 +1529,10 @@ bool WebMediaPlayerImpl::WouldTaintOrigin() const {
return true;
}

if (!mb_data_source_)
return false;

// When the resource is redirected to another origin we think it as
// tainted. This is actually not specified, and is under discussion.
// See https://github.com/whatwg/fetch/issues/737.
if (!mb_data_source_->HasSingleOrigin() &&
mb_data_source_->cors_mode() == UrlData::CORS_UNSPECIFIED) {
return true;
}

return mb_data_source_->IsCorsCrossOrigin();
// TODO(crbug/1377053): The default |false| value might have to be
// re-considered for MediaPlayerRenderer, but for now, leave behavior the
// same as it was.
return data_source_ ? data_source_->WouldTaintOrigin() : false;
}

double WebMediaPlayerImpl::MediaTimeForTimeValue(double timeValue) const {
Expand Down Expand Up @@ -4143,6 +4135,9 @@ bool WebMediaPlayerImpl::PassedTimingAllowOriginCheck() const {
// revealed via the MediaSource or WebMediaPlayer that's using MSE.
// TODO(1266991): Ensure that this returns the correct value for HLS media,
// based on the TAO checks performed on those resources.
// TODO(crbug/1377053): The default |true| value might have to be
// re-considered for MediaPlayerRenderer, but for now, leave behavior the
// same as it was.
return data_source_ ? data_source_->PassedTimingAllowOriginCheck() : true;
}

Expand Down

0 comments on commit 7565e23

Please sign in to comment.