generated from privacycg/template
-
Notifications
You must be signed in to change notification settings - Fork 6
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
martinthomson
wants to merge
28
commits into
main
Choose a base branch
from
site-names
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
2cc5aa8
Add procedures for parsing site names from strings
martinthomson 371a7e3
Some rather obvious fixes
martinthomson a2e6cef
Expand on the measureConversion example
martinthomson c0809a2
Replace DOMString with USVString in URL/site-typed fields
apasel422 1295d11
Fix minor typo
apasel422 3763325
A better example of saving an impression
martinthomson c44ba49
downcase
martinthomson fe72d71
Invoke the algorithms directly
martinthomson fee794e
Link to enum declarations
apasel422 e08c6f6
Update the readme with updated status
martinthomson a8f4ba6
Missing _
martinthomson 25f1772
trivial changes like this can just be committed directly
martinthomson c7c65b4
Fix erroneous usage of some method in example code
apasel422 ecdd818
Make aggregationServices a maplike
apasel422 9ec15b7
Clean up specification links, terminology, and TODOs
apasel422 e1a7dcf
Link "histogram indexes"
apasel422 58fa348
TODO
apasel422 0bd77ad
Explain difference in defaulting of filterData fields
apasel422 2750ad0
Move definition of attribution logic enum up
martinthomson 687c572
Add validation for value vs maxValue
martinthomson 2a347b0
Avoid useless noop calls
martinthomson 825647e
Add a heading for last-touch and reorder
martinthomson 711f445
Fix bad rebase
martinthomson fc428c0
Merge branch 'main' into site-names
martinthomson e289db9
Fix rebase error
martinthomson e7c6a91
Merge branch 'main' into site-names
martinthomson 38f8fcb
Merge branch 'main' into site-names
martinthomson 7f62ed2
Rework the saveImpression algorithm to include an actual impression
martinthomson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 makinghttps
inherent? ARA supports that, for example.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: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.There was a problem hiding this comment.
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 wholelocalhost
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...).
There was a problem hiding this comment.
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.