CORS-RFC1918: Introduce the 'addressSpace' IDL attributes.
As defined at https://mikewest.github.io/cors-rfc1918/#feature-detect,
this patch adds attributes to 'Document' and 'WorkerGlobalScope' in order
to detect both support for the CORS-RFC1918 preflight mechanism, as well
as the current state of the context.
This patch also fixes a pretty bad bug with our counting of IPv6
addresses by ensuring that they're properly bracketed before being
processed as "reserved" or not. Alas, this means that we've been
miscategorizing some unknown subset of documents and resources as
"public" that should have been "private". I'm not sure if this is
going to make the numbers at
https://www.chromestatus.com/metrics/feature/timeline/popularity/530
better or worse. :/
BUG=591672
Committed: https://crrev.com/ba0ae85476038f7fdb7394e9f18e8a0c73fc0855
Cr-Commit-Position: refs/heads/master@{#379282}
Mind taking a look at this Philip? It's all behind a flag, so there shouldn't ...
4 years, 9 months ago
(2016-03-03 11:11:22 UTC)
#2
Mind taking a look at this Philip? It's all behind a flag, so there shouldn't be
any web-facing changes; just a bit more work on the infrastructure I need for
the CORS-RFC1918 prototype.
Mike West
https://codereview.chromium.org/1754713006/diff/20001/third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-worker-basic.html File third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-worker-basic.html (right): https://codereview.chromium.org/1754713006/diff/20001/third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-worker-basic.html#newcode21 third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-worker-basic.html:21: // TODO(mkwst): Broken. These will be unbroken in https://codereview.chromium.org/1760523004, ...
4 years, 9 months ago
(2016-03-03 12:04:49 UTC)
#3
lgtm https://codereview.chromium.org/1754713006/diff/40001/content/child/web_url_loader_impl.cc File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1754713006/diff/40001/content/child/web_url_loader_impl.cc#newcode958 content/child/web_url_loader_impl.cc:958: WebString::fromUTF8(info.socket_address.HostForURL())); Is this the "pretty bad bug"? I ...
4 years, 9 months ago
(2016-03-04 04:35:09 UTC)
#4
Thanks Philip! I'll address your comments for reals once I'm in the office later this ...
4 years, 9 months ago
(2016-03-04 05:53:54 UTC)
#5
Thanks Philip! I'll address your comments for reals once I'm in the office later
this morning. For the moment, I'll just post my responses, and thank you for
your time. :)
https://codereview.chromium.org/1754713006/diff/40001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):
https://codereview.chromium.org/1754713006/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:958:
WebString::fromUTF8(info.socket_address.HostForURL()));
On 2016/03/04 at 04:35:08, philipj_UTC7 wrote:
> Is this the "pretty bad bug"?
Yes. `host` returns an IPv6 address as `::1`, while `HostForURL` returns it with
brackets, as `[::1]`. The latter format is what we need to pass into the
platform API that tells us whether or not the IP address is reserved.
> I can't review here. Do any of the tests guard against regressing this?
I discovered the bug when writing the layout tests for this patch. I'll see if I
can add a unit test somewhere in //content that verifies the format.
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-document-basic.html
(right):
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-document-basic.html:2:
<html>
On 2016/03/04 at 04:35:08, philipj_UTC7 wrote:
>
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...
Huh. I've never ever done that. :(
In this case, I think I need to introduce the `<body>` tag before executing the
scripts, as they append to `document.body`. Won't script tags be inserted into
the synthetic `<head>` by the parser?
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-document-basic.html:49:
assert_equals(e.data.addressSpace, "private");
On 2016/03/04 at 04:35:08, philipj_UTC7 wrote:
> So this isn't what I would have guessed, but then I haven't read the spec.
Given the name "addressSpace" I thought that it would probably be a function of
the IP address only, but apparently not?
>
> https://mikewest.github.io/cors-rfc1918/#address-space doesn't give an example
and I don't know what keyword to search for in RFC1918
I thought I'd added a comment here...
The idea is that we check the address space from which a document is served in
order to determine its status. That is, `example.com` usually resolves to
`93.184.216.34`, so it's attribute would be "public". If I poison DNS to make it
resolve to `127.0.0.1`, then its attribute would be "local".
That's the idea I'm getting at with these tests, but there's a caveat: I don't
know how to test "public" from Blink's layout tests. Since everything is running
on the same machine, the response's IP address is always `127.0.0.1` or `::1`. I
can distinguish between "local" and "private" pretty easily, as these tests do,
but "public" would mean talking to a machine outside the local network (or, I
suppose, building up some special-case mock endpoint that pretended to be an
external address?).
Ideas welcome.
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-worker-basic.html
(right):
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-worker-basic.html:21:
// TODO(mkwst): Broken.
On 2016/03/04 at 04:35:08, philipj_UTC7 wrote:
> Oh, there's nothing that actually calls setAddressSpace in the worker case...
TODO now or later?
That's https://codereview.chromium.org/1760523004, which I would love for you to
take a look at if you have tons of free time. :)
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/resources/post-addressspace-from-worker.html
(right):
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/resources/post-addressspace-from-worker.html:6:
w.postMessage("go");
On 2016/03/04 at 04:35:08, philipj_UTC7 wrote:
> Why this little indirection? Timing problem?
I wasn't sure if the handler would catch a `postMessage` from the Worker if its
startup was racing this code. I didn't even try it without this little poke.
Would it have worked?
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:84: NOTREACHED();
On 2016/03/04 at 04:35:08, philipj_UTC7 wrote:
> ASSERT_NOT_REACHED? I see NOTREACHED() only in Source/platform. Not sure if
those are right either since there are lots of ASSERT_NOT_REACHED in
Source/platform too.
*sigh* Wouldn't it be nice if style-folks would concentrate on removing these
little annoyances rather than forcing me into tiny 80-column boxes? :)
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-document-basic.html File third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-document-basic.html (right): https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-document-basic.html#newcode2 third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-document-basic.html:2: <html> On 2016/03/04 05:53:54, Mike West wrote: > On ...
4 years, 9 months ago
(2016-03-04 10:42:12 UTC)
#8
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-document-basic.html
(right):
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-document-basic.html:2:
<html>
On 2016/03/04 05:53:54, Mike West wrote:
> On 2016/03/04 at 04:35:08, philipj_UTC7 wrote:
> >
>
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...
>
> Huh. I've never ever done that. :(
>
> In this case, I think I need to introduce the `<body>` tag before executing
the
> scripts, as they append to `document.body`. Won't script tags be inserted into
> the synthetic `<head>` by the parser?
When I need a <body> I usually just add that before the script (but no </body>)
But if your other tests in and around this area are already in this style, just
pretend I didn't say anything. It's fine :)
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-document-basic.html:49:
assert_equals(e.data.addressSpace, "private");
On 2016/03/04 05:53:54, Mike West wrote:
> On 2016/03/04 at 04:35:08, philipj_UTC7 wrote:
> > So this isn't what I would have guessed, but then I haven't read the spec.
> Given the name "addressSpace" I thought that it would probably be a function
of
> the IP address only, but apparently not?
> >
> > https://mikewest.github.io/cors-rfc1918/#address-space doesn't give an
example
> and I don't know what keyword to search for in RFC1918
>
> I thought I'd added a comment here...
>
> The idea is that we check the address space from which a document is served in
> order to determine its status. That is, `example.com` usually resolves to
> `93.184.216.34`, so it's attribute would be "public". If I poison DNS to make
it
> resolve to `127.0.0.1`, then its attribute would be "local".
>
> That's the idea I'm getting at with these tests, but there's a caveat: I don't
> know how to test "public" from Blink's layout tests. Since everything is
running
> on the same machine, the response's IP address is always `127.0.0.1` or `::1`.
I
> can distinguish between "local" and "private" pretty easily, as these tests
do,
> but "public" would mean talking to a machine outside the local network (or, I
> suppose, building up some special-case mock endpoint that pretended to be an
> external address?).
>
> Ideas welcome.
So my surprise was that this is "private" instead of "local", on the assumption
that example.test resolves to 127.0.0.1. What makes the difference between
"local" and "private" in these tests?
This sure seems like the kind of thing where unit tests are less terrible than
layout tests. But I guess you already have those tests out in Chromium
somewhere, and it still won't cover all of the layers in between. So, no great
ideas :(
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-worker-basic.html
(right):
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-worker-basic.html:21:
// TODO(mkwst): Broken.
On 2016/03/04 05:53:54, Mike West wrote:
> On 2016/03/04 at 04:35:08, philipj_UTC7 wrote:
> > Oh, there's nothing that actually calls setAddressSpace in the worker
case...
> TODO now or later?
>
> That's https://codereview.chromium.org/1760523004, which I would love for you
to
> take a look at if you have tons of free time. :)
Haha, pass on that one :)
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/resources/post-addressspace-from-worker.html
(right):
https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/resources/post-addressspace-from-worker.html:6:
w.postMessage("go");
On 2016/03/04 05:53:54, Mike West wrote:
> On 2016/03/04 at 04:35:08, philipj_UTC7 wrote:
> > Why this little indirection? Timing problem?
>
> I wasn't sure if the handler would catch a `postMessage` from the Worker if
its
> startup was racing this code. I didn't even try it without this little poke.
> Would it have worked?
Oh, I misread this. I thought the "go" message would end up at the handler right
above here, but it's for the worker script. But the race condition cannot happen
I think, because even if the worker starts and posts its message before the
message event handler is added on the next line, that message can only be
processed and turned into an event dispatch after this script has finished and
returned to the message loop. Right?
philipj_slow
Thanks, still LGTM! https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Source/core/dom/SecurityContext.cpp File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right): https://codereview.chromium.org/1754713006/diff/40001/third_party/WebKit/Source/core/dom/SecurityContext.cpp#newcode84 third_party/WebKit/Source/core/dom/SecurityContext.cpp:84: NOTREACHED(); On 2016/03/04 05:53:54, Mike West ...
4 years, 9 months ago
(2016-03-04 10:47:14 UTC)
#9
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1754713006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1754713006/60001
4 years, 9 months ago
(2016-03-04 14:21:24 UTC)
#12
4 years, 9 months ago
(2016-03-04 14:26:57 UTC)
#13
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
commit-bot: I haz the power
Description was changed from ========== CORS-RFC1918: Introduce the 'addressSpace' IDL attributes. As defined at https://mikewest.github.io/cors-rfc1918/#feature-detect, ...
4 years, 9 months ago
(2016-03-04 14:27:59 UTC)
#14
Message was sent while issue was closed.
Description was changed from
==========
CORS-RFC1918: Introduce the 'addressSpace' IDL attributes.
As defined at https://mikewest.github.io/cors-rfc1918/#feature-detect,
this patch adds attributes to 'Document' and 'WorkerGlobalScope' in order
to detect both support for the CORS-RFC1918 preflight mechanism, as well
as the current state of the context.
This patch also fixes a pretty bad bug with our counting of IPv6
addresses by ensuring that they're properly bracketed before being
processed as "reserved" or not. Alas, this means that we've been
miscategorizing some unknown subset of documents and resources as
"public" that should have been "private". I'm not sure if this is
going to make the numbers at
https://www.chromestatus.com/metrics/feature/timeline/popularity/530
better or worse. :/
BUG=591672
==========
to
==========
CORS-RFC1918: Introduce the 'addressSpace' IDL attributes.
As defined at https://mikewest.github.io/cors-rfc1918/#feature-detect,
this patch adds attributes to 'Document' and 'WorkerGlobalScope' in order
to detect both support for the CORS-RFC1918 preflight mechanism, as well
as the current state of the context.
This patch also fixes a pretty bad bug with our counting of IPv6
addresses by ensuring that they're properly bracketed before being
processed as "reserved" or not. Alas, this means that we've been
miscategorizing some unknown subset of documents and resources as
"public" that should have been "private". I'm not sure if this is
going to make the numbers at
https://www.chromestatus.com/metrics/feature/timeline/popularity/530
better or worse. :/
BUG=591672
Committed: https://crrev.com/ba0ae85476038f7fdb7394e9f18e8a0c73fc0855
Cr-Commit-Position: refs/heads/master@{#379282}
==========
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/ba0ae85476038f7fdb7394e9f18e8a0c73fc0855 Cr-Commit-Position: refs/heads/master@{#379282}
4 years, 9 months ago
(2016-03-04 14:28:01 UTC)
#15
Issue 1754713006: CORS-RFC1918: Introduce the 'addressSpace' IDL attributes.
(Closed)
Created 4 years, 9 months ago by Mike West
Modified 4 years, 9 months ago
Reviewers: jochen (gone - plz use gerrit), philipj_slow
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 28
Chromium Code Reviews
Issue
1754713006:
CORS-RFC1918: Introduce the 'addressSpace' IDL attributes. (Closed)
