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

Improve HTTP status codes for the some remote git operations #1209

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MGSousa
Copy link

@MGSousa MGSousa commented Jan 17, 2023

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.

@welcome
Copy link

welcome bot commented Jan 17, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch MGSousa/jupyterlab-git/master

Copy link
Member

@fcollonval fcollonval left a 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.

@MGSousa
Copy link
Author

MGSousa commented Feb 14, 2023

Done!

@lufisaal
Copy link

Hello @fcollonval :)
Is there any news regarding this topic?

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks for the ping @lufisaal and sorry for not getting back at you @MGSousa

I have one comment that prevent to get it merge as is.

@@ -98,7 +98,11 @@ async def post(self, path: str = ""):
)

if response["code"] != 0:
self.set_status(500)
if response["code"] == 128:
Copy link
Member

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:

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.

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.

None yet

3 participants