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: env leak detection #11203

Closed
wants to merge 12 commits into from
Closed

Conversation

florian-lefebvre
Copy link
Member

@florian-lefebvre florian-lefebvre commented Jun 6, 2024

Changes

  • A new leakDetectionMiddleware is exported from astro/env/middleware. If a server var ends up on the client, an error will be thrown. Inspired by DMNO

Testing

Added a test

Docs

Changeset, I'll update the RFC after approval

Copy link

changeset-bot bot commented Jun 6, 2024

🦋 Changeset detected

Latest commit: 4eee601

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

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

@github-actions github-actions bot added pkg: example Related to an example package (scope) pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Jun 6, 2024
@github-actions github-actions bot removed the pkg: example Related to an example package (scope) label Jun 12, 2024
@florian-lefebvre florian-lefebvre marked this pull request as ready for review June 12, 2024 10:06
@florian-lefebvre florian-lefebvre changed the title feat: experiment with leak detection feat: env leak detection Jun 12, 2024
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

It took me a while to understand that this middleware needs to inspect the Response. I would highlight this important detail, because putting this middleware on the front could allow users to change the secrets before the check.

.changeset/forty-crabs-judge.md Outdated Show resolved Hide resolved
.changeset/forty-crabs-judge.md Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Show resolved Hide resolved
If you're using `astro:env`, you can now use a middleware to detect server envrionment variables leaks on the client:

```ts
import { leakDetectionMiddleware } from 'astro/env/middleware'
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should make this astro:env/middleware instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes things a bit more complicated. There's no codegen involved for the middleware code so I'm a bit reluctant to move it to a virtual import. We were also doing this for astro:env/setup and ended up moving it to astro/env/setup

@matthewp
Copy link
Contributor

Let's set up a call and discuss this feature. Although it's very cool, I worry that the maintenance burden might be too high and not sure if we should take it on. But open to discussing for sure. We can set up a maintainers call to go over it.

@florian-lefebvre
Copy link
Member Author

We need a proper RFC, this is a sensitive feature

@bluwy bluwy deleted the feat/astro-env-leak-protection branch October 8, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants