Dan6erbond / sk-auth

Authentication library for use with SvelteKit featuring built-in OAuth providers and zero restriction customization!
MIT License
577 stars 70 forks source link

Potential secret key exposure #13

Closed srmullen closed 3 years ago

srmullen commented 3 years ago

Hey! Nice work on this library.

I noticed that in a few places you're prefixing secret keys with 'VITE_'. These variables can end up in your client bundle and shouldn't contain any sensitive information.

I'm happy to open a pull request to fix this if you'd like.

Thanks!

Dan6erbond commented 3 years ago

Hey! Thanks for the feedback! You're right, I did use the VITE_ prefix, from what I found those can only be leaked if they're actually used on the client-side, though, no, and Vite doesn't even expose .env variables without the prefix?

Env Variables and Modes | Vite

Either way, if there's a way to keep them safer I'm all ears! One thing I'm considering for the future is to have Auth include a factory to build a client library based on Svelte's stores, something like the following syntax:

export const appAuth = new SvelteKitAuth({
  ...config,
});

export const client = appAuth.buildClient();

This would have the benefit of being able to generate a client that already has its basePath and available providers configured, for additional type-safety, but it'll be even more important to make sure the secrets stay secure on the server-side only.

srmullen commented 3 years ago

I'm sure your right that if a variable isn't referenced in the client-side code then it doesn't get written to the bundle. My worry is that leaving the variable that way, and documenting its use like that, increases the chance that a mistake is made, especially in a framework like SvelteKit, where there are less clear distinctions between what is server-side vs. client-side.

To solve this I've used dotenv to load secret keys rather than relying on Vite.

Dan6erbond commented 3 years ago

Certainly a good point! I'll add that to the documentation, or feel free to make a pull request that mentions this. Especially in the future with that builder it might be necessary to use a server-side store that SvelteKitAuth accesses via context, so that the client itself has no reference to those environment variables. That's where I mostly worry about possible leaks.

Scott-Fischer commented 3 years ago

This was one of my first questions and came straight to the issues. I took a stab at adding some information to the docs for future users that are curious/unaware. (PR: #38 )

Btw, great repo. I'm looking forward to playing around with it for a PoC I'm working on!