ServerError: Inconsistent handling of invalid input

[nikita-fuchs]

Please try understanding the severity of this issue for running Strapi in production.

Bug report

Required System information

Node.js version: 16.14.2
NPM version: 8.5.0
Strapi version: 4.6.0
Database: Postgres
Operating system: Ubuntu 20.04

Describe the bug

Currently, wherever API data is checked 'manually' and not through the validation mechanism, the default behaviour is throwing a server error. A small excerpt:

This leads to two problems:

1. How is a frontend supposed to know what the issue with some user's input is, if the only reply is a 500 server error? Is it a bug in the code my client's users should report to us, or was there just some issue with a wrong password?

2. If you throw an error right away every time some user enters a wrong password, your event monitoring tool will look like this and it's impossible to filter out real issues:

Expected behaviour

No errors thrown when there isn't actually something failing. The correct thing to do here is a proper HTTP response, ideally formatted the same way as the regular validation errors.

Come on guys, I see the amount of hard work you've put in this awesome project, don't let bad execution of basic practices overshadow all the hard effort. 🙏

[derrickmehaffy]

Hello,

In order to keep our GitHub issues clean and only related to bug reports I've moved your enhancement / feature requested over to our feedback database.

You can find your request here: https://feedback.strapi.io/developer-experience/p/error-codes

Thanks!

[nikita-fuchs]

Hey, I made a quick video about the severity of this: https://www.youtube.com/watch?v=bCPx2WE_bus

I also wouldn't call this a feature request, because the user (developer), whose perspective a product needs to be seen from, is used to getting the properly formatted error responses for faulty input data from strapi's content api. And suddenly getting a server error in the same situation is clearly a bug at first glance. One cannot expect users to guess or know that responding to 37 different cases of malformed input at specific certain spots with a 500 error was a deliberate choice made at some point. And at least living up to HTTP status code practices serving proper 4xx when some password was forgotten instead of everything being a server error is really not an 'enhancement' or feature request.

[derrickmehaffy]

Hey @nikita-fuchs can you share that on the feedback link up above?

[nikita-fuchs]

hey, @derrickmehaffy sure, done.

[nikita-fuchs]

Hey, after 1,5 years https://github.com/strapi/strapi/blob/a64ced5364618f917adb31b8684b4e3c01c58862/packages/plugins/users-permissions/server/controllers/auth.js is still filled with Erorrs being thrown for basic things instead of returning proper API responses, e.g.:

strapi/packages/plugins/users-permissions/server/controllers/auth.js

Lines 65 to 66 in a64ced5

	if (!user) {
	throw new ValidationError('Invalid identifier or password');

The error log console of my google cloud deployment is clogged with errors for failed logins, can't activate notifications to my mobile phone to be notified about actual errors. I've patched things myself with

 if (!user) {
        // throw new ValidationError('Invalid identifier or password');
        return this.respondWithError(ctx, 'provider', 'This provider is disabled');
      }

using

  respondWithError(ctx, path, message) {
    return ctx.send(
      this.generateValidationErrorResponse(
        [this.generateValidationError(path, message)]
        ));
  },

  generateValidationErrorResponse(errors) { 
    return {
      "data": null,
      "error": {
          "status": 400,
          "name": "ValidationError",
          "message": `${errors.length} errors occurred"`,
          "details": {
              "errors": [...errors]
          }
      }
  }
  },  

   generateValidationError(path, message) {
    return {
      path: [path],
      message,
      name: 'ValidationError'
    }},

Feel free to copy this or just take it as an example, but this really needs to be fixed, shouldn't take more than 2h !

[strapi-bot]

This issue has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/proper-api-responses-instead-of-thrown-errors/39157/1

[nikita-fuchs]

@derrickmehaffy can't help but to ask to reopen? This is really against common best practices and makes strapi really hard to run and monitor, driving any devops insane.

[derrickmehaffy]

@derrickmehaffy can't help but to ask to reopen? This is really against common best practices and makes strapi really hard to run and monitor, driving any devops insane.

Given how old this is, it would be better to open a new one, specifically if it can be validated on the v5 beta. At best this would be a medium sev and medium sev bugs most likely won't be fixed on v4

[nikita-fuchs]

Given the time we've waited for this to be fixed it's okay to wait a little longer for it to be tackled in v5, soon ;)

It definitely seems to be applicable to v5, just a quick glimpse shows many cases: