search

t3-oss / t3-env

https://env.t3.gg
MIT License
2.77k stars 86 forks source link

Could this tool be improved to warn new devs about potential bad anti-patterns Astro SSR? #65

Open alexanderniebuhr opened 1 year ago

alexanderniebuhr commented 1 year ago

I think I found a way to leak variables to the client in Astro using Islands with client:only...

Originally posted by @alexanderniebuhr in https://github.com/t3-oss/t3-env/issues/60#issuecomment-1551645066

chungweileong94 commented 1 year ago

I'm not sure if I understand it correctly, it would be good if you can create a small example repo that can reproduce the problem that you describe.

There's always a way to leak ENV with/without t3-env. For instance, if you try to access a server-only ENV on server then pass it to client-side, this will leak the env for sure.

alexanderniebuhr commented 1 year ago

For instance, if you try to access a server-only ENV on server then pass it to client-side, this will leak the env for sure.

That's actually the case I tried to describe with the first bullet. I am wondering if there is a way to at least warn the user about this. I am a more advance user and know the patterns to avoid this, but as I am currently working on educational content for new-comers. I wanted to include t3-env, and it would be great if the tool can not experienced devs about bad pattern or potential leak either in SSR Html or serialised component props.

Not sure if it is feasible, but maybe the tool could somehow parse and check if the env is accessed in a way, which could leak.

chungweileong94 commented 1 year ago

Not sure if it is feasible, but maybe the tool could somehow parse and check if the env is accessed in a way, which could leak.

Well, the tool kinda did, but not quite. It throws an error if you try to use a server env on client, but of course that's not enough to prevent people from leaking the env.

Maybe this is something that ESLint can help, but again there's too many ways/patterns to leak server env, which I think this is something people should really know and learn how to prevent it. But I agree with you that it would be good if the tool can prevent this from the first place.

juliusmarminge commented 1 year ago

I dont see how though? ESLint maybe can detect this?

alexanderniebuhr commented 1 year ago

I dont see how though?

In Astro, we should know which parts of the code are SSR. I would think the assumption that everything is SSR except specifically set an client directive like client:, could be good enough.

If someone uses env variables in these places, it should check if the user tries to access client or server vars and print a warning if server vars are used. The only question I have, how much parsing is necessary and does it impact perf.

intagaming commented 1 year ago

Tried searching for my server variables in the client bundles. Indeed I can see my build-time server-side variables leaked in the bundle, but all the runtime server-side variables are not bundled. I think it's important to point out the specific types of variables here, as:

So the problems to solve here are:

The former might need us to separate different env.ts files for the client and the server. Maybe env.client.ts and env.server.ts. Then make sure env.server.ts is not being imported in the client JS bundles. The latter is like a kind of SQL-injection that either needs dev's attention or linting like discussed on above comments.

alexanderniebuhr commented 1 year ago

build-time server-side variables leaked in the bundle

I saw the built-time envs in my bundle too, after I used your setup. But I changed my implementation a little bit (https://github.com/t3-oss/t3-env/issues/60#issuecomment-1551645066), and now it seems there are not visible in the bundle anymore. It seems vite takes care of them.

The runtime leak is definitely something the dev has to prevent, and not an technical issue. But I think if the tool could still warn about this, it would be great!

intagaming commented 1 year ago

Lesson learned: One must build the project, then go to the DevTools and search for their envs on the client, just to be sure. Maybe even inspect the server bundle in dist/server/chunks/pages/ to see if the build-time envs are in place.

Luckily my project has not gone to the production yet. Before any linting plugin exists, just make sure to double check. You can't trust linting tools to go eslint --autofix and expect them to do it correctly forever. Linting tools have limitations too. Users of Astro SSR or any SSR framework, be warned.

Also take a look at https://github.com/withastro/astro/blob/main/packages/astro/src/vite-plugin-env/index.ts. It is a bit magical what is has done on top of Vite. Oh and I have some documentation about all these in my issue here #60.

juliusmarminge commented 1 year ago

Can you explain what variables are leaked and how? The framework shouldn't let server-side variables be defined on the client, no matter what file you're using it in. There is no way i can use process.env.DATABASE_URL on the client, and thus not env.DATABASE_URL either.

What am I missing?

intagaming commented 1 year ago

Can you explain what variables are leaked and how? The framework shouldn't let server-side variables be defined on the client, no matter what file you're using it in. There is no way i can use process.env.DATABASE_URL on the client, and thus not env.DATABASE_URL either.

What am I missing?

It's actually a little bit of an anti-pattern: if you use Vite's define and use the defined thing in t3-env.ts, then that thing will be included both in the client and the server. The purpose was to define build-time server-side variables by defining process.env.XXX with something so that Vite will go replace it at build time, but actually you can just do that and use import.meta.env.XXX instead of using process.env.XXX or whatever you defined in Vite's define.

Actually that has nothing to do with t3-env at all. Just a Vite thing. I wanted to validate that at build time via t3-env, but that isn't related to the above "leak" too much.

The second "leakability" is rendering the server variables in the HTML, which can happen everywhere, not just client:only. You can just read it in the Astro's frontmatter and render it.

That also isn't related to t3-env at all, and can happen anywhere there is server-side rendering involved. So I guess nothing can be done from t3-env except trying to provide a kind of SQL injection prevention but for env vars.

alexanderniebuhr commented 1 year ago

Can you explain what variables are leaked and how?

As far as I can tell, if you setup your envs for Astro correctly nothing is leaked per definition. But you can use process.env.DATABASE_URL in code which is server-side rendered and on the server it will replace this with the raw value. This raw value is sent to the client and rendered from HTML. This is something you can do in any framework, and you can't fully prevent, but helping devs catch this would be great. So this issue pivoted to discussion a potential solution to warn users about this anti-pattern.

juliusmarminge commented 1 year ago

Ok so the same would be true for RSC if you store a value from env to a variable and passet as prop. Same can be true if you return a value in an api endpoint.

I dont think its the job of t3-env to present this. Maybe we can make a @t3-env/eslint-plugin that can analyze the code and see if a pattern like this is present and warn about it? Would be a pretty cool thing

alexanderniebuhr commented 1 year ago

Ok so the same would be true for RSC if you store a value from env to a variable and passet as prop. Same can be true if you return a value in an api endpoint.

Correct!

I dont think its the job of t3-env to present this. Maybe we can make a @t3-env/eslint-plugin that can analyze the code and see if a pattern like this is present and warn about it?

Agree, that it would be pretty cool to have and it will be a lint warning instead of an actual thrown error. However not sure if an eslint plugin would be the best solution. Not everyone uses eslint (I use rome tools).

I wonder if this linting step could be included in t3-env as an opt-in configuration option? So it can be chosen to be used and it will offer this to devs.