Send clear-site-data header for invalid cookies
#9113
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the custom SafeSimpleCookie implementation with Python's standard library SimpleCookie for cookie parsing, removing defensive parsing logic.
- Removed custom
SafeSimpleCookieclass with injection-attack protection - Replaced
cookies.parse()withcookies.load()method - Added exception handling around cookie loading in auth extension
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| edb/server/protocol/http.py | Removed entirely - deleted custom SafeSimpleCookie class with defensive parsing |
| edb/server/protocol/protocol.pyx | Updated to use standard SimpleCookie and load() method instead of custom implementation |
| edb/server/protocol/auth_ext/http.py | Added try-except block around load_cookies() to handle parsing errors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def load_cookies(self): | ||
| self.cookies.parse(self.cookie_header.decode('ascii')) | ||
| self.cookies.load(self.cookie_header.decode('ascii')) |
There was a problem hiding this comment.
The standard library's SimpleCookie.load() may raise exceptions on malformed cookie headers. While the auth extension now wraps this call in a try-except, the load_cookies() method could be called from other locations in the codebase without exception handling. Consider adding a try-except within this method to handle CookieError and other exceptions gracefully, or ensure all call sites are protected.
| self.cookies.load(self.cookie_header.decode('ascii')) | |
| try: | |
| self.cookies.load(self.cookie_header.decode('ascii')) | |
| except http.cookies.CookieError: | |
| # Malformed cookie header; ignore cookies | |
| self.cookies = http.cookies.SimpleCookie() | |
| except Exception: | |
| # Catch-all for any other unexpected errors | |
| self.cookies = http.cookies.SimpleCookie() |
| request.load_cookies() | ||
| try: | ||
| request.load_cookies() | ||
| except Exception as ex: |
There was a problem hiding this comment.
Catching a broad Exception is too generic. The standard library's SimpleCookie.load() can raise specific exceptions like http.cookies.CookieError. Consider catching specific cookie-related exceptions instead of all exceptions to avoid masking unexpected errors.
| except Exception as ex: | |
| except http.cookies.CookieError as ex: |
edb/server/protocol/auth_ext/http.py
Outdated
| raise errors.InvalidData( | ||
| f'invalid cookies header: {ex}, ' | ||
| f'try clearing cookies and trying again' |
There was a problem hiding this comment.
Including the raw exception message in the error response could expose sensitive information or implementation details to clients. Consider using a generic error message instead, while logging the detailed exception for debugging purposes.
| raise errors.InvalidData( | |
| f'invalid cookies header: {ex}, ' | |
| f'try clearing cookies and trying again' | |
| logger.exception("Failed to load cookies header") | |
| raise errors.InvalidData( | |
| 'Invalid cookies header. Try clearing cookies and trying again.' |
There was a problem hiding this comment.
This copilot review makes sense, though I think we should log the contents of the cookie header in the logfile.
|
On localhost I think everything (like the gel server, the user's app, etc.) all share the same cookies, so this would also wipe any cookies the user app has set as well right? Is that the intention? |
I think cookies are bound to the host + post, right? |
Avoids leaking exception information back to the client
No, just the host: https://datatracker.ietf.org/doc/html/rfc6265#section-8.5 |
[...]
Ahh, thanks for the chapter and verse on that! 🙏 Given that, I'm fine with not doing this. I guess an alternative to this would be to detect which cookie failed to parse and send a new cookie header to "delete" that cookie, but I think now we're getting far afield from the original intention here. Should I just close this and maybe we stick with the behavior in the base branch? @mmastrac since you made this suggestion, wdyt about the possibility of accidentally having a larger blast radius than intended? |
Instead of silently swallowing invalid cookie headers, we return an error and send
clear-site-data: "cookies"in the response header. This reverts the custom, tolerant cookie parser we added in the base branch back to the built-in one.Some additional notes:
httptools.parseritself to fail (like including non-ASCII characters) do not get caught here.clear-site-dataheader set instead of erroring?