nksaraf / vinxi

The Full Stack JavaScript SDK
https://vinxi.vercel.app
MIT License
2.19k stars 80 forks source link

Prevent js assets from being added into the public assets when targeting server #363

Closed SeanCassiere closed 2 months ago

SeanCassiere commented 2 months ago

From TanStack Start discussion thread: https://discord.com/channels/719702312431386674/1280228822104674334


Currently, routers with target: "server" and type: "http", have ALL their "assets" output into the "public" directory.

Builds with shared assets therefore cause outputs in the public folder resembling a structure similar to this. In this example, the router name is "api" with its base set to "/api".

public/
  assets/
    regular-client-side-bundles.js
  api/
    assets/
      index-B-8U_3IY.js
      json-Bxz_4eXb.js

See full TanStack Start build-output: https://github.com/SeanCassiere/vigilant-journey/tree/master/.vercel/output/static

Unless "non-built js" assets are found, the "public/api/" directory could probably be omitted during build. Therefore, just outputting:

public/
  assets/
    regular-client-side-bundles.js

From @nksaraf

I think we started doing this when people were importing asset stuff in their server side and were pushing those urls to the client.. those cases we needed to make the sure the assets on the server are available from the client.. but we shouldn’t do this for the js .. just the assets probably So yeah it’s probably a bug we should raise in Vinxi

alfredomariamilano commented 2 months ago

Yeah, I just realised I leaked some API keys this way. I think I'm tapping out. Using vinxi/solidstart is a mess and they are not very responsive. I had hoped that having solid start and tanstack start as users of vinxi would create a better ecosystem, but alas ti hasn't happened. I am grateful for all the awesome work people are pouring into these amazing technologies, but at this point in time they're not safe for production.

SeanCassiere commented 2 months ago

The shared JS assets being leaked into the public folder are not currently a security concern for TanStack Start, since what's being leaked is our handler implementation and our util for easily returning json. Especially since it is common practice to store and read sensitive variables either from an external service or environment variables.

@nksaraf had solid reasoning for why the "assets" were being sent into the public folder was necessary (https://github.com/nksaraf/vinxi/pull/52 issue originating in SolidStart), as well as acknowledging that the built js probably doesn't need to be shipped to be into the public folder.

I'd try implementing a "fix" myself, but I'm just too unfamiliar with this codebase to make any changes with confidence 😅

alfredomariamilano commented 2 months ago

The shared JS assets being leaked into the public folder are not currently a security concern for TanStack Start, since what's being leaked is our handler implementation and our util for easily returning json. Especially since it is common practice to store and read sensitive variables either from an external service or environment variables.

@nksaraf had solid reasoning for why the "assets" were being sent into the public folder was necessary (#52 issue originating in SolidStart), as well as acknowledging that the built js probably doesn't need to be shipped to be into the public folder.

I'd try implementing a "fix" myself, but I'm just too unfamiliar with this codebase to make any changes with confidence 😅

I may have found the issue in one file in which I dynamically import another file, so that might be the cause of it, but still, the issue is that it was leaked in the public folder when it shouldn't, especially since it's duplicated in server and public. Duplication is also another issue, since most JS files are duplicated, like file.js, file2.js, file3.js, file32.js (!) for some reason. I'm considering a move to either Astro or Vike. I would love to contribute to the project and help fix these things, but I'm close to launching my SaaS, so time is tight.