fix(dav): return 401 instead of 503 for disabled user login attempts#58804
fix(dav): return 401 instead of 503 for disabled user login attempts#58804boris324 wants to merge 1 commit intonextcloud:masterfrom
Conversation
When a disabled user's device tries to sync via WebDAV, a LoginException was caught by the generic Exception handler in Auth::check(), resulting in a 503 ServiceUnavailable response logged at error level. This caused constant fatal errors in logs for disabled users whose devices continue to attempt sync. Catch LoginException specifically and return 401 NotAuthenticated instead, logging at info level since this is expected behavior. Fixes nextcloud#22758
boris324
requested review from
ArtificialOwl,
leftybournes,
nfebe and
sorbaugh
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR adjusts the DAV authentication error handling so that disabled-user login attempts (and other LoginExceptions) result in a 401 NotAuthenticated instead of being treated as a server-side 503 ServiceUnavailable, reducing unnecessary high-severity log noise during repeated sync attempts.
Changes:
- Catch
OC\User\LoginExceptioninOCA\DAV\Connector\Sabre\Auth::check()and convert it intoSabre\DAV\Exception\NotAuthenticatedwhile logging atinfolevel. - Add a unit test covering the
LoginException→NotAuthenticatedbehavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/dav/lib/Connector/Sabre/Auth.php | Adds a LoginException catch to return 401 NotAuthenticated instead of 503 ServiceUnavailable. |
| apps/dav/tests/unit/Connector/Sabre/AuthTest.php | Adds a unit test asserting LoginException is mapped to NotAuthenticated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (LoginException $e) { | ||
| Server::get(LoggerInterface::class)->info($e->getMessage(), ['exception' => $e]); | ||
| throw new NotAuthenticated($e->getMessage(), 0, $e); |
There was a problem hiding this comment.
When a LoginException bubbles up from validateUserPass()/auth(), the PHP session may still be open (validateUserPass() does not close the session on LoginException). This catch block converts the exception but doesn't close the session either, which can keep the session lock held longer than necessary and potentially slow down concurrent requests. Call $this->session->close() (ideally in a finally or at least before logging/throwing) on this path as well.
| Server::get(LoggerInterface::class)->info($e->getMessage(), ['exception' => $e]); | ||
| throw new NotAuthenticated($e->getMessage(), 0, $e); |
There was a problem hiding this comment.
Propagating the LoginException message to the client (via NotAuthenticated) can reveal account state (e.g. disabled vs. non-existent/wrong password), enabling user/account enumeration over WebDAV. Consider returning a generic NotAuthenticated message (consistent with other auth failures) while logging the detailed reason server-side.
|
There may be room for improving the logging/log level, but I believe the 50x versus 401 may be related to a (additional?) safeguard preventing inadvertent date deletion client-side... though I may be misremembering. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Fixes #22758
When a disabled user's device tries to sync via WebDAV, the
LoginExceptionwas caught by the genericExceptionhandler inAuth::check(), causing a 503 ServiceUnavailable logged at error/fatal level. This spams logs when disabled users have devices that keep trying to sync.This PR catches
LoginExceptionspecifically and returns 401 NotAuthenticated instead, logging at info level since a disabled user attempting login is expected behavior, not a server error.