New issue
Advanced search Search tips

Issue 661283 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Feature



Sign in to add a comment

Feature Policy: Support sync-xhr feature

Project Member Reported by iclell...@chromium.org, Nov 1 2016

Issue description

Add support for the "sync-xhr" feature in Feature Policy.

The feature has been defined already in FeaturePolicy.cpp as kSyncXHR. It should be checked before allowing a synchronous XMLHttpRequest.open call to succeed.

If not allowed by policy, the call to XMLHttpRequest.open should fail with an InvalidAccessError if the |async| flag is set to false.
 
Components: Blink>FeaturePolicy
The original failure mode described in #0 may not be practical for the open web; there may be pages which depend on XHR.open succeeding and which never check for InvalidAccessError (and which may generally expect the call to succeed, especially if it is transferred from the same origin server as the requesting document)

In that case, we may want to consider unloading a document which violates policy, rather than possibly failing (effectively) silently. In a policy-aware document -- one which has set its own policy for syncxhr -- we may choose to fail with the IAError instead.
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/224ff964f4e15546d6a11fe39596e60cee2120d2

commit 224ff964f4e15546d6a11fe39596e60cee2120d2
Author: Ian Clelland <iclelland@google.com>
Date: Thu Oct 12 18:09:20 2017

Add SyncXHR as an experimental policy-controlled feature

This CL disables synchronous XHR in documents by throwing an
InvalidAccessError on open() if it is disallowed by feature policy.

SyncXHR is allowed by default in all frames; to disable in a particular
iframe, use the allow attribute, like

<iframe src="..." allow="sync-xhr 'none'"></iframe>

Or include a Feature-Policy HTTP header with a document which either
only allows synchronous XHR in certain origins:

Feature-Policy: sync-xhr 'self' https://example.com

or disallows it completely:

Feature-Policy: sync-xhr 'none'

The feature currently requires the 'FeaturePolicyExperimentalFeatures'
runtime flag.


Bug:  661283 
Change-Id: If511e5990623670128f63f9c1d7233bca659b7e6
Reviewed-on: https://chromium-review.googlesource.com/656722
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Reviewed-by: Rick Byers <rbyers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508370}
[add] https://crrev.com/224ff964f4e15546d6a11fe39596e60cee2120d2/third_party/WebKit/LayoutTests/external/wpt/XMLHttpRequest/xmlhttprequest-sync-default-feature-policy.sub-expected.txt
[add] https://crrev.com/224ff964f4e15546d6a11fe39596e60cee2120d2/third_party/WebKit/LayoutTests/external/wpt/XMLHttpRequest/xmlhttprequest-sync-default-feature-policy.sub.html
[modify] https://crrev.com/224ff964f4e15546d6a11fe39596e60cee2120d2/third_party/WebKit/LayoutTests/external/wpt/feature-policy/resources/featurepolicy.js
[modify] https://crrev.com/224ff964f4e15546d6a11fe39596e60cee2120d2/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
[modify] https://crrev.com/224ff964f4e15546d6a11fe39596e60cee2120d2/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp

Owner: iclell...@chromium.org
Status: Started (was: Available)
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/245bf895c9c98f7b07cb840c0a553e9dc16f4d09

commit 245bf895c9c98f7b07cb840c0a553e9dc16f4d09
Author: Ian Clelland <iclelland@google.com>
Date: Fri Jan 19 16:12:09 2018

Catch unexpected exceptions in sync-xhr test.

With this change, only the expected NetworkError from send() will be
propagated to the feature policy test framework. Other errors (including
NetworkErrors from other calls) will be prefixed with
"UnexpectedException:" and will cause the tests to fail.

Bug:  661283 
Change-Id: I75e2fa6526211cc87c65f5f03e926903ae1ba22e
Reviewed-on: https://chromium-review.googlesource.com/874270
Commit-Queue: Philip J├Ągenstedt <foolip@chromium.org>
Reviewed-by: Philip J├Ągenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530527}
[modify] https://crrev.com/245bf895c9c98f7b07cb840c0a553e9dc16f4d09/third_party/WebKit/LayoutTests/external/wpt/xhr/xmlhttprequest-sync-default-feature-policy.sub.html

Status: Fixed (was: Started)
Labels: M-65

Comment 9 by woxxom@gmail.com, Apr 13 2018

This feature breaks extensions that use sync XHR in the content script to get dynamic data from their background pages at document_start before the page starts loading. For example, Tampermonkey - when configured to use Instant inject mode.

With this change, and in the absence of any extension API to pass data synchronously at document_start, extension developers will no longer be able to provide dynamically configurable service in the web pages (like adding a custom API or the abovementioned userscripts in Tampermonkey) *before* the page starts running its scripts.
This should only break pages that actually choose to embed content with the feature disabled -- synchronous XHR is still enabled by default in all web content. (Including extensions)

Is there a site you can point to that is currently broken with this change?

Comment 11 by woxxom@gmail.com, Apr 13 2018

>This should only break pages that actually choose to embed content with the feature disabled

Extensions may run content scripts in any web iframe, including the sandboxed ones because extensions act on behalf of the user and so they must have a way to be exempt from web restrictions. This is how extensions always worked in all browsers including Chrome. This change should not have broken extensions at all.

>Is there a site you can point to that is currently broken with this change?

I can make one right now but what for? In the future there might be millions of iframes that break extensions in the way I've described. The problem here is not the amount of sites, but the basic approach: extensions should not be affected by web limits.

The detrimental effect could be lessened if someone implements a proper extensions API to pass dynamic data at document_start so it's available immediately in the content script's first statement (that is same behavior as sync XHR implements right now).

Sign in to add a comment