hapijs / hapijs/cookie

Different classes of validation failures

Open
#253 0 comments 0 reactions 0 assignees View on GitHub
support
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.