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 HTTPCookieAuth for token auth in req cookies #166

Closed

Conversation

mattproetsch
Copy link

This allows users to create a HTTPCookieAuth, which is used like HTTPTokenAuth, but reads the token value from a cookie in the request instead of from an HTTP header.

Using a cookie with httponly=true; samesite=strict; secure=true flags can be more secure than reading from an HTTP header because the browser will never allow JavaScript to read the token, which defends against XSS attacks sending the token to other servers. It is just sent automatically by the browser along with any request to the site which set the cookie.

@miguelgrinberg
Copy link
Owner

which defends against XSS attacks

Not really. This would allow any JS code that runs in your page (like code for one of your dependencies or code that was injected by a malicious user) to send authenticated requests to your server.

@@ -195,6 +195,50 @@ def ensure_sync(self, f):
return f


class HTTPCookieAuth(HTTPAuth):
def __init__(self, scheme=None, realm=None, cookie_name=None):
super(HTTPCookieAuth, self).__init__(scheme or 'Bearer', realm, cookie_name)
Copy link
Owner

Choose a reason for hiding this comment

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

What is the purpose of scheme or realm when using cookies?

Copy link
Author

@mattproetsch mattproetsch Aug 3, 2024

Choose a reason for hiding this comment

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

Scheme mimics the behavior from HTTPTokenAuth with a user-specified header so that when scheme='ApiKey' the entire cookie value is treated as the token. When scheme is any other, the value of the cookie is expected to be a space-delimited value like '<Scheme> <Token>'. Realm isn't used for authentication, but will be passed along in WWW-Authenticate if auth fails.

@mattproetsch
Copy link
Author

which defends against XSS attacks

Not really. This would allow any JS code that runs in your page (like code for one of your dependencies or code that was injected by a malicious user) to send authenticated requests to your server.

Yes, good feedback. I should have said that a cookie with those flags helps defend specifically against credential-stealing XSS attacks. Thanks for approving the run. I forgot to lint before submitting this PR, so let me fix that.


@cookie_auth.error_handler
def error_handler():
return 'error', 401, {'WWW-Authenticate': 'MyToken realm="Foo"'}
Copy link
Owner

Choose a reason for hiding this comment

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

This is really not something I can agree with. Here you are mixing your own custom authentication implementation based on cookies with parts of the HTTP Authentication standard, which uses the Authorization header as vehicle for the client to send credentials. There is no good reason to return the WWW-Authenticate header that I can see and I have never seen any implementation that does it.

@@ -195,6 +195,49 @@ def ensure_sync(self, f):
return f


class HTTPCookieAuth(HTTPAuth):
def __init__(self, scheme=None, realm=None, cookie_name=None):
Copy link
Owner

Choose a reason for hiding this comment

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

Scheme and Realm apply to HTTP Authentication. I don't see how they help when using cookies.

return self.ensure_sync(self.verify_cookie_callback)(cookie)

def get_auth(self):
expected_cookie_name = self.cookie_name or 'Authorization'
Copy link
Owner

Choose a reason for hiding this comment

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

Using a cookie named Authorization is confusing. Does anybody do this? I have never seen it.

@miguelgrinberg
Copy link
Owner

miguelgrinberg commented Aug 3, 2024

Hi, thanks for the PR! I'm trying to see how this can benefit other users. What would be the benefit of using a home grown cookie-based authentication that you disguise as being somewhat similar to HTTP Authentication, instead of using what everybody uses with Flask, which is Flask-Login? This isn't more (or less) secure than Flask-Login, so why not go with the solution most people use?

I've asked above, I would be curious to know if there is any implementations out there that use a solution like the one you are proposing.

@mattproetsch
Copy link
Author

Hi Miguel, I really only needed the ApiKey-like functionality in a HttpOnly cookie, which Flask-Login already provides. I should be using that instead. Thanks for pointing me in the right direction.

The behavior of the WWW-Authenticate header and the scheme/realm is because I was trying to keep all the tests passing after starting with the tests for HTTPTokenAuth tests and modifying them to use a cookie instead of a header as a transport - there was no real protocol or product that I was trying to support.

Thanks for your patience (and sorry for the misguided PR).

@mattproetsch mattproetsch deleted the cookie-auth branch August 3, 2024 18:57
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.

2 participants