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

Explainer change for separate rate-limits for embedded site #1457

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

linnan-github
Copy link
Collaborator

@linnan-github linnan-github commented Oct 28, 2024

Fixes #1452


Preview | Diff

@linnan-github
Copy link
Collaborator Author

@jolynyao Could you PTAL, thanks!

@linnan-github linnan-github force-pushed the embeddedBudget branch 4 times, most recently from 5144c38 to f6c6a38 Compare October 28, 2024 19:47
@jolynyao
Copy link

LGTM thanks @linnan-github!

@apasel422 apasel422 self-requested a review October 29, 2024 00:50
EVENT.md Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
EVENT.md Outdated Show resolved Hide resolved
verbose_debugging_reports.md Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
[=navigable/top-level traversable=]'s [=navigable/active document=]'s [=origin=].
To obtain the <dfn export for=node>context origin</dfn> of a [=node=] |node|:

1. Optionally, return the embedded origin if it exists.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be helpful for people reading the spec to have some definition of embedded origin here, either in a note or in a link.

index.bs Outdated
[=navigable/top-level traversable=]'s [=navigable/active document=]'s [=origin=].
To obtain the <dfn export for=node>context origin</dfn> of a [=node=] |node|:

1. Optionally, return the embedded origin if it exists.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does optionally here mean that different implementations can choose whether they care about embedded origins and still be compliant with the specification?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is the intention to leave it defined by the user agent.

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 we should clarify that. Otherwise, it could be interpreted as "some of the time, do this," when it really means that the implementation can choose whether it supports the notion of embedded origins, but if it does, this algorithm should be consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It could also apply to the case "some of the time, do this". For example, if we only want to do this for sources but not triggers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but the point is that the algorithm should be consistent under particular static circumstances. For example, it is not in the spirit of this specification for the implementation to generate a random number to decide whether to return the embedded origin or actual origin.

index.bs Outdated
Comment on lines 1884 to 1886
Note: The user agent may return the embedded origin to allow separate limits
for specific use cases. Embedded origin could be the effective top-level origin,
e.g. the origin of the website displayed in the AMP viewer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't think someone reading this is going to understand what an embedded origin is, as it's not a standard term as far as I know. An example would definitely help, but we should also avoid referring to AMP in the spec if possible. Maybe we can give a neutral example like comparing https://foo.example/ vs https://foo.example/bar.example?

[=navigable/active document=]'s [=origin=].

Note: The user agent may return the embedded origin to allow separate limits
for specific use cases. Embedded origin could be the effective top-level origin.
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
for specific use cases. Embedded origin could be the effective top-level origin.
for specific use cases. The embedded origin could be the effective top-level origin.

Comment on lines +1886 to +1887
For example, https://foo.example/bar.example is embedded within https://foo.example,
where https://foo.example is the top-level origin and https://bar.example is the embedded origin.
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
For example, https://foo.example/bar.example is embedded within https://foo.example,
where https://foo.example is the top-level origin and https://bar.example is the embedded origin.
For example, a URL like `https://foo.example/bar.example` could be interpreted as embedding the origin `https://bar.example`.

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.

Allow separate privacy budgets for embedded domains
4 participants