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.

Sign up for GitHub

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

Nits #120

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Nits #120

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e1d063
add public IPAddressSpace
iVanlIsh Nov 9, 2023
0d95ff9
Apply suggestions from code review
iVanlIsh Nov 9, 2023
5c65f08
Fetch API
iVanlIsh Nov 9, 2023
da3866e
Apply suggestions from code review
iVanlIsh Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions index.src.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ <h3 id="private-network-request-heading">Private Network Request</h3>
a change is not deemed worth the payoff for now. This can be shipped as an
incremental improvement later on.

NOTE: Some [=local network requests=] are more challenging to secure than
NOTE: Some [=private network requests=] are more challenging to secure than
others. See [[#rollout-difficulties]] for more details.

<h3 id="headers">Additional CORS Headers</h3>
Expand Down Expand Up @@ -918,14 +918,17 @@ <h4 id="fetching">Fetching</h4>
"<a http-header>`Private-Network-Access-ID`</a>" and
|response|'s [=response/header list=].

1. if |targetId| is invalid, return a [=network error=].
1. if |targetId| does not a string of 6 hexadecimal bytes
iVanlIsh marked this conversation as resolved.
Show resolved Hide resolved
separated by colons, return a [=network error=].

1. Let |targetName| be the result of [=extracting header list
values=] given
"<a http-header>`Private-Network-Access-Name`</a>" and
|response|'s [=response/header list=].

1. if |targetName| is invalid, return a [=network error=].
1. if |targetName| does not match the [ECMAScript] regexp
/^[a-z0-9_-.]+$/ or has more than 248 UTF-8 code units,
return a [=network error=].

1. Let |state| be the result of [=requesting permission to use=]
the following descriptor:
Expand Down Expand Up @@ -980,6 +983,10 @@ <h4 id="fetch-api">Fetch API</h4>
|request|'s [=request/client=] is a [=secure context=], then switch on
|init|["{{RequestInit/targetAddressSpace}}"]:
iVanlIsh marked this conversation as resolved.
Show resolved Hide resolved
<dl class=switch>
<dt>public
<dd>Set |request|'s [=target IP address space=] to [=IP address
space/public=]
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 throw a TypeError here if we already know the value isn't valid.

I'm still not sure that we really need public in the IPAddressSpace type since we don't have a valid case to use it. I guess that's why I called it RequestTargetAddressSpace in the first place so that we only need to include the valid values that should be used.

Suggested change
<dd>Set |request|'s [=target IP address space=] to [=IP address
space/public=]
<dd>[=Throw=] a {{TypeError}}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we need this because we need to define if a request's IP address space is less public then the initiator's. In this case, we need to define initiator's IP address space first, which can be public.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is actually that we shouldn't put this general definition in the Fetch API section. I think it will make more sense to put it somewhere more generic.

Copy link
Collaborator

@johnathan79717 johnathan79717 Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We have already defined the public address space in https://wicg.github.io/private-network-access/#ip-address-space-public. I meant we didn't need it in the IDL, i.e. the JS API. My understanding is that we should define what we will use in the Javascript in the IDL.

Copy link
Collaborator

@johnathan79717 johnathan79717 Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If you insist keeping "public" in the IDL, I still think throwing a TypeError here is better than a network error later in the algorithm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I meant we should share the same enum other than create a new one for fetch API only.

I don't think this is a TypeError on one hand, and on the other, "public" is forbidden to bypass mixed content check sounds like a better logic than a fetch request cannot be "public" targetAddressSpace.


<dt>private
<dd>Set |request|'s [=target IP address space=] to [=IP address
space/private=]
iVanlIsh marked this conversation as resolved.
Show resolved Hide resolved
Expand Down