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

Initial spec draft #120

Merged
merged 4 commits into from
Jan 5, 2023
Merged

Initial spec draft #120

merged 4 commits into from
Jan 5, 2023

Conversation

johannhof
Copy link
Member

@johannhof johannhof commented Dec 8, 2022

This is an initial spec draft for how user agents should integrate with FPS (as a companion document to the submission guidelines that specify submission format and server-side checks). It's largely complete but might be lacking some polish here and there, and I still need to add inline issues for some things @jyasskin identified in a quick review offline.


Preview | Diff

@johannhof
Copy link
Member Author

@krgovind this should be ready for a review now :)


The FPS list is a [=UTF-8=] encoded file containing contents parseable as a JSON object, conforming to the [[JSON-SCHEMA index]] described in the [[SUBMISSION-GUIDELINES]].

Note: Conformance to the schema is validated at submission time. Hence, it is not required for the user agent to validate conformance again on the client. The algorithms in this specification describe how user agents should parse the FPS list, and when a particular set should be considered valid from the client’s perspective.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it does make sense for the user agent to re-validate the registrable domain check (See https://github.com/GoogleChrome/first-party-sets/blob/main/FPS-Submission_Guidelines.md#set-level-technical-validation), since that depends on the version of the PSL being used by the UA?

cc @cfredric since we've had this conversation in the past.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the spec doesn't need to require the UA to re-validate the list, but I think it should leave wiggle room for the UA to do so and prune out domains/sets that no longer pass for some reason or other.

(Seems like some revalidation of sites is already implied in the "if |json| is not an [=ordered map=], ..." clause? But that sentence says the UA must throw out the entire list, not just a single set/domain in case of any invalid input.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 on giving UAs wiggle room.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, yeah, that's true. Let me add something about PSL versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or, actually, in the interest of getting this merged that task seems self-contained enough to be moved into a follow-up issue, if that's okay with you @cfredric @krgovind ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #125 and will add an inline issue. Thanks!

2. If |json| is a parsing exception, or if |json| is not an [=ordered map=], or if |json|[“sets”] does not exist, return and optionally retry fetching the list, or perform other error recovery tasks.
3. For each |entry| of |json|:
1. Let |set| be a [=first-party set=].
2. If |entry|[“primary”] does not exist, continue.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Ignore me if this is a normative way to specifying JSON parsing] Does there need to be an if-condition here? Almost seems like this should be an assertion/DCHECK?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I actually discussed this with @cfredric before and I think Chrome currently fails the entire list on an invalid member (so diverges from this spec), but I said it might be preferable to just skip invalid entries. We should align either spec or implementation. I could file a follow-up issue for discussing that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #126 and will add an inline issue.

1. Let |set| be a [=first-party set=].
2. If |entry|[“primary”] does not exist, continue.
3. Set |set|’s [=first-party set/primary=] to |entry|[“primary”].
4. Let |ccTLDs| be the result of [=parse an equivalence map|parsing an equivalence map=] from |entry|[“ccTLDs”]. If |ccTLDs| is failure, continue.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If |ccTLDs| is failure

Should this be s/failure/empty/? Or does this represent a parsing failure?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this seems to be the way to represent a parsing failure, not extremely readable, I admit. It's a pattern I stole re-used from other specs, thus I'm inclined to keep it unless you feel strongly about this :)

spec.bs Outdated
2. [=Clear cookies for origin=].
3. [=Clear DOM-accessible storage for origin=].
4. Let |descriptor| be a newly-created {{PermissionDescriptor}} with {{PermissionDescriptor/name}} initialized to “storage-access”.
5. Remove all permission store entries for |descriptor|, where key[0] is |site| or key[1] is |origin|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how closely aligned this needs to be with practical implementation details; but I believe with recent spec discussions on SAA moving to a per-frame model, the storage-access permission doesn't persist across browser restarts (except where browser features such as session-restores occur, but I'm not sure if that feature is specified), and so this type of clearing may not be required?

cc @shuranhuang

Copy link
Collaborator

Choose a reason for hiding this comment

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

Browsers do persist the storage-access permission across restarts; IIRC Safari's permission lasts 30 days of usage time. (Maybe session-restore gets involved there such that it's actually considered a single 30-day-long session, but I don't think we can assume that.)

However, as long as the browser's behavior is indistinguishable from what it would be if the browser implemented the spec exactly, it's fine. So a browser is free to implement this part of the spec as "delete the matching permission store entries", or as "don't bother to persist these permission store entries", since they both give the same observable behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with what Chris said. The permissions should be listed out as a clearing item in the spec, so that the actual implementation from different browsers does not matter as long as the behavior is indistinguishable. To add more implementation detail, right now Chrome and Edge both persist the SAA (implicit) permissions, but when a new user session is created during browser restarts, Edge can continue the life of those permissions if the restore feature is enabled, whereas Chrome removes those permissions (autogranted by FPS) regardless the restore feature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @cfredric and @shuranhuang . Okay, I'm not sure what's recommended for specs in such situations; but I'll leave it to @johannhof 's judgement.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's important to list this even if some implementations may have different lifetimes on the permission, to avoid leakage :)

spec.bs Outdated
2. If |entry|[“primary”] does not exist, continue.
3. Set |set|’s [=first-party set/primary=] to |entry|[“primary”].
4. Let |ccTLDs| be the result of [=parse an equivalence map|parsing an equivalence map=] from |entry|[“ccTLDs”]. If |ccTLDs| is failure, continue.
5. Set set’s ccTLDs to |ccTLDs|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
5. Set set’s ccTLDs to |ccTLDs|.
5. Set |set|’s ccTLDs to |ccTLDs|.


The FPS list is a [=UTF-8=] encoded file containing contents parseable as a JSON object, conforming to the [[JSON-SCHEMA index]] described in the [[SUBMISSION-GUIDELINES]].

Note: Conformance to the schema is validated at submission time. Hence, it is not required for the user agent to validate conformance again on the client. The algorithms in this specification describe how user agents should parse the FPS list, and when a particular set should be considered valid from the client’s perspective.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the spec doesn't need to require the UA to re-validate the list, but I think it should leave wiggle room for the UA to do so and prune out domains/sets that no longer pass for some reason or other.

(Seems like some revalidation of sites is already implied in the "if |json| is not an [=ordered map=], ..." clause? But that sentence says the UA must throw out the entire list, not just a single set/domain in case of any invalid input.)

spec.bs Outdated
2. [=Clear cookies for origin=].
3. [=Clear DOM-accessible storage for origin=].
4. Let |descriptor| be a newly-created {{PermissionDescriptor}} with {{PermissionDescriptor/name}} initialized to “storage-access”.
5. Remove all permission store entries for |descriptor|, where key[0] is |site| or key[1] is |origin|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Browsers do persist the storage-access permission across restarts; IIRC Safari's permission lasts 30 days of usage time. (Maybe session-restore gets involved there such that it's actually considered a single 30-day-long session, but I don't think we can assume that.)

However, as long as the browser's behavior is indistinguishable from what it would be if the browser implemented the spec exactly, it's fine. So a browser is free to implement this part of the spec as "delete the matching permission store entries", or as "don't bother to persist these permission store entries", since they both give the same observable behavior.

@johannhof
Copy link
Member Author

Thanks for the comments, I'll merge this with some follow-ups deferred to issues :)

@johannhof johannhof merged commit e9b53c8 into WICG:main Jan 5, 2023
@johannhof johannhof deleted the initial-spec branch January 5, 2023 14:40
github-actions bot added a commit that referenced this pull request Jan 5, 2023
SHA: e9b53c8
Reason: push, by johannhof

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jan 9, 2023
SHA: e9b53c8
Reason: push, by johannhof

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

None yet

4 participants