-
Notifications
You must be signed in to change notification settings - Fork 4k
fix(sveltekit): use env/static/private for env vars
#10117
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
@dievardump is attempting to deploy a commit to the authjs Team on Vercel. A member of the Team first needs to authorize it. |
|
Hmm so my local project using |
|
I am using @sveltejs/kit 2.5.1 and @sveltejs/adapter-node 4.0.1 When building I get the error:
And when I change the dynamic lines for static, it is followed with an error with reading .length on an undefined ( So yes my bad, it's not while building but while pre-rendering some of the page. |
|
Yeah so the way that Svelte works, the I'm not super experienced with Svelte though, is using the env vars here out of |
|
For me, if you expect the variables to be set at build time, then you go for static. If you expect those variable can change during the life of the app, or they should only be set at runtime, then go for dynamic. Because of the nature of the current module, accessed mostly as the first thing seen in the server through hook, and because you would expect the AUTH vars to be known and set at build time, I would suggest to use static over dynamic the doc says: dynamic
static
|
env/static/private for env vars
|
@dievardump okay yeah that makes sense. I've updated from If yes, then I think we can go ahead with this! |
|
@dievardump great attempt, but I don't think this is a good idea. This comment puts it nicely: #9436 (comment) Let us think a better approach to this issue. Is this related? #9809 |
|
I think #9809 is specifically corrected by my changes to packages/frameworks-sveltekit/src/lib/env.ts I hadn't seen that issue before doing my PR. Building failed, I went into node_modules, I fixed, then I made a PR. But both errors mentioned in that issue should be fixed by my PR yes. About the security of having the vars in the files: I can imagine why a security audit would flag it, but I would consider anyone having access to the "build image" as a trusted party. Because if they have access to the result of the build, they have access to the running environment, and therefore to the process.env vars. So using dynamic or static here wouldn't make much difference in term of security. If you can scan the files, you can do an "echo $AUTH_SECRET" But if this is a problem, then I would use |
|
@dievardump after some internal discussion, we've decided to stick with the dynamic env vars for the following reasons:
That being said, I appreciate the effort here and would still like to avoid the issue yuo've raised here about prerendering faililng if one of the env var's we've used in the library aren't defined by the user. As a concrete example, I'd also like to keep your change moving the |
|
The problem is still that it will not be possible to pre-render any route if using SvelteAuth and Would you then consider using ps: using |
|
Would really like a solution to this, prerender is really important for quick page loads and even setting one page to breaks the build process. Is there a temporary fix like adding |
|
What you can do today to fix that is to only add But the better solution for the future would be for the plugin to use |
|
Hey chiming in with an update, so we definitely don't want to go with the We took another look at this and talked to some other svelte folks and it looks like basically during prerendering you shuoldn'tn rely on any authenticated info to begin with. Therefore, we're tryign to find a way to skip calling the auth routes during build. Like But the problem is, those routes are all dynamic from the Anyway, long story short. I don't think we want to merge any of these changes unfortunately. I've opened a new PR attempting to solve the prerender issue and we'd still love yuor help there if yuo're interested 🙏 |
I can't find the actual part of the SvelteKit code that causes the import of `"$env/dynamic/private"` inside a node_module to fail, but I fixed our example so the problem surfaced. I also discovered sveltejs/kit#12028 which states that a warning should be added that third-party libraries should not use this import. And nextauthjs/next-auth#9809 + nextauthjs/next-auth#10117 which highlight problems when it is used. Perhaps we'll need to explore something like nextauthjs/next-auth#6247 but it seems that `process.env` is populated in SvelteKit so we can use it for now. Fixes #982
|
Its still failing :( |
SvelteKit does not build with the latest version.
Two reasons:
build, sveltekit can only access "static" private env var, not dynamicsetEnvDefaultswas not settingconfig.basePathwhich is read in thehandlefunction, throwing an error for reading undefined.