Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add status code 429 for too many requests on ocs API requests #43611

Closed
wants to merge 1 commit into from

Conversation

akhil1508
Copy link
Contributor

  • Resolves: No issue

Summary

  • The status code is 500 with an "Internal Server Error" when there is a too many requests exception with ocs API
  • It should be a 429 instead

Checklist

@solracsf solracsf requested review from a team, ArtificialOwl, icewind1991, blizzz and nickvergessen and removed request for a team February 16, 2024 16:09
@nickvergessen
Copy link
Member

On which endpoint do you receive a 500?

@@ -81,6 +81,9 @@
OC_API::respond($ex->getResult(), OC_API::requestedFormat());
} catch (\OC\User\LoginException $e) {
OC_API::respond(new \OC\OCS\Result(null, \OCP\AppFramework\OCSController::RESPOND_UNAUTHORISED, 'Unauthorised'));
} catch (\OCP\Security\Bruteforce\MaxDelayReached) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't already handled a few lines up (line 67 in this code block) due to recent #41937? Or do I need to finish drinking my morning coffee? :)

@akhil1508 Did you by chance see this in a slightly outdated environment in the wild (latest v26, v27, and v28 should all handle this already)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you by chance see this in a slightly outdated environment in the wild (latest v26, v27, and v28 should all handle this already)?

Ahhh yes, my bad. Apologies!

@nickvergessen
Copy link
Member

Closing as the check is already there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants