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

Define preconnect and dns-prefetch with CSP #1620

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

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Mar 20, 2023

Preconnect & dns-prefetch test a fake prefetch request for CSP, and then obtain a connection/resolve an origin.

See whatwg/html#9035

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno (not for CORS changes): …
  • MDN issue is filed: …

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

fetch.bs Outdated Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

It's unfortunate these have to be separate algorithms. I was hoping we could do CSP checks as part of the normal "obtain a connection" routine.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Apr 24, 2023

It's unfortunate these have to be separate algorithms. I was hoping we could do CSP checks as part of the normal "obtain a connection" routine.

We can do that but then there would be double CSP checks, one for the request and one for the "fake" request, or we can add a flag to obtain/resolve of whether to perform the CSP check.

fetch.bs Outdated
and <a href="https://en.wikipedia.org/wiki/Proxy_auto-config">proxy auto-config (PAC)</a> come
into play. The "<code>DIRECT</code>" value means to not use a proxy for this particular
<var>url</var>.
<li><p>Let <var>proxies</var> of calling <a>resolve proxies</a>.
Copy link
Member

Choose a reason for hiding this comment

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

This is missing words.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What words are missing? The note was moved to resolve proxies.

fetch.bs Outdated
Comment on lines 2907 to 2908
<p>To <dfn>resolve proxies</dfn>, return the result of finding proxies for <var>url</var> in an
<a>implementation-defined</a> manner. If there are no proxies, return « "<code>DIRECT</code>" ».
Copy link
Member

Choose a reason for hiding this comment

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

Where does url come from here? If it needs to be an argument, let's make it explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

fetch.bs Outdated
@@ -3023,6 +3027,75 @@ details of reused connections are not exposed and time values are coarsened.
</div>


<h3 id=preemptive-connections>Preemptive connections</h3>

<div algorithm="preemmptively-obtain-a-connection">
Copy link
Member

Choose a reason for hiding this comment

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

Does algorithm need this value? Normally it works without.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

fetch.bs Outdated
Comment on lines 3042 to 3044
<p><a>Check CSP for preemptive operation</a> with <var>environment</var> and <var>url</var>. If
that returns <b>allowed</b>, <a>obtain a connection</a> with <var>key</var>, <var>url</var> and
<var>credentials</var>.
Copy link
Member

Choose a reason for hiding this comment

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

Oxford comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

fetch.bs Outdated
Comment on lines 3083 to 3086
<div algorithm="check-preemptive-operation-csp">
<p>To <dfn>check CSP for preemptive operation</dfn>, given an
<a>environment settings object</a> <var>environment</var>, and a <a for=/>URL</a> <var>url</var>,
run these steps:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this can have a more descriptive name such as "does CSP allow a connection" and we normalize the return value from CSP to true/false while we're here.

Seems this went unaddressed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated
</ol>
</div>

<div algorithm="check-preemptive-operation-csp">
Copy link
Member

Choose a reason for hiding this comment

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

This attribute value isn't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

fetch.bs Outdated
</ol>
</div>

<div>
Copy link
Member

Choose a reason for hiding this comment

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

This needs an algorithm attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

fetch.bs Outdated
</div>

<div algorithm="check-preemptive-operation-csp">
<p>To <dfn>Should connection be allowed by Content Security Policy?</dfn>, given an
Copy link
Member

Choose a reason for hiding this comment

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

This algorithm needs a better name. E.g., "determine if Content Security allows connection". The name shouldn't have a question mark and should read naturally after "To" if you're using to to lead it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Foxed

fetch.bs Outdated
<div algorithm="check-preemptive-operation-csp">
<p>To <dfn>Should connection be allowed by Content Security Policy?</dfn>, given an
<a>environment settings object</a> <var>environment</var>, and a <a for=/>URL</a> <var>url</var>,
run these steps:
Copy link
Member

Choose a reason for hiding this comment

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

No need for "run these steps" when you lead it in with "To".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

fetch.bs Outdated
Comment on lines 3094 to 3095
<li><p>Return the result of running <a>should request be blocked by Content Security Policy?</a>
given <var>request</var>.
Copy link
Member

Choose a reason for hiding this comment

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

We should put the result in a variable first and then convert that to a boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

noamr and others added 8 commits May 8, 2023 14:06
Co-authored-by: Valentin Gosu <1454649+valenting@users.noreply.github.com>
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Apologies for the delay here. Found a couple more issues to address.

Comment on lines +3054 to +3062
<p class=note>The user agent can attempt to initiate a preconnect and perform the full
connection handshake (DNS+TCP for HTTP, and DNS+TCP+TLS for HTTPS origins) whenever possible, but
is allowed to elect to perform a partial handshake (DNS only for HTTP, and DNS or DNS+TCP for
HTTPS origins), or skip it entirely, due to resource constraints or other reasons.</p>

<p class=note>The optimal number of connections per origin is dependent on the negotiated
protocol, users current connectivity profile, available device resources, global connection
limits, and other context specific variables. As a result, the decision for how many connections
should be opened is deferred to the user agent.</p>
Copy link
Member

Choose a reason for hiding this comment

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

These notes feel like normative material. One even contains "should"! In the step above where we obtain the connection perhaps we should have a "with these caveats" ending that then leads into some bullet points?

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
noamr and others added 2 commits September 28, 2023 14:57
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants