diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..6f5b998 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,34 @@ +name: CI +on: + pull_request: + push: + branches: [main] + workflow_dispatch: +permissions: + contents: read +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true +jobs: + check: + name: Check + runs-on: ubuntu-24.04 + timeout-minutes: 30 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + timeout-minutes: 3 + - uses: jdx/mise-action@7a4e45a543138629540c9a1616d08632b893e492 # v5.0.1 + with: + version: 2026.9.12 + timeout-minutes: 15 + - run: make install + timeout-minutes: 15 + - run: make check + timeout-minutes: 20 + - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + timeout-minutes: 5 + with: + name: spritesmith-cli-${{ github.sha }} + path: dist/ + if-no-files-found: error + retention-days: 7 diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..fff9ec9 --- /dev/null +++ b/.gitignore @@ -0,0 +1,7 @@ +/dist/ +/.artifacts/ +*.png +*.svg +.env + +spritesmith-alpha.png diff --git a/.mise.toml b/.mise.toml new file mode 100644 index 0000000..f42f969 --- /dev/null +++ b/.mise.toml @@ -0,0 +1,5 @@ +[tools] +go = "1.27.2" +python = "3.13.11" +actionlint = "1.7.12" +"go:golang.org/x/vuln/cmd/govulncheck" = "1.7.0" diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..f12b7fe --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,3 @@ +# Go HAS CHANGED + +Read the exact versioned docs at https://go.dev/doc/go1.27 before writing any code. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..43c994c --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..4826772 --- /dev/null +++ b/Makefile @@ -0,0 +1,33 @@ +SHELL := /bin/bash +.SHELLFLAGS := -eu -o pipefail -c +.DEFAULT_GOAL := help +.NOTPARALLEL: +VERSION ?= dev +.PHONY: help install lint unit integration test build artifact-check vuln check dev stop clean +help: + @echo 'make check: lint, race tests, five-platform archives, artifact verification, vulnerability scan' +install: + mise trust .mise.toml + mise install + mise exec -- go mod download +lint: + mise exec -- bash scripts/lint.sh + mise exec -- actionlint +unit: + mise exec -- go test -race -count=1 ./internal/token ./cmd/spritesmith +integration: + mise exec -- go test -race -count=1 ./internal/client +test: unit integration +build: + mise exec -- env VERSION="$(VERSION)" bash scripts/build.sh +artifact-check: + mise exec -- python3 scripts/check-artifacts.py +vuln: + mise exec -- govulncheck ./... +check: lint test build artifact-check vuln +dev: + mise exec -- go run ./cmd/spritesmith $(ARGS) +stop: + @echo 'unsupported: the CLI owns no background service' +clean: + mise exec -- python3 -c 'import shutil; [shutil.rmtree(p, ignore_errors=True) for p in ("dist", ".artifacts")]' diff --git a/README.md b/README.md index 836fc5c..334dc06 100644 --- a/README.md +++ b/README.md @@ -1 +1,45 @@ -# spritesmith \ No newline at end of file +# Spritesmith + +Command-line client for the Spritesmith API. Create raster or vector sprites and download finished PNG or SVG files. + +The client requires an API service and an account token. It does not contain a local renderer or generation engine. + +## Install + +With Go 1.27.2 or later: + +```sh +go install github.com/aviorstudio/spritesmith/cmd/spritesmith@latest +``` + +## Configure + +Set `SPRITESMITH_API_BASE` to your service URL and `SPRITESMITH_API_TOKEN` to your account's Clerk API key. The token can also live in a private `spritesmith/token` file under your operating system's user configuration directory. Run `spritesmith --help` to see its path. Token files must not be accessible to other users. + +Hosted tokens require HTTPS. Loopback HTTP is supported for local development. + +## Create sprites + +```sh +spritesmith prompt --operation-id hero-v1 --out hero.png 'A small forest ranger' +spritesmith prompt --kind vector --format svg --operation-id emblem-v1 --out emblem.svg 'A golden shield emblem' +spritesmith prompt --kind vector --format png --out emblem.png 'A golden shield emblem' +spritesmith image --out ranger.png reference.png +``` + +Flags come before the prompt or image filename. Vector creation requires a prompt; an image may be supplied as a reference. + +Raster sprites export as PNG. Vector sprites export as SVG or PNG. SVG contains editable artwork geometry. + +Generation runs as a server job. The client waits for completion and downloads the requested file. Retain `--operation-id` and reuse it with the same input after an interruption. Existing output files are never overwritten. An uncertain job requires reconciliation; the client does not automatically submit another paid request. + +The default API is `http://127.0.0.1:8080`; configure your hosted service explicitly. Default output is `spritesmith-alpha.png`, or `spritesmith.svg` for SVG downloads. + +## Development + +```sh +make install +make check +``` + +Checks include Go race tests, HTTP client integration tests, five platform archives, checksums, source identity verification, and vulnerability scanning. The client builds independently of private repositories. diff --git a/cmd/spritesmith/main.go b/cmd/spritesmith/main.go new file mode 100644 index 0000000..d5136d1 --- /dev/null +++ b/cmd/spritesmith/main.go @@ -0,0 +1,277 @@ +package main + +import ( + "errors" + "flag" + "fmt" + "io" + "os" + "strings" + + "github.com/aviorstudio/spritesmith/internal/client" + "github.com/aviorstudio/spritesmith/internal/token" +) + +var version = "dev" +var revision = "unknown" + +const defaultAPIBase = "http://127.0.0.1:8080" + +type commonFlags struct { + operationID *string + apiBase *string + out *string + kind *string + format *string +} + +func main() { + os.Exit(run(os.Args[1:])) +} + +func run(args []string) int { + if len(args) == 0 { + printUsage(os.Stderr) + return 2 + } + + switch args[0] { + case "--version", "version": + fmt.Fprintf(os.Stdout, "spritesmith %s (%s)\n", version, revision) + return 0 + case "image": + return runImage(args[1:]) + case "prompt": + return runPrompt(args[1:]) + case "-h", "--help", "help": + printUsage(os.Stdout) + return 0 + default: + fmt.Fprintf(os.Stderr, "unknown command %q\n\n", args[0]) + printUsage(os.Stderr) + return 2 + } +} + +func runImage(args []string) int { + apiBase, req, ok := parseImage(args) + if !ok { + return 2 + } + return submit(apiBase, req) +} + +func runPrompt(args []string) int { + apiBase, req, ok := parsePrompt(args) + if !ok { + return 2 + } + return submit(apiBase, req) +} + +// parseImage and parsePrompt are separate from the submit they feed so that +// something can test the seam between a flag and the request it builds. +// Folded into run* they were untestable, and losing a line there is invisible: +// dropping OutputPath makes --out do nothing, and the CLI still prints a path +// and still writes a PNG, just never the one that was asked for. +func parseImage(args []string) (apiBase string, req client.Request, ok bool) { + fs, flags := newCommandFlags("image") + if err := fs.Parse(args); err != nil { + return "", client.Request{}, false + } + if fs.NArg() != 1 { + fmt.Fprintln(os.Stderr, "usage: spritesmith image [flags] IMAGE") + return "", client.Request{}, false + } + + return *flags.apiBase, client.Request{ + OperationID: *flags.operationID, + Mode: client.ModeImage, + SourcePath: fs.Arg(0), + OutputPath: outputPath(flags), + Kind: *flags.kind, Format: *flags.format, + }, true +} + +func parsePrompt(args []string) (apiBase string, req client.Request, ok bool) { + fs, flags := newCommandFlags("prompt") + terminated := promptFlagsTerminated(args, fs) + if err := fs.Parse(args); err != nil { + return "", client.Request{}, false + } + if misplaced := misplacedPromptFlag(fs, terminated); misplaced != "" { + fmt.Fprintf(os.Stderr, "misplaced flag %q: flags must come before PROMPT; try: spritesmith prompt --out x.png \"a duck\"\n", misplaced) + return "", client.Request{}, false + } + prompt := strings.TrimSpace(strings.Join(fs.Args(), " ")) + if prompt == "" { + fmt.Fprintln(os.Stderr, "usage: spritesmith prompt [flags] PROMPT") + return "", client.Request{}, false + } + + return *flags.apiBase, client.Request{ + OperationID: *flags.operationID, + Mode: client.ModePrompt, + Prompt: prompt, + OutputPath: outputPath(flags), + Kind: *flags.kind, Format: *flags.format, + }, true +} + +func misplacedPromptFlag(fs *flag.FlagSet, terminated bool) string { + remaining := fs.Args() + if len(remaining) == 0 || terminated { + return "" + } + + for _, arg := range remaining[1:] { + name := strings.TrimLeft(arg, "-") + if name == arg || name == "" { + continue + } + if before, _, found := strings.Cut(name, "="); found { + name = before + } + if fs.Lookup(name) != nil { + return arg + } + } + return "" +} + +func promptFlagsTerminated(args []string, fs *flag.FlagSet) bool { + for len(args) > 0 { + arg := args[0] + if arg == "--" { + return true + } + if len(arg) < 2 || arg[0] != '-' { + return false + } + + name := strings.TrimPrefix(strings.TrimPrefix(arg, "-"), "-") + name, _, hasValue := strings.Cut(name, "=") + defined := fs.Lookup(name) + if defined == nil { + return false + } + args = args[1:] + if hasValue { + continue + } + if boolean, ok := defined.Value.(interface{ IsBoolFlag() bool }); ok && boolean.IsBoolFlag() { + continue + } + if len(args) == 0 { + return false + } + args = args[1:] + } + return false +} + +func newCommandFlags(name string) (*flag.FlagSet, commonFlags) { + fs := flag.NewFlagSet(name, flag.ContinueOnError) + fs.SetOutput(os.Stderr) + flags := commonFlags{ + operationID: fs.String("operation-id", "", "reuse the same logical generation after an interrupted submission"), + apiBase: fs.String("api", envDefault("SPRITESMITH_API_BASE", defaultAPIBase), "spritesmith API base URL"), + out: fs.String("out", "", "output filename"), + kind: fs.String("kind", "raster", "raster or vector"), + format: fs.String("format", "png", "png or svg (vector only)"), + } + return fs, flags +} + +func outputPath(flags commonFlags) string { + if *flags.out != "" { + return *flags.out + } + if *flags.format == "svg" { + return "spritesmith.svg" + } + return client.DefaultOutputPath +} + +func submit(apiBase string, req client.Request) int { + loader := token.New() + credential, err := loader.Load() + if err != nil { + fmt.Fprintln(os.Stderr, err) + return 1 + } + if credential.Warning != "" { + fmt.Fprintln(os.Stderr, "warning: "+credential.Warning) + } + + resp, err := client.Client{BaseURL: apiBase, Token: credential.Value}.Process(req) + if err != nil { + // A bare "401 Unauthorized" tells somebody nothing they can act on. + // This is the one error the CLI can explain, so it does. + if errors.Is(err, client.ErrUnauthorized) { + fmt.Fprintln(os.Stderr, unauthorizedHelp(loader, credential)) + return 1 + } + fmt.Fprintln(os.Stderr, err) + return 1 + } + fmt.Fprintln(os.Stdout, resp.OutputPath) + return 0 +} + +// unauthorizedHelp turns a 401 into the fix. It names the source of the token +// and never the token itself. +func unauthorizedHelp(loader token.Loader, credential token.Token) string { + if credential.Value == "" { + path := loader.PathOrHint() + return "unauthorized: this API requires a spritesmith API token and none is configured.\n" + + " Create a Clerk API key for your spritesmith account, then either:\n" + + " export " + token.EnvVar + "=\n" + + " or write the token to " + path + " and run: chmod 600 " + path + } + return "unauthorized: the API rejected the spritesmith API token from " + credential.Source + ".\n" + + " It may have been revoked, or it may have expired. Replace it and try again." +} + +// printUsage writes the help. Takes an io.Writer rather than *os.File so a +// test can read what it says -- the authentication section below is the only +// place anybody is told how to get a token, so it is worth asserting on. +func printUsage(out io.Writer) { + fmt.Fprintln(out, "usage: spritesmith COMMAND [flags]") + fmt.Fprintln(out) + fmt.Fprintln(out, "Commands:") + fmt.Fprintln(out, " image IMAGE make a sprite from a reference image through the API") + fmt.Fprintln(out, " prompt PROMPT make a sprite from a text prompt through the API") + fmt.Fprintln(out) + fmt.Fprintln(out, "Flags:") + fmt.Fprintln(out, " --kind raster|vector creation method, default raster") + fmt.Fprintln(out, " --format png|svg download format, default png; SVG requires vector") + fmt.Fprintln(out, " --operation-id ID retain this ID for safe retries") + fmt.Fprintln(out, " --api URL API base URL, default http://127.0.0.1:8080") + fmt.Fprintf(out, " --out FILE where to write the sprite, default %s\n", client.DefaultOutputPath) + fmt.Fprintln(out) + tokenPath := token.New().PathOrHint() + fmt.Fprintln(out, "Authentication:") + fmt.Fprintln(out, " A hosted spritesmith API requires a token: a Clerk API key created for your") + fmt.Fprintln(out, " spritesmith account. An explicitly configured local development API needs none.") + fmt.Fprintln(out) + fmt.Fprintf(out, " export %s=\n", token.EnvVar) + // -D, so this works on a machine that has never had the directory. + fmt.Fprintf(out, " # or: install -D -m 600 /dev/null %s && $EDITOR %s\n", tokenPath, tokenPath) + fmt.Fprintln(out) + fmt.Fprintf(out, " %s wins when both are set. There is no --token flag on purpose:\n", token.EnvVar) + fmt.Fprintln(out, " an argument is visible in the shell history and in the process list.") + fmt.Fprintln(out, " The token is only ever sent over https, or to a loopback address.") + fmt.Fprintln(out) + fmt.Fprintln(out, "Output:") + fmt.Fprintf(out, " Each run downloads one PNG or SVG, to %s unless --out says\n", client.DefaultOutputPath) + fmt.Fprintln(out, " otherwise. An existing file is never overwritten: spritesmith stops and says so,") + fmt.Fprintln(out, " because the same request does not produce the same image twice.") +} + +func envDefault(key string, fallback string) string { + if value := os.Getenv(key); value != "" { + return value + } + return fallback +} diff --git a/cmd/spritesmith/main_test.go b/cmd/spritesmith/main_test.go new file mode 100644 index 0000000..1e79ea5 --- /dev/null +++ b/cmd/spritesmith/main_test.go @@ -0,0 +1,344 @@ +package main + +import ( + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + + "github.com/aviorstudio/spritesmith/internal/client" + "github.com/aviorstudio/spritesmith/internal/token" +) + +const testToken = "ak_ZmFrZS1zZWNyZXQtZm9yLXRlc3Rz" + +func testLoader() token.Loader { + return token.Loader{ + Getenv: func(string) string { return "" }, + ConfigDir: func() (string, error) { return "/home/somebody/.config", nil }, + } +} + +// A 401 with no token configured is the case the CLI exists to explain. "401 +// Unauthorized" tells somebody nothing they can act on; this has to tell them +// where a token comes from and where to put it. +func TestTheHelpForAMissingTokenNamesTheFix(t *testing.T) { + message := unauthorizedHelp(testLoader(), token.Token{}) + + for _, want := range []string{ + token.EnvVar, + "/home/somebody/.config/spritesmith/token", + "Clerk", + "chmod 600", + } { + if !strings.Contains(message, want) { + t.Errorf("the message does not mention %q:\n%s", want, message) + } + } +} + +// A 401 WITH a token is a different problem and needs a different answer: +// nothing is missing, the credential is dead. Naming the source is the whole +// value, because the token may be coming from somewhere the caller forgot. +func TestTheHelpForARejectedTokenNamesItsSourceAndNotItsValue(t *testing.T) { + credential := token.Token{Value: testToken, Source: "$" + token.EnvVar} + + message := unauthorizedHelp(testLoader(), credential) + + if !strings.Contains(message, "$"+token.EnvVar) { + t.Errorf("the message does not name where the token came from:\n%s", message) + } + if !strings.Contains(message, "revoked") { + t.Errorf("the message does not say what is likely wrong:\n%s", message) + } + if strings.Contains(message, testToken) { + t.Fatalf("the message contains the token:\n%s", message) + } +} + +// The token is a credential and this message is printed to a terminal, so the +// promise holds for a file source too -- where the source is a path that the +// value must not be confused with. +func TestNoUnauthorizedMessageEverContainsTheToken(t *testing.T) { + for _, credential := range []token.Token{ + {}, + {Value: testToken, Source: "$" + token.EnvVar}, + {Value: testToken, Source: "/home/somebody/.config/spritesmith/token"}, + } { + if message := unauthorizedHelp(testLoader(), credential); strings.Contains(message, testToken) { + t.Errorf("the message contains the token:\n%s", message) + } + } +} + +// --help is the only place anybody is told how to authenticate, which is half +// of what this issue is. +func TestHelpExplainsHowToGetAndSetAToken(t *testing.T) { + var out strings.Builder + printUsage(&out) + help := out.String() + + for _, want := range []string{ + token.EnvVar, + "Clerk API key", + "spritesmith/token", + // The local API needs none, or somebody will go hunting for a token + // they do not need to run `make cli`. + "local development API", + } { + if !strings.Contains(help, want) { + t.Errorf("--help does not mention %q:\n%s", want, help) + } + } +} + +// A recipe in the help is a command somebody pastes. `install` without -D does +// not create the parent directory, so the version without it fails on every +// machine that has never had a spritesmith config directory -- which is every +// machine reading this help for the first time. +func TestHelpUsesARecipeThatWorksOnAFreshMachine(t *testing.T) { + var out strings.Builder + printUsage(&out) + help := out.String() + + if !strings.Contains(help, "install -D -m 600") { + t.Errorf("--help suggests an install that does not create the directory:\n%s", help) + } +} + +// The output-format toggles are gone, not merely undocumented. A flag that +// still parses is a flag somebody's script still passes, and `--sheet` in +// particular would go on parsing forever while quietly meaning nothing. +func TestTheOutputFormatFlagsNoLongerExist(t *testing.T) { + for _, gone := range []string{"-sheet", "-debug"} { + t.Run(gone, func(t *testing.T) { + fs, _ := newCommandFlags("image") + fs.SetOutput(io.Discard) + if err := fs.Parse([]string{gone, "source.png"}); err == nil { + t.Errorf("%s still parses, so it is still part of the interface", gone) + } + }) + } +} + +// --out names the PNG now, not a directory to go looking in. The default has +// to be a file for that to be true of the path somebody gets with no flags at +// all -- which is the path almost everybody gets. +func TestOutDefaultsToASinglePNGFileInTheCurrentDirectory(t *testing.T) { + fs, flags := newCommandFlags("image") + if err := fs.Parse(nil); err != nil { + t.Fatal(err) + } + + got := outputPath(flags) + if filepath.Ext(got) != ".png" { + t.Errorf("the default output %q is not a .png file", got) + } + if dir := filepath.Dir(got); dir != "." { + t.Errorf("the default output %q is under %q; it should land in the current directory, not a directory spritesmith invents", got, dir) + } +} + +// The seam between a flag and the request built from it. Nothing else crosses +// it: the flags are tested here, the writing is tested in internal/client, and +// dropping OutputPath from either struct literal used to leave both halves +// green while --out silently did nothing -- the CLI would still print a path +// and still write a PNG, just never the one that was asked for. +func TestTheFlagsReachTheRequestForBothCommands(t *testing.T) { + t.Run("image", func(t *testing.T) { + apiBase, req, ok := parseImage([]string{"--api", "https://api.example.com", "--out", "build/sprite.png", "source.png"}) + if !ok { + t.Fatal("parsing a valid image command failed") + } + if apiBase != "https://api.example.com" { + t.Errorf("apiBase = %q, want the --api that was given", apiBase) + } + if req.Mode != client.ModeImage { + t.Errorf("Mode = %q, want %q", req.Mode, client.ModeImage) + } + if req.SourcePath != "source.png" { + t.Errorf("SourcePath = %q, want the positional argument", req.SourcePath) + } + if req.OutputPath != "build/sprite.png" { + t.Errorf("OutputPath = %q, want the --out that was given; --out is being ignored", req.OutputPath) + } + }) + + t.Run("prompt", func(t *testing.T) { + apiBase, req, ok := parsePrompt([]string{"--api", "https://api.example.com", "--out", "build/sprite.png", "a", "duck"}) + if !ok { + t.Fatal("parsing a valid prompt command failed") + } + if apiBase != "https://api.example.com" { + t.Errorf("apiBase = %q, want the --api that was given", apiBase) + } + if req.Mode != client.ModePrompt { + t.Errorf("Mode = %q, want %q", req.Mode, client.ModePrompt) + } + if req.Prompt != "a duck" { + t.Errorf("Prompt = %q, want the joined positional arguments", req.Prompt) + } + if req.OutputPath != "build/sprite.png" { + t.Errorf("OutputPath = %q, want the --out that was given; --out is being ignored", req.OutputPath) + } + }) +} + +// With no --out the request carries the default, which is what makes the +// no-flags run land somewhere sensible rather than nowhere. +func TestWithNoOutFlagTheRequestCarriesTheDefaultFile(t *testing.T) { + _, req, ok := parsePrompt([]string{"a duck"}) + if !ok { + t.Fatal("parsing a valid prompt command failed") + } + if req.OutputPath != client.DefaultOutputPath { + t.Errorf("OutputPath = %q, want %q", req.OutputPath, client.DefaultOutputPath) + } +} + +// Both commands still reject the argument shapes they cannot act on, and do it +// without building a request. +func TestACommandWithTheWrongArgumentsIsRejected(t *testing.T) { + if _, _, ok := parseImage(nil); ok { + t.Error("image with no file was accepted") + } + if _, _, ok := parseImage([]string{"one.png", "two.png"}); ok { + t.Error("image with two files was accepted") + } + for _, args := range [][]string{nil, {"--out", "x.png"}, {"--out=x.png"}, {" "}} { + message := captureStderr(t, func() { + if _, _, ok := parsePrompt(args); ok { + t.Errorf("prompt arguments %q were accepted", args) + } + }) + if !strings.Contains(message, "usage: spritesmith prompt [flags] PROMPT") { + t.Errorf("prompt arguments %q did not print usage", args) + } + } +} + +func TestAFlagAfterThePromptIsRejectedBeforeARequest(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + requests.Add(1) + })) + t.Cleanup(server.Close) + + for name, args := range map[string][]string{ + "ordinary": {"--api", server.URL, "a duck", "--out", "x.png"}, + "consumed -- as value": {"--api", server.URL, "--out", "--", "duck", "--out", "x.png"}, + } { + t.Run(name, func(t *testing.T) { + var exitCode int + message := captureStderr(t, func() { exitCode = runPrompt(args) }) + if exitCode != 2 || requests.Load() != 0 { + t.Errorf("exit code = %d, requests = %d; want 2, 0", exitCode, requests.Load()) + } + for _, want := range []string{`misplaced flag "--out"`, `spritesmith prompt --out x.png "a duck"`} { + if !strings.Contains(message, want) { + t.Errorf("error does not contain %q:\n%s", want, message) + } + } + }) + } +} + +func TestAFlagBeforeThePromptStillBuildsTheRequestedOutput(t *testing.T) { + _, req, ok := parsePrompt([]string{"--out", "x.png", "a duck"}) + if !ok { + t.Fatal("flag-first prompt was rejected") + } + if req.Prompt != "a duck" || req.OutputPath != "x.png" { + t.Errorf("request = %#v, want prompt %q and output %q", req, "a duck", "x.png") + } +} + +func TestTheFlagTerminatorStillAllowsAPromptBeginningWithADash(t *testing.T) { + _, req, ok := parsePrompt([]string{"--", "-a duck"}) + if !ok { + t.Fatal("prompt escaped with -- was rejected") + } + if req.Prompt != "-a duck" { + t.Errorf("Prompt = %q, want %q", req.Prompt, "-a duck") + } +} + +// image is the control: unlike prompt it accepts exactly one positional +// argument, so the same misplaced flag shape was already rejected. +func TestImageStillRejectsAFlagAfterItsArgument(t *testing.T) { + if _, _, ok := parseImage([]string{"source.png", "--out", "x.png"}); ok { + t.Error("image accepted a flag after its positional argument") + } +} + +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + stderr, err := os.CreateTemp(t.TempDir(), "stderr") + if err != nil { + t.Fatal(err) + } + original := os.Stderr + os.Stderr = stderr + defer func() { os.Stderr = original }() + + fn() + if err := stderr.Close(); err != nil { + t.Fatal(err) + } + output, err := os.ReadFile(stderr.Name()) + if err != nil { + t.Fatal(err) + } + return string(output) +} + +// The help describes one output. Every word below names a shape spritesmith does +// not produce, and leaving one in the help is how somebody goes looking for a +// flag or a directory that is not there. +// +// The archive and index formats are absent from this list on purpose, not by +// oversight: #56 asks that `grep -ri` for their names over cli/ come back +// empty, and a list here that spelled them would be the one hit. That grep is +// the stronger guard anyway -- it covers the whole tree, help text included, +// rather than this one string. +func TestHelpDescribesOneOutputAndNothingThatNoLongerExists(t *testing.T) { + var out strings.Builder + printUsage(&out) + // The help embeds the reader's own config directory, which this test does + // not control and which could contain any of the words below. Scanning it + // would make the test fail on somebody's laptop for a reason that has + // nothing to do with spritesmith. + help := strings.ToLower(out.String()) + help = strings.ReplaceAll(help, strings.ToLower(token.New().PathOrHint()), "") + + for _, gone := range []string{"sheet", "slice", "bundle", "spritesmith-out", "individual", "assets", "directory"} { + if strings.Contains(help, gone) { + t.Errorf("--help still mentions %q:\n%s", gone, out.String()) + } + } + for _, want := range []string{ + "one png or svg", + // --out takes a file, and saying DIR would send somebody looking + // inside it for the result. + "--out file", + } { + if !strings.Contains(help, want) { + t.Errorf("--help does not say %q:\n%s", want, out.String()) + } + } +} + +// The refusal to overwrite is a decision somebody has to be able to find +// before they hit it, and it lives in exactly one place. +func TestHelpSaysAnExistingFileIsNotOverwritten(t *testing.T) { + var out strings.Builder + printUsage(&out) + + if !strings.Contains(strings.ToLower(out.String()), "never overwritten") { + t.Errorf("--help does not say what happens when the output path is taken:\n%s", out.String()) + } +} diff --git a/engineering-profile.json b/engineering-profile.json new file mode 100644 index 0000000..3920949 --- /dev/null +++ b/engineering-profile.json @@ -0,0 +1,38 @@ +{ + "version": 1, + "profile": "go-cli", + "commands": { + "install": [ + "make", + "install" + ], + "lint": [ + "make", + "lint" + ], + "test": [ + "make", + "test" + ], + "build": [ + "make", + "build" + ], + "check": [ + "make", + "check" + ], + "clean": [ + "make", + "clean" + ], + "dev": [ + "make", + "dev" + ], + "stop": { + "unsupported": "The CLI owns no background service." + } + }, + "max_job_minutes": 30 +} diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..43777f4 --- /dev/null +++ b/go.mod @@ -0,0 +1,3 @@ +module github.com/aviorstudio/spritesmith + +go 1.27.2 diff --git a/internal/client/client.go b/internal/client/client.go new file mode 100644 index 0000000..7abe5bb --- /dev/null +++ b/internal/client/client.go @@ -0,0 +1,518 @@ +package client + +import ( + "bytes" + "context" + "crypto/rand" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "io" + "io/fs" + "mime" + "mime/multipart" + "net" + "net/http" + "net/url" + "os" + "path/filepath" + "strings" + "time" +) + +// ErrUnauthorized is a 401 from the API: the request needed an identity and +// did not establish one. +// +// Returned as a sentinel with no detail because the useful message depends on +// something this package does not know -- whether a token was configured at +// all, and where it would go. The command builds that; see cmd/spritesmith. +var ErrUnauthorized = errors.New("the API rejected this request as unauthenticated") + +type Mode string + +const ( + ModeImage Mode = "image" + ModePrompt Mode = "prompt" +) + +// DefaultOutputPath is where a result lands when the caller names nowhere. A +// file rather than a directory, because there is exactly one of them, and the +// same name the app saves so a spritesmith result is the same thing wherever it +// came from. +const DefaultOutputPath = "spritesmith-alpha.png" + +type Request struct { + Kind string + Format string + OperationID string + Mode Mode + Prompt string + SourcePath string + OutputPath string +} + +type Response struct { + OutputPath string + ContentType string +} + +type Client struct { + BaseURL string + HTTPClient *http.Client + // Token is the Clerk API key (`ak_…`), sent as a bearer credential. Empty + // sends no Authorization header at all, which is what a local API with no + // Clerk configuration expects and is why `make cli` works with nothing set + // up. See internal/token for where it comes from. + Token string +} + +func (c Client) Process(req Request) (Response, error) { + if req.Kind == "" { + req.Kind = "raster" + } + if req.Format == "" { + req.Format = "png" + } + if (req.Kind != "raster" && req.Kind != "vector") || (req.Format != "png" && req.Format != "svg") || (req.Kind == "raster" && req.Format == "svg") { + return Response{}, errors.New("choose raster/png, vector/png, or vector/svg") + } + if c.BaseURL == "" { + c.BaseURL = "http://127.0.0.1:8080" + } + // Before anything is built, let alone sent. A token is a bearer credential: + // anyone who reads it off the wire is that person until it is revoked. + // + // First of the two pre-flight checks, ahead of the output path, because + // when both would fail this is the one that matters: being told to move a + // file, fixing that, and only then learning the token was one step from + // crossing the network in the clear gets the order exactly backwards. + if c.Token != "" { + if err := checkTokenTransport(c.BaseURL); err != nil { + return Response{}, err + } + } + // An all-whitespace path is a mistake, but a path is otherwise taken + // verbatim -- trailing spaces in a filename are legal and not ours to trim. + outputPath := req.OutputPath + if strings.TrimSpace(outputPath) == "" { + outputPath = DefaultOutputPath + } + // Checked before the request rather than after it. A generation costs an + // OpenAI call and a minute of somebody's attention; discovering the + // collision on the way to disk means paying for a result that is then + // thrown away. The write itself re-checks atomically -- see writeOutput. + if err := checkOutputPathFree(outputPath); err != nil { + return Response{}, err + } + httpClient := c.HTTPClient + if httpClient == nil { + httpClient = http.DefaultClient + } + if c.Token != "" { + httpClient = guardRedirects(httpClient) + } + + var body bytes.Buffer + writer := multipart.NewWriter(&body) + if err := writer.WriteField("kind", req.Kind); err != nil { + return Response{}, err + } + // Keep compatibility with older API deployments. Current APIs ignore this + // field and always serve the packed transparent PNG. + if err := writer.WriteField("sheet", "true"); err != nil { + return Response{}, err + } + + endpoint := "/v1/image" + switch req.Mode { + case ModeImage: + if err := addFile(writer, "image", req.SourcePath); err != nil { + return Response{}, err + } + case ModePrompt: + endpoint = "/v1/prompt" + if strings.TrimSpace(req.Prompt) == "" { + return Response{}, errors.New("prompt is required") + } + if err := writer.WriteField("prompt", req.Prompt); err != nil { + return Response{}, err + } + default: + return Response{}, fmt.Errorf("unsupported mode %q", req.Mode) + } + if err := writer.Close(); err != nil { + return Response{}, err + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + defer cancel() + httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, strings.TrimRight(c.BaseURL, "/")+endpoint, &body) + if err != nil { + return Response{}, err + } + httpReq.Header.Set("Content-Type", writer.FormDataContentType()) + httpReq.Header.Set("Accept", "image/png, image/svg+xml, application/json") + operation := req.OperationID + if operation == "" { + value := make([]byte, 16) + rand.Read(value) + operation = hex.EncodeToString(value) + } + if len(operation) > 128 { + return Response{}, errors.New("operation ID is too long") + } + for _, character := range operation { + if !(character >= 'a' && character <= 'z' || character >= 'A' && character <= 'Z' || character >= '0' && character <= '9' || character == '-' || character == '_') { + return Response{}, errors.New("invalid operation ID") + } + } + httpReq.Header.Set("Idempotency-Key", operation) + if c.Token != "" { + httpReq.Header.Set("Authorization", "Bearer "+c.Token) + } + + resp, err := httpClient.Do(httpReq) + if err != nil { + return Response{}, fmt.Errorf("generation submission interrupted; retain --operation-id %s for a safe retry: %w", operation, err) + } + if resp.StatusCode == http.StatusAccepted { + initial := resp + resp, err = c.awaitGeneration(ctx, httpClient, initial, req.Format) + initial.Body.Close() + if err != nil { + return Response{}, err + } + } + defer resp.Body.Close() + + // A 401 is answered by the caller, not reported. The API's own message is + // deliberately uninformative -- telling bad tokens apart out loud is free + // reconnaissance -- so repeating it here would be a bare 401 dump and + // nothing else. See ErrUnauthorized. + if resp.StatusCode == http.StatusUnauthorized { + return Response{}, ErrUnauthorized + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + message, err := decodeError(resp.Body) + if err != nil { + return Response{}, fmt.Errorf("API returned %s", resp.Status) + } + return Response{}, errors.New(message) + } + + // A 200 is not a PNG. Accept is advisory, and a captive portal, a proxy + // error page or a JSON body carrying a 200 would otherwise be written + // verbatim to a .png -- an asset that is silently not an image, and, since + // the path is now taken, a retry that gets refused. Content-Type is the + // only signal there is before the body reaches disk. + contentType := resp.Header.Get("Content-Type") + expectedType := "image/png" + if req.Format == "svg" { + expectedType = "image/svg+xml" + } + if mediaType, _, err := mime.ParseMediaType(contentType); err != nil || mediaType != expectedType { + return Response{}, fmt.Errorf("the API answered %s with content type %q, which is not the sprite format requested", resp.Status, contentType) + } + + if err := writeOutput(outputPath, resp.Body); err != nil { + return Response{}, err + } + return Response{OutputPath: outputPath, ContentType: contentType}, nil +} + +func (c Client) awaitGeneration(ctx context.Context, client *http.Client, initial *http.Response, format string) (*http.Response, error) { + var job struct { + ID string `json:"id"` + } + if err := json.NewDecoder(io.LimitReader(initial.Body, 4096)).Decode(&job); err != nil || !validJobID(job.ID) { + return nil, errors.New("API returned an invalid generation identity") + } + endpoint := strings.TrimRight(c.BaseURL, "/") + "/v1/generations/" + job.ID + for { + request, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) + if err != nil { + return nil, err + } + if c.Token != "" { + request.Header.Set("Authorization", "Bearer "+c.Token) + } + response, err := client.Do(request) + if err != nil { + return nil, fmt.Errorf("generation %s remains recoverable: %w", job.ID, err) + } + if response.StatusCode != http.StatusOK { + response.Body.Close() + return nil, fmt.Errorf("generation %s status unavailable (%d)", job.ID, response.StatusCode) + } + var state struct { + Status string `json:"status"` + } + err = json.NewDecoder(io.LimitReader(response.Body, 4096)).Decode(&state) + response.Body.Close() + if err != nil { + return nil, err + } + switch state.Status { + case "succeeded": + request, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint+"/result?format="+format, nil) + if err != nil { + return nil, err + } + if c.Token != "" { + request.Header.Set("Authorization", "Bearer "+c.Token) + } + return client.Do(request) + case "uncertain": + return nil, fmt.Errorf("generation %s needs reconciliation; no paid resubmission was attempted", job.ID) + case "failed": + return nil, fmt.Errorf("generation %s failed", job.ID) + case "receiving", "queued", "claimed", "submitting", "processing": + timer := time.NewTimer(500 * time.Millisecond) + select { + case <-ctx.Done(): + timer.Stop() + return nil, fmt.Errorf("generation %s remains recoverable: %w", job.ID, ctx.Err()) + case <-timer.C: + } + default: + return nil, errors.New("API returned an unknown generation state") + } + } +} + +func validJobID(id string) bool { + if len(id) != 32 { + return false + } + for _, c := range id { + if !(c >= '0' && c <= '9' || c >= 'a' && c <= 'f') { + return false + } + } + return true +} + +// checkOutputPathFree refuses a path that is already taken. +// +// Overwriting is the wrong default for output that costs money to produce and +// cannot be reproduced: the same prompt does not generate the same image +// twice, so a clobbered result is gone. Suffixing was the other candidate and +// is worse where it matters -- `spritesmith prompt --out out.png ... && optimize +// out.png` would quietly read yesterday's file while today's sat beside it +// under a name nobody asked for. Refusing fails loudly instead, which a script +// can act on and a person can read. +// +// Lstat, not Stat, so this agrees with the O_EXCL below: a symlink pointing at +// nothing exists as far as the kernel is concerned, and reporting it free here +// would only move the failure later. +// +// It also catches a --out that names a directory. That was what --out meant +// until spritesmith collapsed to one output, so `--out spritesmith-out/` is the mistake +// to expect from anybody with the old command in their shell history -- and +// left to the write it would surface as a bare "is a directory" errno, after +// the generation had been paid for. +func checkOutputPathFree(path string) error { + if hasTrailingSeparator(path) { + return fmt.Errorf("--out %s names a directory, and spritesmith writes a single PNG.\n"+ + " Give it a file path instead, for example %s%s", path, path, DefaultOutputPath) + } + info, err := os.Lstat(path) + if err == nil { + if info.IsDir() { + return fmt.Errorf("%s is a directory, and spritesmith writes a single PNG.\n"+ + " Pass --out with a file path instead", path) + } + return errOutputExists(path) + } + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return err +} + +func hasTrailingSeparator(path string) bool { + if path == "" { + return false + } + last := path[len(path)-1] + return last == '/' || last == os.PathSeparator +} + +func errOutputExists(path string) error { + return fmt.Errorf("%s already exists, and spritesmith will not overwrite it.\n"+ + " Move or delete that file, or pass --out with a different path", path) +} + +// writeOutput saves the one PNG. +// +// O_EXCL rather than os.Create, for two reasons. It closes the gap between the +// pre-flight check and here -- a whole network round trip, during which +// anything may appear at this path -- so the promise not to overwrite is the +// kernel's rather than a hope about timing. And because O_EXCL fails on a +// symlink, a link sitting AT the output path is refused rather than followed. +// Only that last component: the directories above it are resolved normally, so +// this is not a defence against a planted parent directory, and no comment here +// should suggest it is. +// +// A failed copy takes the file with it. O_EXCL has already created it, so +// without the removal a dropped connection leaves a truncated PNG that the +// caller never asked for -- and the refusal above, seeing a file, would then +// block the retry with a message about protecting a result that is really half +// a download. Nothing but this call could have created the file, so removing it +// destroys nothing. +func writeOutput(path string, body io.Reader) error { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return err + } + out, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) + if err != nil { + if errors.Is(err, fs.ErrExist) { + return errOutputExists(path) + } + return err + } + _, copyErr := io.Copy(out, body) + closeErr := out.Close() + if copyErr != nil || closeErr != nil { + if removeErr := os.Remove(path); removeErr != nil && !errors.Is(removeErr, fs.ErrNotExist) { + return fmt.Errorf("%w (and the incomplete file could not be removed: %v)", errors.Join(copyErr, closeErr), removeErr) + } + return errors.Join(copyErr, closeErr) + } + return nil +} + +// checkTokenTransport refuses to put a bearer credential on a connection that +// anybody in the path can read. +// +// --api and SPRITESMITH_API_BASE are ordinary strings, and http:// is the default +// this CLI ships with, so pointing a token at a plaintext host is one typo or +// one stale shell export away. On the wire the token is not a password to +// crack, it IS the access -- for as long as it takes somebody to notice and +// revoke it, which for a long-lived key is forever. +// +// Loopback is allowed because that is the local API, the traffic never leaves +// the machine, and refusing it would break the one workflow this CLI is used +// for daily. +func checkTokenTransport(base string) error { + parsed, err := url.Parse(base) + if err != nil { + return fmt.Errorf("invalid API base URL %q: %w", base, err) + } + switch strings.ToLower(parsed.Scheme) { + case "https": + return nil + case "http": + if isLoopback(parsed.Hostname()) { + return nil + } + } + return fmt.Errorf( + "refusing to send the spritesmith API token to %s: it is neither https nor a local address, so the token would cross the network in the clear.\n"+ + " Point --api or SPRITESMITH_API_BASE at an https:// URL, or unset %s to send no token", + base, "SPRITESMITH_API_TOKEN") +} + +// guardRedirects re-runs the transport check on every hop, because checking +// the base URL alone does not hold the token safe. +// +// Go copies the Authorization header across a redirect whenever the +// destination is the same host or a subdomain of it -- net/http's +// shouldCopyHeaderOnRedirect compares hostnames and says nothing about the +// scheme. So `https://api.spritesmith.ai` answering `302 http://api.spritesmith.ai/...` +// puts the credential on the wire in cleartext, having passed a check that +// only ever saw the https base. The multipart body is a bytes.Buffer, so +// GetBody is set and even a 307 replays cleanly: this is reachable, not +// theoretical. +// +// Same-host only, which closes a second copy of the same hole. Go's rule is +// host OR SUBDOMAIN, so `https://api.spritesmith.ai` answering +// `302 https://anything.spritesmith.ai/...` also carries the token -- over TLS, but +// to a host that never had it. That is a subdomain takeover away from being a +// credential handover, and the spritesmith API has no reason to redirect across +// hosts at all. Same host with a different path or port stays allowed, which +// is every legitimate redirect there is. +// +// The client is COPIED. Mutating http.DefaultClient would change redirect +// behaviour for every other caller in the process, and mutating one the caller +// passed in is not this function's to do. +func guardRedirects(client *http.Client) *http.Client { + guarded := *client + previous := guarded.CheckRedirect + guarded.CheckRedirect = func(req *http.Request, via []*http.Request) error { + // The caller's own policy runs FIRST, so a policy that stops the + // redirect -- http.ErrUseLastResponse, say -- is honoured rather than + // overridden by a refusal about a hop that was never going to be + // followed. Anything this guard refuses below is a hop that WOULD have + // been followed with the token attached, which is the only case it is + // about. + if previous != nil { + if err := previous(req, via); err != nil { + return err + } + } else if len(via) >= 10 { + // net/http's own default, which replacing CheckRedirect switches + // off. Without it the CLI hangs forever on a redirect loop. + return errors.New("stopped after 10 redirects") + } + + if err := checkTokenTransport(req.URL.String()); err != nil { + return err + } + // via[0] is the ORIGINAL request, not the previous hop, which is both + // what Go's own header-copy rule compares against and the only choice + // that is safe: comparing against the previous hop would allow a chain + // to launder its way across, one plausible step at a time. + if origin := via[0].URL.Hostname(); !strings.EqualFold(req.URL.Hostname(), origin) { + return fmt.Errorf( + "refusing to follow a redirect from %s to %s while carrying the spritesmith API token: "+ + "%s is not the host you pointed the CLI at.\n"+ + " Point --api or SPRITESMITH_API_BASE at %s directly, or unset %s to send no token", + origin, req.URL.Hostname(), req.URL.Hostname(), req.URL.Hostname(), "SPRITESMITH_API_TOKEN") + } + return nil + } + return &guarded +} + +func isLoopback(host string) bool { + if strings.EqualFold(host, "localhost") { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} + +func addFile(writer *multipart.Writer, field string, path string) error { + file, err := os.Open(path) + if err != nil { + return fmt.Errorf("%s: %w", field, err) + } + defer file.Close() + + part, err := writer.CreateFormFile(field, filepath.Base(path)) + if err != nil { + return err + } + _, err = io.Copy(part, file) + return err +} + +func decodeError(reader io.Reader) (string, error) { + data, err := io.ReadAll(reader) + if err != nil { + return "", err + } + var payload struct { + Error string `json:"error"` + } + if json.Unmarshal(data, &payload) == nil && payload.Error != "" { + return payload.Error, nil + } + message := strings.TrimSpace(string(data)) + if message == "" { + return "", fmt.Errorf("empty error body") + } + return message, nil +} diff --git a/internal/client/client_test.go b/internal/client/client_test.go new file mode 100644 index 0000000..4d9f2cc --- /dev/null +++ b/internal/client/client_test.go @@ -0,0 +1,937 @@ +package client + +import ( + "context" + "errors" + "fmt" + "io" + "io/fs" + "log" + "net" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + "time" +) + +// What the CLI puts on the wire, and what it refuses to. + +const testToken = "ak_ZmFrZS1zZWNyZXQtZm9yLXRlc3Rz" + +// apiResponseBody is what the stand-in API serves. Distinctive on purpose, so +// a test can tell the API's answer apart from anything else that might end up +// on disk under the same name. +const apiResponseBody = "not really a png, but it is what the API sent" + +// recordingAPI is a stand-in for the spritesmith API that keeps what the CLI put on +// the wire: the one header the auth work is about, and the form fields that +// decide what shape of output comes back. +type recordingAPI struct { + authorization string + form url.Values + seen bool + status int +} + +func (r *recordingAPI) ServeHTTP(w http.ResponseWriter, req *http.Request) { + r.seen = true + r.authorization = req.Header.Get("Authorization") + // Parsed by the same multipart reader the real API uses, rather than + // scanned for as a substring of the raw body -- a body-level check would be + // satisfied by a field name appearing anywhere, including inside a prompt. + r.form = url.Values{} + if err := req.ParseMultipartForm(1 << 20); err == nil && req.MultipartForm != nil { + for name, values := range req.MultipartForm.Value { + r.form[name] = values + } + } + if r.status != 0 { + w.WriteHeader(r.status) + _, _ = w.Write([]byte(`{"error":"missing or invalid session token"}`)) + return + } + w.Header().Set("Content-Type", "image/png") + _, _ = w.Write([]byte(apiResponseBody)) +} + +func promptRequest(t *testing.T) Request { + t.Helper() + return Request{ + Mode: ModePrompt, + Prompt: "a duck", + OutputPath: filepath.Join(t.TempDir(), "out.png"), + } +} + +func TestTheTokenIsSentAsABearerCredential(t *testing.T) { + api := &recordingAPI{} + srv := httptest.NewServer(api) + t.Cleanup(srv.Close) + + if _, err := (Client{BaseURL: srv.URL, Token: testToken}).Process(promptRequest(t)); err != nil { + t.Fatal(err) + } + if want := "Bearer " + testToken; api.authorization != want { + t.Errorf("Authorization = %q, want %q", api.authorization, want) + } +} + +// No token must mean no header at all, not an empty one. A local API with no +// Clerk configuration serves unauthenticated, and `Bearer ` with nothing after +// it would be a token it has to reject. +func TestWithNoTokenNoAuthorizationHeaderIsSent(t *testing.T) { + api := &recordingAPI{} + srv := httptest.NewServer(api) + t.Cleanup(srv.Close) + + if _, err := (Client{BaseURL: srv.URL}).Process(promptRequest(t)); err != nil { + t.Fatal(err) + } + if api.authorization != "" { + t.Errorf("Authorization = %q, want no header", api.authorization) + } +} + +// A 401 comes back as the sentinel, so the command can explain it. Reporting +// the API's own message instead would be a bare 401 dump: it says "missing or +// invalid session token" on purpose, because telling bad tokens apart out loud +// is free reconnaissance. +func TestAnUnauthorizedResponseIsTheSentinel(t *testing.T) { + srv := httptest.NewServer(&recordingAPI{status: http.StatusUnauthorized}) + t.Cleanup(srv.Close) + + _, err := (Client{BaseURL: srv.URL, Token: testToken}).Process(promptRequest(t)) + if !errors.Is(err, ErrUnauthorized) { + t.Fatalf("err = %v, want ErrUnauthorized", err) + } +} + +// Every other failure keeps reporting what the API said. +func TestOtherFailuresStillReportTheAPIsMessage(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"missing required prompt"}`)) + })) + t.Cleanup(srv.Close) + + _, err := (Client{BaseURL: srv.URL}).Process(promptRequest(t)) + if err == nil || !strings.Contains(err.Error(), "missing required prompt") { + t.Fatalf("err = %v, want the API's own message", err) + } + if errors.Is(err, ErrUnauthorized) { + t.Error("a 400 was reported as unauthorized") + } +} + +// refusingTransport fails the test if it is ever asked to send anything. +type refusingTransport struct{ t *testing.T } + +func (r refusingTransport) RoundTrip(*http.Request) (*http.Response, error) { + r.t.Error("a request was sent even though the transport had been refused") + return nil, errors.New("this transport never sends") +} + +// The guard that matters most here. http:// is this CLI's default and --api is +// an ordinary string, so pointing a long-lived bearer credential at a +// plaintext host is one stale shell export away. +func TestTheTokenIsNotSentOverPlaintextToARemoteHost(t *testing.T) { + bases := []string{ + "http://api.spritesmith.ai", + "http://192.0.2.10:8080", + "http://example.com/v1", + // Not a scheme this CLI can use, and certainly not one to leak into. + "ftp://example.com", + } + + for _, base := range bases { + t.Run(base, func(t *testing.T) { + c := Client{ + BaseURL: base, + Token: testToken, + HTTPClient: &http.Client{Transport: refusingTransport{t}}, + } + + _, err := c.Process(promptRequest(t)) + if err == nil { + t.Fatal("the token was sent in the clear") + } + if strings.Contains(err.Error(), testToken) { + t.Errorf("the refusal message contains the token: %q", err) + } + // The message has to name the fix, not just the problem. + if !strings.Contains(err.Error(), "SPRITESMITH_API_TOKEN") || !strings.Contains(err.Error(), "https") { + t.Errorf("err = %q, want it to name the fix", err) + } + }) + } +} + +// And the same URLs are perfectly fine with no token, because then there is +// nothing to leak. Refusing them outright would break plaintext local setups +// that never had a credential. +func TestAPlaintextRemoteHostIsAllowedWithNoToken(t *testing.T) { + srv := httptest.NewServer(&recordingAPI{}) + t.Cleanup(srv.Close) + + // httptest is loopback, so exercise the check directly for the remote case. + if err := checkTokenTransport("http://api.spritesmith.ai"); err == nil { + t.Fatal("checkTokenTransport accepted a plaintext remote host") + } + if _, err := (Client{BaseURL: srv.URL}).Process(promptRequest(t)); err != nil { + t.Fatalf("a tokenless request over plaintext was refused: %v", err) + } +} + +// Loopback over plaintext is the local API, which is the workflow this CLI is +// used for daily. The traffic never leaves the machine. +func TestTheTokenIsSentToALoopbackAPIOverPlaintext(t *testing.T) { + for _, base := range []string{ + "http://127.0.0.1:8080", + "http://localhost:8080", + "http://[::1]:8080", + "http://LOCALHOST:8080", + } { + t.Run(base, func(t *testing.T) { + if err := checkTokenTransport(base); err != nil { + t.Errorf("checkTokenTransport(%q) = %v, want nil", base, err) + } + }) + } + + // End to end against the real local server httptest gives us. + api := &recordingAPI{} + srv := httptest.NewServer(api) + t.Cleanup(srv.Close) + if _, err := (Client{BaseURL: srv.URL, Token: testToken}).Process(promptRequest(t)); err != nil { + t.Fatal(err) + } + if api.authorization == "" { + t.Error("no token reached the local API") + } +} + +// Checking only the base URL does not hold the token safe. Go copies the +// Authorization header across a redirect to the same host or a subdomain of +// it, comparing hostnames and ignoring the scheme entirely -- so an https base +// answering `302 http://same-host/...` is a cleartext credential that passed +// every check. The guard has to run on each hop. +func TestAnHTTPSToHTTPRedirectOnTheSameHostIsRefused(t *testing.T) { + var hops atomic.Int64 + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hops.Add(1) + if r.Header.Get("Authorization") != "" && r.TLS == nil { + t.Error("the token arrived over a plaintext connection") + } + http.Redirect(w, r, "http://example.com/v1/prompt", http.StatusFound) + })) + t.Cleanup(srv.Close) + + // A transport that dials the test server whatever hostname is asked for, + // so both legs are genuinely the same host and Go's own rule to copy the + // header applies. Without this the redirect would cross hosts and net/http + // would drop the header for its own reasons rather than ours. The hostname + // is example.com because that is what httptest's certificate is issued for. + transport := srv.Client().Transport.(*http.Transport).Clone() + transport.DialContext = func(ctx context.Context, network, _ string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, network, strings.TrimPrefix(srv.URL, "https://")) + } + + c := Client{ + BaseURL: "https://example.com", + Token: testToken, + HTTPClient: &http.Client{Transport: transport}, + } + + _, err := c.Process(promptRequest(t)) + if err == nil { + t.Fatal("a downgrade redirect was followed with the token attached") + } + if !strings.Contains(err.Error(), "refusing to send the spritesmith API token") { + t.Fatalf("err = %v, want the transport refusal", err) + } + if strings.Contains(err.Error(), testToken) { + t.Errorf("the refusal message contains the token: %q", err) + } + if got := hops.Load(); got != 1 { + t.Errorf("%d requests reached the server, want 1 (the https leg only)", got) + } +} + +// The guard refuses a downgrade, not redirects. Blanket-refusing them would +// be the lazy way to close the hole above and would break any deployment that +// redirects at all -- and the 307 replay is worth pinning too, since the body +// is a bytes.Buffer and only replays because GetBody is set. +func TestARedirectToAnAllowedTargetIsStillFollowed(t *testing.T) { + final := &recordingAPI{} + target := httptest.NewServer(final) + t.Cleanup(target.Close) + + var hops atomic.Int64 + front := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hops.Add(1) + http.Redirect(w, r, target.URL+"/v1/prompt", http.StatusTemporaryRedirect) + })) + t.Cleanup(front.Close) + + if _, err := (Client{BaseURL: front.URL, Token: testToken}).Process(promptRequest(t)); err != nil { + t.Fatalf("a redirect between two allowed addresses was refused: %v", err) + } + if got := hops.Load(); got != 1 { + t.Errorf("%d requests to the redirector, want 1", got) + } + if want := "Bearer " + testToken; final.authorization != want { + t.Errorf("the redirect target got Authorization = %q, want %q", final.authorization, want) + } +} + +// Guarding redirects must not change redirect behaviour for anybody else in +// the process, which mutating http.DefaultClient in place would. +// +// Asserted against guardRedirects DIRECTLY rather than through a request, and +// with no skip. The first version of this test read http.DefaultClient and +// skipped when its CheckRedirect was already set -- which, under the very +// mutation it existed to catch, the buggy code set itself, via an earlier test +// in the same package that uses the default client. So it skipped, the package +// stayed green, and it only failed when run alone. Same shape as the +// millisecond fixture from Review 1: an assertion gated on a precondition the +// bug controls proves nothing, and CI runs packages, not single tests. +func TestGuardingRedirectsDoesNotMutateTheClientItIsGiven(t *testing.T) { + original := &http.Client{Timeout: 42 * time.Second} + + guarded := guardRedirects(original) + + if original.CheckRedirect != nil { + t.Error("the caller's own client was mutated; every other user of it now redirects differently") + } + if guarded == original { + t.Fatal("the same client was returned, so there was nothing to mutate but the caller's") + } + if guarded.CheckRedirect == nil { + t.Error("the copy carries no redirect guard, so the token is unprotected after the first hop") + } + // A copy, not a fresh client: everything else about it has to survive. + if guarded.Timeout != original.Timeout { + t.Errorf("Timeout = %v, want the caller's %v", guarded.Timeout, original.Timeout) + } +} + +// And the same fact end to end, through a real request that takes +// http.DefaultClient because no HTTPClient was supplied. That global is the +// one whose mutation would be worst, so it is worth exercising for real rather +// than only through guardRedirects. +// +// The field is reset and restored around the request. Two earlier versions of +// this test failed to catch its own mutation for want of that: the first +// SKIPPED when CheckRedirect was already set, and the second compared against +// a `before` value that the bug -- via an earlier test in this package using +// the same global -- had already set. Both times the assertion depended on +// state the bug controlled. Owning the field outright is what makes it +// order-independent. +func TestARequestWithATokenDoesNotMutateHTTPDefaultClient(t *testing.T) { + before := http.DefaultClient.CheckRedirect + http.DefaultClient.CheckRedirect = nil + t.Cleanup(func() { http.DefaultClient.CheckRedirect = before }) + + srv := httptest.NewServer(&recordingAPI{}) + t.Cleanup(srv.Close) + if _, err := (Client{BaseURL: srv.URL, Token: testToken}).Process(promptRequest(t)); err != nil { + t.Fatal(err) + } + + if http.DefaultClient.CheckRedirect != nil { + t.Error("http.DefaultClient was mutated; every other caller in the process now redirects differently") + } +} + +// Replacing CheckRedirect switches off net/http's own hop limit, so the limit +// has to be put back. Without it the CLI does not fail against a server that +// redirects in a loop -- it hangs, on an authenticated request, forever. +func TestARedirectLoopStopsRatherThanHanging(t *testing.T) { + var hops atomic.Int64 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hops.Add(1) + http.Redirect(w, r, r.URL.Path, http.StatusFound) + })) + t.Cleanup(srv.Close) + + done := make(chan error, 1) + go func() { + _, err := (Client{BaseURL: srv.URL, Token: testToken}).Process(promptRequest(t)) + done <- err + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("a redirect loop was followed to completion, which cannot happen") + } + case <-time.After(20 * time.Second): + t.Fatal("a redirect loop hung instead of stopping; the hop limit is gone") + } + if got := hops.Load(); got > 11 { + t.Errorf("%d hops before stopping, want at most 11", got) + } +} + +// Go attaches the token to a SUBDOMAIN of the original host too, over TLS, to +// a host that never had it. The spritesmith API has no reason to redirect across +// hosts, so it is refused. +func TestARedirectToAnotherHostIsRefusedWhileCarryingTheToken(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "https://evil.example.com/v1/prompt", http.StatusFound) + })) + t.Cleanup(srv.Close) + + transport := srv.Client().Transport.(*http.Transport).Clone() + transport.DialContext = func(ctx context.Context, network, _ string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, network, strings.TrimPrefix(srv.URL, "https://")) + } + + c := Client{ + BaseURL: "https://example.com", + Token: testToken, + HTTPClient: &http.Client{Transport: transport}, + } + + _, err := c.Process(promptRequest(t)) + if err == nil { + t.Fatal("the token was handed to another host by a redirect") + } + if !strings.Contains(err.Error(), "refusing to follow a redirect") { + t.Fatalf("err = %v, want the cross-host refusal", err) + } + if strings.Contains(err.Error(), testToken) { + t.Errorf("the refusal message contains the token: %q", err) + } + // The same standard the plaintext refusal is held to: name the fix, not + // just the problem. This is the refusal somebody is most likely to hit on a + // real deployment -- an apex host redirecting to the API host. + if !strings.Contains(err.Error(), "--api") || !strings.Contains(err.Error(), "SPRITESMITH_API_TOKEN") { + t.Errorf("err = %q, want it to name the fix", err) + } + // And it must not assert a mechanism that is not true here: evil.example.com + // is not a subdomain of example.com, so Go would have stripped the header + // anyway. Refusing is still right; claiming the wrong reason is not. + if strings.Contains(err.Error(), "subdomain") { + t.Errorf("the message explains this refusal with a mechanism that does not apply: %q", err) + } +} + +// A caller's own redirect policy runs first, so a policy that stops the +// redirect is honoured rather than overridden by a refusal about a hop that +// was never going to be followed -- and the token never moves either way. +func TestACallersOwnRedirectPolicyIsHonouredFirst(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "http://example.com/v1/prompt", http.StatusFound) + })) + t.Cleanup(srv.Close) + + c := Client{ + BaseURL: srv.URL, + Token: testToken, + HTTPClient: &http.Client{ + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + }, + } + + // The caller's policy stops the redirect, so the 302 comes back as the + // response. It is not a 2xx, so Process reports it -- what matters is that + // the failure is the API's answer and not our refusal about a hop nobody + // was going to take. + _, err := c.Process(promptRequest(t)) + if err != nil && strings.Contains(err.Error(), "refusing to send the spritesmith API token") { + t.Fatalf("the guard overrode the caller's own redirect policy: %v", err) + } +} + +// And https anywhere, which is what production is. +func TestTheTokenIsSentOverHTTPS(t *testing.T) { + api := &recordingAPI{} + srv := httptest.NewTLSServer(api) + t.Cleanup(srv.Close) + + c := Client{BaseURL: srv.URL, Token: testToken, HTTPClient: srv.Client()} + if _, err := c.Process(promptRequest(t)); err != nil { + t.Fatal(err) + } + if want := "Bearer " + testToken; api.authorization != want { + t.Errorf("Authorization = %q, want %q", api.authorization, want) + } +} + +// Every request keeps the legacy field for compatibility with older API +// deployments. Current APIs ignore it and always return one transparent PNG. +func TestEveryRequestAsksForThePackedSheet(t *testing.T) { + source := filepath.Join(t.TempDir(), "source.png") + if err := os.WriteFile(source, []byte("this is the source image, not the result"), 0o600); err != nil { + t.Fatal(err) + } + + requests := []Request{ + // The prompt says "sheet" so that a check which merely found the word + // somewhere in the request would pass here and prove nothing. The + // assertion below reads a parsed form field instead. + {Mode: ModePrompt, Prompt: "a duck standing on a bedsheet"}, + {Mode: ModeImage, SourcePath: source}, + } + + for _, req := range requests { + t.Run(string(req.Mode), func(t *testing.T) { + api := &recordingAPI{} + srv := httptest.NewServer(api) + t.Cleanup(srv.Close) + + req.OutputPath = filepath.Join(t.TempDir(), "out.png") + if _, err := (Client{BaseURL: srv.URL}).Process(req); err != nil { + t.Fatal(err) + } + + if got := api.form.Get("sheet"); got != "true" { + t.Errorf(`sheet = %q, want "true" for compatibility with older APIs`, got) + } + // The removed user-facing debug option must not return by accident. + if _, present := api.form["debug"]; present { + t.Errorf("the request still sends debug=%q", api.form.Get("debug")) + } + }) + } +} + +// One PNG, where it was asked for, and nothing beside it. +func TestASuccessfulRunWritesOneFileAtTheNamedPath(t *testing.T) { + srv := httptest.NewServer(&recordingAPI{}) + t.Cleanup(srv.Close) + + dir := t.TempDir() + // Deliberately not DefaultOutputPath. Code that ignored OutputPath and fell + // back to the default would still produce exactly one PNG somewhere, and a + // test that accepted "one PNG called spritesmith-alpha.png" would pass for it. + want := filepath.Join(dir, "sprite.png") + + resp, err := (Client{BaseURL: srv.URL}).Process(Request{ + Mode: ModePrompt, + Prompt: "a duck", + OutputPath: want, + }) + if err != nil { + t.Fatal(err) + } + if resp.OutputPath != want { + t.Errorf("OutputPath = %q, want %q", resp.OutputPath, want) + } + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 { + names := make([]string, 0, len(entries)) + for _, entry := range entries { + names = append(names, entry.Name()) + } + t.Fatalf("the run left %d entries %v, want exactly one PNG", len(entries), names) + } + if entries[0].Name() != "sprite.png" || entries[0].IsDir() { + t.Errorf("wrote %q (dir=%v), want the file sprite.png", entries[0].Name(), entries[0].IsDir()) + } + // The bytes have to be the API's answer. Creating a file of the right name + // is not the same as saving the result into it. + data, err := os.ReadFile(want) + if err != nil { + t.Fatal(err) + } + if string(data) != apiResponseBody { + t.Errorf("the file holds %q, want the API's response body", string(data)) + } +} + +// A result costs an OpenAI call and cannot be reproduced -- the same prompt +// does not generate the same image twice -- so an existing file is refused +// rather than overwritten. Refused BEFORE the request, too: finding out +// afterwards means paying for a generation that is then discarded. +func TestAnExistingOutputFileIsRefusedBeforeAnythingIsSent(t *testing.T) { + existing := filepath.Join(t.TempDir(), "out.png") + const precious = "yesterday's generation, which nobody can make again" + if err := os.WriteFile(existing, []byte(precious), 0o600); err != nil { + t.Fatal(err) + } + + c := Client{ + BaseURL: "http://127.0.0.1:8080", + // Fails the test if a request is sent at all. + HTTPClient: &http.Client{Transport: refusingTransport{t}}, + } + + _, err := c.Process(Request{Mode: ModePrompt, Prompt: "a duck", OutputPath: existing}) + if err == nil { + t.Fatal("an existing file was accepted as an output path") + } + if !strings.Contains(err.Error(), existing) { + t.Errorf("err = %q, want it to name the path in the way", err) + } + // Name the fix, the standard every other refusal in this package is held to. + if !strings.Contains(err.Error(), "--out") { + t.Errorf("err = %q, want it to name the fix", err) + } + + data, err := os.ReadFile(existing) + if err != nil { + t.Fatal(err) + } + if string(data) != precious { + t.Fatalf("the existing file was modified: %q", string(data)) + } +} + +// The pre-flight check saves a wasted generation; it is not the guarantee. A +// whole network round trip separates it from the write, so the write refuses +// on its own -- through the kernel, which cannot lose that race. +func TestTheWriteRefusesAFileThatAppearedAfterTheCheck(t *testing.T) { + path := filepath.Join(t.TempDir(), "out.png") + const precious = "landed here while the request was in flight" + if err := os.WriteFile(path, []byte(precious), 0o600); err != nil { + t.Fatal(err) + } + + err := writeOutput(path, strings.NewReader(apiResponseBody)) + if err == nil { + t.Fatal("the write truncated a file that appeared after the check") + } + if !strings.Contains(err.Error(), path) { + t.Errorf("err = %q, want it to name the path", err) + } + // The raw *fs.PathError from OpenFile already reads "... file exists" and + // already contains the path, so naming the path proves nothing about the + // mapping to errOutputExists. This does: only the mapped message names the + // fix, and without it the write-time refusal degrades to a bare errno while + // the pre-flight one stays helpful. + if !strings.Contains(err.Error(), "--out") { + t.Errorf("err = %q, want the same message the pre-flight refusal gives, naming the fix", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(data) != precious { + t.Fatalf("the file was overwritten: %q", string(data)) + } +} + +// A symlink at the output path is refused, which is what keeps spritesmith from +// being talked into writing a file somewhere else entirely -- a link planted +// in a world-writable directory, pointed at something the caller can write and +// did not mean to replace. +// +// It also pins the two halves of the guard to the same definition of "taken". +// A dangling symlink does not exist to Stat but does to O_EXCL, so a +// pre-flight built on Stat would call this path free and only fail later, at +// the write, having already spent the generation. +func TestASymlinkAtTheOutputPathIsRefusedAndNotFollowed(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "somewhere-else.png") + link := filepath.Join(dir, "out.png") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks are unavailable here: %v", err) + } + + if err := checkOutputPathFree(link); err == nil { + t.Error("a symlink was reported as a free output path, so the refusal comes only after the generation is paid for") + } + if err := writeOutput(link, strings.NewReader(apiResponseBody)); err == nil { + t.Fatal("the result was written through a symlink") + } + if _, err := os.Lstat(target); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("the symlink target exists, so the write followed the link: %v", err) + } +} + +// Naming a path under a directory that does not exist yet still works: the +// parent is created. Convenience, but it is existing behaviour and losing it +// silently would break anybody scripting `--out build/assets/thing.png`. +func TestAMissingParentDirectoryIsCreated(t *testing.T) { + srv := httptest.NewServer(&recordingAPI{}) + t.Cleanup(srv.Close) + + want := filepath.Join(t.TempDir(), "build", "assets", "thing.png") + if _, err := (Client{BaseURL: srv.URL}).Process(Request{ + Mode: ModePrompt, + Prompt: "a duck", + OutputPath: want, + }); err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(want) + if err != nil { + t.Fatal(err) + } + if string(data) != apiResponseBody { + t.Errorf("the file holds %q, want the API's response body", string(data)) + } +} + +// failingReader stops partway, the way a dropped connection does. +type failingReader struct{ sent bool } + +func (f *failingReader) Read(p []byte) (int, error) { + if f.sent { + return 0, errors.New("the connection dropped") + } + f.sent = true + return copy(p, []byte("half a PNG and then nothing")), nil +} + +// A download that fails partway must leave nothing behind. +// +// O_EXCL has already created the file by the time the copy starts, so without +// the removal a network hiccup leaves a truncated PNG the caller never asked +// for -- and the refusal to overwrite, seeing a file, then blocks the retry +// with a message about protecting a result that is really half a download. +// Refusing to overwrite is only safe if spritesmith never leaves a broken file. +func TestAFailedDownloadLeavesNothingBehind(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "out.png") + + err := writeOutput(path, &failingReader{}) + if err == nil { + t.Fatal("a failed copy was reported as success") + } + if !strings.Contains(err.Error(), "the connection dropped") { + t.Errorf("err = %q, want the underlying failure", err) + } + + if _, statErr := os.Lstat(path); !errors.Is(statErr, fs.ErrNotExist) { + data, _ := os.ReadFile(path) + t.Fatalf("a truncated file was left at the output path (%q); the next run would refuse to overwrite it", string(data)) + } + // Nothing else either -- not an empty file under another name, not the + // parent left holding a stray. + entries, readErr := os.ReadDir(dir) + if readErr != nil { + t.Fatal(readErr) + } + if len(entries) != 0 { + t.Errorf("the failed run left %d entries behind, want none", len(entries)) + } +} + +// And the same thing end to end, because the path that matters is the one +// Process takes: the caller must be free to simply run the command again. +func TestAFailedDownloadLeavesTheOutputPathFreeForARetry(t *testing.T) { + var attempts atomic.Int64 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "image/png") + if attempts.Add(1) == 1 { + // Promise more than is delivered, then hang up: io.Copy sees an + // unexpected EOF partway through. + w.Header().Set("Content-Length", "4096") + _, _ = w.Write([]byte("half a PNG")) + if flusher, canFlush := w.(http.Flusher); canFlush { + flusher.Flush() + } + panic(http.ErrAbortHandler) + } + _, _ = w.Write([]byte(apiResponseBody)) + })) + t.Cleanup(srv.Close) + srv.Config.ErrorLog = log.New(io.Discard, "", 0) + + path := filepath.Join(t.TempDir(), "out.png") + req := Request{Mode: ModePrompt, Prompt: "a duck", OutputPath: path} + + if _, err := (Client{BaseURL: srv.URL}).Process(req); err == nil { + t.Fatal("a truncated download was reported as success") + } + + // The retry, which is the whole point. + if _, err := (Client{BaseURL: srv.URL}).Process(req); err != nil { + t.Fatalf("the retry was refused, so the failed run wedged the output path: %v", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(data) != apiResponseBody { + t.Errorf("the file holds %q, want the retry's complete response", string(data)) + } +} + +// A 200 that is not a PNG must not be written. Accept is advisory, so a +// captive portal or a proxy error page answering 200 with HTML would otherwise +// land in spritesmith-alpha.png as a perfectly valid file that is not an image -- +// and, the path now being taken, the retry would be refused too. +func TestANonPNGSuccessResponseIsRefusedAndNotWritten(t *testing.T) { + for _, answer := range []struct { + name string + contentType string + body string + }{ + {"html", "text/html; charset=utf-8", "sign in to this network"}, + {"json", "application/json", `{"status":"ok"}`}, + {"none", "", "who knows"}, + } { + t.Run(answer.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if answer.contentType != "" { + w.Header().Set("Content-Type", answer.contentType) + } else { + // Go sniffs a type when none is set, so unset it outright. + w.Header()["Content-Type"] = nil + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(answer.body)) + })) + t.Cleanup(srv.Close) + + dir := t.TempDir() + path := filepath.Join(dir, "out.png") + + _, err := (Client{BaseURL: srv.URL}).Process(Request{ + Mode: ModePrompt, + Prompt: "a duck", + OutputPath: path, + }) + if err == nil { + t.Fatal("a non-PNG 200 was accepted as a result") + } + if _, statErr := os.Lstat(path); !errors.Is(statErr, fs.ErrNotExist) { + data, _ := os.ReadFile(path) + t.Errorf("the non-PNG body was written to disk: %q", string(data)) + } + entries, readErr := os.ReadDir(dir) + if readErr != nil { + t.Fatal(readErr) + } + if len(entries) != 0 { + t.Errorf("%d entries were left behind, want none", len(entries)) + } + }) + } +} + +// The real API serves the sheet through gin's c.File, which sets a bare +// image/png; a parameter on it is still a PNG and must not be refused. +func TestAPNGContentTypeWithParametersIsAccepted(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "image/png; name=alpha.png") + _, _ = w.Write([]byte(apiResponseBody)) + })) + t.Cleanup(srv.Close) + + path := filepath.Join(t.TempDir(), "out.png") + if _, err := (Client{BaseURL: srv.URL}).Process(Request{ + Mode: ModePrompt, + Prompt: "a duck", + OutputPath: path, + }); err != nil { + t.Fatalf("a PNG with a content-type parameter was refused: %v", err) + } +} + +// --out meant a directory until spritesmith collapsed to one output, so the old +// command is still in shell histories. Caught before the request it costs +// nothing; caught at the write it costs a generation and reports a bare errno. +func TestADirectoryShapedOutIsRefusedBeforeAnythingIsSent(t *testing.T) { + existing := t.TempDir() + + for _, path := range []string{ + // Does not exist yet, so only the trailing separator gives it away. + filepath.Join(t.TempDir(), "spritesmith-out") + string(os.PathSeparator), + // Exists, and is a directory. + existing, + } { + t.Run(path, func(t *testing.T) { + c := Client{ + BaseURL: "http://127.0.0.1:8080", + HTTPClient: &http.Client{Transport: refusingTransport{t}}, + } + + _, err := c.Process(Request{Mode: ModePrompt, Prompt: "a duck", OutputPath: path}) + if err == nil { + t.Fatal("a directory was accepted as the output path") + } + // The message has to say what is wrong with it. "already exists, + // and spritesmith will not overwrite it. Move or delete that file" is + // the wrong noun and the wrong advice for a directory. + if !strings.Contains(err.Error(), "directory") { + t.Errorf("err = %q, want it to say the path is a directory", err) + } + if !strings.Contains(err.Error(), "--out") { + t.Errorf("err = %q, want it to name the fix", err) + } + }) + } +} + +// An empty or whitespace-only OutputPath falls back to the default rather than +// resolving to ".", which filepath treats a bare "" as and which is a +// directory, not somewhere a PNG can be written. +func TestAnEmptyOutputPathFallsBackToTheDefaultFile(t *testing.T) { + srv := httptest.NewServer(&recordingAPI{}) + t.Cleanup(srv.Close) + + for _, given := range []string{"", " "} { + t.Run(fmt.Sprintf("%q", given), func(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + resp, err := (Client{BaseURL: srv.URL}).Process(Request{ + Mode: ModePrompt, + Prompt: "a duck", + OutputPath: given, + }) + if err != nil { + t.Fatal(err) + } + if resp.OutputPath != DefaultOutputPath { + t.Errorf("OutputPath = %q, want %q", resp.OutputPath, DefaultOutputPath) + } + if _, statErr := os.Lstat(filepath.Join(dir, DefaultOutputPath)); statErr != nil { + t.Errorf("nothing was written to the default path: %v", statErr) + } + }) + } + + // A path somebody actually gave is used exactly as given -- not trimmed, + // because a trailing space in a filename is legal and not ours to remove. + // The space is at the very END of the path on purpose: an earlier version + // of this fixture used "sprite .png", where the space is interior and + // TrimSpace would not have touched it, so the assertion held under the + // exact mutation it exists to catch. + dir := t.TempDir() + odd := filepath.Join(dir, "sprite.png") + " " + resp, err := (Client{BaseURL: srv.URL}).Process(Request{ + Mode: ModePrompt, + Prompt: "a duck", + OutputPath: odd, + }) + if err != nil { + t.Fatal(err) + } + if resp.OutputPath != odd { + t.Errorf("OutputPath = %q, want the path exactly as given, %q", resp.OutputPath, odd) + } +} + +// The default name is a decision, not an accident: it is what the app saves +// (RESULT_NAME in app/src/lib/spritesmith-request.ts), so a result is the same thing +// wherever it came from. cli/.gitignore hardcodes it too, and would go quietly +// stale if the constant moved -- `make cli` runs from cli/, so the default +// lands in the source tree. +func TestTheDefaultOutputNameIsTheOneEverythingElseExpects(t *testing.T) { + if DefaultOutputPath != "spritesmith-alpha.png" { + t.Errorf("DefaultOutputPath = %q; the app saves spritesmith-alpha.png and a result should be the same thing wherever it came from", DefaultOutputPath) + } + + ignore, err := os.ReadFile(filepath.Join("..", "..", ".gitignore")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(ignore), DefaultOutputPath) { + t.Errorf("cli/.gitignore does not list %q, so `make cli` leaves a result in the source tree:\n%s", DefaultOutputPath, ignore) + } +} diff --git a/internal/client/jobs_test.go b/internal/client/jobs_test.go new file mode 100644 index 0000000..3f39b12 --- /dev/null +++ b/internal/client/jobs_test.go @@ -0,0 +1,76 @@ +package client + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "testing" +) + +func TestProcessRetrievesDurableGenerationWithoutResubmission(t *testing.T) { + posts := 0 + id := strings.Repeat("a", 32) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/v1/prompt": + posts++ + if r.Header.Get("Idempotency-Key") == "" { + t.Error("missing logical operation key") + } + w.WriteHeader(202) + json.NewEncoder(w).Encode(map[string]string{"id": id}) + case "/v1/generations/" + id: + json.NewEncoder(w).Encode(map[string]string{"status": "succeeded"}) + case "/v1/generations/" + id + "/result": + w.Header().Set("Content-Type", "image/png") + io.WriteString(w, "synthetic-png") + default: + http.NotFound(w, r) + } + })) + defer server.Close() + _, err := (Client{BaseURL: server.URL}).Process(Request{Mode: ModePrompt, Prompt: "fixture", OutputPath: filepath.Join(t.TempDir(), "result.png")}) + if err != nil || posts != 1 { + t.Fatalf("job flow: %v posts=%d", err, posts) + } +} + +func TestVectorPromptDownloadsSVGWithoutResubmission(t *testing.T) { + posts := 0 + id := strings.Repeat("b", 32) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer fixture-token" { + t.Error("missing authentication") + } + switch r.URL.Path { + case "/v1/prompt": + posts++ + if err := r.ParseMultipartForm(1 << 20); err != nil { + t.Error(err) + } + if r.FormValue("kind") != "vector" || r.FormValue("prompt") != "a triangle" { + t.Error("vector prompt missing") + } + w.WriteHeader(202) + json.NewEncoder(w).Encode(map[string]string{"id": id}) + case "/v1/generations/" + id: + json.NewEncoder(w).Encode(map[string]string{"status": "succeeded"}) + case "/v1/generations/" + id + "/result": + if r.URL.Query().Get("format") != "svg" { + t.Error("wrong artifact requested") + } + w.Header().Set("Content-Type", "image/svg+xml") + io.WriteString(w, ``) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + _, err := (Client{BaseURL: server.URL, Token: "fixture-token"}).Process(Request{Mode: ModePrompt, Kind: "vector", Format: "svg", Prompt: "a triangle", OutputPath: filepath.Join(t.TempDir(), "sprite.svg")}) + if err != nil || posts != 1 { + t.Fatalf("vector flow: %v posts=%d", err, posts) + } +} diff --git a/internal/token/token.go b/internal/token/token.go new file mode 100644 index 0000000..f0d3142 --- /dev/null +++ b/internal/token/token.go @@ -0,0 +1,202 @@ +// Package token finds the credential the CLI presents to the spritesmith API. +// +// The credential is a Clerk API key: an opaque `ak_...` secret created against +// one spritesmith account, which the API verifies with Clerk on every request. It +// is long-lived and it is not scoped down -- whoever holds it is that person as +// far as the API is concerned -- so everything here is arranged around not +// leaking it: +// +// - it is never returned in an error, a warning, or anything else this +// package produces. Messages name the SOURCE and never the value. +// - it is never accepted on the command line, which is why there is no +// --token flag. An argument is in the shell history of everyone who typed +// it and in the process table of everyone on the machine. +// - the file it can live in is checked for being readable by other users, +// which is the one thing about it this package can see. +// +// Two sources, in order: the SPRITESMITH_API_TOKEN environment variable, then a +// file under the user's config directory. Environment first because that is +// what a shell, a CI job, or a `direnv` sets, and it must be able to override +// whatever is on disk for one invocation. +package token + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" +) + +// EnvVar is the environment variable holding the token. Named here so the help +// text, the error messages and the loader cannot disagree about it. +const EnvVar = "SPRITESMITH_API_TOKEN" + +// dirName and fileName locate the token file: $XDG_CONFIG_HOME/spritesmith/token on +// Linux, ~/Library/Application Support/spritesmith/token on macOS. +const ( + dirName = "spritesmith" + fileName = "token" +) + +// maxLength bounds what will be accepted as a token. Clerk's API key secrets +// are a few dozen characters. A file that is much larger is a file that is not +// a token -- a stray log, a whole config, a private key pasted by mistake -- +// and sending it as a header is neither going to work nor worth doing. +// +// Deliberately the same number as the API's own maxMachineTokenLength +// (api/internal/clerkauth/machine.go), which cannot be referenced from here -- +// separate modules -- so both are pinned by a literal in their own tests. +// When the CLI's bound was the looser of the two, a value between them was +// accepted here, sent, refused there, and reported back as "the API rejected +// your token, it may have been revoked" -- a confident wrong diagnosis for a +// value that was never a token. Refusing it locally says what is wrong. +// +// The two bounds are the same NUMBER and not the same SET, which is worth +// knowing before changing either. The API's applies only to `ak_` machine +// tokens; this one applies to whatever is in SPRITESMITH_API_TOKEN or the token +// file. A Clerk SESSION token pasted there -- 600-900 bytes of JWT -- is +// therefore refused here rather than being forwarded. That is intended: a +// session token lives about a minute and is useless to a CLI, so failing at +// once with "that is not a token" beats a 401 sixty seconds later. +const maxLength = 256 + +// Token is a credential and where it was found. +type Token struct { + // Value is the token itself, or "" when there is none. An absent token is + // not an error: a local API with no Clerk configuration serves without one, + // which is what makes `make cli` work on a laptop with nothing set up. + Value string + // Source describes where Value came from, for messages. Never the value. + Source string + // Warning is a problem worth saying out loud that is not worth refusing + // over. Empty when there is none, and never contains the value. + Warning string +} + +// Loader reads the token. The two dependencies are fields rather than direct +// calls so the tests can exercise every branch without depending on the host's +// idea of a config directory, which differs per platform. +type Loader struct { + Getenv func(string) string + ConfigDir func() (string, error) +} + +// New returns a Loader reading the real environment and config directory. +func New() Loader { + return Loader{Getenv: os.Getenv, ConfigDir: os.UserConfigDir} +} + +// Path is where the token file lives. Reported in help and in the error a +// missing token produces, so somebody is told exactly where to put one. +func (l Loader) Path() (string, error) { + dir, err := l.ConfigDir() + if err != nil { + return "", err + } + return filepath.Join(dir, dirName, fileName), nil +} + +// PathOrHint is Path, degraded to a description when the platform will not say +// where its config directory is. Used in help text, which must always render. +func (l Loader) PathOrHint() string { + path, err := l.Path() + if err != nil { + return filepath.Join("", dirName, fileName) + } + return path +} + +// Load returns the token, or an empty one when there is none. +// +// An error means a token was found and is not usable -- not that none was +// found. The difference matters: a missing token is normal against a dev API, +// while a malformed one is a mistake that would otherwise surface as an +// unexplained 401 much later. +func (l Loader) Load() (Token, error) { + if value := strings.TrimSpace(l.Getenv(EnvVar)); value != "" { + source := "$" + EnvVar + if err := validate(value, source); err != nil { + return Token{}, err + } + return Token{Value: value, Source: source, Warning: l.shadowedFileWarning()}, nil + } + + path, err := l.Path() + if err != nil { + // No config directory on this platform or no HOME. Not fatal: the + // environment variable is still a way in, and no token at all is a + // legitimate state. + return Token{}, nil + } + + data, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return Token{}, nil + } + if err != nil { + return Token{}, fmt.Errorf("read the spritesmith API token from %s: %w", path, err) + } + + value := strings.TrimSpace(string(data)) + if value == "" { + return Token{}, nil + } + source := path + if err := validate(value, source); err != nil { + return Token{}, err + } + return Token{Value: value, Source: source, Warning: permissionWarning(path)}, nil +} + +// shadowedFileWarning says that a token file exists and is being ignored. +// +// Without it, somebody who rotates the file and keeps failing has no way to +// see that a variable set in a shell profile months ago is what the API is +// actually rejecting. +func (l Loader) shadowedFileWarning() string { + path, err := l.Path() + if err != nil { + return "" + } + if _, err := os.Stat(path); err != nil { + return "" + } + return fmt.Sprintf("%s is being ignored because %s is set", path, EnvVar) +} + +// permissionWarning reports a token file other users on the machine can read. +// +// A warning rather than a refusal. On a single-user laptop a 644 token file is +// a bad habit and not a breach, and a CLI that stops working because of the +// umask that wrote the file -- with no way to override -- would be worse than +// the problem. The message names the fix so it is one command to clear. +func permissionWarning(path string) string { + info, err := os.Stat(path) + if err != nil { + return "" + } + if info.Mode().Perm()&0o077 == 0 { + return "" + } + return fmt.Sprintf("%s is readable by other users on this machine; run: chmod 600 %s", path, path) +} + +// validate refuses a value that cannot be a working token, naming the source +// and never the value. +func validate(value, source string) error { + if len(value) > maxLength { + return fmt.Errorf("the spritesmith API token in %s is %d bytes; that is not a token", source, len(value)) + } + // A header value cannot contain spaces, tabs, newlines or control bytes, + // and Go's HTTP client refuses one that does with an error that says + // nothing about where the value came from. Caught here instead: this is + // what a file with two lines in it, or a copy-paste that took the + // surrounding quotes and a newline, actually looks like. + for _, r := range value { + if r <= ' ' || r == 0x7f { + return fmt.Errorf("the spritesmith API token in %s contains whitespace or control characters; it should be a single line", source) + } + } + return nil +} diff --git a/internal/token/token_test.go b/internal/token/token_test.go new file mode 100644 index 0000000..b28ddbe --- /dev/null +++ b/internal/token/token_test.go @@ -0,0 +1,332 @@ +package token + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Where the credential comes from, and -- more importantly -- everything this +// package must never say out loud about it. + +const secret = "ak_ZmFrZS1zZWNyZXQtZm9yLXRlc3Rz" + +// loaderIn returns a Loader whose config directory is a temporary one and +// whose environment is whatever the test hands it. Nothing here touches the +// real HOME, which differs per platform and belongs to whoever is running the +// suite. +func loaderIn(t *testing.T, env map[string]string) (Loader, string) { + t.Helper() + dir := t.TempDir() + return Loader{ + Getenv: func(key string) string { return env[key] }, + ConfigDir: func() (string, error) { return dir, nil }, + }, dir +} + +func writeTokenFile(t *testing.T, loader Loader, contents string, mode os.FileMode) string { + t.Helper() + path, err := loader.Path() + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(contents), mode); err != nil { + t.Fatal(err) + } + // WriteFile is subject to the umask, so ask for the mode explicitly. + if err := os.Chmod(path, mode); err != nil { + t.Fatal(err) + } + return path +} + +func TestTheEnvironmentVariableIsRead(t *testing.T) { + loader, _ := loaderIn(t, map[string]string{EnvVar: secret}) + + got, err := loader.Load() + if err != nil { + t.Fatal(err) + } + if got.Value != secret { + t.Errorf("value = %q, want the token", got.Value) + } + if got.Source != "$"+EnvVar { + t.Errorf("source = %q, want the variable name", got.Source) + } +} + +// Surrounding whitespace is what a copy-paste and a here-doc both produce. +func TestSurroundingWhitespaceIsTrimmed(t *testing.T) { + loader, _ := loaderIn(t, map[string]string{EnvVar: " " + secret + "\n"}) + + got, err := loader.Load() + if err != nil { + t.Fatal(err) + } + if got.Value != secret { + t.Errorf("value = %q, want the token with no surrounding whitespace", got.Value) + } +} + +func TestTheTokenFileIsRead(t *testing.T) { + loader, _ := loaderIn(t, nil) + path := writeTokenFile(t, loader, secret+"\n", 0o600) + + got, err := loader.Load() + if err != nil { + t.Fatal(err) + } + if got.Value != secret { + t.Errorf("value = %q, want the token", got.Value) + } + if got.Source != path { + t.Errorf("source = %q, want %q", got.Source, path) + } + if got.Warning != "" { + t.Errorf("warning = %q, want none for a 0600 file", got.Warning) + } +} + +// The environment has to win, or a variable set for one invocation could not +// override a file, which is the whole point of having both. +func TestTheEnvironmentWinsOverTheFile(t *testing.T) { + loader, _ := loaderIn(t, map[string]string{EnvVar: secret}) + path := writeTokenFile(t, loader, "ak_the-file-one", 0o600) + + got, err := loader.Load() + if err != nil { + t.Fatal(err) + } + if got.Value != secret { + t.Errorf("value = %q, want the environment's token", got.Value) + } + // And it has to SAY so. Somebody who rotates the file and keeps failing + // otherwise has no way to see that a shell profile from months ago is what + // the API is actually rejecting. + if !strings.Contains(got.Warning, path) || !strings.Contains(got.Warning, EnvVar) { + t.Errorf("warning = %q, want it to name the ignored file and the variable", got.Warning) + } +} + +// An empty variable is a variable that is not set. Falling through means +// `SPRITESMITH_API_TOKEN= spritesmith ...` does not silently mean "no token" while a +// perfectly good file sits there. +func TestAnEmptyEnvironmentVariableFallsThroughToTheFile(t *testing.T) { + loader, _ := loaderIn(t, map[string]string{EnvVar: " "}) + writeTokenFile(t, loader, secret, 0o600) + + got, err := loader.Load() + if err != nil { + t.Fatal(err) + } + if got.Value != secret { + t.Errorf("value = %q, want the file's token", got.Value) + } +} + +// No token anywhere is not an error. A local API with no Clerk configuration +// serves without one, and that is what makes `make cli` work on a laptop with +// nothing set up. +func TestNoTokenAnywhereIsNotAnError(t *testing.T) { + loader, _ := loaderIn(t, nil) + + got, err := loader.Load() + if err != nil { + t.Fatalf("a missing token was an error: %v", err) + } + if got.Value != "" || got.Source != "" { + t.Errorf("got %+v, want an empty token", got) + } +} + +func TestAnEmptyTokenFileIsTreatedAsNoToken(t *testing.T) { + loader, _ := loaderIn(t, nil) + writeTokenFile(t, loader, "\n\n \n", 0o600) + + got, err := loader.Load() + if err != nil { + t.Fatalf("an empty token file was an error: %v", err) + } + if got.Value != "" { + t.Errorf("value = %q, want empty", got.Value) + } +} + +// A token file other users can read is the one thing about the credential this +// package can actually see. Warned rather than refused -- see permissionWarning +// -- but it must be warned. +func TestAWorldReadableTokenFileIsWarnedAbout(t *testing.T) { + loader, _ := loaderIn(t, nil) + path := writeTokenFile(t, loader, secret, 0o644) + + got, err := loader.Load() + if err != nil { + t.Fatal(err) + } + if got.Value != secret { + t.Error("the token was not returned; this is a warning, not a refusal") + } + if !strings.Contains(got.Warning, path) || !strings.Contains(got.Warning, "chmod 600") { + t.Errorf("warning = %q, want it to name the file and the fix", got.Warning) + } +} + +func TestAGroupReadableTokenFileIsWarnedAbout(t *testing.T) { + loader, _ := loaderIn(t, nil) + writeTokenFile(t, loader, secret, 0o640) + + got, err := loader.Load() + if err != nil { + t.Fatal(err) + } + if got.Warning == "" { + t.Error("a group-readable token file produced no warning") + } +} + +// Refused, because these produce an HTTP error from deep inside net/http that +// says nothing about where the value came from -- and because a file with two +// lines in it is a mistake worth naming. +func TestATokenThatCannotBeAHeaderIsRefused(t *testing.T) { + tests := []struct { + name string + value string + }{ + {"two lines", "ak_one\nak_two"}, + {"an embedded space", "ak_one two"}, + {"a tab", "ak_one\tak_two"}, + {"a control byte", "ak_one\x01two"}, + {"a delete byte", "ak_one\x7ftwo"}, + {"far too long to be a token", strings.Repeat("a", maxLength+1)}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + loader, _ := loaderIn(t, map[string]string{EnvVar: tc.value}) + + got, err := loader.Load() + if err == nil { + t.Fatalf("this was accepted as a token: %+v", got) + } + if got.Value != "" { + t.Error("a refused token was still returned") + } + // The message has to be usable without being a leak. + if !strings.Contains(err.Error(), EnvVar) { + t.Errorf("err = %q, want it to name the source", err) + } + }) + } +} + +// The bound is pinned to a LITERAL size, not to maxLength, because the point +// of the number is that it agrees with the API's own maxMachineTokenLength. +// A test written as maxLength+1 follows the constant wherever it goes and +// cannot notice the two drifting apart -- which is how a 300-byte value came +// to be accepted here, sent, refused by the API as implausibly long, and +// reported back to the user as a revoked or expired token. +func TestAValueTheAPIWouldRefuseIsRefusedLocallyInstead(t *testing.T) { + loader, _ := loaderIn(t, map[string]string{EnvVar: "ak_" + strings.Repeat("a", 300)}) + + _, err := loader.Load() + if err == nil { + t.Fatal("a 303-byte value was accepted; the API refuses anything over 256 and the user is told their token was revoked") + } + if !strings.Contains(err.Error(), "not a token") { + t.Errorf("err = %q, want it to say this is not a token", err) + } +} + +// The central promise of this package. Nothing it produces -- value aside -- +// may contain the credential, because all of it is printed to a terminal. +func TestNothingThisPackageProducesEverContainsTheToken(t *testing.T) { + t.Run("the warning on a loose file", func(t *testing.T) { + loader, _ := loaderIn(t, nil) + writeTokenFile(t, loader, secret, 0o644) + got, err := loader.Load() + if err != nil { + t.Fatal(err) + } + if strings.Contains(got.Warning, secret) { + t.Errorf("the warning contains the token: %q", got.Warning) + } + if strings.Contains(got.Source, secret) { + t.Errorf("the source contains the token: %q", got.Source) + } + }) + + t.Run("the warning on a shadowed file", func(t *testing.T) { + loader, _ := loaderIn(t, map[string]string{EnvVar: secret}) + writeTokenFile(t, loader, secret, 0o600) + got, err := loader.Load() + if err != nil { + t.Fatal(err) + } + if strings.Contains(got.Warning, secret) { + t.Errorf("the warning contains the token: %q", got.Warning) + } + }) + + t.Run("the error on a malformed token", func(t *testing.T) { + malformed := secret + " " + secret + loader, _ := loaderIn(t, map[string]string{EnvVar: malformed}) + _, err := loader.Load() + if err == nil { + t.Fatal("a malformed token was accepted") + } + if strings.Contains(err.Error(), secret) { + t.Errorf("the error contains the token: %q", err) + } + }) + + t.Run("the error on an unreadable file", func(t *testing.T) { + loader, _ := loaderIn(t, nil) + path := writeTokenFile(t, loader, secret, 0o000) + if os.Geteuid() == 0 { + t.Skip("root reads a 0000 file regardless") + } + _, err := loader.Load() + if err == nil { + t.Fatal("an unreadable token file was not an error") + } + if !strings.Contains(err.Error(), path) { + t.Errorf("err = %q, want it to name the file", err) + } + if strings.Contains(err.Error(), secret) { + t.Errorf("the error contains the token: %q", err) + } + }) +} + +func TestThePathIsUnderTheConfigDirectory(t *testing.T) { + loader, dir := loaderIn(t, nil) + + path, err := loader.Path() + if err != nil { + t.Fatal(err) + } + if want := filepath.Join(dir, dirName, fileName); path != want { + t.Errorf("path = %q, want %q", path, want) + } +} + +// Help text has to render on a machine with no config directory, or `--help` +// would fail for exactly the person who needs to read it. +func TestThePathHintSurvivesAPlatformWithNoConfigDirectory(t *testing.T) { + loader := Loader{ + Getenv: func(string) string { return "" }, + ConfigDir: func() (string, error) { return "", os.ErrNotExist }, + } + + if hint := loader.PathOrHint(); hint == "" || !strings.Contains(hint, fileName) { + t.Errorf("hint = %q, want something naming the token file", hint) + } + // And Load must not fail either: the environment variable still works. + if _, err := loader.Load(); err != nil { + t.Errorf("Load failed with no config directory: %v", err) + } +} diff --git a/scripts/build.sh b/scripts/build.sh new file mode 100644 index 0000000..c74db17 --- /dev/null +++ b/scripts/build.sh @@ -0,0 +1,36 @@ +#!/usr/bin/env bash +set -euo pipefail +cd "$(dirname "${BASH_SOURCE[0]}")/.." +VERSION="${VERSION:-dev}" +[[ "$VERSION" == dev || "$VERSION" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]] || { echo 'Invalid release version'; exit 1; } +revision="$(git rev-parse HEAD)" +mkdir -p dist +for target in linux/amd64 linux/arm64 darwin/amd64 darwin/arm64 windows/amd64; do + target_os="${target%/*}" + target_arch="${target#*/}" + binary=spritesmith + [[ "$target_os" != windows ]] || binary=spritesmith.exe + staging="$(mktemp -d)" + CGO_ENABLED=0 GOOS="$target_os" GOARCH="$target_arch" go build -trimpath \ + -ldflags "-X main.version=$VERSION -X main.revision=$revision" -o "$staging/$binary" ./cmd/spritesmith + cp README.md "$staging/README.md" + archive="spritesmith_${target_os}_${target_arch}" + if [[ "$target_os" == windows ]]; then + python3 - "$staging" "dist/$archive.zip" <<'PY' +from pathlib import Path +import sys, zipfile +with zipfile.ZipFile(sys.argv[2], 'w', zipfile.ZIP_DEFLATED) as archive: + for path in sorted(Path(sys.argv[1]).iterdir()): + archive.write(path, path.name) +PY + else + tar -czf "dist/$archive.tar.gz" -C "$staging" "$binary" README.md + fi + rm -rf "$staging" +done +(cd dist && sha256sum -- *.tar.gz *.zip > checksums.txt) +python3 - "$revision" "$VERSION" <<'PY' +import json, sys +from pathlib import Path +Path('dist/release.json').write_text(json.dumps({'revision':sys.argv[1], 'version':sys.argv[2]}, indent=2)+'\n') +PY diff --git a/scripts/check-artifacts.py b/scripts/check-artifacts.py new file mode 100644 index 0000000..fb37205 --- /dev/null +++ b/scripts/check-artifacts.py @@ -0,0 +1,42 @@ +#!/usr/bin/env python3 +"""Check distributable contents, checksums, revision and native CLI behavior.""" +import hashlib +import json +from pathlib import Path +import subprocess +import tarfile +import tempfile +import zipfile + +root = Path('dist') +expected = {f'spritesmith_{os}_{arch}.{suffix}' for os, arch, suffix in ( + ('linux', 'amd64', 'tar.gz'), ('linux', 'arm64', 'tar.gz'), + ('darwin', 'amd64', 'tar.gz'), ('darwin', 'arm64', 'tar.gz'), ('windows', 'amd64', 'zip'))} +checksums = {} +for line in (root/'checksums.txt').read_text().splitlines(): + digest, name = line.split() + if name in checksums: raise SystemExit('duplicate archive identity') + checksums[name] = digest +if set(checksums) != expected: raise SystemExit('wrong release artifact set') +for name, digest in checksums.items(): + if hashlib.sha256((root/name).read_bytes()).hexdigest() != digest: + raise SystemExit('archive digest mismatch') + binary = 'spritesmith.exe' if name.endswith('.zip') else 'spritesmith' + if name.endswith('.zip'): + with zipfile.ZipFile(root/name) as archive: + names = archive.namelist() + else: + with tarfile.open(root/name) as archive: + names = archive.getnames() + if any(not item.isfile() for item in archive.getmembers()): + raise SystemExit('non-file archive member') + if sorted(names) != sorted([binary, 'README.md']): raise SystemExit('unexpected distributable content') +revision = subprocess.check_output(['git', 'rev-parse', 'HEAD'], text=True).strip() +release = json.loads((root/'release.json').read_text()) +if release['revision'] != revision: raise SystemExit('artifacts belong to another source revision') +with tempfile.TemporaryDirectory() as temp: + with tarfile.open(root/'spritesmith_linux_amd64.tar.gz') as archive: + archive.extractall(temp, filter='data') + output = subprocess.check_output([str(Path(temp)/'spritesmith'), '--version'], text=True) + if revision not in output: raise SystemExit('binary revision differs') +print('Five platform archives, checksums and source identity verified') diff --git a/scripts/lint.sh b/scripts/lint.sh new file mode 100644 index 0000000..58594d1 --- /dev/null +++ b/scripts/lint.sh @@ -0,0 +1,12 @@ +#!/usr/bin/env bash +set -euo pipefail +cd "$(dirname "${BASH_SOURCE[0]}")/.." +test -z "$(gofmt -l cmd internal)" || { echo 'Go formatting differs'; exit 1; } +go vet ./... +# The public client must install without any private source dependency. +python3 - <<'PY' +from pathlib import Path +text = Path('go.mod').read_text() +if 'spritesmith-be' in text or 'cazper-core' in text or 'replace ' in text: + raise SystemExit('Public client depends on private source') +PY