-
Notifications
You must be signed in to change notification settings - Fork 318
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
Improve HTTP status codes for the some remote git operations #1209
base: main
Are you sure you want to change the base?
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion @MGSousa
Although I don't agree with switching any code error to 401. The git commands may fail for other reasons than an authentication error. We could raise an specific error for authentication failure and set the code to 401 in that specific case. But this should not be the default code.
The usage of 404 seems acceptable.
Done! |
Hello @fcollonval :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -98,7 +98,11 @@ async def post(self, path: str = ""): | |||
) | |||
|
|||
if response["code"] != 0: | |||
self.set_status(500) | |||
if response["code"] == 128: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot rely on the error code from git to know what went wrong.
It requires parsing the error message - as done already in the frontend:
Line 9 in 50df369
export const AUTH_ERROR_MESSAGES = [ |
So we should add the same test here to determine which status to set.
The same comment applies for the fetch, push and pull methods.
Improve HTTP status codes by switch some of the Server Error (5xx) to Client Error codes (4xx), since it is related to client specific actions.
Also this allows to not break some security rules for 5xx codes in NGINX and other reverse proxies, maintaining the feature as is.