Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow storage access when the top-level origin is an opaque origin. #49

Merged
merged 4 commits into from
Apr 26, 2021

Conversation

hober
Copy link
Member

@hober hober commented Jun 8, 2020

Fixes #40.

@hober hober requested a review from annevk June 8, 2020 18:59
@hober hober self-assigned this Jun 8, 2020
@hober hober requested a review from johnwilander June 8, 2020 19:00
@@ -180,6 +180,7 @@ When invoked on {{Document}} |doc|, the <dfn export method for=Document><code>ha
1. If |doc|'s [=Document/origin=] is an [=opaque origin=], [=/resolve=] |p| with false and return |p|. <!-- WebKit's DocumentStorageAccess.cpp#L90 --> <!-- Gecko's Document.cpp#l15526 -->
1. If |doc|'s [=Document/browsing context=] is a [=top-level browsing context=], [=/resolve=] |p| with true and return |p|. <!-- WebKit's DocumentStorageAccess.cpp#L95 --> <!-- Gecko's Document.cpp#l15531 -->
1. Let |topDoc| be the [=active document=] of |doc|'s [=Document/browsing context=]'s [=top-level browsing context=].
1. If |topDoc|'s [=Document/origin=] is an [=opaque origin=], [=/resolve=] |p| with false and return |p|. <!-- https://github.com/privacycg/storage-access/issues/40 -->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this it would be slightly better to use doc's relevant settings objects's top-level origin, also for the origin comparison below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I think I've fixed this in 2902e69. Please re-review, @annevk.

@hober hober requested a review from annevk July 6, 2020 18:53
Copy link
Collaborator

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use the same top-level origin below so you don't have to initialize topDoc, but this is fine for now.

@annevk
Copy link
Collaborator

annevk commented Jul 7, 2020

Oh, though it's still somewhat imprecise with the same origin checks in that you're handing it a document at times rather than an origin.

@johannhof
Copy link
Member

@hober as mentioned in chat, do you mind if I steal this PR? :)

@heskey1989
Copy link

[email protected]

@johannhof johannhof requested a review from annevk April 25, 2021 06:01
@johannhof
Copy link
Member

@annevk mind taking another look? :)

@johannhof johannhof self-assigned this Apr 25, 2021
@johannhof johannhof merged commit 6df9503 into main Apr 26, 2021
@johannhof johannhof deleted the opaque-top branch April 26, 2021 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What should happen if the top-level origin is an opaque origin
5 participants