Log in to the default page and ask for the page after, so a missing page leaves no session; send the password over https only (issue 1514 review)

modules/brother/cmd/brother/admin.go

@@ -78,6 +78,9 @@ func (a *Admin) fetch(ctx context.Context, method, rawURL string, form url.Value
 		return nil, nil, fmt.Errorf("the page %s is larger than %d bytes", rawURL, maxPage)
 	}
 	if resp.StatusCode >= 400 {
+		if resp.StatusCode == http.StatusNotFound {
+			return nil, nil, &notFoundError{Path: resp.Request.URL.Path}
+		}
 		return nil, nil, fmt.Errorf("the printer answered %s for %s", resp.Status, resp.Request.URL.Path)
 	}
 	return buf, resp.Request.URL, nil
@@ -105,9 +108,9 @@ func hasField(f *Form, name string) bool {
 }
 
 // Login asks for the printer's login page, reads its login form, and posts the form's own fields with the
-// password in the field the form names and the form's loginurl set to the page that will be asked for. A login
+// password in the field the form names and the form's loginurl set to the default page, which every firmware has; the page asked for is a request of its own afterwards. A login
 // is failed when the answer is a login page again.
-func (a *Admin) Login(ctx context.Context, page string) error {
+func (a *Admin) Login(ctx context.Context) error {
 	body, at, err := a.fetch(ctx, http.MethodGet, a.url(loginStart), nil)
 	if err != nil {
 		return fmt.Errorf("the printer's admin page cannot be asked for its login: %w", err)
@@ -120,8 +123,8 @@ func (a *Admin) Login(ctx context.Context, page string) error {
 	if err != nil {
 		return fmt.Errorf("the login form's action %q is not a URL: %w", form.Action, err)
 	}
-	if target.Host != a.Host {
-		return fmt.Errorf("the login form posts to %s, not to the printer %s: the password is not sent", target.Host, a.Host)
+	if target.Scheme != "https" || target.Host != a.Host {
+		return fmt.Errorf("the login form posts to %s, not to https on the printer %s: the password is not sent", target.Scheme+"://"+target.Host, a.Host)
 	}
 	vals := url.Values{}
 	for _, f := range form.Fields {
@@ -130,11 +133,15 @@ func (a *Admin) Login(ctx context.Context, page string) error {
 		}
 	}
 	if _, ok := vals["loginurl"]; ok {
-		vals.Set("loginurl", page)
+		vals.Set("loginurl", DefaultPage)
 	}
 	vals.Set(pw.Name, a.Password)
 	answer, _, err := a.fetch(ctx, http.MethodPost, target.String(), vals)
 	if err != nil {
+		// The post may have opened a session before its answer failed: end it if there is one.
+		if home, hat, herr := a.Get(context.Background(), DefaultPage); herr == nil {
+			a.Logout(context.Background(), home, hat)
+		}
 		return fmt.Errorf("the login was not answered: %w", err)
 	}
 	// A login page again is a form with a password field and the loginurl field; a page that was asked for may
@@ -198,3 +205,8 @@ func (a *Admin) Logout(ctx context.Context, page []byte, at *url.URL) string {
 	}
 	return "no logout link or form on the page: the session is left to expire on the printer"
 }
+
+// notFoundError is the printer's 404 for a page.
+type notFoundError struct{ Path string }
+
+func (e *notFoundError) Error() string { return "the printer answered 404 Not Found for " + e.Path }

modules/brother/cmd/brother/admin_test.go

@@ -151,7 +151,7 @@ func TestACertificateOtherThanThePinnedOneIsRefusedBeforeAnythingIsSent(t *testi
 	if err != nil {
 		t.Fatal(err)
 	}
-	err = a.Login(context.Background(), DefaultPage)
+	err = a.Login(context.Background())
 	if err == nil {
 		t.Fatal("a certificate that is not the pinned one was accepted")
 	}
@@ -169,7 +169,7 @@ func TestACertificateOtherThanThePinnedOneIsRefusedBeforeAnythingIsSent(t *testi
 func TestThePinnedCertificateIsAccepted(t *testing.T) {
 	f := newFakePrinter(t, "B1891", "hunter2")
 	a, _ := NewAdmin(f.host(), strings.ToLower(f.print()), "hunter2")
-	if err := a.Login(context.Background(), DefaultPage); err != nil {
+	if err := a.Login(context.Background()); err != nil {
 		t.Fatal(err)
 	}
 }
@@ -181,7 +181,7 @@ func TestLoginUsesThePasswordFieldNameTheFormGives(t *testing.T) {
 		f := newFakePrinter(t, name, "s3cret pass")
 		a, _ := NewAdmin(f.host(), f.print(), "s3cret pass")
 		ctx := context.Background()
-		if err := a.Login(ctx, "/general/other.html"); err != nil {
+		if err := a.Login(ctx); err != nil {
 			t.Fatalf("field %s: %v", name, err)
 		}
 		if len(f.posted) != 1 {
@@ -190,7 +190,7 @@ func TestLoginUsesThePasswordFieldNameTheFormGives(t *testing.T) {
 		if got := f.posted[0].Get(name); got != "s3cret pass" {
 			t.Errorf("field %s: the password field holds %q", name, got)
 		}
-		if f.posted[0].Get("loginurl") != "/general/other.html" {
+		if f.posted[0].Get("loginurl") != DefaultPage {
 			t.Errorf("field %s: the form's hidden loginurl was %q", name, f.posted[0].Get("loginurl"))
 		}
 		if len(f.posted[0]) != 2 {
@@ -207,7 +207,7 @@ func TestLoginUsesThePasswordFieldNameTheFormGives(t *testing.T) {
 func TestAWrongPasswordIsAFailedLogin(t *testing.T) {
 	f := newFakePrinter(t, "B1891", "right")
 	a, _ := NewAdmin(f.host(), f.print(), "wrong")
-	err := a.Login(context.Background(), DefaultPage)
+	err := a.Login(context.Background())
 	if err == nil || !strings.Contains(err.Error(), "login failed") {
 		t.Fatalf("error = %v, want a failed login", err)
 	}
@@ -221,7 +221,7 @@ func TestLogoutEndsTheSessionAndAPageWithoutOneSaysSo(t *testing.T) {
 	f := newFakePrinter(t, "B1891", "pw")
 	a, _ := NewAdmin(f.host(), f.print(), "pw")
 	ctx := context.Background()
-	if err := a.Login(ctx, DefaultPage); err != nil {
+	if err := a.Login(ctx); err != nil {
 		t.Fatal(err)
 	}
 	body, at, err := a.Get(ctx, DefaultPage)
@@ -250,8 +250,53 @@ func TestThePasswordIsNotPostedToAnotherHost(t *testing.T) {
 	defer srv.Close()
 	host := strings.TrimPrefix(srv.URL, "https://")
 	a, _ := NewAdmin(host, Fingerprint(srv.TLS.Certificates[0].Certificate[0]), "pw")
-	err := a.Login(context.Background(), DefaultPage)
+	err := a.Login(context.Background())
 	if err == nil || !strings.Contains(err.Error(), "not sent") {
 		t.Fatalf("error = %v", err)
 	}
 }
+
+// 1514 review: a page the printer does not have leaves no session open, and the error says the page is missing,
+// not that the login failed.
+func TestAMissingPageLeavesNoSessionOpenAndSaysSo(t *testing.T) {
+	f := newFakePrinter(t, "B1891", "pw")
+	_, run := printerTools(t, f, "pw")
+	_, err := run(map[string]any{"page": "/general/nope.html"})
+	if err == nil || !strings.Contains(err.Error(), "the printer has no page /general/nope.html") {
+		t.Fatalf("error = %v, want the printer has no page", err)
+	}
+	if strings.Contains(err.Error(), "login was not answered") {
+		t.Errorf("a missing page is said to be a failed login: %v", err)
+	}
+	if f.loggedInNow() != 0 {
+		t.Errorf("%d sessions are still open on the printer", f.loggedInNow())
+	}
+}
+
+// 1514 review: the password is posted to https on the printer's host only. A form whose action is plain http
+// on the same host, or a root that redirects to plain http, is refused and nothing is posted.
+func TestThePasswordIsNotPostedOverPlainHTTP(t *testing.T) {
+	var posted int
+	mux := http.NewServeMux()
+	page := func(action string) string {
+		return `<html><body><form method="post" action="` + action + `"><input type="password" name="B1891"/><input type="hidden" name="loginurl" value="/home/status.html"/></form></body></html>`
+	}
+	var host string
+	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodPost {
+			posted++
+		}
+		w.Write([]byte(page("http://" + host + "/home/status.html")))
+	})
+	srv := httptest.NewTLSServer(mux)
+	defer srv.Close()
+	host = strings.TrimPrefix(srv.URL, "https://")
+	a, _ := NewAdmin(host, Fingerprint(srv.TLS.Certificates[0].Certificate[0]), "pw")
+	err := a.Login(context.Background())
+	if err == nil || !strings.Contains(err.Error(), "password is not sent") {
+		t.Errorf("error = %v, want the plain http action refused", err)
+	}
+	if posted != 0 {
+		t.Errorf("%d posts reached the server", posted)
+	}
+}

modules/brother/cmd/brother/deviceproc.go

@@ -323,7 +323,7 @@ func (w *deviceWorker) read(ctx context.Context, page string) (map[string]any, e
 	if err != nil {
 		return nil, err
 	}
-	if err := a.Login(ctx, page); err != nil {
+	if err := a.Login(ctx); err != nil {
 		return nil, err
 	}
 	// Log out with a context of its own: the ask's may be what ended this.
@@ -334,6 +334,10 @@ func (w *deviceWorker) read(ctx context.Context, page string) (map[string]any, e
 		if home, hat, herr := a.Get(lctx, DefaultPage); herr == nil {
 			a.Logout(lctx, home, hat)
 		}
+		var nf *notFoundError
+		if errors.As(err, &nf) {
+			return nil, fmt.Errorf("the printer has no page %s", page)
+		}
 		return nil, fmt.Errorf("page %s cannot be read: %w", page, err)
 	}
 	logout := a.Logout(lctx, body, at)

modules/brother/cmd/brother/pin_test.go

@@ -50,7 +50,7 @@ func TestAPinListingTwoFingerprintsAcceptsTheCertificateTheServerPresents(t *tes
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err := a.Login(context.Background(), DefaultPage); err != nil {
+	if err := a.Login(context.Background()); err != nil {
 		t.Fatalf("a pin listing the served certificate was refused: %v", err)
 	}
 }
@@ -59,7 +59,7 @@ func TestAPinListingTwoFingerprintsAcceptsTheCertificateTheServerPresents(t *tes
 func TestAPinListingOnlyOtherCertificatesStillRefuses(t *testing.T) {
 	f := newFakePrinter(t, "B1891", "hunter2")
 	a, _ := NewAdmin(f.host(), printerECDSAPrint+" , "+printerRSAPrint, "hunter2")
-	err := a.Login(context.Background(), DefaultPage)
+	err := a.Login(context.Background())
 	if err == nil {
 		t.Fatal("a certificate that is in no pinned fingerprint was accepted")
 	}