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.
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
adds Related Origin Requests #2040
adds Related Origin Requests #2040
Changes from 12 commits
0522807
d48a18c
2a18351
689647c
3aad054
5187e72
881ac8f
9062a15
2315db8
8ea0303
28a2b0f
b51bd16
a21babf
7466f32
e7d0c7c
b3bf34c
bdba742
de25c37
71e4e80
26d350c
a2ac319
b120220
bcec7fa
3ba2ff3
a108a2c
c0c40d1
ebf03eb
e4f24d9
2e9bdcf
2462fd8
9da2c4b
017a5e3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This should explicitly define callerOrigin and rpIdRequested as parameters of the procedure, and the references in the client algorithms should explicitly specify the arguments.
I think we should also put a
<dfn>
here and reference that rather than simply linking to the section with a different link text. Right now the link text "related origins validation procedure" appears nowhere else in the document, so readers have to rely solely on the hyperlink in order to discover what is actually referenced (this is not the only case of that throughout the spec, but IMO it's preferable to minimize them).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.
Do you have an example of what that should look like in Bikeshed?
Not sure I understand. What would a definition look like for a procedure? Isn't that just the section that defines the procedue?
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.
We have a few examples in the spec:
So for example, this section could start with:
and then the references in
create()
andget()
could look like this:This has a few advantages: most importantly it makes the connection clearer and less reliant on hidden metadata, so it's more likely to survive format conversions such as into PDF or plain text (or even actual print, if someone were to actually do that), or quotations. Less importantly it also shows Bikeshed's list of references when you click the definition, which doesn't happen for links to sections.
Alternatively, the link to the section should be a plain section reference without replacing the link text, so that it renders the whole section heading, for the reasons above.
Like I said, we don't always respect these concerns throughout the spec, and I don't think we always should, but I think this is one of the cases where there's little or nothing to gain from not doing so.
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.
But to be fair, there is also at least this counter-example where we do link to a section with a (slightly) different link text:
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.
Makes total sense. Addressed in 9da2c4b. Thanks for explaining @emlun!
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 have a question about the fetching of the well-known URL. Could this be abused given that the origin of the page that contains the WebAuthn call is necessarily a different origin from the site where the well-known document will be retrieved? This seems to constitute a cross origin request.
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.
It is the WebAuthn client software fetching the document, not anything in the web session. What types of abuse were you thinking?
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.
@nsatragno can you comment based on the discussion on the call?