From 747d470b487e9e4b7f245490c65494edae95fb9e Mon Sep 17 00:00:00 2001 From: Raj Nakarja Date: Fri, 21 Aug 2026 10:51:50 +0200 Subject: [PATCH] Treat the server and the filesystem as untrusted input Two functions in internal/api carry the policy, so it cannot drift apart at the two dozen sites that need it. Printable escapes with Go's own rules, which cover the control characters a terminal acts on and the bidirectional overrides that reorder what is around them, while leaving accented letters, other scripts, emoji and spaces byte-identical. Decode bounds a body at 32 MiB before reading it, which is set against the server's caps of 100 fleets per user and 100 fleet keys per fleet. A device name is renamable by any member of the fleet, so a name carrying an erase-line sequence used to erase its own row and hide the device from the owner who pays for it. All five tables, both browser links, the one-time code, the fleet key and the relayed server sentence now go through Printable. Every success body was decoded unbounded while every error body was already bounded, and twelve sites returned encoding/json's own words to the user. A captive portal now reads as a sentence rather than "invalid character '<'". The same was true of the filesystem. Seven sites returned an os path error raw, and two of them fired after an irreversible act had already succeeded: deleting an account whose stored login could not then be removed swallowed "Account deleted." entirely. Both say what happened and what is left to do. The stored login lands by rename from a temporary file in the same folder, so an interrupted write cannot destroy a working login and a file already at a wider mode cannot keep it. Four smaller holes on the same theme: key create could print an empty fleet key and exit zero, logout looped forever on an empty stored login rather than treating it as logged out, one unreadable last-seen time hid the whole device table behind Go's layout string, and three fleet-name lookups rendered a blank cell for a fleet that had gone. account delete now reads the status of the probe it already makes, so an invalid login is reported before the question rather than after it. Fifteen of the sixteen new guards were checked against the mutation that breaks them. The exception is the scheme check in openBrowser, which cannot be exercised without launching a real handler on the machine running the tests. No version change, and nothing under .github, flake.nix, .goreleaser.yaml or go.mod is touched. --- internal/account/account.go | 36 +++++++++---- internal/account/account_test.go | 30 +++++++++++ internal/api/api_test.go | 89 +++++++++++++++++++++++++++++++- internal/api/balances.go | 3 +- internal/api/client.go | 23 +++++++-- internal/api/devices.go | 3 +- internal/api/fleets.go | 3 +- internal/api/keys.go | 3 +- internal/api/session.go | 21 ++++++-- internal/api/text.go | 11 ++++ internal/api/text_test.go | 52 +++++++++++++++++++ internal/device/device.go | 40 ++++++++------ internal/device/device_test.go | 4 +- internal/fleet/fleet.go | 14 ++--- internal/fleet/fleet_test.go | 6 +++ internal/key/key.go | 28 +++++++--- internal/key/key_test.go | 24 ++++++++- internal/login/login.go | 62 ++++++++++++++++------ internal/login/login_test.go | 77 ++++++++++++++++++++++++++- internal/member/member.go | 15 +++--- internal/member/member_test.go | 7 +++ 21 files changed, 468 insertions(+), 83 deletions(-) create mode 100644 internal/api/text.go create mode 100644 internal/api/text_test.go diff --git a/internal/account/account.go b/internal/account/account.go index 7b49f9e..d9419d0 100644 --- a/internal/account/account.go +++ b/internal/account/account.go @@ -81,18 +81,28 @@ func Balance(session api.Session, arguments []string) error { idWidth := len("ID") nameWidth := len("NAME") + nameValues := make([]string, len(balances)) + amountValues := make([]string, len(balances)) - for _, balance := range balances { + for index, balance := range balances { + name, known := fleetNames[balance.Fleet] + + if !known { + name = "-" + } + + formatted, _, _ := api.FormatBalance(balance) + + nameValues[index] = api.Printable(name) + amountValues[index] = api.Printable(formatted) idWidth = max(idWidth, len(strconv.FormatInt(balance.Fleet, 10))) - nameWidth = max(nameWidth, len(fleetNames[balance.Fleet])) + nameWidth = max(nameWidth, len(nameValues[index])) } fmt.Fprintf(session.Out, "%-*s %-*s %s\n", idWidth, "ID", nameWidth, "NAME", "BALANCE") - for _, balance := range balances { - formatted, _, _ := api.FormatBalance(balance) - - fmt.Fprintf(session.Out, "%-*d %-*s %s\n", idWidth, balance.Fleet, nameWidth, fleetNames[balance.Fleet], formatted) + for index, balance := range balances { + fmt.Fprintf(session.Out, "%-*d %-*s %s\n", idWidth, balance.Fleet, nameWidth, nameValues[index], amountValues[index]) } return nil @@ -132,13 +142,13 @@ func Topup(session api.Session, arguments []string) error { Url string `json:"url"` }{} - err = json.NewDecoder(response.Body).Decode(&opened) + err = api.Decode(response, &opened) if err != nil || opened.Url == "" { return errors.New("could not open the top-up page, try again") } - fmt.Fprintf(session.Out, "Open this link to choose an amount and pay:\n\n %s\n\nThe credit appears on the balance once the top-up completes.\nPress enter to open the browser.\n", opened.Url) + fmt.Fprintf(session.Out, "Open this link to choose an amount and pay:\n\n %s\n\nThe credit appears on the balance once the top-up completes.\nPress enter to open the browser.\n", api.Printable(opened.Url)) _, err = bufio.NewReader(session.In).ReadString('\n') @@ -170,6 +180,10 @@ func Delete(session api.Session, arguments []string) error { response.Body.Close() + if response.StatusCode == http.StatusUnauthorized { + return errors.New("you are not logged in, run login first") + } + fmt.Fprint(session.Out, "Delete your account, its logins, and your access to every fleet? This cannot be undone. [y/N] ") answer, _ := bufio.NewReader(session.In).ReadString('\n') @@ -199,6 +213,8 @@ func Delete(session api.Session, arguments []string) error { return api.ServerError(response) } + fmt.Fprintln(session.Out, "Account deleted.") + path, err := api.KeyPath() if err != nil { @@ -208,10 +224,8 @@ func Delete(session api.Session, arguments []string) error { err = os.Remove(path) if err != nil && !errors.Is(err, fs.ErrNotExist) { - return err + return fmt.Errorf("the login stored at %s is no longer valid but could not be removed, delete it yourself", path) } - fmt.Fprintln(session.Out, "Account deleted.") - return nil } diff --git a/internal/account/account_test.go b/internal/account/account_test.go index 32cc467..fdf7924 100644 --- a/internal/account/account_test.go +++ b/internal/account/account_test.go @@ -38,6 +38,14 @@ func TestAccountBalance(t *testing.T) { wantLines: []string{"pilot", "€0.00"}, wantAbsent: []string{"crew"}, }, + { + name: "a fleet name with control characters is escaped", + arguments: []string{}, + fleets: `[{"id":1,"name":"\u001b[2Kquiet","owner":true}]`, + balances: `[{"fleet":1,"balance":"15.000000","currency":"eur"}]`, + wantLines: []string{`\x1b[2Kquiet`}, + wantAbsent: []string{"\x1b"}, + }, { name: "machine readable", arguments: []string{"--json"}, @@ -394,3 +402,25 @@ func TestAccountDeleteAsksNothingWhenTheServerIsGone(t *testing.T) { t.Errorf("it asked %q before finding the server was gone", out.String()) } } + +func TestAccountDeleteStopsWhenTheStoredLoginIsNoLongerValid(t *testing.T) { + mux := http.NewServeMux() + + mux.HandleFunc("GET /", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + }) + + session, out := apitest.LoggedInSession(t, mux) + + session.In = strings.NewReader("y\n") + + err := Delete(session, nil) + + if err == nil || !strings.Contains(err.Error(), "not logged in") { + t.Fatalf("error = %v, want it to say the login is not valid", err) + } + + if out.String() != "" { + t.Errorf("output = %q, want the question never to be put", out.String()) + } +} diff --git a/internal/api/api_test.go b/internal/api/api_test.go index b3bab3a..70bbe1d 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -3,12 +3,14 @@ package api_test import ( "bytes" "fmt" + "io" "net/http" "os" "path/filepath" "runtime" "strings" "testing" + "time" "github.com/siliconwitchery/superstack-cli/internal/api" "github.com/siliconwitchery/superstack-cli/internal/api/apitest" @@ -171,7 +173,8 @@ func TestFetchKeysFailures(t *testing.T) { wantError string }{ {name: "server refusal", status: http.StatusServiceUnavailable, body: "keys unavailable", wantError: "keys unavailable"}, - {name: "undecodable body", status: http.StatusOK, body: `{`, wantError: "unexpected EOF"}, + {name: "undecodable body", status: http.StatusOK, body: `{`, wantError: "could not be read"}, + {name: "a refusal carrying control characters", status: http.StatusServiceUnavailable, body: "\x1b[2Kgone", wantError: `\x1b[2Kgone`}, } for _, test := range tests { @@ -248,3 +251,87 @@ func TestFormatBalance(t *testing.T) { }) } } + +func TestDecode(t *testing.T) { + tests := []struct { + name string + body string + wantId int64 + wantError string + }{ + {name: "a whole object", body: `{"id":7}`, wantId: 7}, + {name: "a truncated object", body: `{"id":`, wantError: "could not be read"}, + {name: "a page from something that is not the server", body: "bad gateway", wantError: "could not be read"}, + {name: "nothing at all", body: "", wantError: "could not be read"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + response := &http.Response{Body: io.NopCloser(strings.NewReader(test.body))} + + value := struct { + Id int64 `json:"id"` + }{} + + err := api.Decode(response, &value) + + if test.wantError != "" { + if err == nil || !strings.Contains(err.Error(), test.wantError) { + t.Fatalf("error = %v, want it to mention %q", err, test.wantError) + } + + for _, machinery := range []string{"unexpected EOF", "invalid character", "json", "EOF"} { + if strings.Contains(err.Error(), machinery) { + t.Errorf("the error %q shows the user %q", err, machinery) + } + } + + return + } + + if err != nil { + t.Fatal(err) + } + + if value.Id != test.wantId { + t.Errorf("id = %d, want %d", value.Id, test.wantId) + } + }) + } +} + +type endlessBody struct{} + +func (endlessBody) Read(destination []byte) (int, error) { + for index := range destination { + destination[index] = 'x' + } + + return len(destination), nil +} + +func TestDecodeStopsReadingABodyThatNeverEnds(t *testing.T) { + response := &http.Response{ + Body: io.NopCloser(io.MultiReader(strings.NewReader(`{"name":"`), endlessBody{})), + } + + value := struct { + Name string `json:"name"` + }{} + + finished := make(chan error, 1) + + go func() { + finished <- api.Decode(response, &value) + }() + + select { + case err := <-finished: + if err == nil { + t.Fatal("Decode accepted a body that never ends") + } + + case <-time.After(30 * time.Second): + t.Fatal("Decode is still reading a body that never ends") + } +} diff --git a/internal/api/balances.go b/internal/api/balances.go index 5f6bcb4..d82258a 100644 --- a/internal/api/balances.go +++ b/internal/api/balances.go @@ -1,7 +1,6 @@ package api import ( - "encoding/json" "errors" "fmt" "net/http" @@ -35,7 +34,7 @@ func FetchBalances(session Session) ([]BalanceEntry, error) { balances := []BalanceEntry{} - err = json.NewDecoder(response.Body).Decode(&balances) + err = Decode(response, &balances) if err != nil { return nil, err diff --git a/internal/api/client.go b/internal/api/client.go index aec8162..1fbe2b8 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -1,6 +1,7 @@ package api import ( + "encoding/json" "errors" "io" "io/fs" @@ -11,6 +12,10 @@ import ( "strings" ) +// Larger than any answer the server can produce: it caps fleets at 100 per +// user and keys at 100 per fleet, and only the device list is uncapped. +const maximumBody = 32 << 20 + func Request(session Session, method string, path string, body io.Reader) (*http.Request, error) { request, err := http.NewRequest(method, strings.TrimSuffix(session.Base, "/")+path, body) @@ -37,7 +42,7 @@ func AuthenticatedRequest(session Session, method string, path string, body io.R } if err != nil { - return nil, err + return nil, errors.New("the login stored on this computer could not be read") } key := strings.TrimSpace(string(keyBytes)) @@ -66,7 +71,17 @@ func ServerError(response *http.Response) error { return errors.New("that did not go through, try again in a moment") } - return errors.New(detail) + return errors.New(Printable(detail)) +} + +func Decode(response *http.Response, value any) error { + err := json.NewDecoder(io.LimitReader(response.Body, maximumBody)).Decode(value) + + if err != nil { + return errors.New("the server's answer could not be read, try again in a moment") + } + + return nil } func KeyPath() (string, error) { @@ -78,7 +93,7 @@ func KeyPath() (string, error) { home, err := os.UserHomeDir() if err != nil { - return "", err + return "", errors.New("your home folder could not be found, so the login has nowhere to live") } stateHome = filepath.Join(home, ".local", "state") @@ -90,7 +105,7 @@ func KeyPath() (string, error) { configDirectory, err := os.UserConfigDir() if err != nil { - return "", err + return "", errors.New("your settings folder could not be found, so the login has nowhere to live") } return filepath.Join(configDirectory, "superstack", "key"), nil diff --git a/internal/api/devices.go b/internal/api/devices.go index 99d4081..5df2cd5 100644 --- a/internal/api/devices.go +++ b/internal/api/devices.go @@ -1,7 +1,6 @@ package api import ( - "encoding/json" "errors" "net/http" ) @@ -37,7 +36,7 @@ func FetchDevices(session Session) ([]DeviceEntry, error) { devices := []DeviceEntry{} - err = json.NewDecoder(response.Body).Decode(&devices) + err = Decode(response, &devices) if err != nil { return nil, err diff --git a/internal/api/fleets.go b/internal/api/fleets.go index 06e05b6..5c8593e 100644 --- a/internal/api/fleets.go +++ b/internal/api/fleets.go @@ -1,7 +1,6 @@ package api import ( - "encoding/json" "errors" "net/http" ) @@ -33,7 +32,7 @@ func FetchFleets(session Session) ([]FleetEntry, error) { fleets := []FleetEntry{} - err = json.NewDecoder(response.Body).Decode(&fleets) + err = Decode(response, &fleets) if err != nil { return nil, err diff --git a/internal/api/keys.go b/internal/api/keys.go index a84c9c9..2e78885 100644 --- a/internal/api/keys.go +++ b/internal/api/keys.go @@ -1,7 +1,6 @@ package api import ( - "encoding/json" "errors" "net/http" ) @@ -34,7 +33,7 @@ func FetchKeys(session Session) ([]KeyEntry, error) { keys := []KeyEntry{} - err = json.NewDecoder(response.Body).Decode(&keys) + err = Decode(response, &keys) if err != nil { return nil, err diff --git a/internal/api/session.go b/internal/api/session.go index c58fce0..9f18277 100644 --- a/internal/api/session.go +++ b/internal/api/session.go @@ -3,6 +3,7 @@ package api import ( "io" "net/http" + "net/url" "os/exec" "runtime" "time" @@ -20,7 +21,7 @@ type Session struct { Client *http.Client In io.Reader Out io.Writer - OpenBrowser func(url string) + OpenBrowser func(link string) } func NewSession(base string, version string, in io.Reader, out io.Writer) Session { @@ -36,18 +37,28 @@ func NewSession(base string, version string, in io.Reader, out io.Writer) Sessio } } -func openBrowser(url string) { +func openBrowser(link string) { + address, err := url.Parse(link) + + if err != nil { + return + } + + if address.Scheme != "http" && address.Scheme != "https" { + return + } + var command *exec.Cmd switch runtime.GOOS { case "darwin": - command = exec.Command("open", url) + command = exec.Command("open", link) case "windows": - command = exec.Command("rundll32", "url.dll,FileProtocolHandler", url) + command = exec.Command("rundll32", "url.dll,FileProtocolHandler", link) default: - command = exec.Command("xdg-open", url) + command = exec.Command("xdg-open", link) } _ = command.Start() diff --git a/internal/api/text.go b/internal/api/text.go new file mode 100644 index 0000000..8468895 --- /dev/null +++ b/internal/api/text.go @@ -0,0 +1,11 @@ +package api + +import "strconv" + +// Escapes with Go's own rules, which cover the control characters a terminal +// acts on and the bidirectional overrides that reorder what is around them. +func Printable(text string) string { + quoted := strconv.QuoteToGraphic(text) + + return quoted[1 : len(quoted)-1] +} diff --git a/internal/api/text_test.go b/internal/api/text_test.go new file mode 100644 index 0000000..c6c792a --- /dev/null +++ b/internal/api/text_test.go @@ -0,0 +1,52 @@ +package api_test + +import ( + "strings" + "testing" + "unicode" + + "github.com/siliconwitchery/superstack-cli/internal/api" +) + +func TestPrintable(t *testing.T) { + tests := []struct { + name string + text string + want string + }{ + {name: "plain text is untouched", text: "weather-north", want: "weather-north"}, + {name: "an empty string stays empty", text: "", want: ""}, + {name: "spaces are kept", text: "north south", want: "north south"}, + {name: "accented letters are kept", text: "Ekbläd-Fjärrsond", want: "Ekbläd-Fjärrsond"}, + {name: "other scripts are kept", text: "気象観測機", want: "気象観測機"}, + {name: "emoji are kept", text: "roof \U0001F6F0", want: "roof \U0001F6F0"}, + {name: "an erase-line sequence is escaped", text: "\x1b[2K\rhidden", want: `\x1b[2K\rhidden`}, + {name: "a newline is escaped", text: "north\nsouth", want: `north\nsouth`}, + {name: "a tab is escaped", text: "north\tsouth", want: `north\tsouth`}, + {name: "a bell is escaped", text: "north\asouth", want: `north\asouth`}, + {name: "a right-to-left override is escaped", text: "a\u202eb", want: "a\\u202eb"}, + {name: "a zero-width joiner is escaped", text: "a\u200db", want: "a\\u200db"}, + {name: "a quote is escaped", text: `my"device`, want: `my\"device`}, + {name: "a backslash is escaped", text: `my\device`, want: `my\\device`}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := api.Printable(test.text) + + if got != test.want { + t.Errorf("Printable(%q) = %q, want %q", test.text, got, test.want) + } + + for _, letter := range got { + if unicode.IsControl(letter) { + t.Errorf("Printable(%q) left the control character %U in %q", test.text, letter, got) + } + } + + if strings.ContainsAny(got, "\x1b\r\n") { + t.Errorf("Printable(%q) = %q, which a terminal would still act on", test.text, got) + } + }) + } +} diff --git a/internal/device/device.go b/internal/device/device.go index 16dfc36..5c8cee8 100644 --- a/internal/device/device.go +++ b/internal/device/device.go @@ -180,21 +180,21 @@ func List(session api.Session, arguments []string) error { if device.LastSeenAt != nil { seenAt, err := time.Parse(time.RFC3339, *device.LastSeenAt) - if err != nil { - return err - } + lastSeen = "unknown" + + if err == nil { + age := time.Since(seenAt) - age := time.Since(seenAt) - - switch { - case age < 2*time.Minute: - lastSeen = "just now" - case age < time.Hour: - lastSeen = fmt.Sprintf("%d min ago", int(age.Minutes())) - case age < 24*time.Hour: - lastSeen = fmt.Sprintf("%d h ago", int(age.Hours())) - default: - lastSeen = fmt.Sprintf("%d d ago", int(age.Hours()/24)) + switch { + case age < 2*time.Minute: + lastSeen = "just now" + case age < time.Hour: + lastSeen = fmt.Sprintf("%d min ago", int(age.Minutes())) + case age < 24*time.Hour: + lastSeen = fmt.Sprintf("%d h ago", int(age.Hours())) + default: + lastSeen = fmt.Sprintf("%d d ago", int(age.Hours()/24)) + } } } @@ -228,9 +228,15 @@ func List(session api.Session, arguments []string) error { storage = fmt.Sprintf("%s of %s", formatBytes(*device.StorageUsed), formatBytes(*device.StorageTotal)) } - imeiValues[index] = device.Imei - nameValues[index] = name - fleetValues[index] = fleetNames[device.FleetId] + fleetName, known := fleetNames[device.FleetId] + + if !known { + fleetName = "-" + } + + imeiValues[index] = api.Printable(device.Imei) + nameValues[index] = api.Printable(name) + fleetValues[index] = api.Printable(fleetName) stateValues[index] = state storageValues[index] = storage lastSeenValues[index] = lastSeen diff --git a/internal/device/device_test.go b/internal/device/device_test.go index 3aa025c..d27bbc6 100644 --- a/internal/device/device_test.go +++ b/internal/device/device_test.go @@ -188,7 +188,9 @@ func TestDeviceList(t *testing.T) { {name: "unknown fleet", arguments: []string{"9"}, wantError: "no such fleet"}, {name: "two ids", arguments: []string{"3", "4"}, wantError: "takes at most one fleet id"}, {name: "wordy id", arguments: []string{"pilot"}, wantError: "shown by fleet list"}, - {name: "bad last seen time", devices: `[{"imei":"111111111111111","name":"roof","fleet_id":3,"last_seen_at":"yesterday"}]`, wantError: `cannot parse "yesterday"`}, + {name: "an unreadable last seen time leaves the rest of the table", devices: `[{"imei":"111111111111111","name":"roof","fleet_id":3,"last_seen_at":"yesterday"}]`, wantShown: []string{"111111111111111 roof pilot unknown - unknown"}}, + {name: "a fleet the list does not name", devices: `[{"imei":"888888888888888","name":"orphan","fleet_id":99}]`, wantShown: []string{"888888888888888 orphan - unknown - never"}}, + {name: "a name with control characters is escaped", devices: `[{"imei":"111111111111111","name":"\u001b[2K\rhidden","fleet_id":3}]`, wantShown: []string{`\x1b[2K\rhidden`}, wantHidden: []string{"\x1b"}}, {name: "minutes ago", devices: fmt.Sprintf(`[{"imei":"111111111111111","name":"roof","fleet_id":3,"last_seen_at":%q}]`, now.Add(-12*time.Minute).Format(time.RFC3339)), wantShown: []string{"12 min ago"}}, {name: "days ago", devices: fmt.Sprintf(`[{"imei":"111111111111111","name":"roof","fleet_id":3,"last_seen_at":%q}]`, now.Add(-49*time.Hour).Format(time.RFC3339)), wantShown: []string{"2 d ago"}}, {name: "stopped and undefined states", devices: `[{"imei":"444444444444444","name":"halted","fleet_id":3,"reported_state":3},{"imei":"555555555555555","name":"odd","fleet_id":3,"reported_state":1}]`, wantShown: []string{"stopped", "unknown"}}, diff --git a/internal/fleet/fleet.go b/internal/fleet/fleet.go index ea01162..44bf6b1 100644 --- a/internal/fleet/fleet.go +++ b/internal/fleet/fleet.go @@ -49,7 +49,7 @@ func Create(session api.Session, arguments []string) error { Name string `json:"name"` }{} - err = json.NewDecoder(response.Body).Decode(&created) + err = api.Decode(response, &created) if err != nil { return err @@ -84,22 +84,24 @@ func List(session api.Session, arguments []string) error { idWidth := len("ID") nameWidth := len("NAME") + nameValues := make([]string, len(fleets)) - for _, fleet := range fleets { + for index, fleet := range fleets { + nameValues[index] = api.Printable(fleet.Name) idWidth = max(idWidth, len(strconv.FormatInt(fleet.Id, 10))) - nameWidth = max(nameWidth, len(fleet.Name)) + nameWidth = max(nameWidth, len(nameValues[index])) } fmt.Fprintf(session.Out, "%-*s %-*s %s\n", idWidth, "ID", nameWidth, "NAME", "ROLE") - for _, fleet := range fleets { + for index, fleet := range fleets { role := "member" if fleet.Owner { role = "owner" } - fmt.Fprintf(session.Out, "%-*d %-*s %s\n", idWidth, fleet.Id, nameWidth, fleet.Name, role) + fmt.Fprintf(session.Out, "%-*d %-*s %s\n", idWidth, fleet.Id, nameWidth, nameValues[index], role) } return nil @@ -283,7 +285,7 @@ func Delete(session api.Session, arguments []string) error { } if value > 0 { - forfeited = formatted + forfeited = api.Printable(formatted) } } diff --git a/internal/fleet/fleet_test.go b/internal/fleet/fleet_test.go index e6b8327..031a42b 100644 --- a/internal/fleet/fleet_test.go +++ b/internal/fleet/fleet_test.go @@ -100,6 +100,12 @@ func TestFleetList(t *testing.T) { fleets: `[{"id":1,"name":"field trial","owner":true},{"id":2,"name":"rooftop","owner":false}]`, wantShown: []string{"ID", "NAME", "ROLE", "field trial", "owner", "rooftop", "member"}, }, + { + name: "a name with control characters is escaped", + fleets: `[{"id":1,"name":"\u001b[2K\rhidden","owner":true}]`, + wantShown: []string{`\x1b[2K\rhidden`}, + wantAbsent: []string{"\x1b"}, + }, { name: "no fleets", fleets: `[]`, diff --git a/internal/key/key.go b/internal/key/key.go index 8f44fc2..830fb61 100644 --- a/internal/key/key.go +++ b/internal/key/key.go @@ -56,13 +56,17 @@ func Create(session api.Session, arguments []string) error { Key string `json:"key"` }{} - err = json.NewDecoder(response.Body).Decode(&created) + err = api.Decode(response, &created) if err != nil { return err } - fmt.Fprintf(session.Out, "Created fleet key %d.\n\n %s\n\nAnyone holding it can send data to the fleet, and you will not see it again.\n", created.Id, created.Key) + if created.Key == "" { + return errors.New("the fleet key was not created, try again") + } + + fmt.Fprintf(session.Out, "Created fleet key %d.\n\n %s\n\nAnyone holding it can send data to the fleet, and you will not see it again.\n", created.Id, api.Printable(created.Key)) return nil } @@ -135,19 +139,31 @@ func List(session api.Session, arguments []string) error { idWidth := len("ID") fleetIdWidth := len("FLEET") fleetNameWidth := len("FLEET NAME") + fleetNameValues := make([]string, len(keys)) + suffixValues := make([]string, len(keys)) + labelValues := make([]string, len(keys)) - for _, key := range keys { + for index, key := range keys { + fleetName, known := fleetNames[key.Fleet] + + if !known { + fleetName = "-" + } + + fleetNameValues[index] = api.Printable(fleetName) + suffixValues[index] = api.Printable(key.Suffix) + labelValues[index] = api.Printable(key.Label) idWidth = max(idWidth, len(strconv.FormatInt(key.Id, 10))) fleetIdWidth = max(fleetIdWidth, len(strconv.FormatInt(key.Fleet, 10))) - fleetNameWidth = max(fleetNameWidth, len(fleetNames[key.Fleet])) + fleetNameWidth = max(fleetNameWidth, len(fleetNameValues[index])) } fmt.Fprintf(session.Out, "%-*s %-*s %-*s %-8s %s\n", idWidth, "ID", fleetIdWidth, "FLEET", fleetNameWidth, "FLEET NAME", "KEY", "LABEL") - for _, key := range keys { + for index, key := range keys { fmt.Fprintf(session.Out, "%-*d %-*d %-*s ...%s %s\n", - idWidth, key.Id, fleetIdWidth, key.Fleet, fleetNameWidth, fleetNames[key.Fleet], key.Suffix, key.Label) + idWidth, key.Id, fleetIdWidth, key.Fleet, fleetNameWidth, fleetNameValues[index], suffixValues[index], labelValues[index]) } return nil diff --git a/internal/key/key_test.go b/internal/key/key_test.go index 70d082a..69dac1c 100644 --- a/internal/key/key_test.go +++ b/internal/key/key_test.go @@ -16,9 +16,18 @@ func TestKeyCreate(t *testing.T) { arguments []string wantPath string wantLabel string + answer string refusal string wantError string }{ + { + name: "the server answers without a key", + arguments: []string{"3", "deploy server"}, + wantPath: "/fleets/3/keys", + wantLabel: "deploy server", + answer: `{"id":1}`, + wantError: "was not created", + }, { name: "a labelled key", arguments: []string{"3", "deploy server"}, @@ -87,7 +96,13 @@ func TestKeyCreate(t *testing.T) { t.Errorf("the request carried label %q, want %q", sent.Label, test.wantLabel) } - fmt.Fprint(w, `{"id":1,"key":"ssf_testtesttestab2de"}`) + answer := test.answer + + if answer == "" { + answer = `{"id":1,"key":"ssf_testtesttestab2de"}` + } + + fmt.Fprint(w, answer) }) session, out := apitest.LoggedInSession(t, mux) @@ -150,6 +165,13 @@ func TestKeyList(t *testing.T) { wantShown: []string{"ID FLEET FLEET NAME", "crew", "...ab2de"}, wantHidden: []string{"skunkworks", "f9hjk", "lab sensor"}, }, + { + name: "a label with control characters is escaped", + arguments: []string{}, + keys: `[{"id":1,"fleet":3,"label":"\u001b[2Kquiet","suffix":"ab2de"}]`, + wantShown: []string{`\x1b[2Kquiet`}, + wantHidden: []string{"\x1b"}, + }, { name: "no keys", arguments: []string{}, diff --git a/internal/login/login.go b/internal/login/login.go index 8752288..0741002 100644 --- a/internal/login/login.go +++ b/internal/login/login.go @@ -49,7 +49,7 @@ func Login(session api.Session, arguments []string) error { GitlabClientId string `json:"gitlab_client_id"` }{} - err = json.NewDecoder(providersResponse.Body).Decode(&providers) + err = api.Decode(providersResponse, &providers) if err != nil { return err @@ -108,7 +108,7 @@ func Login(session api.Session, arguments []string) error { Error string `json:"error"` }{} - err = json.NewDecoder(codeResponse.Body).Decode(&code) + err = api.Decode(codeResponse, &code) if err != nil { return fmt.Errorf("%s would not start the login, try again", provider) @@ -124,8 +124,8 @@ func Login(session api.Session, arguments []string) error { enterAt = code.VerificationUriComplete } - fmt.Fprintf(session.Out, "Copy your one-time code: %s\n", code.UserCode) - fmt.Fprintf(session.Out, "Then enter it at %s\n", enterAt) + fmt.Fprintf(session.Out, "Copy your one-time code: %s\n", api.Printable(code.UserCode)) + fmt.Fprintf(session.Out, "Then enter it at %s\n", api.Printable(enterAt)) fmt.Fprintln(session.Out, "Press enter to open the browser.") go func() { @@ -181,7 +181,7 @@ func Login(session api.Session, arguments []string) error { Error string `json:"error"` }{} - err = json.NewDecoder(pollResponse.Body).Decode(&poll) + err = api.Decode(pollResponse, &poll) pollResponse.Body.Close() @@ -247,7 +247,7 @@ func Login(session api.Session, arguments []string) error { Email string `json:"email"` }{} - err = json.NewDecoder(loginResponse.Body).Decode(&login) + err = api.Decode(loginResponse, &login) if err != nil { return err @@ -263,19 +263,42 @@ func Login(session api.Session, arguments []string) error { return err } - err = os.MkdirAll(filepath.Dir(path), 0o700) + directory := filepath.Dir(path) + + err = os.MkdirAll(directory, 0o700) if err != nil { - return err + return fmt.Errorf("the login could not be saved to %s, so you are not logged in", path) } - err = os.WriteFile(path, []byte(login.Key+"\n"), 0o600) + temporary, err := os.CreateTemp(directory, "key") if err != nil { - return err + return fmt.Errorf("the login could not be saved to %s, so you are not logged in", path) + } + + defer os.Remove(temporary.Name()) + + _, err = temporary.WriteString(login.Key + "\n") + + if err != nil { + temporary.Close() + return fmt.Errorf("the login could not be saved to %s, so you are not logged in", path) } - fmt.Fprintf(session.Out, "Logged in as %s.\n", login.Email) + err = temporary.Close() + + if err != nil { + return fmt.Errorf("the login could not be saved to %s, so you are not logged in", path) + } + + err = os.Rename(temporary.Name(), path) + + if err != nil { + return fmt.Errorf("the login could not be saved to %s, so you are not logged in", path) + } + + fmt.Fprintf(session.Out, "Logged in as %s.\n", api.Printable(login.Email)) return nil } @@ -299,7 +322,14 @@ func Logout(session api.Session, arguments []string) error { } if err != nil { - return err + return errors.New("the login stored on this computer could not be read") + } + + key := strings.TrimSpace(string(keyBytes)) + + if key == "" { + fmt.Fprintln(session.Out, "Not logged in.") + return nil } revokeRequest, err := api.Request(session, http.MethodPost, "/logout", nil) @@ -308,7 +338,7 @@ func Logout(session api.Session, arguments []string) error { return err } - revokeRequest.Header.Set("Authorization", "Bearer "+strings.TrimSpace(string(keyBytes))) + revokeRequest.Header.Set("Authorization", "Bearer "+key) revokeResponse, err := session.Client.Do(revokeRequest) @@ -322,13 +352,13 @@ func Logout(session api.Session, arguments []string) error { return fmt.Errorf("you are still logged in: %s", api.ServerError(revokeResponse)) } + fmt.Fprintln(session.Out, "Logged out.") + err = os.Remove(path) if err != nil { - return err + return fmt.Errorf("the login stored at %s is no longer valid but could not be removed, delete it yourself", path) } - fmt.Fprintln(session.Out, "Logged out.") - return nil } diff --git a/internal/login/login_test.go b/internal/login/login_test.go index 8840533..cabbe96 100644 --- a/internal/login/login_test.go +++ b/internal/login/login_test.go @@ -449,6 +449,7 @@ func TestLogout(t *testing.T) { name string arguments []string storedKey string + storeEmptyKey bool serverDown bool revokeStatus int wantError string @@ -472,6 +473,12 @@ func TestLogout(t *testing.T) { name: "nothing stored", wantShown: "Not logged in.\n", }, + { + name: "an empty stored login", + storeEmptyKey: true, + wantShown: "Not logged in.\n", + wantKeyKept: true, + }, { name: "server refuses the revocation", storedKey: "ssk_test", @@ -520,7 +527,7 @@ func TestLogout(t *testing.T) { t.Fatal(err) } - if test.storedKey != "" { + if test.storedKey != "" || test.storeEmptyKey { err = os.MkdirAll(filepath.Dir(path), 0o700) if err != nil { @@ -568,3 +575,71 @@ func TestLogout(t *testing.T) { }) } } + +func TestLoginReplacesAStoredLoginLeftTooOpen(t *testing.T) { + apitest.IsolateKeyStorage(t) + + path, err := api.KeyPath() + + if err != nil { + t.Fatal(err) + } + + err = os.MkdirAll(filepath.Dir(path), 0o700) + + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(path, []byte("ssk_stale\n"), 0o600) + + if err != nil { + t.Fatal(err) + } + + err = os.Chmod(path, 0o644) + + if err != nil { + t.Fatal(err) + } + + _, providerBase := fakeProviderForLogin(t, "gitlab", 1, "", []string{`{"access_token": "glpat-test"}`}) + session, _ := fakeSuperstack(t, "", "") + session.GitlabBase = providerBase + + err = Login(session, []string{"gitlab"}) + + if err != nil { + t.Fatal(err) + } + + information, err := os.Stat(path) + + if err != nil { + t.Fatal(err) + } + + if information.Mode().Perm() != 0o600 { + t.Errorf("the stored login is mode %v, want it narrowed to 0600", information.Mode().Perm()) + } + + stored, err := os.ReadFile(path) + + if err != nil { + t.Fatal(err) + } + + if strings.TrimSpace(string(stored)) != "ssk_test" { + t.Errorf("the stored login is %q, want the one just issued", strings.TrimSpace(string(stored))) + } + + entries, err := os.ReadDir(filepath.Dir(path)) + + if err != nil { + t.Fatal(err) + } + + if len(entries) != 1 { + t.Errorf("the login folder holds %d files, want only the stored login", len(entries)) + } +} diff --git a/internal/member/member.go b/internal/member/member.go index 4993e84..37e8800 100644 --- a/internal/member/member.go +++ b/internal/member/member.go @@ -96,7 +96,7 @@ func List(session api.Session, arguments []string) error { Members []string `json:"members"` }{} - err = json.NewDecoder(response.Body).Decode(&people) + err = api.Decode(response, &people) if err != nil { return err @@ -106,17 +106,20 @@ func List(session api.Session, arguments []string) error { return json.NewEncoder(session.Out).Encode(people) } - emailWidth := max(len("EMAIL"), len(people.Owner)) + owner := api.Printable(people.Owner) + members := make([]string, len(people.Members)) + emailWidth := max(len("EMAIL"), len(owner)) - for _, email := range people.Members { - emailWidth = max(emailWidth, len(email)) + for index, email := range people.Members { + members[index] = api.Printable(email) + emailWidth = max(emailWidth, len(members[index])) } fmt.Fprintf(session.Out, "%-*s %s\n", emailWidth, "EMAIL", "ROLE") - fmt.Fprintf(session.Out, "%-*s owner\n", emailWidth, people.Owner) + fmt.Fprintf(session.Out, "%-*s owner\n", emailWidth, owner) - for _, email := range people.Members { + for _, email := range members { fmt.Fprintf(session.Out, "%-*s member\n", emailWidth, email) } diff --git a/internal/member/member_test.go b/internal/member/member_test.go index f8863ac..faa0036 100644 --- a/internal/member/member_test.go +++ b/internal/member/member_test.go @@ -108,6 +108,13 @@ func TestMemberList(t *testing.T) { wantFleet: "3", wantShown: []string{"EMAIL", "ROLE", "owner@example.com", "owner", "member@example.com", "member"}, }, + { + name: "addresses with control characters are escaped", + arguments: []string{"3"}, + people: `{"owner":"\u001bowner@example.com","members":["\u001bmember@example.com"]}`, + wantFleet: "3", + wantShown: []string{`\x1bowner@example.com`, `\x1bmember@example.com`}, + }, { name: "nobody but the owner", arguments: []string{"7"},