Protect `*.server.*` files in workspace

Rich-Harris commented 1 month ago

Describe the problem

Files that contain .server. in the filename can't be accidentally imported into client-side code, meaning it's safe for them to use sensitive things like API keys and database credentials.

This protection ends at the cwd boundary — anything can be imported into client-side code from outside the current directory. This is unhelpful if your app is in a workspace, importing code from a package elsewhere in that workspace:

<script>
  import { oops } from 'my-workspace-package/module.server.js';
</script>

<h1>{oops()}</h1>

Describe the proposed solution

We could change the behaviour by just removing the !id.startsWith(dirs.cwd) || from this function:

https://github.com/sveltejs/kit/blob/998edb26d431e4ee4d3b4dc792a86960a85a5b45/packages/kit/src/exports/vite/graph_analysis/index.js#L18-L22

That would be a breaking change, so it would presumably need to be behind an option until SvelteKit 3 — something like this:

// svelte.config.js
export default {
  kit: {
    allowWorkspaceServerImports: true
  }
}

Open to bikeshedding.

Alternatives considered

Alternatively (or in addition), we could make things fully customizable: #12477. I'm less keen on that because it makes things less obvious/visible within a codebase.

This proposal doesn't address *.server.* files in node_modules. If a workspace package were published, people might be surprised to learn that the protection no longer applies. But I'm not sure that's an easy thing to fix, because of things like prebundling (and in any case we can't just liberally apply this policy to node_modules contents, because of false positives).

Published packages should probably use a solution like server-only instead. Maybe svelte-package could inject server-only imports into server-only modules?

Importance

nice to have

Additional Information

No response

Kapsonfire-DE commented 3 weeks ago

What about if svelte-kit checks for a special configuration file in all node_modules and cwd? i.e. .sveltekit.server.only and this contains RELATIVE Paths that should be protected, following default glob syntax?

dominikg commented 3 weeks ago

https://www.npmjs.com/package/server-only?activeTab=code

{
  "name": "server-only",
  "description": "This is a marker package to indicate that a module can only be used in Server Components.",
  "keywords": [
    "react"
  ],
  "version": "0.0.1",
  "homepage": "https://reactjs.org/",
  "bugs": "https://github.com/facebook/react/issues",
  "license": "MIT",
  "files": ["index.js", "empty.js"],
  "main": "index.js",
  "exports": {
    ".": {
      "react-server": "./empty.js",
      "default": "./index.js"
    }
  }
}

server-only should really be called react-server-only, as they use the react-server condition. Rather than that we could leverage esm-envs "BROWSER" to throw if that is set, but this could end up causing trouble in vitest (you might have to split tests for non-browser code into a run that doesn't use the browser condition).

I am not convinced sveltekit needs first party support for protecting files outside of its own root. Unless we design sveltekit as "monorepo" aware tool where this would be one of the features. (others could involve awareness about local vs node_modules libs, nested apps etc)

Another option would be a separate vite plugin that checks the filename for .server. or /server/and throws on resolve. That would be usable completely outside of sveltekit.

Kapsonfire-DE commented 3 weeks ago

well sveltekit could define a "server-only" builtin and not use the reacts one and check for its import at resolve, but i feel like every other method than detecting by "path"-rules will have a performance impact? im totally in for a seperate plugin, probably a class you can inherit and override by svelte config

sveltejs / kit