Make HTTP sessions work #1008
Make HTTP sessions work #1008
Conversation
❌ Deploy Preview for pixlet failed.
|
dinosaursrarr
force-pushed
the
cookiejar
branch
from
8681f54
to
afd81ae
Compare
|
Don't think the netlify failures are specific to this change, are they? Looks like sw.js (service worker?) not found during npm run build:wasm. That sounds like an infra problem / command that needs to be updated. |
|
Would completely understand if you're wary about making any change that could affect behaviour for all apps. An alternative could be to add a new method to the http starlark module, so clients can specify the behaviour they want in their app. Something like app calls WDYT? |
|
@dinosaursrarr Sorry, for some reason we were not getting notified of PR's in this repo. Will take a look and follow up now. |
|
I think this makes sense. The Netlify errors are unrelated. I'm going to look into whether we can or need to limit the size of this cookie jar though. |
|
Not sure you need to limit it, if the module and client are instantiated anew each time a script is executed. |
|
Ah that's a good point, but for exactly the opposite reason. We reuse the same HTTP client for multiple app executions (since there's currently no shared state). So this change won't work as is, since cookies would leak between apps. Instead we need to figure out some way to scope the HTTP client to each |
|
Ah yes, the Seems like one easy way to make the change is lazy loading the http client.
Creating a client each time should be pretty lightweight and this seems like a minimal amount of plumbing changes. Wasn't too bad to do. Have just added another commit to this PR with that change. The updated test failed when using a singleton http client, but passes when using the factory. |
dinosaursrarr
force-pushed
the
cookiejar
branch
8 times, most recently
from
29735a4
to
5bb73b5
Compare
User simonahac reported a thorny problem on discord yesterday. See discussion beginning at https://discord.com/channels/928484660785336380/928485908842426389/1210584191142723615 They want to use a service that relies on sessions. First you POST to a login URL. That sets a cookie and redirects to a resource. The resource only works if you provide the cookie value in the GET request. This works out of the box in python, Postman, and most other clients. But not pixlet. Looks like the underlying cause is the default behaviour of golang's http.Client. By default, it will ignore any cookies set in responses when making the redirect. When sending the redirected request, it would just copy over headers and cookies from the original outgoing response -- which won't have the cookie set. If the client has a CookieJar, however, it just works as expected. Cookies from responses will be added to the jar, and all the cookies from the jar will automatically be added to all outgoing requests from that point. https://cs.opensource.google/go/go/+/master:src/net/http/client.go;drc=b899e0b8cc1afc4534758c9ebe1e051e5220bfbd;l=88 I think this should be a safe change. There's only one existing community app (avatarsinpixels) that manually accesses the "Set-Cookie" header on a response. I've checked it works the same with and without this change.
This ensures that cookies from one app aren't leaked to other apps. Instead of InitHTTP setting a single global instance of http.Client, it now sets a factory method. Each time the http module is loaded, that factory is used to create a new http client. Those clients all share the same cache, but not cookie jars
dinosaursrarr
force-pushed
the
cookiejar
branch
from
c44547e
to
ec22f51
Compare
|
Ugh, I hate git. But this is rebased and resolved now. |
|
Just a heads up, this hasn't fallen off my radar but it's taking a bit longer due to testing how it works with our production backend. |
User simonahac reported a thorny problem on discord yesterday. See discussion beginning at 1:49PM yesterday.
They want to use a service that relies on sessions. First you POST to a login URL. The response sets a cookie and redirects to a resource. The resource only works if you provide the cookie value in the GET request.
This works out of the box in python, Postman, and most other clients. But not pixlet. Looks like the underlying cause is the default behaviour of golang's http.Client.
By default, go's client will ignore any cookies set in responses when making the redirect. When sending the redirected request, it would just copy over headers and cookies from the original outgoing response -- which won't have had the cookie set yet. This is the same for your httpcache client too.
If the client has a CookieJar, however, things work as expected. Cookies from responses will be added to the jar, and all the cookies from the jar will automatically be added to all outgoing requests from that point onwards, including on redirections.
https://cs.opensource.google/go/go/+/master:src/net/http/client.go;drc=b899e0b8cc1afc4534758c9ebe1e051e5220bfbd;l=88
I think this should be a safe change. Modules are instantiated separately each time we run a starlark script, so cookies from different runs won't be in the same cookie jar. There's only one existing community app (avatarsinpixels) that manually accesses the "Set-Cookie" header on a response. I've checked it works the same with and without this change.
Should also be OK with your cache -- since the cache key is the entire serialised request, different users will provide different login credentials and different cookies in their requests.