feat: Removing stack trace info in production env

[MomentQYC]

What

Remove stack trace information from production errors, and replace a small number of unnecessary errors with other types of output.

Why

This is what instance/queue/ currently looks like when accessed without logging in (e.g. misskey.io/queue)

Compare the changed look

Many times in production environments stack trace information is unnecessary and a security risk.
For a small number of output changes, I think an average user would prefer a program that only outputs logs as opposed to a program that reports errors when a login action is required.

Additional info (optional)

Checklist

• Read the contribution guide
• Test working in a local environment
• (If needed) Add story of storybook
• (If needed) Update CHANGELOG.md
• (If possible) Add tests

[codecov]

Codecov Report

Merging #11657 (91b83b8) into develop (792622a) will decrease coverage by 0.11%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           develop   #11657      +/-   ##
===========================================
- Coverage    78.83%   78.73%   -0.11%     
===========================================
  Files          922      922              
  Lines        97709    97709              
  Branches      7767     7741      -26     
===========================================
- Hits         77032    76929     -103     
- Misses       20677    20780     +103     

Files Changed	Coverage Δ
...ages/backend/src/server/web/ClientServerService.ts	93.74% <66.66%> (+0.28%)	⬆️

... and 16 files with indirect coverage changes

[tamaina]

I think it's better to use ApiError in API execution service

[tamaina]

There should be no need to change the frontend

[tamaina]

I don't know why the test is failing

[tamaina]

Somehow Jest can't use ./error.js

[MomentQYC]

There should be no need to change the frontend

The main consideration is to facilitate centralized management and reduce unnecessary errors.

[MomentQYC]

There should be no need to change the frontend

The main consideration is to facilitate centralized management and reduce unnecessary errors.

For Example, https://github.com/misskey-dev/misskey/blob/827616f63094997bd44458b8c072a50da176af32/packages/frontend/src/scripts/please-login.ts#L25C3-L25C3

[tamaina]

facilitate centralized management and reduce unnecessary errors.

I don't want them on the frontend.

[tamaina]

I think it is not preferable to remove stack traces or change Errors to console.log on the front end.

[MomentQYC]

I think it is not preferable to remove stack traces or change Errors to console.log on the front end.

I have modified it to use console.log in the production environment, while still throwing an error in other environments. Is this feasible?

[tamaina]

Do not rewrite throw Errors in frontend. They are there to stop calling(upstream) functions, for example.

[MomentQYC]

Do not rewrite throw Errors in frontend. They are there to stop calling(upstream) functions, for example.

OK, I will remove all frontend changes first.

[tamaina]

This PR should not include frontend changes.

[saschanaz]

I'm not sure we want throw ErrorHandling, because errors can be from anywhere, from libraries and native JS functions. What we probably want is to customize Fastify error handler: https://fastify.dev/docs/latest/Reference/Server/#seterrorhandler

[saschanaz]

As above.

[MomentQYC]

I'm not sure we want throw ErrorHandling, because errors can be from anywhere, from libraries and native JS functions. What we probably want is to customize Fastify error handler: https://fastify.dev/docs/latest/Reference/Server/#seterrorhandler

I would like to address the existing problems first and then focus on improving error-related problems in the future. Since I'm not very familiar with TypeScript, it might take some time for me to make changes to the code (if necessary).

[saschanaz]

Given that this PR now sets an error handler that drops stack info, the changes in other files are not needed, right?

[MomentQYC]

Given that this PR now sets an error handler that drops stack info, the changes in other files are not needed, right?

My original intention was to minimize stack trace information in production environments as much as possible, and in particular, as much as possible, not to expose the user to stack trace information especially from the back-end part.
So I think especially in scenarios like the one in the very first screenshot, targeted removal of stack trace information is necessary, other than that it's optional.

[saschanaz]

You mean there will be any situation that the stack will still be exposed even with the error handler?

[MomentQYC]

You mean there will be any situation that the stack will still be exposed even with the error handler?

When I tested along the lines of the code here in the production environment, I still found the stack trace.
https://github.com/misskey-dev/misskey/blob/ab58b651f79e182c20a238f9c07f0c0006a58599/packages/backend/src/server/web/ClientServerService.ts#L148C5-L151C6

[saschanaz]

Hmm, that's specific to Misskey's bull-board usage where the library registers its own error handler (and thus yours is being ignored). It seems reasonable to not throw an error but explicitly send an object instead in https://github.com/misskey-dev/misskey/blob/ab58b651f79e182c20a238f9c07f0c0006a58599/packages/backend/src/server/web/ClientServerService.ts#L148C5-L151C6, as their example does, and later investigate this bull-board issue separately.

For other cases the error handler should be sufficient.

[MomentQYC]

This is where it's most likely to be seen by the user, so here's where I'd like to apply additional error handling (removing the stack trace).
This has been changed to reply.code().send() in the latest changes.
Like this:

[syuilo]

やる場合、new Errorの使用を禁止する何らかの機構(lintとかgithub actionsとか)が無いとこのPRの目的を維持するのは難しそう

[saschanaz]

This is where it's most likely to be seen by the user, so here's where I'd like to apply additional error handling (removing the stack trace).
This has been changed to reply.code().send() in the latest changes.
Like this:

What I'm saying is that it's doable with error handler, it's the sole purpose of error handler anyway. The reason it's not working for bull-board one is explained above, and I suggest using send() only for that exception for now and reverting others. And maybe add some test by adding some test-only route.

[MomentQYC]

reverting others

Do you mean like now?

[saschanaz]

Hmm, I locally tested what Fastify does by adding a random error, but it seems Fastify already does not expose error stack information by default at all. The details field you saw in /queue was from bull-board custom error handler and nothing to do with Fastify default behavior, which only adds statusCode, error, and message.

tl;dr: we don't need setErrorHandler at all, this PR should be sufficient only with changes on ClientServerService.ts.

[MomentQYC]

we don't need setErrorHandler at all

Reverted

[saschanaz]

👍

[syuilo]

👍🏻