Defer loading cookies until needed in auth ext + return better error message #8869
Conversation
scotttrinh
left a comment
scotttrinh
left a comment
There was a problem hiding this comment.
I don't think we need to address my question right this second, since it'll be hard to even test for this until we see what the exception actually says to know what code path we're hitting. So 
edb/server/protocol/auth_ext/http.py
Outdated
| except http.cookies.CookieError as ex: | ||
| raise errors.InvalidData( | ||
| f'invalid cookies header: {ex}, ' | ||
| f'try clearing cookies and trying again' | ||
| ) |
There was a problem hiding this comment.
Seems better than the status quo, for sure! I wonder if we can do one better and actually ignore and clear out bad cookies based on the error? Conceptually, I always feel like cookies are things owned by the server, not by the client. If course, it's possible that the cookies are just completely mangled to the point where it's impossible to even know which key needs to be cleared.
There was a problem hiding this comment.
I think our error pages should be a bit richer / nicely designed, but also have a JS snippet that can nuke cookies and any other local state (this error page could just do that, right?)
There was a problem hiding this comment.
I always feel like cookies are things owned by the server
Normally, yes. But for local instances we're using localhost, and the users app is likely also using localhost, so we effectively share cookies with their app. It feels like we're just going to cause more weird, hard to debug, problems if we start destroying cookies when the users app has a bug that causes it to set bad cookies.
There was a problem hiding this comment.
Maybe the best approach is to have some custom cookie parsing that just ignores invalid cookies (we can log a warning), as we only need to read the known good cookies we set anyway.
There was a problem hiding this comment.
Ok, I've updated it to ignore invalid cookies: 2f060fd
There was a problem hiding this comment.
The downside of ignoring invalid cookies is that you don't know it's messed up, right? I guess another approach is to lazily load cookies at the point when we need them and to do it selectively rather than populating a whole cookie jar, just ask for the morsels we need? Because then if something is messed up, there is clearly a bug. This isn't a blocker, though: this is still superior to the status quo in every way.
|
Should we merge this? |
|
Can we send back |
scotttrinh
force-pushed
the
defer-cookies-loading
branch
from
2f060fd to
e8d8cf2
Compare
If there are invalid cookies in the request to any of the http endpoints, the server returns a not very helpful
HttpParserCallbackError: User callback errorerror message.As only the auth ext endpoints actually read the cookies, defer parsing the cookie header until the request is being handled by the auth ext, and return a better error message in the case of a
CookieError.