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

feat(netlify): verification for edge middleware #152

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Feb 5, 2024

Changes

  • Ensures that when edgeMiddleware is enabled, only the edge middleware can call the internal serverless function.

Testing

Manual

Docs

Copy link

changeset-bot bot commented Feb 5, 2024

🦋 Changeset detected

Latest commit: 4ac43a5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@astrojs/netlify Minor
@test/netlify-hosted-astro-project Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 5, 2024

!preview edge-middleware-verification

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Snapshots have been released for the following packages:

  • @astrojs/netlify@experimental--edge-middleware-verification
Publish Log
🦋  warn ===============================IMPORTANT!===============================
🦋  warn Packages will be released under the experimental--edge-middleware-verification tag
🦋  warn ----------------------------------------------------------------------
🦋  info npm info @astrojs/cloudflare
🦋  info npm info @astrojs/netlify
🦋  warn @astrojs/cloudflare is not being published because version 9.0.0 is already published on npm
🦋  info @astrojs/netlify is being published because our local version (0.0.0-edge-middleware-verification-20240205170200) has not been published on npm
🦋  info Publishing "@astrojs/netlify" at "0.0.0-edge-middleware-verification-20240205170200"
🦋  success packages published successfully:
🦋  @astrojs/netlify@0.0.0-edge-middleware-verification-20240205170200
🦋  Creating git tag...
🦋  New tag:  @astrojs/netlify@0.0.0-edge-middleware-verification-20240205170200
Build Log

> root@0.0.0 build /home/runner/work/adapters/adapters
> turbo run build --filter="@astrojs/*"

• Packages in scope: @astrojs/cloudflare, @astrojs/netlify, @astrojs/test-utils
• Running build in 3 packages
• Remote caching disabled
::group::@astrojs/netlify:build
cache miss, executing f45dadaafc342c58

> @astrojs/netlify@0.0.0-edge-middleware-verification-20240205170200 build /home/runner/work/adapters/adapters/packages/netlify
> tsc

::endgroup::
::group::@astrojs/cloudflare:build
cache miss, executing 04ab7c5839174536

> @astrojs/cloudflare@9.0.0 build /home/runner/work/adapters/adapters/packages/cloudflare
> tsc

::endgroup::

 Tasks:    2 successful, 2 total
Cached:    0 cached, 2 total
  Time:    4.22s 

Copy link
Contributor

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

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

This is a risky change, and it's not obvious to me what attack vector this protects against. Could you outline that, either here or via email if you feel like it's too sensitive?

@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 5, 2024

@Skn0tt
Copy link
Contributor

Skn0tt commented Feb 5, 2024

Got it, thanks! That looks like a spooky vuln definitely o.O

My understanding of the PR is that you want to ensure that the SSR function can only be reached via the Middleware, if there's a middleware configured. I don't think that attack vector exists currently, because the middleware is configured to run on all requests (except for the requests to Image CDN and static assets):

path: "/*", excludedPath: ["/_astro/*", "/.netlify/images/*"]

So there's no way of circumventing the Middleware function. We're already ensuring this works as expected inside of the CDN, I don't think we need another check in the adapter on top of that. Am I missing something here?

@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 5, 2024

Yeah, looks like Netlify gives us a guard rail, which is great. It would be even better to have a safety net on top of it.

@Skn0tt
Copy link
Contributor

Skn0tt commented Feb 6, 2024

Your call! If the behaviour is the same across all adapters, then it's fine I guess.

@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 6, 2024

@Skn0tt I have added you to a private thread in the #netlify channel of the astro discord. You might want to take that into account as well.

@lilnasy lilnasy requested a review from Skn0tt February 6, 2024 13:34
Copy link
Member

@alexanderniebuhr alexanderniebuhr left a comment

Choose a reason for hiding this comment

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

Pretty sure the changeset is good, but let's have docs take a look ;)

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Changeset looks great!! 💪

@lilnasy lilnasy merged commit 816bdc4 into main Feb 6, 2024
8 checks passed
@lilnasy lilnasy deleted the netlify-middleware-verification branch February 6, 2024 17:02
@github-actions github-actions bot mentioned this pull request Feb 6, 2024
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.

4 participants