Skip to content

Add procedures for parsing site names from strings #106

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

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

Conversation

martinthomson
Copy link
Member

@martinthomson martinthomson commented Feb 26, 2025

This is built on #104.

This contributes to #102 by laying some of the ground work regarding
sites. It should now be possible to "parse a site" when preparing to
invoke the matching logic. That comes next.


Preview | Diff

This is built on #104.
This contributes to #102 by laying some of the ground work regarding
sites.  It should now be possible to "parse a site" when preparing to
invoke the matching logic.  That comes next.
and a [=set=] of [=impression/conversion sites=].

These [=sites=] MUST all be in [=scheme-and-host=] form,
with a [=scheme=] of "`https`".
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is a major issue, but do we have any concerns about local development/debugging not being able to use http://localhost as a result of making https inherent? ARA supports that, for example.

Copy link
Member Author

@martinthomson martinthomson Feb 27, 2025

Choose a reason for hiding this comment

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

Personally, I'm 100% OK with that. We can talk about what site identity means for localhost, which is a whole other rats nest, but the obvious way out there is to use HTTPS and take steps to avoid the certificate warnings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO we should try to use https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-origin as the check before converting the input to a site, which is well-specified and used in many other parts of the platform. That link provides the following justification for local addresses:

treating such resources as potentially trustworthy is convenient for developers building an application before deploying it to the public.

This should allow localhost for user agents that conform to the name resolution rules in let localhost be localhost. Regarding site identity, we should just be able to use https://html.spec.whatwg.org/multipage/browsers.html#obtain-a-site as-is, which should support localhost.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem this is trying to solve is something that "obtain a site" cannot address, because we're looking to turn a string into a site. That means we could define a parse function for a serialized site, which would also mean that we force people to include "https://" in every string they provide. Or, as I'm suggesting, we encourage people to setup HTTPS from the outset (mkcert really is very convenient) and we can thereby avoid the whole localhost mess.

I know that it's not ideal, but local development will require quite a bit more setup than that, because the set of aggregation servers might also need to be overridden (or we might need a developer flag to enable reports that are trivially decoded...).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I was thinking we should just force https:// and use the URL parser. That seems totally fine with me given that it makes even clearer to callers that the site needs to be secure.

Co-authored-by: Andrew Paseltiner <[email protected]>
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.

3 participants