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

Support Private Network CORS #277

Open
2 tasks done
danjenkins opened this issue Nov 14, 2023 · 8 comments
Open
2 tasks done

Support Private Network CORS #277

danjenkins opened this issue Nov 14, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@danjenkins
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

There are changes coming to how we access private networks (localhost for example) from non localhost https websites. https://developer.chrome.com/blog/private-network-access-update/#cors-preflight-requests explains about all the changes plus the changes to CORS.

Basically needing to add [Access-Control-Request-Private-Network](https://wicg.github.io/private-network-access/#http-headerdef-access-control-request-private-network): true to the headers.

I've attached an example of what I've added as a patch to @fastify/cors which will need some extra work if these changes are accepted... like only setting the response if the Access-Control-Request-Private-Network: true header is in the request.

But before I did the extra work I wanted to see if this was useful/interesting.

Motivation

No response

Example

diff --git a/node_modules/@fastify/cors/index.js b/node_modules/@fastify/cors/index.js
index 28dfc9a..912853c 100644
--- a/node_modules/@fastify/cors/index.js
+++ b/node_modules/@fastify/cors/index.js
@@ -215,6 +215,10 @@ function addCorsHeaders (req, reply, originOption, corsOptions) {
     reply.header('Access-Control-Allow-Credentials', 'true')
   }
 
+  if (corsOptions.allowPrivateNetwork) {
+    reply.header('Access-Control-Allow-Private-Network', 'true')
+  }
+
   if (corsOptions.exposedHeaders !== null) {
     reply.header(
       'Access-Control-Expose-Headers',
@mcollina
Copy link
Member

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina mcollina added enhancement New feature or request good first issue Good for newcomers labels Nov 14, 2023
@Rai-Sahil
Copy link

Hi @danjenkins

I was thinking of working on this. I'm wondering do we need to do this part as well, if I fix the CORS thing. Also that is the error without fixing the CORS thing.

I've attached an example of what I've added as a patch to @fastify/cors which will need some extra work if these > changes are accepted... like only setting the response if the Access-Control-Request-Private-Network: true header is in > the request.

@danjenkins
Copy link
Author

danjenkins commented Nov 14, 2023

@Rai-Sahil do you mean what is the error? The error is that the request fails because the browser doesn't get given the right headers in the response.

To do it properly we need to check if the request has the header Access-Control-Request-Private-Network before setting Access-Control-Allow-Private-Network in the response.

If you'd like to take this on and make a PR feel free :) Don't forget about the unit tests though. If not I can do a PR with unit tests.

@Rai-Sahil
Copy link

@danjenkins Thanks, I'll start working on it. I'll comment here if I have any further questions

@Fdawgs
Copy link
Member

Fdawgs commented Nov 14, 2023

@danjenkins @Rai-Sahil

I'm -1 on this for the time being until all major browsers support this, not just Chromium. Thanks for raising it though!

Firefox's support for this is in prototype stage, and WebKit's position on this is tentative.

@danjenkins
Copy link
Author

@Fdawgs yeah, I'm in two minds but it's causing problems with things running in chrome canary and I'm pretty certain it will be causing problems in other versions of chrome too... hence why I wrote the patch :)

I'm good waiting to see how it all pans out and to carry on using my patch file for now...

🤷‍♂️

@Fdawgs Fdawgs removed the good first issue Good for newcomers label Nov 18, 2023
@climba03003
Copy link
Member

I would treats it the same as fastify/fastify-cookie#261

In order to get the PR accepted,

  • Mark the feature as experimental (meaning it would change or remove in any time without major bump).
  • Details on README.md to explain why it is marked as experimental.

@Fdawgs
Copy link
Member

Fdawgs commented Feb 29, 2024

I would treats it the same as fastify/fastify-cookie#261

In order to get the PR accepted,

* Mark the feature as experimental (meaning it would change or remove in any time without major bump).

* Details on README.md to explain why it is marked as experimental.

I think fastify/fastify-cookie#261 is a bit different though in that the other browsers are supportive of the change but have yet to implement it. With this, the responses seem a bit "meh".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants