diff --git a/modules/brother/cmd/brother/admin.go b/modules/brother/cmd/brother/admin.go index ce7aed23..35c148ac 100644 --- a/modules/brother/cmd/brother/admin.go +++ b/modules/brother/cmd/brother/admin.go @@ -19,8 +19,10 @@ type Admin struct { HTTP *http.Client } -// loginPath is a page that needs a login: asking for it is answered by the login page. -const loginPath = "/general/status.html" +// loginStart is where a login starts: the root, which the printer answers with a redirect to the page it serves +// its login form on (/home/status.html). Asking for a page behind the login instead is answered by a redirect +// to an error page, and a loginurl naming a page this firmware lacks is a 404 after the login (issue 1514). +const loginStart = "/" // maxPage bounds a page of the admin site. const maxPage = 4 << 20 @@ -93,17 +95,26 @@ func passwordForm(p Page) (*Form, *Field) { return nil, nil } -// Login asks for a page that needs one, reads the login form it is answered with, and posts the form's own -// fields with the password in the field the form names. A login is failed when the answer is a login page -// again. -func (a *Admin) Login(ctx context.Context) error { - body, at, err := a.fetch(ctx, http.MethodGet, a.url(loginPath), nil) +func hasField(f *Form, name string) bool { + for _, fl := range f.Fields { + if fl.Name == name { + return true + } + } + return false +} + +// 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 +// is failed when the answer is a login page again. +func (a *Admin) Login(ctx context.Context, page string) 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) } form, pw := passwordForm(ParsePage(body)) if form == nil { - return fmt.Errorf("the printer's answer to %s holds no login form with a password field", loginPath) + return fmt.Errorf("the printer's answer to %s holds no login form with a password field", at.Path) } target, err := at.Parse(form.Action) if err != nil { @@ -118,12 +129,17 @@ func (a *Admin) Login(ctx context.Context) error { vals.Set(f.Name, f.Value) } } + if _, ok := vals["loginurl"]; ok { + vals.Set("loginurl", page) + } vals.Set(pw.Name, a.Password) answer, _, err := a.fetch(ctx, http.MethodPost, target.String(), vals) if err != nil { return fmt.Errorf("the login was not answered: %w", err) } - if again, _ := passwordForm(ParsePage(answer)); again != nil { + // A login page again is a form with a password field and the loginurl field; a page that was asked for may + // hold a password field of its own. + if again, _ := passwordForm(ParsePage(answer)); again != nil && hasField(again, "loginurl") { return fmt.Errorf("the login failed: the printer showed its login page again, so the secret admin-password is not its password") } return nil diff --git a/modules/brother/cmd/brother/admin_test.go b/modules/brother/cmd/brother/admin_test.go index f51f3465..80e52296 100644 --- a/modules/brother/cmd/brother/admin_test.go +++ b/modules/brother/cmd/brother/admin_test.go @@ -44,8 +44,13 @@ func newFakePrinter(t *testing.T, field, password string) *fakePrinter { t.Helper() f := &fakePrinter{field: field, password: password, loggedIn: map[string]bool{}, pages: map[string]string{}} login := strings.ReplaceAll(adminFixture(t, "login-page.html"), "B1891", field) + // The login page the printer serves at /home/status.html carries that page as its loginurl (captured live + // 2026-10-20, testdata/admin/live/home_status_html.body); the page it serves at /etc/loginerror.html carries + // the page that was asked for. + homeLogin := strings.ReplaceAll(login, `value="/general/status.html"`, `value="/home/status.html"`) synthetic := adminFixture(t, "synthetic-status.html") - f.pages["/general/status.html"] = synthetic + // This firmware has no /general/status.html behind the login (issue 1514): asking for it is a 404. + f.pages["/home/status.html"] = synthetic f.pages["/general/other.html"] = adminFixture(t, "synthetic-settings.html") authed := func(r *http.Request) bool { c, err := r.Cookie("session") @@ -64,7 +69,7 @@ func newFakePrinter(t *testing.T, field, password string) *fakePrinter { w.Write([]byte(synthetic)) return } - w.Write([]byte(login)) + w.Write([]byte(homeLogin)) return } r.ParseForm() @@ -76,7 +81,8 @@ func newFakePrinter(t *testing.T, field, password string) *fakePrinter { f.session = "s" + field f.loggedIn[f.session] = true http.SetCookie(w, &http.Cookie{Name: "session", Value: f.session, Path: "/"}) - w.Write([]byte(synthetic)) + // Like Brother's form, a good login goes on to the page named in loginurl. + http.Redirect(w, r, r.PostForm.Get("loginurl"), http.StatusFound) }) mux.HandleFunc("/home/logout.html", func(w http.ResponseWriter, r *http.Request) { f.mu.Lock() @@ -89,16 +95,21 @@ func newFakePrinter(t *testing.T, field, password string) *fakePrinter { mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { f.mu.Lock() defer f.mu.Unlock() + if r.URL.Path == "/" { + http.Redirect(w, r, "/home/status.html", http.StatusMovedPermanently) + return + } f.requests = append(f.requests, r.Method+" "+r.URL.Path) + // Captured: a page behind the login is answered by a redirect to the login page whether it exists or not. + if !authed(r) { + http.Redirect(w, r, "/etc/loginerror.html?url="+url.QueryEscape(r.URL.Path), http.StatusFound) + return + } body, ok := f.pages[r.URL.Path] if !ok { http.NotFound(w, r) return } - if !authed(r) { - http.Redirect(w, r, "/etc/loginerror.html?url="+url.QueryEscape(r.URL.Path), http.StatusFound) - return - } w.Write([]byte(body)) }) f.Server = httptest.NewUnstartedServer(mux) @@ -140,7 +151,7 @@ func TestACertificateOtherThanThePinnedOneIsRefusedBeforeAnythingIsSent(t *testi if err != nil { t.Fatal(err) } - err = a.Login(context.Background()) + err = a.Login(context.Background(), DefaultPage) if err == nil { t.Fatal("a certificate that is not the pinned one was accepted") } @@ -158,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()); err != nil { + if err := a.Login(context.Background(), DefaultPage); err != nil { t.Fatal(err) } } @@ -170,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); err != nil { + if err := a.Login(ctx, "/general/other.html"); err != nil { t.Fatalf("field %s: %v", name, err) } if len(f.posted) != 1 { @@ -179,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/status.html" { + if f.posted[0].Get("loginurl") != "/general/other.html" { t.Errorf("field %s: the form's hidden loginurl was %q", name, f.posted[0].Get("loginurl")) } if len(f.posted[0]) != 2 { @@ -196,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()) + err := a.Login(context.Background(), DefaultPage) if err == nil || !strings.Contains(err.Error(), "login failed") { t.Fatalf("error = %v, want a failed login", err) } @@ -210,10 +221,10 @@ 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); err != nil { + if err := a.Login(ctx, DefaultPage); err != nil { t.Fatal(err) } - body, at, err := a.Get(ctx, "/general/status.html") + body, at, err := a.Get(ctx, DefaultPage) if err != nil { t.Fatal(err) } @@ -239,7 +250,7 @@ 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()) + err := a.Login(context.Background(), DefaultPage) if err == nil || !strings.Contains(err.Error(), "not sent") { t.Fatalf("error = %v", err) } diff --git a/modules/brother/cmd/brother/deviceproc.go b/modules/brother/cmd/brother/deviceproc.go index bbd1702d..826e670b 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); err != nil { + if err := a.Login(ctx, page); err != nil { return nil, err } // Log out with a context of its own: the ask's may be what ended this. diff --git a/modules/brother/cmd/brother/main.go b/modules/brother/cmd/brother/main.go index 41a75d78..f795bc47 100644 --- a/modules/brother/cmd/brother/main.go +++ b/modules/brother/cmd/brother/main.go @@ -165,7 +165,7 @@ func tools(settingsFile string, now func() time.Time) []stdio.Tool { Input: map[string]any{ "type": "object", "properties": map[string]any{ - "page": map[string]any{"type": "string", "description": "a page of the admin site: a path starting with / and ending in .html (default /general/status.html)"}, + "page": map[string]any{"type": "string", "description": "a page of the admin site: a path starting with / and ending in .html (default /home/status.html)"}, }, }, Run: func(args map[string]any) (any, error) { diff --git a/modules/brother/cmd/brother/pin_test.go b/modules/brother/cmd/brother/pin_test.go index 5abe21b7..0fc85e2b 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()); err != nil { + if err := a.Login(context.Background(), DefaultPage); 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()) + err := a.Login(context.Background(), DefaultPage) if err == nil { t.Fatal("a certificate that is in no pinned fingerprint was accepted") } diff --git a/modules/brother/cmd/brother/printer.go b/modules/brother/cmd/brother/printer.go index a755b937..546f67f2 100644 --- a/modules/brother/cmd/brother/printer.go +++ b/modules/brother/cmd/brother/printer.go @@ -20,7 +20,7 @@ import ( var pagePattern = regexp.MustCompile(`^/[A-Za-z0-9_./-]*\.html$`) // DefaultPage is the page printer.settings reads when none is asked for. -const DefaultPage = "/general/status.html" +const DefaultPage = "/home/status.html" // The asks the verbs put to the device process (ADR 0468 clause 1; the shape of ADR 0436's mail-sends): the tools // never hold a credential, so each verb queues an ask on the work-queue stream device-asks, and the device process, diff --git a/modules/brother/cmd/brother/printer_test.go b/modules/brother/cmd/brother/printer_test.go index fc73230d..2ef711a3 100644 --- a/modules/brother/cmd/brother/printer_test.go +++ b/modules/brother/cmd/brother/printer_test.go @@ -211,3 +211,31 @@ func TestTheAdminWorkerSettlesEachAsk(t *testing.T) { t.Errorf("an answer that cannot be written: action %v delay %v, want retry", act, delay) } } + +// 1514: printer.settings with no page, and with /home/status.html, logs in from the page the printer serves its +// login form on (the root redirects to it) and reads that page. The printer has no /general/status.html behind +// the login: the fake, like the captured printer, answers it a 404 once logged in, and a redirect to the login +// page before. +func TestPrinterSettingsDefaultPageAndHomeStatusAreReadAfterTheLogin(t *testing.T) { + for name, args := range map[string]map[string]any{"default": {}, "explicit": {"page": "/home/status.html"}} { + f := newFakePrinter(t, "B1891", "pw") + _, run := printerTools(t, f, "pw") + got, err := run(args) + if err != nil { + t.Fatalf("%s: %v", name, err) + } + raw, _ := json.Marshal(got) + if !strings.Contains(string(raw), "Synthetic status") || !strings.Contains(string(raw), "/home/status.html") { + t.Errorf("%s: answer %.200s", name, raw) + } + if len(f.posted) != 1 || f.posted[0].Get("loginurl") != "/home/status.html" { + t.Errorf("%s: posted %v, want loginurl /home/status.html", name, f.posted) + } + if f.loggedInNow() != 0 { + t.Errorf("%s: the session is still open", name) + } + } + if DefaultPage != "/home/status.html" { + t.Errorf("DefaultPage = %s", DefaultPage) + } +} diff --git a/modules/brother/cmd/brother/testdata/admin/live/.body b/modules/brother/cmd/brother/testdata/admin/live/.body new file mode 100644 index 00000000..e69de29b diff --git a/modules/brother/cmd/brother/testdata/admin/live/.headers b/modules/brother/cmd/brother/testdata/admin/live/.headers new file mode 100644 index 00000000..eaa267f6 --- /dev/null +++ b/modules/brother/cmd/brother/testdata/admin/live/.headers @@ -0,0 +1,8 @@ +HTTP/1.1 301 Moved Permanently +Cache-Control: no-store +X-Frame-Options: DENY +Content-Length: 0 +Connection: close +Pragma: no-cache +Location: /home/status.html + diff --git a/modules/brother/cmd/brother/testdata/admin/live/etc_loginerror_html_url__2Fgeneral_2Fstatus_2Ehtml.body b/modules/brother/cmd/brother/testdata/admin/live/etc_loginerror_html_url__2Fgeneral_2Fstatus_2Ehtml.body new file mode 100644 index 00000000..9ac63bd5 --- /dev/null +++ b/modules/brother/cmd/brother/testdata/admin/live/etc_loginerror_html_url__2Fgeneral_2Fstatus_2Ehtml.body @@ -0,0 +1,3 @@ + + +Brother MFC-L8390CDW series
MFC-L8390CDW series

Please Login

To Access this page you are required to Login, please Login.

Copyright(C) 2000-2023 Brother Industries, Ltd. All Rights Reserved.
\ No newline at end of file diff --git a/modules/brother/cmd/brother/testdata/admin/live/etc_loginerror_html_url__2Fgeneral_2Fstatus_2Ehtml.headers b/modules/brother/cmd/brother/testdata/admin/live/etc_loginerror_html_url__2Fgeneral_2Fstatus_2Ehtml.headers new file mode 100644 index 00000000..d68f9a23 --- /dev/null +++ b/modules/brother/cmd/brother/testdata/admin/live/etc_loginerror_html_url__2Fgeneral_2Fstatus_2Ehtml.headers @@ -0,0 +1,11 @@ +HTTP/1.1 200 OK +Cache-Control: no-store +X-Content-Type-Options: nosniff +X-Frame-Options: DENY +Content-Length: 2974 +Content-Type: text/html +Content-Language: en-gb +Connection: close +X-XSS-Protection: 1; mode=block +Pragma: no-cache + diff --git a/modules/brother/cmd/brother/testdata/admin/live/general_status_html.body b/modules/brother/cmd/brother/testdata/admin/live/general_status_html.body new file mode 100644 index 00000000..e69de29b diff --git a/modules/brother/cmd/brother/testdata/admin/live/general_status_html.headers b/modules/brother/cmd/brother/testdata/admin/live/general_status_html.headers new file mode 100644 index 00000000..276f3f68 --- /dev/null +++ b/modules/brother/cmd/brother/testdata/admin/live/general_status_html.headers @@ -0,0 +1,8 @@ +HTTP/1.1 302 Moved Temporarily +Cache-Control: no-store +X-Frame-Options: DENY +Content-Length: 0 +Connection: close +Pragma: no-cache +Location: /etc/loginerror.html?url=%2Fgeneral%2Fstatus%2Ehtml + diff --git a/modules/brother/cmd/brother/testdata/admin/live/home_status_html.body b/modules/brother/cmd/brother/testdata/admin/live/home_status_html.body new file mode 100644 index 00000000..f4fd4497 --- /dev/null +++ b/modules/brother/cmd/brother/testdata/admin/live/home_status_html.body @@ -0,0 +1,13 @@ + + + + + +Brother MFC-L8390CDW series
MFC-L8390CDW series

Status

Device Status
Automatic Refresh
Toner Level
LowReplace
BlackCyanMagentaYellow
BKCMY
Web Language
Device Location
  • Contact:
  • Location:
+
Copyright(C) 2000-2023 Brother Industries, Ltd. All Rights Reserved.
\ No newline at end of file diff --git a/modules/brother/cmd/brother/testdata/admin/live/home_status_html.headers b/modules/brother/cmd/brother/testdata/admin/live/home_status_html.headers new file mode 100644 index 00000000..39141d9d --- /dev/null +++ b/modules/brother/cmd/brother/testdata/admin/live/home_status_html.headers @@ -0,0 +1,11 @@ +HTTP/1.1 200 OK +Cache-Control: no-store +X-Content-Type-Options: nosniff +X-Frame-Options: DENY +Content-Length: 5180 +Content-Type: text/html +Content-Language: en-gb +Connection: close +X-XSS-Protection: 1; mode=block +Pragma: no-cache +