Refuse a redirect off https or off the printer's host and a re-sent login body in the admin client (issue 1515)

modules/brother/cmd/brother/admin.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -40,6 +41,9 @@ func NewAdmin(host, fingerprint, password string) (*Admin, error) {
 		HTTP: &http.Client{
 			Timeout: 30 * time.Second,
 			Jar:     jar,
+			CheckRedirect: func(req *http.Request, via []*http.Request) error {
+				return checkRedirect(host, req, via)
+			},
 			Transport: &http.Transport{
 				TLSClientConfig:   pinnedTLS(fingerprint),
 				DisableKeepAlives: true,
@@ -48,6 +52,21 @@ func NewAdmin(host, fingerprint, password string) (*Admin, error) {
 	}, nil
 }
 
+// checkRedirect follows a redirect only on https on the printer's own host, and never with a body: a 307 or 308
+// answer to the login post would make the client send the post again, the password with it (issue 1515).
+func checkRedirect(host string, req *http.Request, via []*http.Request) error {
+	if len(via) >= 10 {
+		return errors.New("the printer redirected more than ten times")
+	}
+	if req.URL.Scheme != "https" || req.URL.Host != host {
+		return fmt.Errorf("the printer redirected to %s, not to https on the printer %s: not followed", req.URL.Scheme+"://"+req.URL.Host, host)
+	}
+	if req.Method != http.MethodGet && req.Method != http.MethodHead {
+		return fmt.Errorf("the printer redirected a %s with its body (a 307 or 308): not followed, so the password is not sent again", via[len(via)-1].Method)
+	}
+	return nil
+}
+
 func (a *Admin) url(path string) string { return "https://" + a.Host + path }
 
 // fetch is one request; the body is read whole, up to maxPage. The final URL is the one after redirects.

modules/brother/cmd/brother/admin_test.go

@@ -300,3 +300,52 @@ func TestThePasswordIsNotPostedOverPlainHTTP(t *testing.T) {
 		t.Errorf("%d posts reached the server", posted)
 	}
 }
+
+// 1515: a 307 or 308 after the login post would make the HTTP client send the post again, the password with
+// it, to the place the answer names; a redirect off https or off the printer's host is refused as well.
+func TestNoRedirectResendsThePasswordOrLeavesHTTPSOnThePrinter(t *testing.T) {
+	const loginPage = `<html><body><form action="/login" method="post"><input type="password" name="pw"><input type="hidden" name="loginurl" value="/x"></form></body></html>`
+	cases := []struct {
+		name     string
+		status   int
+		location string // "SAME" is a path on the printer itself
+	}{
+		{"307 on the printer", 307, "/again"},
+		{"308 on the printer", 308, "/again"},
+		{"307 to plain http", 307, "http://192.0.2.1/steal"},
+		{"302 to another host", 302, "https://192.0.2.1/steal"},
+		{"302 to plain http", 302, "http://192.0.2.1/steal"},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			var mu sync.Mutex
+			var posts []string
+			srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				mu.Lock()
+				defer mu.Unlock()
+				switch {
+				case r.Method == http.MethodGet && r.URL.Path == "/":
+					w.Write([]byte(loginPage))
+				case r.Method == http.MethodPost:
+					r.ParseForm()
+					posts = append(posts, r.URL.Path+" "+r.PostForm.Get("pw"))
+					http.Redirect(w, r, c.location, c.status)
+				default:
+					w.Write([]byte("ok"))
+				}
+			}))
+			srv.StartTLS()
+			defer srv.Close()
+			a, _ := NewAdmin(strings.TrimPrefix(srv.URL, "https://"), Fingerprint(srv.TLS.Certificates[0].Certificate[0]), "hunter2")
+			err := a.Login(context.Background())
+			if err == nil {
+				t.Fatalf("the login went on after a %d to %s", c.status, c.location)
+			}
+			mu.Lock()
+			defer mu.Unlock()
+			if len(posts) != 1 {
+				t.Errorf("the password was posted %d times: %v", len(posts), posts)
+			}
+		})
+	}
+}