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

Flesh out spec #114

Merged
merged 1 commit into from
Mar 10, 2025
Merged

Flesh out spec #114

merged 1 commit into from
Mar 10, 2025

Conversation

drubery
Copy link
Collaborator

@drubery drubery commented Mar 10, 2025

This adds a lot more detail to the spec. It's still not fully complete and explicit, but at this point it should be a better reference than the explainer.

@drubery drubery requested a review from thefrog-gh March 10, 2025 19:41
Copy link
Collaborator

@thefrog-gh thefrog-gh left a comment

Choose a reason for hiding this comment

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

All comments are minor and non-blocking since this is still in progress. Note there is a failing automated check.

@@ -206,19 +201,25 @@ A <dfn>device bound session</dfn> is a [=struct=] with the following
:: a [=string=] that is to be used as the next challenge for this session
: [=session scope=]
:: a [=struct=] defining which [=url=]'s' are in scope for this session
: [=session credential=]
:: a [=list=] of [=session credential=] used by the session
: [=session credentials=]
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 know much about this, but: is [=session credentials=] allowed here, when the dfn is actually for the singular version? ("session credential")

Same question for [=scope specifications=] below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What I think I'm doing here is defining a field "session credentials" with type "list of session credential". But I'm not an expert in the notation here either.

spec.bs Outdated

1. If [=session scope=] sets |include site|, return whether [=url=] is
same-site with the scope origin.
1. Return whether [=url=] is same origin with the scopep 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
1. Return whether [=url=] is same origin with the scopep origin.
1. Return whether [=url=] is same origin with the scope origin.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

return the |scope specification|'s type.
1. If the [=url=] matches the |session|'s refresh URL, return false.

1. If [=session scope=] sets |include site|, return whether [=url=] is
Copy link
Collaborator

Choose a reason for hiding this comment

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

The dfn calls it include_site. Should we have it match here?

Suggested change
1. If [=session scope=] sets |include site|, return whether [=url=] is
1. If [=session scope=] sets |include_site|, return whether [=url=] is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made the dfn include site, since most places seem to use distinct words rather than underscores.

1. If the [=session scope=] origin is the eTLD+1 and the [=url=] is
not same-site with the eTLD+1, return false.
1. [=list/for each=] |scope specification| in the |session scope|
1. If the host and path of [=url=] match the |scope specification|,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to do this in a separate PR -- I think we should clarify what it means to match the scope specification, since it doesn't have to be an exact match host-to-host + path-to-path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think that's a good followup. I'm hoping I can just reference somewhere else, given that the Chrome implementation is using a very standard helper function.

spec.bs Outdated
requesting excessive TPM operations (harming the user) or the
refresh endpoint has recently been unreachable (denial of service
risk for the site).
1. Run the steps in [=evaluate session scope=] on the |request|'s URL
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 seeing evaluate session scope in the spec. Should this be something different? (Or are we missing a dfn maybe?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, yep. I meant url-in-scope.

spec.bs Outdated
<div class="algorithm" data-algorithm="session-request">
This algorithm describes how to <dfn export dnf-for="algorithms">send
a request</dfn> for a device bound session registration or refresh. It
takes as input a |originating request|, |key pair|, |destination|, and
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
takes as input a |originating request|, |key pair|, |destination|, and
takes as input an |originating request|, |key pair|, |destination|, and

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

spec.bs Outdated
1. If |signed challenge| is non-null, add header "Sec-Session-Response" with
value |signed challenge| to |request|.
1. Set |request|'s initiator to |originating request|'s initiator.
1. Run HTTP-network-or cache fetch on |request|.
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
1. Run HTTP-network-or cache fetch on |request|.
1. Run HTTP-network-or-cache fetch on |request|.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

1. [=session identifier=] the value of "session_identifier".
1. [=refresh url=] set to |refresh url|.
1. [=defer requests=] set to true.
1. [=session scope=] a new scope with [=origin=] |origin|, [=include
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment about include_site rather than include site here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@drubery drubery force-pushed the spec_updates branch 2 times, most recently from acd996f to 32cc06b Compare March 10, 2025 22:50
This adds a lot more detail to the spec. It's still not fully complete
and explicit, but at this point it should be a better reference than the
explainer.
@drubery drubery merged commit 438eb4d into main Mar 10, 2025
2 checks passed
github-actions bot added a commit that referenced this pull request Mar 10, 2025
SHA: 438eb4d
Reason: push, by drubery

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.

2 participants