Errors surfaced over GraphQL include information about system internals

[molomby]

Describe the bug

There's a bunch of code in app-graphql to nicely format and unnest errors but (as far as I can tell?) it makes no effort to remove sensitive values from the errors returned. As a result, internal information about project dependancies, version, the file system, database structure, etc. are leaked via the public API.

Eg, when hitting a unique constraint on a create the details include information about DB structure:

insert into "public"."ChallengeSubmission" ("challenge", "createdAt", "post", "status") values ($1, $2, $3, $4) returning * - duplicate key value violates unique constraint "ChallengeSubmission_challenge_post_unique"

This occur even when NODE_ENV is production.

To Reproduce

This specific case can be reproduced by attempting to create an item that violates a unique constraint. (Note Slug fields automatically check for existing items and de-dupe by default. To reproduce, you need to set isUnique on a standard field or add a constraint manually in the DB.)

Presumably it's easier to trigger other errors; this is just what's in front of me.

Solutioning

This behaviour might be handy in dev (and should possibly be maintained there) but in production it's a significant security problem. The solution (as I see it) is to return fairly generic error messages and timestamp over the wire while recording detailed information to the error log. The timestamp can be used by anyone investigating user reports to find the detailed info in the logs.

No values should be pulled directly from the original error objects (ie. I wouldn't consider whitelisting object properties to be a good solution). Although, we may wish to inspect the original error to give some broad indication of error category to the caller (eg. "A DB error occurred") or to pick up on specific cases that deserve a more descriptive message.

We might want to introduce an additional config var to control this behaviour but, when NODE_ENV is production, Keystone should default strongly to whatever is most secure (ie. unless it's been specifically overridden).

[molomby]

@JedWatson points out that returning detailed/raw error info is pretty valuable for some frontend dev loops. (Ie. let's you easily see details when hot reloading in a browser). We should ensure we retain the ability to return raw errors but, to be clear, this behaviour should be the exception rather than the rule.

[stale]

It looks like there hasn't been any activity here in over 6 months. Sorry about that! We've flagged this issue for special attention. It wil be manually reviewed by maintainers, not automatically closed. If you have any additional information please leave us a comment. It really helps! Thank you for you contribution. :)