astro:env leak detection

[florian-lefebvre]

Body

Summary

Add an opt-in feature to astro:env to allow detecting leaked secrets.

Background & Motivation

Although secrets are supposed to only be used server-side, it's easy to leak them by mistake:

• On the client, by including the secret in the Astro template
• In the terminal, by logging the secret

Goals

• Have a way to detect secrets leaked client-side and error
• Have a way to detect secrets leaked in the terminal and mask them
• Have a way to opt-out of this feature for some secrets if there's a chance they end up client-side without being leaked (eg. a port like 4321)

Example

This feature is inspired by DMNO (really great tool check it out!).

Client-side leak detection can be done using a middleware, see attempt withastro/astro#11203.

Terminal masking probably require overriding? Not ideal

Questions

Are there any other aspect we need to cover?

[Tc-001]

For opt-out, might make sense to have both access: "dynamic" and access: "secret" that act the same but secret has leak detection enabled.

[florian-lefebvre]

2 thoughts:

• I'd be cautious with introducing new terms, as it's already a bit complicated. Sure there are docs but many people don't read them unfortunately
• I think the idea is to have this as an opt-in feature. It may be a bit odd to have some options in the schema for something that people may not used (hence why it's part of the middleware options in my attempt PR)

[dreyfus92]

@florian-lefebvre This is a great proposal! I have a few questions about all of this if you don't mind me asking here are just a couple that I can think of:

• Will there be a way for users to configure which secrets should be monitored for leaks? If so, how might that configuration look?
• For the terminal masking, how will we handle different types of console logs (e.g., console.log, console.error, etc.)? Should we include examples of what masked outputs will look like?
• How do we plan to handle potential false positives (incorrectly identifying a leak) and false negatives (failing to identify a leak)?

[florian-lefebvre]

Great questions!

Will there be a way for users to configure which secrets should be monitored for leaks? If so, how might that configuration look?

In my attempt PR, the behavior was to check for all server variables and opt-out for some keys by adding them to the excludeKeys array (or similar):

leakDetectionMiddleware({
	excludeKeys: ["FOO"] // typed
})

Happy to change this behavior if needed! This was just the quick and simple way ;)

or the terminal masking, how will we handle different types of console logs (e.g., console.log, console.error, etc.)? Should we include examples of what masked outputs will look like?

I have literally no experience with this part so I can't talk about it! This was brought up during a meeting today so I mentioned it in the RFC but I can't really answer that one

How do we plan to handle potential false positives (incorrectly identifying a leak) and false negatives (failing to identify a leak)?

I don't know how we could fail to identify a leak, do you have a scenario in mind? The initial implementation only takes the response body as text and uses a .includes (that would be a bit different for streaming but still similar behavior).

As for false positives, this can occur if the secret value is common, eg. the value "a". That's the reason why there's a way to opt-out per secret.

[theoephraim]

This is great!!

You can read more about our (DMNO) other security related features here and here.

The leak detecting middleware is quite straightforward. We also inject a check into the vite build process to detect any leaks in built javascript files. We currently don't have any per-item opt-out although we probably will add it. I suspect the false positives will come from users having values set like example, rather than any actual secrets, which in theory is minimized in our system.

Described there is one more feature that I don't think @florian-lefebvre had seen yet - which is intercepting http requests so secrets can only be sent to a list of allowed domains. For example a Stripe secret key is only allowed to be sent to api.stripe.com. The idea there being that it's way too easy to accidentally send secrets to logging / exception tracking services. This of course is a bit easier when you are extending pre-published data types that already include the list of allowed domains, but could be interesting regardless.

For both log redaction and http interception, we are patching node globals. It is surprisingly easy to do for a normal node.js application, but things get a bit dicey when you add in lambas, edge runtimes, and all the various build processes that occur between your code and running in some environments. For example, the Lambda and Vercel function runtimes are already patching console methods to redirect console logs to their own logging services. Extra care also needs to be taken to re-trigger the patching logic at the right times, since it's not always just a single long-running process.

For http interception, we are relying on @mswjs/interceptors.

Please if you want to try it out and get a feel for how it feels, give DMNO a spin - we'd love to know what you think!

Relevant code is here:

• https://github.com/dmno-dev/dmno/blob/main/packages/core/src/lib/redaction-helpers.ts
• https://github.com/dmno-dev/dmno/blob/main/packages/core/src/lib/http-interceptor-utils.ts

[bluwy]

I quite like Astro having first class support for this as it shows astro:env is not only a fancier way to use env vars, but is also security focused.

I wonder if making it an option under env.leakDetection would be better instead? We can do this detection immediately after a page/endpoint is rendered. That way there's less friction to enable tighter security.

About the line where we do leak detection, I think the current middleware logic of scanning responses (or rendered strings somewhere else) is enough. I don't think we should intercept anything pass that, like patching globals. The existing leak detection should be enough to capture most cases.