Different classes of validation failures
- Dominant language
- JavaScript
- Stars
- 229
- Forks
- 99
- PR merge metrics
- No merged PRs in 30d
Description
Opening a new issue since the described issue is broader than the one that was reported.
> I think the behavior discussed in this thread is the cause of a more general problem that I have seen.
>
> In our `validate` function, we run a database query to see if the user still exists, as is recommended in the docs. Most of the time, it works fine. However, if there's a connection failure to the database, the database client throws an error, and errors in `validate` seem to log the user out no matter what.
>
> I don't think it makes sense to log the user out just because the backend can't contact the database. Probably a 500 error should be returned, with the user still logged in so they can try again.
_Originally posted by @sholladay in [#245](https://github.com/hapijs/cookie/issues/245#issuecomment-3226185281)_
The current validation handler assumes any error is caused by *invalid credentials*. Given that it is indeed common to need an external call to perform the validation, another class of validation errors are *(temporary) internal validation* errors. A third class (from #245) are *policy* errors, though these should generally be generated *after* validation. These errors should ideally be handled separately to avoid clearing, what could still be valid, credentials and return a code `500` or similar.
Not clearing the state on these kind of errors could probably be handled as a bug fix. Eg. by only clearing the cookie on a `Boom.unauthorized()` error.
Ideally, the original error would be returned as well, as suggested in #245. This could also be argued as a non-breaking change, especially if non-Boom errors are still treated as `Boom.unauthorized()`.
It should be noted that system errors (like `TypeError`) already don't clear the cookie, and return a code `500` response (as demonstrated in my new tests from #252.
@sholladay FYI, your specific clearing issue can be mitigated by not using the `clearInvalid` option.
Contributor guide
Assessment
This issue has not been assessed yet.