Skip to content

Commit

Permalink
Revert "Move Cors Taint check into DataSource, out of WMPI"
Browse files Browse the repository at this point in the history
This reverts commit 7565e23.

Reason for revert: Breaking https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome


Original change's description:
> Move Cors Taint check into DataSource, out of WMPI
>
> 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}

Bug: 1377053
Change-Id: Ieadc2b51513099584552a7a9000c907eed2f725b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4004956
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067398}
  • Loading branch information
fdegros authored and Chromium LUCI CQ committed Nov 4, 2022
1 parent 3b46291 commit c745d6e
Show file tree
Hide file tree
Showing 10 changed files with 13 additions and 44 deletions.
8 changes: 0 additions & 8 deletions chrome/services/media_gallery_util/ipc_data_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,3 @@ 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: 0 additions & 1 deletion chrome/services/media_gallery_util/ipc_data_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ 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: 0 additions & 4 deletions media/base/data_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,6 @@ 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: 0 additions & 5 deletions media/filters/file_data_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,4 @@ 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: 0 additions & 1 deletion media/filters/file_data_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ 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: 0 additions & 5 deletions media/filters/memory_data_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,4 @@ 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: 0 additions & 1 deletion media/filters/memory_data_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ 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,15 +298,6 @@ 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,8 +83,6 @@ 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: 13 additions & 8 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 `data_source_`->cors_mode() directly, instead
// access what we need from `mb_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,10 +1529,18 @@ bool WebMediaPlayerImpl::WouldTaintOrigin() const {
return true;
}

// 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;
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();
}

double WebMediaPlayerImpl::MediaTimeForTimeValue(double timeValue) const {
Expand Down Expand Up @@ -4135,9 +4143,6 @@ 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 c745d6e

Please sign in to comment.