Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(71)

Issue 1140203002: make file: an effectively unique origin (Closed)

Created:
5 years, 7 months ago by TheJH
Modified:
5 years, 6 months ago
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

make file: an effectively unique origin This, if Chrome is started without flags that reduce security, prevents local files from being same-origin with other files at the same path. This can especially be an issue with pseudofilesystems in which the meaning of a path often changes. BUG=429542 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195464

Patch Set 1 #

Total comments: 2

Patch Set 2 : added test #

Total comments: 11

Patch Set 3 : BROKEN: use test harness #

Patch Set 4 : fix test #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+46 lines, -9 lines) Patch
A LayoutTests/security/cannot-read-self-from-file.html View 1 2 3 1 chunk +21 lines, -0 lines 0 comments Download
A LayoutTests/security/cannot-read-self-from-file-expected.txt View 1 2 3 1 chunk +6 lines, -0 lines 0 comments Download
A LayoutTests/security/resources/cannot-read-self-from-file.html View 1 2 3 1 chunk +18 lines, -0 lines 0 comments Download
M Source/platform/weborigin/SecurityOrigin.h View 1 chunk +0 lines, -1 line 0 comments Download
M Source/platform/weborigin/SecurityOrigin.cpp View 1 2 3 chunks +1 line, -8 lines 2 comments Download

Messages

Total messages: 30 (7 generated)
TheJH
5 years, 7 months ago (2015-05-15 02:59:03 UTC) #2
nasko
Adding alexmos@ and creis@, who have looked into this in the past.
5 years, 7 months ago (2015-05-15 13:32:32 UTC) #6
alexmos
The change looks ok, but let's also get a review from dcheng@, who wrote an ...
5 years, 7 months ago (2015-05-15 17:45:52 UTC) #8
TheJH
On 2015/05/15 17:45:52, alexmos (OOO until 5-26) wrote: > Do we need to do something ...
5 years, 7 months ago (2015-05-15 18:22:46 UTC) #9
TheJH
Oh, I didn't know about this: https://www.chromestatus.com/feature/5755326842273792 Adding mkwst since he is listed as one ...
5 years, 7 months ago (2015-05-15 21:40:17 UTC) #11
dcheng
https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/SecurityOrigin.cpp File Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/SecurityOrigin.cpp#newcode277 Source/platform/weborigin/SecurityOrigin.cpp:277: return false; Doesn't this break layout tests, which do ...
5 years, 7 months ago (2015-05-16 01:55:10 UTC) #12
TheJH
https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/SecurityOrigin.cpp File Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/SecurityOrigin.cpp#newcode277 Source/platform/weborigin/SecurityOrigin.cpp:277: return false; On 2015/05/16 01:55:10, dcheng wrote: > Doesn't ...
5 years, 7 months ago (2015-05-16 02:20:46 UTC) #13
TheJH
https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/SecurityOrigin.cpp File Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/SecurityOrigin.cpp#newcode277 Source/platform/weborigin/SecurityOrigin.cpp:277: return false; On 2015/05/16 01:55:10, dcheng wrote: > Doesn't ...
5 years, 7 months ago (2015-05-16 02:20:46 UTC) #14
Mike West
This looks like something we could try to land on Monday (we're branching for M44 ...
5 years, 7 months ago (2015-05-16 05:07:32 UTC) #15
TheJH
On 2015/05/16 05:07:32, Mike West (holiday in DE) wrote: > Please add tests, however, to ...
5 years, 7 months ago (2015-05-17 02:42:03 UTC) #16
Mike West
A few nits on the tests. Otherwise looking good. https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/cannot-read-self-from-file.html File LayoutTests/security/cannot-read-self-from-file.html (right): https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/cannot-read-self-from-file.html#newcode1 LayoutTests/security/cannot-read-self-from-file.html:1: ...
5 years, 7 months ago (2015-05-17 08:40:49 UTC) #17
Mike West
Also: what is Firefox's behavior for your test? Does it also block the XHR? If ...
5 years, 7 months ago (2015-05-17 08:41:40 UTC) #18
TheJH
On 2015/05/17 08:41:40, Mike West (holiday in DE) wrote: > Also: what is Firefox's behavior ...
5 years, 7 months ago (2015-05-17 13:22:58 UTC) #19
TheJH
https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/cannot-read-self-from-file.html File LayoutTests/security/cannot-read-self-from-file.html (right): https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/cannot-read-self-from-file.html#newcode1 LayoutTests/security/cannot-read-self-from-file.html:1: <html> On 2015/05/17 08:40:48, Mike West (holiday in DE) ...
5 years, 7 months ago (2015-05-17 14:35:19 UTC) #20
TheJH
On 2015/05/17 14:35:19, TheJH wrote: > Can someone who is a bit more familiar with ...
5 years, 7 months ago (2015-05-17 14:41:43 UTC) #21
Mike West
On 2015/05/17 at 14:41:43, jannhorn wrote: > On 2015/05/17 14:35:19, TheJH wrote: > > Can ...
5 years, 7 months ago (2015-05-18 09:04:02 UTC) #22
TheJH
On 2015/05/18 09:04:02, Mike West (holiday in DE) wrote: > If you need the frame ...
5 years, 7 months ago (2015-05-18 10:54:47 UTC) #23
Mike West
On 2015/05/18 at 10:54:47, jannhorn wrote: > On 2015/05/18 09:04:02, Mike West (holiday in DE) ...
5 years, 7 months ago (2015-05-18 10:56:23 UTC) #24
TheJH
On 2015/05/18 10:56:23, Mike West (holiday in DE) wrote: > On 2015/05/18 at 10:54:47, jannhorn ...
5 years, 7 months ago (2015-05-18 11:06:24 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140203002/60001
5 years, 7 months ago (2015-05-18 11:23:21 UTC) #27
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195464
5 years, 7 months ago (2015-05-18 12:46:07 UTC) #28
dcheng
https://codereview.chromium.org/1140203002/diff/60001/Source/platform/weborigin/SecurityOrigin.cpp File Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1140203002/diff/60001/Source/platform/weborigin/SecurityOrigin.cpp#newcode274 Source/platform/weborigin/SecurityOrigin.cpp:274: return !m_enforceFilePathSeparation && !other->m_enforceFilePathSeparation; I think we should rename ...
5 years, 6 months ago (2015-06-01 21:08:53 UTC) #29
TheJH
5 years, 6 months ago (2015-06-02 11:59:19 UTC) #30
Message was sent while issue was closed.
https://codereview.chromium.org/1140203002/diff/60001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1140203002/diff/60001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:274: return
!m_enforceFilePathSeparation && !other->m_enforceFilePathSeparation;
On 2015/06/01 21:08:53, dcheng wrote:
> I think we should rename m_enforceFilePathSeparation to make it clearer what
the
> real purpose of this bool is. There's nothing about "filepath separation"
> anymore after this patch: it's become strictly a "give file URLs access to
other
> file URLs" check. jannhorn@, do you mind cleaning this up in a followup patch?

What would be a good name? m_treatFileAsUniqueOrigin? m_forbidFileFileAccess?

Powered by Google App Engine
This is Rietveld 408576698