Move cookiejar change to the right place

InitHTTP overrides the default client created in the starlarkhttp module. And InitHTTP is called from the render and other commands. So, original commit would have had no effect, since that client isn't used in practice. This is where we need to add the cookie jar

runtime/httpcache.go

@@ -9,12 +9,14 @@ import (
 	"fmt"
 	"math/rand"
 	"net/http"
+	"net/http/cookiejar"
 	"net/http/httputil"
 	"strconv"
 	"strings"
 	"time"
 
 	"github.com/pkg/errors"
+	"golang.org/x/net/publicsuffix"
 	"tidbyt.dev/pixlet/runtime/modules/starlarkhttp"
 )
 
@@ -55,7 +57,16 @@ func InitHTTP(cache Cache) {
 		transport: http.DefaultTransport,
 	}
 
+	// 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}) // never returns non-nil err
+
 	httpClient := &http.Client{
+		Jar:       jar,
 		Transport: cc,
 		Timeout:   HTTPTimeout * 2,
 	}

runtime/httpcache_test.go

@@ -4,11 +4,14 @@ import (
 	"fmt"
 	"math/rand"
 	"net/http"
+	"net/http/httptest"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"go.starlark.net/starlark"
 )
 
 func TestInitHTTP(t *testing.T) {
@@ -182,3 +185,44 @@ func TestDetermineTTLNoHeaders(t *testing.T) {
 	ttl := DetermineTTL(req, res)
 	assert.Equal(t, MinRequestTTL, ttl)
 }
+
+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")  // Occurs if client has no cookie jar
+			}
+			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)
+	c := NewInMemoryCache()
+	InitHTTP(c)
+
+	b, err := os.ReadFile("testdata/httpredirect.star")
+	assert.NoError(t, err)
+
+	app := &Applet{}
+	err = app.Load("httpredirect", "httpredirect.star", b, nil)
+	assert.NoError(t, err)
+
+	_, err = app.Run(map[string]string{})
+	assert.NoError(t, err)
+}

runtime/modules/starlarkhttp/starlarkhttp.go

@@ -32,15 +32,13 @@ 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
@@ -53,18 +51,9 @@ 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.Client{
-		Jar: jar,
-	}
+	StarlarkHTTPClient = http.DefaultClient
 	// 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,41 +140,3 @@ 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/testdata/httpredirect.star

@@ -0,0 +1,10 @@
+load("http.star", "http")
+load("assert.star", "assert")
+load("render.star", "render")
+
+def main():
+    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"})
+    return render.Root(child=render.Text("pass"))