fix: don't send server stack trace to client

[yinonburgansky]

PR Checklist

Please check if your PR fulfills the following requirements:

• Addresses an existing open issue: fixes [Bug?]: [security] Server stack trace sent to client on error in server action #1967
• Tests for the changes have been added (for bug fixes / features) - missing infrastructure to unit test server functions.

Avoid sending Error.stack to the client. Stack traces can leak production file paths, internal function names, and other sensitive info, increasing attack surface.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 9eda232

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[netlify]

❌ Deploy Preview for solid-start-landing-page failed. Why did it fail? →

Name	Link
🔨 Latest commit	9eda232
🔍 Latest deploy log	https://app.netlify.com/projects/solid-start-landing-page/deploys/68bed9510fcd0e00080a648c

[yinonburgansky]

This doesn't compile currently due to Cannot access ambient const enums when 'isolatedModules' is enabled.ts(2748)
see https://ncjamieson.com/dont-export-const-enums/
Seroval export Future as a const enum opened issue

[lxsmnsyc]

I think this has been discussed multiple times before, the reason we don't actually do this is:

• A: Bundled files wouldn't actually be helpful in stack trace, as the trace itself is mangled.
• B: Stack trace is helpful in development, so if we don't consider the reason A, we should still consider reason B.

[yinonburgansky]

* A: Bundled files wouldn't actually be helpful in stack trace, as the trace itself is mangled.

• It is exposing the full path of the source files in the production server which can be used by an attacker in another stage to get the entire production code.
• Security through obscurity shouldn't be relied upon. Mangling just makes it more difficult to understand the code, compiling to binary also makes it difficult to reverse engineer the code, but it is 10X easier than not having the code (black box).

This is considered a well known security risk:
CWE-209: Generation of Error Message Containing Sensitive Information:

An attacker may use the contents of error messages to help launch another, more focused attack. For example, an attempt to exploit a path traversal weakness (CWE-22) might yield the full pathname of the installed application. In turn, this could be used to select the proper number of ".." sequences to navigate to the targeted file. An attack using SQL injection (CWE-89) might not initially succeed, but an error message could reveal the malformed query, which would expose query logic and possibly even passwords or other sensitive information used within the query.

see also:

• https://security.stackexchange.com/questions/19130/is-a-stack-trace-of-a-server-application-a-vulnerability
• https://www.quora.com/Is-it-acceptable-to-log-server-errors-to-a-JavaScript-fronted

* B: Stack trace is helpful in development, so if we don't consider the reason A, we should still consider reason B.

I changed it to remove it only on production.
If some developers want server stack traces on production - it should be opt-in since it is considered an information leak, adding an API to enable it can be done separately if really needed - it shouldn't block a security fix.

BTW I think solid-start should provide a mechanism to decide which errors should be sent to the client, e.g. sending DB errors to the client by default is a very bad idea, potentially exposing schema info, credentials location (e.g. env.DB_URL) and other sensitive information.
Wrapping every server action with a try catch is very verbose see my attempt.

[lxsmnsyc]

There's no way to filter out errors atm since seroval handles the entire thing. Will be discussing the rest of the details with the core team