Ensure stack traces are removed from all server side errors #5541
Conversation
🦋 Changeset detectedLatest commit: a40c141 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| expect(html).not.toMatch("RESOLVED"); | ||
| expect(html).toMatch('{"message":"REJECTED"}'); | ||
| // TODO: I think we should be getting the stack here too? | ||
| expect(html).toMatch(/stack/i); |
There was a problem hiding this comment.
@jacob-ebey Should we get the stack here? I think it might have something to do with not having NODE_ENV set right in the E2E setup?
| let html = await response.text(); | ||
| expect(html).toMatch("Defer Error"); | ||
| expect(html).not.toMatch("RESOLVED"); | ||
| // TODO: is this expected or should we get "Unexpected Server Error" here? |
There was a problem hiding this comment.
@jacob-ebey I think this isn't working also because of NODE_ENV?
| export function sanitizeError<T = unknown>(error: T, serverMode: ServerMode) { | ||
| if (error instanceof Error && serverMode !== ServerMode.Development) { | ||
| let sanitized = new Error("Unexpected Server Error"); | ||
| sanitized.stack = undefined; | ||
| return sanitized; | ||
| } | ||
| return error; | ||
| } |
There was a problem hiding this comment.
This should be the underlying method that every Error runs through
| error: Error, | ||
| serverMode: ServerMode | ||
| ): SerializedError { | ||
| let sanitized = sanitizeError(error, serverMode); |
There was a problem hiding this comment.
Sanitize all errors before serializing them
| @@ -89,7 +89,7 @@ export const createRequestHandler: CreateRequestHandlerFunction = ( | |||
| let match = matches!.find((match) => match.route.id == routeId)!; | |||
| response = await build.entry.module.handleDataRequest(response, { | |||
| context: loadContext, | |||
| params: match.params, | |||
| params: match ? match.params : {}, | |||
There was a problem hiding this comment.
Be defensive in the case of /valid-route?_data=not-a-valid-route
| headers: { | ||
| "X-Remix-Error": "yes", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
errorBoundaryError was only being used in this one spot so I inlined it
| // Sanitize errors outside of development environments | ||
| if (context.errors) { | ||
| context.errors = sanitizeErrors(context.errors, serverMode); | ||
| } |
There was a problem hiding this comment.
Sanitize before we render from this context
brophdawg11
force-pushed
the
brophdawg11/sanitize-errors
branch
from
8ae953f
to
ef3042e
Compare
github-actions
bot
added
the
awaiting release
|
🤖 Hello there, We just published version Thanks! |
Ensure all server-side flows are stripping
Errorstack traces inproductionmode, including document requests, data requests, resource requests, and deferred responses.Closes: #5445
Related: #5446, #5199
Testing Strategy: new integration tests