$env/dynamic/public becomes $env/static/public for pre-rendered pages, breaking app and exposing information

[CaptainCodeman]

Describe the bug

The docs for $env/dynamic/public state:

This module provides access to runtime environment variables, as defined by the platform you're running on.

For pre-rendered pages though, this is incorrect and misleading. They actually behave more like $env/static/public with values coming from build-time .env files used. Not only does this risk exposing information that wasn't intended to be exposed but the behavior of the app becomes indeterminate because the values used in the app depend on the order of page navigation.

As an example, I'm using $env/dynamic/public to read Firebase config from runtime environment values set in Google Cloud Run. Every so often Firebase auth on the client would fail, but a page refresh would always fix it. It depended on which page was initially viewed and exposed development-use configuration that was not intended to be exposed.

Reproduction

See https://github.com/CaptainCodeman/svelte-kit-10008

Logs

No response

System Info

System:
    OS: macOS 13.3.1
    CPU: (6) x64 Intel(R) Core(TM) i5-8500B CPU @ 3.00GHz
    Memory: 36.28 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.10.0 - ~/Library/pnpm/node
    npm: 8.19.2 - ~/Library/pnpm/npm
  Browsers:
    Brave Browser: 106.1.44.112
    Chrome: 113.0.5672.126
    Chrome Canary: 115.0.5786.0
    Firefox: 111.0.1
    Safari: 16.4
    Safari Technology Preview: 16.4
  npmPackages:
    @sveltejs/adapter-node: ^1.2.4 => 1.2.4 
    @sveltejs/kit: ^1.18.0 => 1.18.0 
    svelte: ^3.59.1 => 3.59.1 
    vite: ^4.3.8 => 4.3.8

Severity

annoyance (had to disable pre-rendered pages for correct functionality)

Additional Information

Maybe related to #8946

[tylergannon]

If the rationale is that pre-rendered pages should use $env/public/static, this may be a good case for a linter rule that detects use of $env/public/dynamic on pre-rendered routes.

[CaptainCodeman]

It would be a shame to simply preclude the use of two useful features within the same app.

From what I can tell, the dynamic public env values are embedded into the HTML page output. They are then 'loaded' from an external JS file which uses a rather odd looking snippet that just exports the thing already defined in the embedded JS.

Example JS embedded in the HTML page:

__sveltekit_4vbc3j = {
  base: new URL(".", location).pathname.slice(0, -1),
  env: {"PUBLIC_API_KEY":"proper-public-key"}
};

public.[hash].js file:

const e=globalThis.__sveltekit_4vbc3j.env;export{e};

The problem happens because the wrong thing is embedded in the pre-rendered pages, but if the public.[hash].js file were server generated it could work regardless of the page being SSRd or pre-rendered. Obviously, dynamic server-side variables and the static-adapter aren't compatible, but that applies to any feature that requires any server-side functionality.

Note: I tried using .env.development and while it stops any dev-mode values being exposed, it still causes errors because the env values are then undefined instead on pre-rendered pages. If you use dynamic envs and pre-rendered pages, your app is going to break but you may not notice because it depends on the page people land on first.

And the reason for not using .env / static values is that the correct runtime versions may not even be known at build time. If an app is deployed using automated infrastructure (e.g. Terraform) then cross-references between services can be configured by setting env values for the runtime services.

This way the exact same build can be tested and deployed to dev / test / prod without having to configure and build for each separately. A docker image needs to be built before the deploy can happen.

[ghostdevv]

I feel like it makes sense that the static adapter is using the values it found where it was built, since the point of the static adapter is that you can deploy it to hosts that don't have a server and only serve files

[CaptainCodeman]

No, this is NOT about the static adapter. AFAIK it can only ever use static values. This is about the combination of adapter-node (or potentially any adapter with SSR support) + dynamic env + pre-renderring. The three things together are incompatible / breaks the app.

[ghostdevv]

I don't see how we could fix this with pages that use prerendering & some environment variable. For example say I wanted to fetch an api on a prerendered page, it would use the credentials I have at build time which makes sense. And then I have the same api but on a SSR page it would use the credentials I have at build/runtime (depending on $env/static or dynamic).

I think that for pages in which you want to take advantage of $env/dynamic should not be prerendered, maybe a docs PR would be good for this?

[CaptainCodeman]

I'm pretty sure it can be fixed. Don't put the env values in the HTML, put them in the JS (that is loaded anyway). Any JS module using the values has to wait for that env JS to load anyway, so embedding values into HTML isn't really saving anything IMO - in fact it's inefficient as they have to be served out many more times.

I can create an endpoint in my app to serve up the values, but surely the idea of the $env/dynamic/public system being part of the framework is that the framework should handle this for you. I don't think a mention in the docs are enough, it breaks apps in a horrible way (just by making an innocuous change elsewhere) in a way that may not be immediately obvious (just by using the other features that are listed first in the docs: "Mix and match prerendered pages for maximum performance with dynamic server-side rendering for maximum flexibility")

[benmccann]

I think I mostly agree that it should actually be gotten dynamically rather than prerendered into the page as prerendering it into the page makes it static. Folks who want to prerender dynamic env vars should probably update their apps to use static env vars or to pass in a prop representing the value instead.

That'd be a little tricky to implement. We can't put the values into the JS as you suggest because the JS is also created at build time. We'd need to implement a built-in endpoint. Private env vars don't have this issue because they're only used on the server.

I almost think we should just get rid of $env/dynamic/public and tell users to implement it themselves. That would also give users the option of turning off/on prerendering of the public dynamic env var server route: https://kit.svelte.dev/docs/page-options#prerender-prerendering-server-route.

[CaptainCodeman]

It's not really whether there should be a pre-render option or not, I don't think there is any case to be made for it - unless the app is built on the server it will run on (which would be a bit weird) or you happen to have a local .env file that matches production, the values will always be wrong if they are pre-rendered.

Pre-rendering stops the "dynamic" part doing what it says on the tin. The purpose of dynamic should always be to reflect that values set on the server, not anything that is compiled into the app, otherwise you'd just use the static option.

[dummdidumm]

I'm not sure how to proceed here. Runtime variables can also be added at built time, for example you could set a node env variable in your Vercel project dashboard which is used at build time. Or take the use case in #10737 where prerender is set to auto which means some pages are prerendered while others are dynamic. If you know what you're doing and depend on the dynamic environment variables for the dynamic pages, removing that possibility would break your app without a clear way out. So while yes, in some cases it's a mistake, it's also completely valid in other cases.
I tend towards something like #10805 (warning about it) combined with adjusting the documentation.

[benmccann]

Runtime variables can also be added at built time, for example you could set a node env variable in your Vercel project dashboard which is used at build time

It doesn't really change whether it's a static or dynamic variable. How does it factor into this decision?

If you know what you're doing and depend on the dynamic environment variables for the dynamic pages, removing that possibility would break your app without a clear way out.

You can still do it. We simply won't give you a way to do it that comes with a footgun. But it's really not hard to implement yourself and we could add some documentation around it.

[Rich-Harris]

Thinking out loud:

• yes, the public env values are embedded in the HTML response, and then retrieved from an external JS module. This sounds silly but like many such things it makes more sense when you dig into it. That external module is part of the build, and in many cases will be inlined into a larger chunk. Those chunks are loaded eagerly (with modulepreload) and cached forever (since they are immutable). If we instead imported those values from a module, then it couldn't be part of the build (obviously) so would have to be an endpoint, as @benmccann says. Not only would this mean less efficient chunking, but the dynamic module couldn't be cached immutably, so at minimum you'd have to wait for a 304 response for that module on every single page load before you could boot the app. That would be bad. The current approach is very deliberate
• in general, it doesn't make sense to repopulate $env/dynamic/public on navigation — it would be weird if it changed value during the session (unless a new version of the app is deployed while the session is ongoing, but that's a whole other story)
• prerendering is an exception to that — you don't want to land on a prerendered page and have $env/dynamic/public populated with build-time variables, but equally you don't want to land on a dynamically rendered page and have it be populated with request-time variables that are unexpectedly used on a prerendered page that you later navigate to
• if something is prerendered, you know you're going to be dealing with build-time variables. but in that case, you can just use $env/static/public, which is more efficient anyway.
• conclusion: we should prevent you from using $env/dynamic/public during prerendering

That solves the easy part of the problem. The trickier part is subsequently populating $env/dynamic/public with request-time variables.

Since a prerendered page could access $env/dynamic/public before a navigation (in an event handler, or even in some browser-only code that runs immediately upon rendering), it doesn't seem sufficient to e.g. smuggle the env vars into the data for the next request.

Which means that in the prerendered case I don't think we have a choice other than to serve env vars dynamically, from a ${base}/_env.js module (configurable via config.kit.env.publicModule). This is a bummer — as described, this undermines the performance characteristics of prerendering by forcing you to make a request to a (potentially distant) origin server before you can hydrate an otherwise-fully-prerendered page.

Mitigations:

• in the adapter-static case, the module would also be 'prerendered', so even though you have to wait for a 304, you're at least dealing with a CDN rather than an origin server
• we could skip this stuff if $env/dynamic/public is never imported in the app. this would happen 'for free' if that module basically just re-exports from ../../_env.js if no request-time env vars exist in the HTML
• we can inject a modulepreload for ${base}/_env.js into the prerendered HTML if $env/dynamic/public is imported somewhere in the app

[benmccann]

Will ${base}/_env.js be dynamically generated and served from an endpoint?

[Rich-Harris]

Yes

[Rich-Harris]

(except in the adapter-static case, where it would be generated at build time)

[Rich-Harris]

#11277 fixes this