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

Failed Login should result in the appropriate HTTP-Status Code #657

Open
ccoenen opened this issue Dec 17, 2017 · 4 comments
Open

Failed Login should result in the appropriate HTTP-Status Code #657

ccoenen opened this issue Dec 17, 2017 · 4 comments
Labels
auth provider enhancement Wants to improvide an existing feature upstream This issue belongs to a library or component outside

Comments

@ccoenen
Copy link
Contributor

ccoenen commented Dec 17, 2017

Right now, If you try to login with weird credentials, you'll receive an HTTP 302 redirect and get back to the root of the project. Which will happily claim to be HTTP 200 OK. At no point is there a machine-readable mentioning of the failed login.

This is bad for at least two reasons: You can't properly script a login and browsers will offer to save your credentials, even if they are faulty.

I would like to suggest to change this to actually at some point reply with a HTTP 400-ish response code. Perhaps 401 Unauthorized.

@SISheogorath
Copy link
Contributor

We can add a parameter to the main page call like "?failed-login=true" which results into a 401. Because the redirect itself is part of passport, not HackMD.

@SISheogorath SISheogorath added enhancement Wants to improvide an existing feature security auth provider labels Dec 18, 2017
@ccoenen
Copy link
Contributor Author

ccoenen commented Dec 18, 2017

In that case, I'd rather raise this issue upstream.

@ccoenen ccoenen added upstream This issue belongs to a library or component outside and removed security labels Dec 18, 2017
@ccoenen
Copy link
Contributor Author

ccoenen commented Dec 18, 2017

(i removed the security tag, I believe this was put in in error - it is not a security problem, more a reliability problem)

@SISheogorath
Copy link
Contributor

SISheogorath commented Dec 18, 2017

Just for completion:

https://github.com/hackmdio/hackmd/blob/a8fa88831705625e053c7d493a4b2f83b7b2a4a8/lib/web/auth/github/index.js#L26-L27

This is where we configure it as an example. This is done for every auth provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth provider enhancement Wants to improvide an existing feature upstream This issue belongs to a library or component outside
Projects
None yet
Development

No branches or pull requests

2 participants