Make sessions work

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.

go.mod

@@ -40,6 +40,7 @@ require (
 	github.com/zachomedia/go-bdf v0.0.0-20220611021443-a3af701111be
 	go.starlark.net v0.0.0-20231121155337-90ade8b19d09
 	golang.org/x/image v0.14.0
+	golang.org/x/net v0.19.0
 	golang.org/x/oauth2 v0.15.0
 	golang.org/x/sync v0.5.0
 	golang.org/x/text v0.14.0
@@ -101,7 +102,6 @@ require (
 	golang.org/x/crypto v0.16.0 // indirect
 	golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect
 	golang.org/x/mod v0.12.0 // indirect
-	golang.org/x/net v0.19.0 // indirect
 	golang.org/x/sys v0.15.0 // indirect
 	golang.org/x/tools v0.13.0 // indirect
 	google.golang.org/appengine v1.6.7 // indirect

runtime/modules/starlarkhttp/starlarkhttp.go

@@ -32,13 +32,15 @@ import (
 	"io"
 	"mime/multipart"
 	"net/http"
+	"net/http/cookiejar"
 	"net/url"
 	"strconv"
 	"strings"
 
 	util "github.com/qri-io/starlib/util"
 	"go.starlark.net/starlark"
 	"go.starlark.net/starlarkstruct"
+	"golang.org/x/net/publicsuffix"
 )
 
 // AsString unquotes a starlark string value
@@ -51,9 +53,18 @@ func AsString(x starlark.Value) (string, error) {
 const ModuleName = "http.star"
 
 var (
+	// Providing a cookie jar allows sessions and redirects to work properly. With a
+	// jar present, any cookies set in a response will automatically be added to
+	// subsequent requests. This means that we can follow redirects after logging into
+	// a session. Without a jar, any cookies will be dropped from redirects unless explicitly
+	// set in the original outgoing request.
+	// https://cs.opensource.google/go/go/+/master:src/net/http/client.go;drc=4c394b5638cc2694b1eff6418bc3e7db8132de0e;l=88
+	jar, _ = cookiejar.New(&cookiejar.Options{PublicSuffixList: publicsuffix.List})
 	// StarlarkHTTPClient is the http client used to create the http module. override with
 	// a custom client before calling LoadModule
-	StarlarkHTTPClient = http.DefaultClient
+	StarlarkHTTPClient = &http.Client{
+		Jar: jar,
+	}
 	// StarlarkHTTPGuard is a global RequestGuard used in LoadModule. override with a custom
 	// implementation before calling LoadModule
 	StarlarkHTTPGuard RequestGuard

runtime/modules/starlarkhttp/starlarkhttp_test.go

@@ -140,3 +140,41 @@ func TestSetBody(t *testing.T) {
 		}
 	}
 }
+
+func TestSetCookieOnRedirect(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// Requests to "/login" set a cookie and redirect to /destination
+		if strings.HasSuffix(r.URL.Path, "/login") {
+			w.Header().Set("Set-Cookie", "doodad=foobar; path=/; HttpOnly")
+			w.Header().Set("Location", "/destination")
+			w.WriteHeader(302)
+			return
+		}
+		// Requests to /destination must have cookie set
+		if strings.HasSuffix(r.URL.Path, "/destination") {
+			c, err := r.Cookie("doodad")
+			if err != nil {
+				t.Errorf("Expected cookie `doodad` not present")
+			}
+			if c.Value != "foobar" {
+				t.Errorf("Cookie `doodad` value mismatch. Expected foobar, got %s", c.Value)
+			}
+			if _, err := w.Write([]byte(`{"hello":"world"}`)); err != nil {
+				t.Fatal(err)
+			}
+			return
+		}
+		t.Errorf("Unexpected path requested: %s", r.URL.Path)
+	}))
+	starlark.Universe["test_server_url"] = starlark.String(ts.URL)
+
+	thread := &starlark.Thread{Name: "unittests/setcookieonredirect", Load: testdata.NewLoader(starlarkhttp.LoadModule, starlarkhttp.ModuleName)}
+	starlarktest.SetReporter(thread, t)
+
+	// Execute test file
+	_, err := starlark.ExecFile(thread, "testdata/test_redirect.star", nil, nil)
+	if err != nil {
+		t.Error(err)
+	}
+
+}

runtime/modules/starlarkhttp/testdata/test_redirect.star

@@ -0,0 +1,7 @@
+load("http.star", "http")
+load("assert.star", "assert")
+
+res_1 = http.post(test_server_url + "/login")
+assert.eq(res_1.status_code, 200)
+assert.eq(res_1.body(), '{"hello":"world"}')
+assert.eq(res_1.json(), {"hello": "world"})