From 02076ebe9351fa3b8bb1669237265c3888a5ef17 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 20 Oct 2026 23:21:15 +0200 Subject: [PATCH] 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 | 22 +++++++-- modules/brother/cmd/brother/admin_test.go | 59 ++++++++++++++++++++--- modules/brother/cmd/brother/deviceproc.go | 6 ++- modules/brother/cmd/brother/pin_test.go | 4 +- 4 files changed, 76 insertions(+), 15 deletions(-) diff --git a/modules/brother/cmd/brother/admin.go b/modules/brother/cmd/brother/admin.go index 35c148ac..79114abe 100644 --- a/modules/brother/cmd/brother/admin.go +++ b/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, ¬FoundError{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 } diff --git a/modules/brother/cmd/brother/admin_test.go b/modules/brother/cmd/brother/admin_test.go index 80e52296..bd786582 100644 --- a/modules/brother/cmd/brother/admin_test.go +++ b/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 `
` + } + 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) + } +} diff --git a/modules/brother/cmd/brother/deviceproc.go b/modules/brother/cmd/brother/deviceproc.go index 826e670b..c615ca30 100644 --- a/modules/brother/cmd/brother/deviceproc.go +++ b/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) diff --git a/modules/brother/cmd/brother/pin_test.go b/modules/brother/cmd/brother/pin_test.go index 0fc85e2b..5abe21b7 100644 --- a/modules/brother/cmd/brother/pin_test.go +++ b/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") }