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

api: Add support for Private Network Access header preflight requests #6089

Merged
merged 10 commits into from
Sep 19, 2024

Conversation

nullun
Copy link
Contributor

@nullun nullun commented Jul 31, 2024

Summary

During development of Algorand smart contracts and platforms users will often run local environments consisting of algod, kmd, and indexer via sandbox or more recently algokit. By default all of these services are running on local/private network addresses (e.g. 127.0.0.1), however popular tools such as DappFlow and Lora are hosted on public network addresses and require the user to specify their local endpoints. Additionally some dapps allow their users to provide their own endpoints for a more decentralised experience.

Schedule for Google Chrome 130 (although many users are already experiencing it), PNA protections will be enabled by default, disallowing public websites from making requests to local/private resources without a specific header response during a preflight request. This PR introduces a new configuration option for both algod and kmd that will add middleware to each of their API handlers to support responding to the Private Network Access request header.

Test Plan

I simply copied the only CORS related test I could find and adjusted it to check for the PNA header. I'd be happy to add something more thorough if a suggestion can be offered.

@nullun nullun changed the title Feature/pna headers api: Add support for Private Network Access header preflight requests Jul 31, 2024
Copy link

codecov bot commented Jul 31, 2024

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.

Project coverage is 56.26%. Comparing base (8eca278) to head (53d2216).
Report is 33 commits behind head on master.

Files with missing lines Patch % Lines
daemon/algod/api/server/router.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6089      +/-   ##
==========================================
+ Coverage   55.85%   56.26%   +0.41%     
==========================================
  Files         488      488              
  Lines       69610    69621      +11     
==========================================
+ Hits        38879    39172     +293     
+ Misses      28045    27786     -259     
+ Partials     2686     2663      -23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jasonpaulos jasonpaulos marked this pull request as ready for review August 26, 2024 19:15
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Although the code seems correct (except the test) I cannot see Access-Control-Allow-Private-Network header in response in manual test with private net.

@pbennett
Copy link

pbennett commented Sep 2, 2024

Although the code seems correct (except the test) I cannot see Access-Control-Allow-Private-Network header in response in manual test with private net.

Are you setting Origin in your request? It's required for CORS.

algorandskiy
algorandskiy previously approved these changes Sep 3, 2024
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, the test works as expected now.
In my previous manual testing I used algod-listen.net instead algod.net so I confirm the header appears as expected.

Btw, there is an issue with KMD CORS helper I found while checking this new test - I'll submit a separate PR for this.

Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

Couple questions but it overall makes sense.

daemon/kmd/api/api.go Show resolved Hide resolved
test/e2e-go/cli/goal/expect/corsTest.exp Show resolved Hide resolved
Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests Steve.

@gmalouf gmalouf merged commit 2a02530 into algorand:master Sep 19, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants