fix: canonical hardening (audit findings #1-12)

README.md

@@ -19,8 +19,6 @@ CLI for [QuickBooks Online](https://developer.intuit.com/app/developer/qbo/docs/
 - **Financial reports** — profit & loss, balance sheet, and more with date range filters
 - **Single binary** — install via Homebrew, Scoop, or `go install`
 
-<p align="center"><img src="demo.gif" alt="demo" width="700" /></p>
-
 ```bash
 $ qbo list invoices --where "Balance > '0'" --sandbox --json
 [

SECURITY.md

@@ -0,0 +1,29 @@
+# Security Policy
+
+## Supported versions
+
+Only the latest released version of `qbo` receives security fixes. Please
+upgrade (`brew upgrade qbo`, `scoop update qbo`, or `go install` the latest tag)
+before reporting.
+
+## Reporting a vulnerability
+
+Please report security issues **privately** — do not open a public issue.
+
+- Preferred: use GitHub's [private vulnerability reporting](https://github.com/voska/qbo-cli/security/advisories/new)
+  (the "Report a vulnerability" button on the repository's **Security** tab).
+
+We aim to acknowledge reports within 5 business days and to ship a fix or
+mitigation for confirmed issues as quickly as is practical, crediting reporters
+who wish to be named.
+
+## Handling credentials
+
+`qbo` touches financial data, so a few notes on how it treats secrets:
+
+- OAuth access/refresh tokens are stored in the OS keyring (or an encrypted
+  file fallback), never in plaintext config.
+- `QBO_CLIENT_ID` / `QBO_CLIENT_SECRET` are read from the environment. Never
+  commit them, and prefer a secrets manager over shell history.
+- Data is written to stdout and hints/progress to stderr; avoid piping verbose
+  output into shared logs that might capture tokens or company data.

cmd/qbo/main.go

@@ -1,8 +1,11 @@
 package main
 
 import (
+	"context"
 	"errors"
 	"os"
+	"os/signal"
+	"syscall"
 
 	"github.com/alecthomas/kong"
 	"github.com/voska/qbo-cli/internal/cmd"
@@ -20,19 +23,21 @@ func main() {
 		kong.UsageOnError(),
 	)
 
-	ctx, err := parser.Parse(os.Args[1:])
+	kctx, err := parser.Parse(os.Args[1:])
 	if err != nil {
 		parser.FatalIfErrorf(err)
 	}
 
-	globals, err := cmd.NewGlobals(&cli)
+	ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
+	defer stop()
+
+	globals, err := cmd.NewGlobals(ctx, &cli)
 	if err != nil {
 		handleError(err)
 	}
 	globals.Version = version
 
-	err = ctx.Run(globals)
-	if err != nil {
+	if err := kctx.Run(globals); err != nil {
 		handleError(err)
 	}
 }

internal/api/batch.go

@@ -7,15 +7,11 @@ import (
 	"github.com/voska/qbo-cli/internal/errfmt"
 )
 
-type BatchItem struct {
-	BId       string `json:"bId"`
-	Operation string `json:"operation,omitempty"`
-	Query     string `json:"Query,omitempty"`
-	Entity    string `json:"-"`
-	Body      any    `json:",omitempty"`
-}
-
-func (c *Client) Batch(ctx context.Context, items []BatchItem) (map[string]any, error) {
+// Batch posts a QBO batch request. items are the raw BatchItemRequest entries
+// (each in QBO's native batch format, e.g. {"bId":"1","operation":"create","Invoice":{...}}
+// or {"bId":"2","Query":"select * from Invoice"}), passed through verbatim
+// under the BatchItemRequest envelope.
+func (c *Client) Batch(ctx context.Context, items []json.RawMessage) (map[string]any, error) {
 	payload := map[string]any{
 		"BatchItemRequest": items,
 	}

internal/api/batch_test.go

@@ -0,0 +1,47 @@
+package api
+
+import (
+	"context"
+	"encoding/json"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+// Batch must pass the parsed items through under BatchItemRequest (regression
+// test for the bug where the command discarded its input and posted nil).
+func TestBatchPostsItems(t *testing.T) {
+	var gotBody map[string]any
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/v3/company/123/batch" {
+			t.Errorf("path = %q", r.URL.Path)
+		}
+		b, _ := io.ReadAll(r.Body)
+		_ = json.Unmarshal(b, &gotBody)
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"BatchItemResponse":[{"bId":"1"}]}`))
+	}))
+	defer srv.Close()
+
+	items := []json.RawMessage{
+		json.RawMessage(`{"bId":"1","operation":"create","Invoice":{"x":1}}`),
+		json.RawMessage(`{"bId":"2","Query":"select * from Invoice"}`),
+	}
+	resp, err := testClient(srv).Batch(context.Background(), items)
+	if err != nil {
+		t.Fatalf("Batch: %v", err)
+	}
+
+	arr, ok := gotBody["BatchItemRequest"].([]any)
+	if !ok || len(arr) != 2 {
+		t.Fatalf("BatchItemRequest not passed through: %v", gotBody)
+	}
+	first, _ := arr[0].(map[string]any)
+	if first["bId"] != "1" {
+		t.Errorf("first item not preserved: %v", first)
+	}
+	if _, ok := resp["BatchItemResponse"]; !ok {
+		t.Errorf("response not returned: %v", resp)
+	}
+}

internal/api/client.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"net/url"
 	"strings"
+	"time"
 
 	"github.com/voska/qbo-cli/internal/errfmt"
 	"golang.org/x/oauth2"
@@ -16,6 +17,11 @@ import (
 const (
 	BaseURLProduction = "https://quickbooks.api.intuit.com"
 	BaseURLSandbox    = "https://sandbox-quickbooks.api.intuit.com"
+
+	// requestTimeout bounds a single API call so a stalled connection can't
+	// hang the CLI indefinitely. Per-call cancellation also flows from the
+	// request context (e.g. Ctrl-C), this is the hard upper bound.
+	requestTimeout = 60 * time.Second
 )
 
 type Client struct {
@@ -30,8 +36,10 @@ func NewClient(token *oauth2.Token, realmID string, sandbox bool, minorVersion i
 	if sandbox {
 		base = BaseURLSandbox
 	}
+	httpClient := oauth2.NewClient(context.Background(), oauth2.StaticTokenSource(token))
+	httpClient.Timeout = requestTimeout
 	return &Client{
-		httpClient:   oauth2.NewClient(context.Background(), oauth2.StaticTokenSource(token)),
+		httpClient:   httpClient,
 		baseURL:      base,
 		realmID:      realmID,
 		minorVersion: minorVersion,

internal/auth/oauth.go

@@ -13,20 +13,27 @@ import (
 	"os/exec"
 	"runtime"
 	"strings"
+	"time"
 
 	"github.com/voska/qbo-cli/internal/errfmt"
 	"golang.org/x/oauth2"
 )
 
+// loginTimeout bounds the local browser-callback login so an abandoned login
+// releases the callback port instead of hanging forever.
+const loginTimeout = 5 * time.Minute
+
 type AuthResult struct {
 	Token   *oauth2.Token
 	RealmID string
 }
 
-func GenerateState() string {
+func GenerateState() (string, error) {
 	b := make([]byte, 16)
-	_, _ = rand.Read(b)
-	return hex.EncodeToString(b)
+	if _, err := rand.Read(b); err != nil {
+		return "", errfmt.Wrap(errfmt.ExitError, "cannot generate secure OAuth state", err)
+	}
+	return hex.EncodeToString(b), nil
 }
 
 func GetAuthURL(clientID, clientSecret, redirectURL, state string) string {
@@ -80,14 +87,20 @@ func LoginInteractive(ctx context.Context, clientID, clientSecret, redirectURL s
 }
 
 func loginLocal(ctx context.Context, clientID, clientSecret, redirectURL string) (*AuthResult, error) {
+	ctx, cancel := context.WithTimeout(ctx, loginTimeout)
+	defer cancel()
+
 	addr := fmt.Sprintf("127.0.0.1:%d", DefaultCallbackPort)
 	listener, err := net.Listen("tcp", addr)
 	if err != nil {
 		return nil, errfmt.Wrap(errfmt.ExitError, fmt.Sprintf("cannot listen on port %d — is another qbo login running?", DefaultCallbackPort), err)
 	}
 
 	cfg := OAuthConfig(clientID, clientSecret, redirectURL)
-	state := GenerateState()
+	state, err := GenerateState()
+	if err != nil {
+		return nil, err
+	}
 
 	resultCh := make(chan *AuthResult, 1)
 	errCh := make(chan error, 1)
@@ -140,7 +153,10 @@ func loginLocal(ctx context.Context, clientID, clientSecret, redirectURL string)
 
 func loginManual(ctx context.Context, clientID, clientSecret, redirectURL string) (*AuthResult, error) {
 	cfg := OAuthConfig(clientID, clientSecret, redirectURL)
-	state := GenerateState()
+	state, err := GenerateState()
+	if err != nil {
+		return nil, err
+	}
 
 	authURL := cfg.AuthCodeURL(state, oauth2.AccessTypeOffline)
 	fmt.Fprintf(os.Stderr, "Open this URL in your browser:\n\n  %s\n\n", authURL)

internal/auth/token.go

@@ -13,6 +13,11 @@ import (
 
 const keyringService = "qbo-cli"
 
+// expiryLeeway treats a token as expired slightly before its real expiry so a
+// request doesn't go out with a token that lapses mid-flight (mirrors the
+// oauth2 library's default expiry delta).
+const expiryLeeway = 30 * time.Second
+
 func tokenDir() string {
 	if d := os.Getenv("QBO_CONFIG_DIR"); d != "" {
 		return filepath.Join(d, "tokens")
@@ -90,5 +95,5 @@ func ListTokenKeys() ([]string, error) {
 }
 
 func IsTokenExpired(token *oauth2.Token) bool {
-	return token.Expiry.Before(time.Now())
+	return token.Expiry.Add(-expiryLeeway).Before(time.Now())
 }

internal/cmd/batch.go

@@ -17,8 +17,16 @@ func (c *BatchCmd) Run(g *Globals) error {
 		return err
 	}
 
+	var items []json.RawMessage
+	if err := json.Unmarshal(body, &items); err != nil {
+		return errfmt.Wrap(errfmt.ExitUsage, "invalid batch JSON (expected a JSON array of batch items)", err)
+	}
+	if len(items) == 0 {
+		return errfmt.Usage("batch file contains no items")
+	}
+
 	if g.CLI.DryRun {
-		output.Hint("[dry-run] POST /v3/company/{id}/batch")
+		output.Hint("[dry-run] POST /v3/company/{id}/batch (%d items)", len(items))
 		DryRunLog(g.CLI, "POST", "batch", body)
 		return nil
 	}
@@ -28,12 +36,7 @@ func (c *BatchCmd) Run(g *Globals) error {
 		return err
 	}
 
-	var items []map[string]any
-	if err := json.Unmarshal(body, &items); err != nil {
-		return errfmt.Wrap(errfmt.ExitUsage, "invalid batch JSON", err)
-	}
-
-	result, err := client.Batch(g.Ctx, nil)
+	result, err := client.Batch(g.Ctx, items)
 	if err != nil {
 		return err
 	}

internal/cmd/cmd_test.go

@@ -0,0 +1,73 @@
+package cmd
+
+import (
+	"context"
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/voska/qbo-cli/internal/errfmt"
+)
+
+func exitCode(err error) int {
+	var e *errfmt.Error
+	if errors.As(err, &e) {
+		return e.Code
+	}
+	return -1
+}
+
+func testGlobals(cli *CLI) *Globals {
+	return &Globals{Ctx: context.Background(), CLI: cli}
+}
+
+func TestDeleteUnknownEntity(t *testing.T) {
+	err := (&DeleteCmd{Entity: "bogus", ID: "1"}).Run(testGlobals(&CLI{}))
+	if got := exitCode(err); got != errfmt.ExitUsage {
+		t.Fatalf("exit = %d, want %d (%v)", got, errfmt.ExitUsage, err)
+	}
+}
+
+// --no-input must refuse (fail), not silently delete — only --force authorizes it.
+func TestDeleteNoInputWithoutForceFails(t *testing.T) {
+	err := (&DeleteCmd{Entity: "invoice", ID: "5"}).Run(testGlobals(&CLI{NoInput: true}))
+	if got := exitCode(err); got != errfmt.ExitUsage {
+		t.Fatalf("--no-input delete should fail with ExitUsage, got exit %d (%v)", got, err)
+	}
+}
+
+// Dry-run must return before constructing the API client / making any call.
+func TestDeleteDryRunNoNetwork(t *testing.T) {
+	if err := (&DeleteCmd{Entity: "invoice", ID: "5"}).Run(testGlobals(&CLI{DryRun: true})); err != nil {
+		t.Fatalf("dry-run delete returned error: %v", err)
+	}
+}
+
+func TestCreateUnknownEntity(t *testing.T) {
+	err := (&CreateCmd{Entity: "bogus"}).Run(testGlobals(&CLI{}))
+	if got := exitCode(err); got != errfmt.ExitUsage {
+		t.Fatalf("exit = %d, want %d (%v)", got, errfmt.ExitUsage, err)
+	}
+}
+
+func TestBatchRejectsEmptyAndInvalid(t *testing.T) {
+	dir := t.TempDir()
+	write := func(name, content string) string {
+		p := filepath.Join(dir, name)
+		if err := os.WriteFile(p, []byte(content), 0o600); err != nil {
+			t.Fatal(err)
+		}
+		return p
+	}
+
+	empty := write("empty.json", "[]")
+	if got := exitCode((&BatchCmd{File: empty}).Run(testGlobals(&CLI{}))); got != errfmt.ExitUsage {
+		t.Errorf("empty batch: exit = %d, want %d", got, errfmt.ExitUsage)
+	}
+
+	bad := write("bad.json", "{not json")
+	if got := exitCode((&BatchCmd{File: bad}).Run(testGlobals(&CLI{}))); got != errfmt.ExitUsage {
+		t.Errorf("invalid batch: exit = %d, want %d", got, errfmt.ExitUsage)
+	}
+}

internal/cmd/delete.go

@@ -33,7 +33,10 @@ func (c *DeleteCmd) Run(g *Globals) error {
 		return nil
 	}
 
-	if !g.CLI.Force && !g.CLI.NoInput {
+	if !g.CLI.Force {
+		if g.CLI.NoInput {
+			return errfmt.Usage("refusing to delete without --force in --no-input mode")
+		}
 		fmt.Fprintf(os.Stderr, "Delete %s %s? [y/N] ", entity.Name, c.ID)
 		var confirm string
 		_, _ = fmt.Scanln(&confirm)

internal/cmd/list.go

@@ -44,5 +44,11 @@ func (c *ListCmd) Run(g *Globals) error {
 	if err != nil {
 		return err
 	}
-	return WriteOutput(g.Ctx, result)
+	if err := WriteOutput(g.Ctx, result); err != nil {
+		return err
+	}
+	if output.IsEmpty(result) {
+		return errfmt.Empty()
+	}
+	return nil
 }