From 24367cf3c12cd12c1ed1e497be61f5b6d9bfcd3b Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Tue, 9 Jun 2026 17:07:47 +0300 Subject: [PATCH 1/6] Add input validation against malformed inputs --- cmd/root.go | 10 ++ internal/config/config.go | 20 ++++ internal/config/env_validation_test.go | 52 ++++++++++ internal/snapshot/destination.go | 9 +- internal/snapshot/destination_test.go | 25 +++++ internal/validate/validate.go | 128 +++++++++++++++++++++++++ internal/validate/validate_test.go | 108 +++++++++++++++++++++ 7 files changed, 347 insertions(+), 5 deletions(-) create mode 100644 internal/config/env_validation_test.go create mode 100644 internal/validate/validate.go create mode 100644 internal/validate/validate_test.go diff --git a/cmd/root.go b/cmd/root.go index d056bdd2..5aacfe7e 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -24,6 +24,7 @@ import ( "github.com/localstack/lstk/internal/tracing" "github.com/localstack/lstk/internal/ui" "github.com/localstack/lstk/internal/update" + "github.com/localstack/lstk/internal/validate" "github.com/localstack/lstk/internal/version" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -260,6 +261,15 @@ func Execute(ctx context.Context) error { resolvedToken = token } } + // Trim surrounding whitespace: env-injected tokens (e.g. CI secrets) commonly + // carry a trailing newline. Then reject clearly malformed tokens before they + // reach the platform API, telemetry, or the container environment. + resolvedToken = strings.TrimSpace(resolvedToken) + if err := validate.AuthToken(resolvedToken); err != nil { + err = fmt.Errorf("invalid auth token: %w", err) + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + return err + } cfg.AuthToken = resolvedToken tel.SetAuthToken(resolvedToken) diff --git a/internal/config/config.go b/internal/config/config.go index 8a9fb544..e95040b2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -9,6 +9,7 @@ import ( "regexp" "strings" + "github.com/localstack/lstk/internal/validate" "github.com/pelletier/go-toml/v2" "github.com/spf13/viper" ) @@ -184,5 +185,24 @@ func Get() (*Config, error) { return nil, fmt.Errorf("invalid container config: %w", err) } } + if err := validateNamedEnvs(cfg.Env); err != nil { + return nil, err + } return &cfg, nil } + +// validateNamedEnvs rejects malformed variables defined in the top-level [env.*] +// config sections before they are injected into a container's environment. +func validateNamedEnvs(envs map[string]map[string]string) error { + for name, vars := range envs { + for key, value := range vars { + if err := validate.EnvVarName(key); err != nil { + return fmt.Errorf("invalid variable in [env.%s]: %w", name, err) + } + if err := validate.NoControlChars("value for "+key, value); err != nil { + return fmt.Errorf("invalid variable in [env.%s]: %w", name, err) + } + } + } + return nil +} diff --git a/internal/config/env_validation_test.go b/internal/config/env_validation_test.go new file mode 100644 index 00000000..b69f0e18 --- /dev/null +++ b/internal/config/env_validation_test.go @@ -0,0 +1,52 @@ +package config + +import "testing" + +func TestValidateNamedEnvs(t *testing.T) { + t.Parallel() + tests := []struct { + name string + envs map[string]map[string]string + wantErr bool + }{ + {"nil", nil, false}, + {"empty", map[string]map[string]string{}, false}, + { + name: "valid", + envs: map[string]map[string]string{ + "debug": {"ls_log": "trace", "debug": "1"}, + "ci": {"services": "s3,sqs"}, + }, + wantErr: false, + }, + { + name: "control char in value", + envs: map[string]map[string]string{"bad": {"debug": "1\x00"}}, + wantErr: true, + }, + { + name: "hyphen in key", + envs: map[string]map[string]string{"bad": {"my-key": "1"}}, + wantErr: true, + }, + { + name: "equals in key", + envs: map[string]map[string]string{"bad": {"a=b": "1"}}, + wantErr: true, + }, + { + name: "key starts with digit", + envs: map[string]map[string]string{"bad": {"1var": "1"}}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := validateNamedEnvs(tt.envs) + if (err != nil) != tt.wantErr { + t.Errorf("validateNamedEnvs() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/internal/snapshot/destination.go b/internal/snapshot/destination.go index 22939a1c..2b97eb7a 100644 --- a/internal/snapshot/destination.go +++ b/internal/snapshot/destination.go @@ -7,9 +7,10 @@ import ( "net/url" "os" "path/filepath" - "regexp" "strings" "time" + + "github.com/localstack/lstk/internal/validate" ) // ErrHomeNotSet is returned when a path needs "~" expansion but no home directory was provided. @@ -24,8 +25,6 @@ var ( // ErrCredentialsInS3URL is returned when an s3:// ref embeds credential query // params. Credentials must come from the environment or --profile, never the URL. ErrCredentialsInS3URL = errors.New("do not put credentials in the s3:// URL; use AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY or --profile") - - validPodName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]*$`) ) const ( @@ -70,8 +69,8 @@ func IsS3Ref(ref string) bool { // ValidatePodName validates a user-supplied pod name (the identity of a snapshot // on a remote), using the same rules as pod: refs. func ValidatePodName(name string) error { - if !validPodName.MatchString(name) { - return fmt.Errorf("invalid pod name %q: use letters, digits, hyphens, and underscores only, starting with a letter or digit", name) + if err := validate.ResourceName("pod name", name); err != nil { + return fmt.Errorf("invalid pod name %q: %w", name, err) } return nil } diff --git a/internal/snapshot/destination_test.go b/internal/snapshot/destination_test.go index 53f9c2dd..836b3933 100644 --- a/internal/snapshot/destination_test.go +++ b/internal/snapshot/destination_test.go @@ -178,6 +178,16 @@ func TestParseSource(t *testing.T) { input: "pod:-bad", wantErr: "invalid pod name", }, + { + name: "pod: percent encoding rejected", + input: "pod:staging%2Fpod", + wantErr: "invalid pod name", + }, + { + name: "pod: shell metacharacters rejected", + input: "pod:a;rm", + wantErr: "invalid pod name", + }, // --- remote schemes --- { @@ -522,6 +532,21 @@ func TestParseDestination(t *testing.T) { wantKind: snapshot.KindPod, wantPodName: "ci_test-underscore", }, + { + name: "pod: percent encoding rejected", + input: "pod:staging%2Fpod", + wantErr: "invalid pod name", + }, + { + name: "pod: embedded query rejected", + input: "pod:abc?fields=name", + wantErr: "invalid pod name", + }, + { + name: "pod: shell metacharacters rejected", + input: "pod:a;rm", + wantErr: "invalid pod name", + }, // --- unknown schemes --- { diff --git a/internal/validate/validate.go b/internal/validate/validate.go new file mode 100644 index 00000000..5692dbac --- /dev/null +++ b/internal/validate/validate.go @@ -0,0 +1,128 @@ +// Package validate provides reusable, deterministic validators for user-supplied +// CLI inputs. It exists to make the CLI a safe target for AI agents and scripts, +// which can produce malformed or hostile input — control characters, path +// traversal, percent-encoding, embedded query parameters, shell metacharacters — +// in ways humans rarely do. +// +// Validators return an *Error carrying a machine-classifiable Rule so callers can +// surface a precise, stable reason (and, in JSON output mode, a stable error +// code) instead of a generic "invalid input" message. Error() returns the bare +// reason so it composes cleanly when wrapped with caller context. +package validate + +import ( + "fmt" + "regexp" + "strings" + "unicode" +) + +// Rule classifies why a value was rejected. The values are stable and intended to +// be surfaced as machine-readable error codes. +const ( + RuleEmpty = "empty" + RuleControlChars = "control_chars" + RuleEncoding = "encoding" + RuleTraversal = "traversal" + RuleEmbedded = "embedded" + RuleMetachars = "metachars" + RuleFormat = "format" + RuleRange = "range" +) + +type Error struct { + Field string + Rule string + Msg string +} + +func (e *Error) Error() string { return e.Msg } + +func newError(field, rule, msg string) *Error { + return &Error{Field: field, Rule: rule, Msg: msg} +} + +// containsControlChars reports whether s contains any control character other +// than tab, newline, or carriage return. +func containsControlChars(s string) bool { + for _, r := range s { + if r == '\t' || r == '\n' || r == '\r' { + continue + } + if unicode.IsControl(r) { + return true + } + } + return false +} + +func NoControlChars(field, value string) error { + if containsControlChars(value) { + return newError(field, RuleControlChars, "contains control characters") + } + return nil +} + +var envVarKeyRegexp = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`) + +// EnvVarName validates an environment variable name (the key of a KEY=VALUE pair). +func EnvVarName(name string) error { + if !envVarKeyRegexp.MatchString(name) { + return newError("env", RuleFormat, fmt.Sprintf("env key %q contains invalid characters", name)) + } + return nil +} + +var resourceNameRegexp = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]*$`) + +// shellMetaChars are characters that enable command injection if an identifier is +// ever interpolated into a shell. The slash, question mark, and hash are handled +// separately as embedded path/query characters and are not repeated here. +const shellMetaChars = ";&|$\x60<>(){}[]!*'~\\\"" + +// ResourceName validates an opaque resource identifier such as a Cloud Pod name. +// It runs ordered deny-checks so the most specific reason wins, then a strict +// allow-list. The deny-checks exist to give precise, machine-classifiable +// feedback; the allow-list alone would reject every invalid value. +func ResourceName(field, value string) error { + switch { + case value == "": + return newError(field, RuleEmpty, "must not be empty") + case containsControlChars(value): + return newError(field, RuleControlChars, "contains control characters") + case strings.Contains(value, "%"): + return newError(field, RuleEncoding, "contains percent-encoding (pass the decoded value)") + case strings.Contains(value, ".."): + return newError(field, RuleTraversal, "contains a path traversal sequence (..)") + case strings.ContainsAny(value, "/?#"): + return newError(field, RuleEmbedded, "contains path or query characters (/, ?, #)") + case strings.ContainsAny(value, shellMetaChars): + return newError(field, RuleMetachars, "contains shell metacharacters") + case !resourceNameRegexp.MatchString(value): + return newError(field, RuleFormat, "use letters, digits, hyphens, and underscores only, starting with a letter or digit") + } + return nil +} + +// AuthToken validates a LocalStack auth token. The character set is intentionally +// not restricted — tokens are opaque — so only clearly malformed values are +// rejected: control characters, embedded whitespace, or an implausible length. An +// empty token is allowed (it means none is set). Callers should TrimSpace first, +// since environment injection (e.g. CI secrets) commonly appends a trailing newline. +func AuthToken(value string) error { + if value == "" { + return nil + } + for _, r := range value { + if unicode.IsControl(r) { + return newError("auth token", RuleControlChars, "contains control characters") + } + if unicode.IsSpace(r) { + return newError("auth token", RuleFormat, "contains whitespace") + } + } + if len(value) > 1024 { + return newError("auth token", RuleRange, "is implausibly long (over 1024 characters)") + } + return nil +} diff --git a/internal/validate/validate_test.go b/internal/validate/validate_test.go new file mode 100644 index 00000000..e10e4b94 --- /dev/null +++ b/internal/validate/validate_test.go @@ -0,0 +1,108 @@ +package validate + +import ( + "errors" + "strings" + "testing" +) + +func TestNoControlChars(t *testing.T) { + t.Parallel() + tests := []struct { + name string + value string + wantErr bool + }{ + {"clean string", "hello world", false}, + {"with tab", "hello\tworld", false}, + {"with newline", "hello\nworld", false}, + {"with null byte", "hello\x00world", true}, + {"with bell", "hello\x07world", true}, + {"with escape", "hello\x1bworld", true}, + {"with delete", "hello\x7fworld", true}, + {"empty", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := NoControlChars("test", tt.value) + if (err != nil) != tt.wantErr { + t.Errorf("NoControlChars() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestResourceName(t *testing.T) { + t.Parallel() + tests := []struct { + name string + value string + wantErr bool + wantRule string + }{ + {"simple", "my-baseline", false, ""}, + {"alphanumeric", "abc123", false, ""}, + {"single char", "a", false, ""}, + {"long hyphenated", "my-long-pod-name-123", false, ""}, + {"empty", "", true, RuleEmpty}, + {"control char", "ba\x00d", true, RuleControlChars}, + {"percent encoding", "staging%2Fpod", true, RuleEncoding}, + {"path traversal", "../etc", true, RuleTraversal}, + {"embedded query", "abc?fields=name", true, RuleEmbedded}, + {"slash", "a/b", true, RuleEmbedded}, + {"fragment", "id#frag", true, RuleEmbedded}, + {"shell metachar semicolon", "a;rm", true, RuleMetachars}, + {"shell metachar subshell", "a$(id)", true, RuleMetachars}, + {"shell metachar backtick", "a`id`", true, RuleMetachars}, + {"underscore", "my_pod", false, ""}, + {"leading hyphen", "-bad", true, RuleFormat}, + {"leading dot", ".hidden", true, RuleFormat}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := ResourceName("name", tt.value) + if (err != nil) != tt.wantErr { + t.Fatalf("ResourceName(%q) error = %v, wantErr %v", tt.value, err, tt.wantErr) + } + if tt.wantRule != "" { + var ve *Error + if !errors.As(err, &ve) { + t.Fatalf("ResourceName(%q) error is not *validate.Error: %v", tt.value, err) + } + if ve.Rule != tt.wantRule { + t.Errorf("ResourceName(%q) Rule = %q, want %q", tt.value, ve.Rule, tt.wantRule) + } + } + }) + } +} + +func TestAuthToken(t *testing.T) { + t.Parallel() + tests := []struct { + name string + value string + wantErr bool + }{ + {"empty is allowed", "", false}, + {"typical token", "ls-example-token", false}, + {"alphanumeric", "exampletoken123", false}, + {"with null byte", "tok\x00en", true}, + {"with escape", "tok\x1ben", true}, + {"with newline", "token\n", true}, + {"with tab", "tok\ten", true}, + {"with space", "tok en", true}, + {"too long", strings.Repeat("a", 1025), true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := AuthToken(tt.value) + if (err != nil) != tt.wantErr { + t.Errorf("AuthToken(%q) error = %v, wantErr %v", tt.value, err, tt.wantErr) + } + }) + } +} From 6f1b5cb1d83bcef47df3557f8d6b2016074dd2a4 Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Wed, 22 Jul 2026 16:02:56 +0300 Subject: [PATCH 2/6] Document internal/validate package in CLAUDE.md Co-Authored-By: Claude --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CLAUDE.md b/CLAUDE.md index 26c7c3fb..2525c143 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -68,6 +68,7 @@ Notes: - `tracing/` - OpenTelemetry setup (`LSTK_OTEL=1`) - `ui/` - Bubble Tea views for interactive output - `update/` - Self-update logic: version check via GitHub API, binary/Homebrew/npm update paths, archive extraction; also detects multiple lstk installs on PATH (`FindInstalls`/`WarnMultipleInstalls`, warned on `lstk update` and the start-path update notification) + - `validate/` - Reusable input validators for user-supplied CLI values (pod names, env var names, auth tokens) rejecting malformed/hostile input (control chars, path traversal, percent-encoding, shell metacharacters) - `version/` - Version info - `volume/` - `lstk volume` domain logic From 8434e083b5fa830616fbfefe10a3c9d7eb6b5bc3 Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Wed, 22 Jul 2026 16:44:14 +0300 Subject: [PATCH 3/6] Add input-validation conventions to agent instructions Co-Authored-By: Claude --- .claude/skills/add-command/SKILL.md | 3 +++ .claude/skills/review-pr/SKILL.md | 1 + CLAUDE.md | 1 + 3 files changed, 5 insertions(+) diff --git a/.claude/skills/add-command/SKILL.md b/.claude/skills/add-command/SKILL.md index b26000c4..c6354277 100644 --- a/.claude/skills/add-command/SKILL.md +++ b/.claude/skills/add-command/SKILL.md @@ -19,6 +19,7 @@ Before writing any code, understand what the command should do and whether the a 3. **Does it need configuration?** (determines whether `PreRunE: initConfigDeferCreate(nil)` is needed) 4. **Does it need authentication?** (determines whether auth flow is involved) 5. **Does it need any new event types?** (e.g., a new kind of progress, a new status phase — if yes, use `/add-event` for each) +6. **What user-supplied inputs does it accept?** (args, flags, config values — determines which `internal/validate` validators apply at the boundary) **Also ask context-specific questions** that aren't in this list but would help clarify behavior — e.g., edge cases ("what happens if no emulators are running?"), flags/arguments the command should accept, expected output format, whether it should be idempotent, etc. @@ -48,6 +49,7 @@ Create `cmd/$ARGUMENTS.go` with: - Non-interactive: call domain function with `output.NewPlainSink(os.Stdout)` - If the command only groups subcommands (like `config`, `setup`, `snapshot`), call `requireSubcommand(cmd)` so a bare invocation shows help (exit 0) while an unknown subcommand exits non-zero - No business logic — only Cobra wiring, dependency creation, and output mode selection +- Validate user-supplied args/flags where they are first accepted, via `internal/validate` — identifiers through `validate.ResourceName`, opaque secrets through loose checks like `validate.AuthToken`, paths/URLs through their normal parsers (`filepath`, `net/url`). If no validator fits, add one to `internal/validate` (with rule-code tests); never write an inline validation regexp - `Short`/`Long` help text: write each paragraph as one unbroken line (blank line between paragraphs); never hard-wrap a sentence across source lines. The help template (`cmd/help.go`) word-wraps to the terminal width at render time, and `lstk docs` reads the raw text — manual breaks fight both. Indent example/output blocks; the wrapper leaves indented lines untouched. ## Step 2: Create the domain logic package @@ -96,6 +98,7 @@ Create `test/integration/_test.go` with: - Use `requireDocker(t)` if Docker is needed - Use `cleanup()` and `t.Cleanup(cleanup)` for container state - Use `context.WithTimeout` for all tests +- If the command accepts identifiers or free-form values, include malformed-input cases (control characters, `../` traversal, percent-encoding, shell metacharacters) asserting a clear rejection ## Telemetry diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index d5aa0bc6..d74928d6 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -48,6 +48,7 @@ Go through each changed file and check for violations. Flag only actual problems - [ ] Domain code never reads from stdin directly - [ ] Interactive input uses `UserInputRequestEvent` + `ResponseCh` pattern - [ ] Non-TTY mode fails early with a helpful error if input would be required +- [ ] New user-supplied inputs (args, flags, config values) are validated at the boundary via `internal/validate`; no new inline validation regexp duplicating it (identifiers → `ResourceName`; opaque secrets → loose checks like `AuthToken`; paths/URLs → their existing parsers) and malformed-input cases are tested ### TUI (if UI changes) diff --git a/CLAUDE.md b/CLAUDE.md index 2525c143..ee1cf371 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -183,6 +183,7 @@ The release job (`.github/workflows/ci.yml`) builds the npm packages with `gorel - Never print directly to stdout/stderr (e.g., `fmt.Fprintf(os.Stderr, …)`). For user-facing output, emit events through `output.Sink`. For internal diagnostics, use `log.Logger`. If neither is available (e.g., during logger setup), return errors to the caller and let them decide. - Don't deprecate commands with Cobra's `Deprecated` field: it prints the notice raw to `os.Stderr` (bypassing `output.Sink`) and silently hides the command from `--help` and generated `lstk docs`. Remove the old command outright instead; if a transition period is genuinely needed, keep the command visible and emit the deprecation notice through the sink. - Do not call `config.Get()` from domain/business-logic packages. Instead, extract the values you need at the command boundary (`cmd/`) and pass them as explicit function arguments. This keeps domain functions testable without requiring Viper/config initialization. +- Validate user/agent-supplied values where they are first accepted (the command boundary, or the domain parser that owns the format) via `internal/validate` — never an inline one-off regexp. Route by value class: identifier-shaped values (pod names, resource names) → `validate.ResourceName`; opaque secrets → only loose malformed-ness checks (`validate.AuthToken` style — no charset restriction); paths and URLs → their existing parsers (`filepath`, `net/url`), not `ResourceName`. If no validator fits, add one to `internal/validate` with rule-code tests instead of forking rules locally — parallel validators for the same value class drift (the pod-name rules forked exactly this way before #293 re-unified them). # Shell Completion From 81a90b706a334713ee8f5c789fd1137f37073e73 Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Thu, 23 Jul 2026 16:12:57 +0300 Subject: [PATCH 4/6] Scope pod name validation Co-Authored-By: Claude --- .claude/skills/add-command/SKILL.md | 2 +- .claude/skills/review-pr/SKILL.md | 2 +- CLAUDE.md | 2 +- internal/snapshot/destination.go | 2 +- internal/validate/validate.go | 18 +++++++++--------- internal/validate/validate_test.go | 16 ++++++++++------ test/integration/snapshot_show_test.go | 19 +++++++++++++++++++ 7 files changed, 42 insertions(+), 19 deletions(-) diff --git a/.claude/skills/add-command/SKILL.md b/.claude/skills/add-command/SKILL.md index c6354277..017fddbb 100644 --- a/.claude/skills/add-command/SKILL.md +++ b/.claude/skills/add-command/SKILL.md @@ -49,7 +49,7 @@ Create `cmd/$ARGUMENTS.go` with: - Non-interactive: call domain function with `output.NewPlainSink(os.Stdout)` - If the command only groups subcommands (like `config`, `setup`, `snapshot`), call `requireSubcommand(cmd)` so a bare invocation shows help (exit 0) while an unknown subcommand exits non-zero - No business logic — only Cobra wiring, dependency creation, and output mode selection -- Validate user-supplied args/flags where they are first accepted, via `internal/validate` — identifiers through `validate.ResourceName`, opaque secrets through loose checks like `validate.AuthToken`, paths/URLs through their normal parsers (`filepath`, `net/url`). If no validator fits, add one to `internal/validate` (with rule-code tests); never write an inline validation regexp +- Validate user-supplied args/flags where they are first accepted, via `internal/validate` — pod names through `validate.PodName`, opaque secrets through loose checks like `validate.AuthToken`, and paths/URLs through their normal parsers (`filepath`, `net/url`). For other identifiers, follow the owning API's documented contract and add a dedicated validator if needed. If no validator fits, add one to `internal/validate` (with rule-code tests); never write an inline validation regexp - `Short`/`Long` help text: write each paragraph as one unbroken line (blank line between paragraphs); never hard-wrap a sentence across source lines. The help template (`cmd/help.go`) word-wraps to the terminal width at render time, and `lstk docs` reads the raw text — manual breaks fight both. Indent example/output blocks; the wrapper leaves indented lines untouched. ## Step 2: Create the domain logic package diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index d74928d6..f6542dd0 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -48,7 +48,7 @@ Go through each changed file and check for violations. Flag only actual problems - [ ] Domain code never reads from stdin directly - [ ] Interactive input uses `UserInputRequestEvent` + `ResponseCh` pattern - [ ] Non-TTY mode fails early with a helpful error if input would be required -- [ ] New user-supplied inputs (args, flags, config values) are validated at the boundary via `internal/validate`; no new inline validation regexp duplicating it (identifiers → `ResourceName`; opaque secrets → loose checks like `AuthToken`; paths/URLs → their existing parsers) and malformed-input cases are tested +- [ ] New user-supplied inputs (args, flags, config values) are validated at the boundary via `internal/validate`; no new inline validation regexp duplicates an existing validator (pod names → `PodName`; opaque secrets → loose checks like `AuthToken`; paths/URLs → their existing parsers; other identifiers → the owning API's documented contract) and malformed-input cases are tested ### TUI (if UI changes) diff --git a/CLAUDE.md b/CLAUDE.md index ee1cf371..9567bd15 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -183,7 +183,7 @@ The release job (`.github/workflows/ci.yml`) builds the npm packages with `gorel - Never print directly to stdout/stderr (e.g., `fmt.Fprintf(os.Stderr, …)`). For user-facing output, emit events through `output.Sink`. For internal diagnostics, use `log.Logger`. If neither is available (e.g., during logger setup), return errors to the caller and let them decide. - Don't deprecate commands with Cobra's `Deprecated` field: it prints the notice raw to `os.Stderr` (bypassing `output.Sink`) and silently hides the command from `--help` and generated `lstk docs`. Remove the old command outright instead; if a transition period is genuinely needed, keep the command visible and emit the deprecation notice through the sink. - Do not call `config.Get()` from domain/business-logic packages. Instead, extract the values you need at the command boundary (`cmd/`) and pass them as explicit function arguments. This keeps domain functions testable without requiring Viper/config initialization. -- Validate user/agent-supplied values where they are first accepted (the command boundary, or the domain parser that owns the format) via `internal/validate` — never an inline one-off regexp. Route by value class: identifier-shaped values (pod names, resource names) → `validate.ResourceName`; opaque secrets → only loose malformed-ness checks (`validate.AuthToken` style — no charset restriction); paths and URLs → their existing parsers (`filepath`, `net/url`), not `ResourceName`. If no validator fits, add one to `internal/validate` with rule-code tests instead of forking rules locally — parallel validators for the same value class drift (the pod-name rules forked exactly this way before #293 re-unified them). +- Validate user/agent-supplied values where they are first accepted (the command boundary, or the domain parser that owns the format) via `internal/validate` — never an inline one-off regexp. Route by value class: pod names → `validate.PodName`; opaque secrets → only loose malformed-ness checks (`validate.AuthToken` style — no charset restriction); paths and URLs → their existing parsers (`filepath`, `net/url`). For other identifiers, follow the owning API's documented contract and add a dedicated validator if needed. If no validator fits, add one to `internal/validate` with rule-code tests instead of forking rules locally — parallel validators for the same value class drift (the pod-name rules forked exactly this way before #293 re-unified them). # Shell Completion diff --git a/internal/snapshot/destination.go b/internal/snapshot/destination.go index 2b97eb7a..2e0962ab 100644 --- a/internal/snapshot/destination.go +++ b/internal/snapshot/destination.go @@ -69,7 +69,7 @@ func IsS3Ref(ref string) bool { // ValidatePodName validates a user-supplied pod name (the identity of a snapshot // on a remote), using the same rules as pod: refs. func ValidatePodName(name string) error { - if err := validate.ResourceName("pod name", name); err != nil { + if err := validate.PodName(name); err != nil { return fmt.Errorf("invalid pod name %q: %w", name, err) } return nil diff --git a/internal/validate/validate.go b/internal/validate/validate.go index 5692dbac..2b99c1bd 100644 --- a/internal/validate/validate.go +++ b/internal/validate/validate.go @@ -23,7 +23,6 @@ const ( RuleEmpty = "empty" RuleControlChars = "control_chars" RuleEncoding = "encoding" - RuleTraversal = "traversal" RuleEmbedded = "embedded" RuleMetachars = "metachars" RuleFormat = "format" @@ -73,18 +72,19 @@ func EnvVarName(name string) error { return nil } -var resourceNameRegexp = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]*$`) +var podNameRegexp = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`) -// shellMetaChars are characters that enable command injection if an identifier is +// shellMetaChars are characters that enable command injection if a pod name is // ever interpolated into a shell. The slash, question mark, and hash are handled // separately as embedded path/query characters and are not repeated here. const shellMetaChars = ";&|$\x60<>(){}[]!*'~\\\"" -// ResourceName validates an opaque resource identifier such as a Cloud Pod name. +// PodName validates a Cloud Pod name. // It runs ordered deny-checks so the most specific reason wins, then a strict // allow-list. The deny-checks exist to give precise, machine-classifiable // feedback; the allow-list alone would reject every invalid value. -func ResourceName(field, value string) error { +func PodName(value string) error { + const field = "pod name" switch { case value == "": return newError(field, RuleEmpty, "must not be empty") @@ -92,14 +92,14 @@ func ResourceName(field, value string) error { return newError(field, RuleControlChars, "contains control characters") case strings.Contains(value, "%"): return newError(field, RuleEncoding, "contains percent-encoding (pass the decoded value)") - case strings.Contains(value, ".."): - return newError(field, RuleTraversal, "contains a path traversal sequence (..)") case strings.ContainsAny(value, "/?#"): return newError(field, RuleEmbedded, "contains path or query characters (/, ?, #)") case strings.ContainsAny(value, shellMetaChars): return newError(field, RuleMetachars, "contains shell metacharacters") - case !resourceNameRegexp.MatchString(value): - return newError(field, RuleFormat, "use letters, digits, hyphens, and underscores only, starting with a letter or digit") + case len(value) > 128: + return newError(field, RuleRange, "must be 128 characters or fewer") + case !podNameRegexp.MatchString(value): + return newError(field, RuleFormat, "use letters, digits, periods, hyphens, and underscores only, starting with a letter or digit") } return nil } diff --git a/internal/validate/validate_test.go b/internal/validate/validate_test.go index e10e4b94..ff4347f8 100644 --- a/internal/validate/validate_test.go +++ b/internal/validate/validate_test.go @@ -33,7 +33,7 @@ func TestNoControlChars(t *testing.T) { } } -func TestResourceName(t *testing.T) { +func TestPodName(t *testing.T) { t.Parallel() tests := []struct { name string @@ -45,10 +45,14 @@ func TestResourceName(t *testing.T) { {"alphanumeric", "abc123", false, ""}, {"single char", "a", false, ""}, {"long hyphenated", "my-long-pod-name-123", false, ""}, + {"period", "release.v1", false, ""}, + {"consecutive periods", "release..v1", false, ""}, + {"maximum length", strings.Repeat("a", 128), false, ""}, + {"too long", strings.Repeat("a", 129), true, RuleRange}, {"empty", "", true, RuleEmpty}, {"control char", "ba\x00d", true, RuleControlChars}, {"percent encoding", "staging%2Fpod", true, RuleEncoding}, - {"path traversal", "../etc", true, RuleTraversal}, + {"path traversal", "../etc", true, RuleEmbedded}, {"embedded query", "abc?fields=name", true, RuleEmbedded}, {"slash", "a/b", true, RuleEmbedded}, {"fragment", "id#frag", true, RuleEmbedded}, @@ -62,17 +66,17 @@ func TestResourceName(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := ResourceName("name", tt.value) + err := PodName(tt.value) if (err != nil) != tt.wantErr { - t.Fatalf("ResourceName(%q) error = %v, wantErr %v", tt.value, err, tt.wantErr) + t.Fatalf("PodName(%q) error = %v, wantErr %v", tt.value, err, tt.wantErr) } if tt.wantRule != "" { var ve *Error if !errors.As(err, &ve) { - t.Fatalf("ResourceName(%q) error is not *validate.Error: %v", tt.value, err) + t.Fatalf("PodName(%q) error is not *validate.Error: %v", tt.value, err) } if ve.Rule != tt.wantRule { - t.Errorf("ResourceName(%q) Rule = %q, want %q", tt.value, ve.Rule, tt.wantRule) + t.Errorf("PodName(%q) Rule = %q, want %q", tt.value, ve.Rule, tt.wantRule) } } }) diff --git a/test/integration/snapshot_show_test.go b/test/integration/snapshot_show_test.go index 5ef62cfa..200c13bc 100644 --- a/test/integration/snapshot_show_test.go +++ b/test/integration/snapshot_show_test.go @@ -110,6 +110,25 @@ func TestSnapshotShowWithoutResources(t *testing.T) { assert.NotContains(t, stdout, "Resources", "Resources section must be omitted when no counts are available") } +func TestSnapshotShowPodNameWithPeriod(t *testing.T) { + t.Parallel() + + const name = "release.v1" + var cap showCapture + body := `{"pod_name": "release.v1", "max_version": 1, "versions": [{"version": 1}]}` + srv := mockCloudPodServer(t, name, body, &cap) + + _, stderr, err := runLstk(t, testContext(t), t.TempDir(), + listEnv(t, srv, "test-token"), + "--non-interactive", "snapshot", "show", "pod:"+name, + ) + require.NoError(t, err, "lstk snapshot show failed: %s", stderr) + + called, path, _ := cap.get() + require.True(t, called, "the single-pod endpoint should have been called") + assert.Equal(t, "/v1/cloudpods/"+name, path) +} + func TestSnapshotShowSendsBasicAuthHeader(t *testing.T) { t.Parallel() From 938d50f56bbf416f8176d221f19d76670ca71be9 Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Thu, 23 Jul 2026 16:38:10 +0300 Subject: [PATCH 5/6] Match pod name validation to the platform contract Co-Authored-By: Claude --- internal/snapshot/destination_test.go | 16 +++++++------- internal/validate/validate.go | 15 +++++++++---- internal/validate/validate_test.go | 7 ++++--- test/integration/snapshot_show_test.go | 29 +++++++++++++++++++++++--- 4 files changed, 50 insertions(+), 17 deletions(-) diff --git a/internal/snapshot/destination_test.go b/internal/snapshot/destination_test.go index 836b3933..d65090a7 100644 --- a/internal/snapshot/destination_test.go +++ b/internal/snapshot/destination_test.go @@ -174,9 +174,10 @@ func TestParseSource(t *testing.T) { wantErr: "invalid pod name", }, { - name: "pod: leading hyphen", - input: "pod:-bad", - wantErr: "invalid pod name", + name: "pod: leading hyphen accepted (platform allows it)", + input: "pod:-baseline", + wantKind: snapshot.KindPod, + wantPodName: "-baseline", }, { name: "pod: percent encoding rejected", @@ -521,10 +522,11 @@ func TestParseDestination(t *testing.T) { wantErr: "invalid pod name", }, { - // pod name starting with hyphen - name: "pod: leading hyphen", - input: "pod:-invalid", - wantErr: "invalid pod name", + // leading hyphen is inside the platform's POD_NAME_PATTERN + name: "pod: leading hyphen accepted (platform allows it)", + input: "pod:-baseline", + wantKind: snapshot.KindPod, + wantPodName: "-baseline", }, { name: "pod: underscore allowed", diff --git a/internal/validate/validate.go b/internal/validate/validate.go index 2b99c1bd..e58c9620 100644 --- a/internal/validate/validate.go +++ b/internal/validate/validate.go @@ -72,15 +72,22 @@ func EnvVarName(name string) error { return nil } -var podNameRegexp = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`) +// podNameRegexp mirrors the platform API's POD_NAME_PATTERN (also enforced +// identically by the emulator's pods bootstrap): letters, digits, underscores, +// and hyphens only. Anything else — including dots — is rejected server-side +// with HTTP 400 "Invalid name for cloud pod", so accepting it here would only +// defer the failure. +var podNameRegexp = regexp.MustCompile(`^[a-zA-Z0-9_-]+$`) // shellMetaChars are characters that enable command injection if a pod name is // ever interpolated into a shell. The slash, question mark, and hash are handled // separately as embedded path/query characters and are not repeated here. const shellMetaChars = ";&|$\x60<>(){}[]!*'~\\\"" -// PodName validates a Cloud Pod name. -// It runs ordered deny-checks so the most specific reason wins, then a strict +// PodName validates a Cloud Pod name against the platform's contract. The +// platform pattern has no length bound; the 128-character cap here is a local +// sanity limit against absurd inputs, not part of the platform contract. +// It runs ordered deny-checks so the most specific reason wins, then the // allow-list. The deny-checks exist to give precise, machine-classifiable // feedback; the allow-list alone would reject every invalid value. func PodName(value string) error { @@ -99,7 +106,7 @@ func PodName(value string) error { case len(value) > 128: return newError(field, RuleRange, "must be 128 characters or fewer") case !podNameRegexp.MatchString(value): - return newError(field, RuleFormat, "use letters, digits, periods, hyphens, and underscores only, starting with a letter or digit") + return newError(field, RuleFormat, "use letters, digits, hyphens, and underscores only") } return nil } diff --git a/internal/validate/validate_test.go b/internal/validate/validate_test.go index ff4347f8..e7dc2c47 100644 --- a/internal/validate/validate_test.go +++ b/internal/validate/validate_test.go @@ -45,14 +45,16 @@ func TestPodName(t *testing.T) { {"alphanumeric", "abc123", false, ""}, {"single char", "a", false, ""}, {"long hyphenated", "my-long-pod-name-123", false, ""}, - {"period", "release.v1", false, ""}, - {"consecutive periods", "release..v1", false, ""}, + {"leading underscore", "_baseline", false, ""}, + {"leading hyphen", "-baseline", false, ""}, {"maximum length", strings.Repeat("a", 128), false, ""}, {"too long", strings.Repeat("a", 129), true, RuleRange}, {"empty", "", true, RuleEmpty}, {"control char", "ba\x00d", true, RuleControlChars}, {"percent encoding", "staging%2Fpod", true, RuleEncoding}, {"path traversal", "../etc", true, RuleEmbedded}, + {"period", "release.v1", true, RuleFormat}, + {"consecutive periods", "release..v1", true, RuleFormat}, {"embedded query", "abc?fields=name", true, RuleEmbedded}, {"slash", "a/b", true, RuleEmbedded}, {"fragment", "id#frag", true, RuleEmbedded}, @@ -60,7 +62,6 @@ func TestPodName(t *testing.T) { {"shell metachar subshell", "a$(id)", true, RuleMetachars}, {"shell metachar backtick", "a`id`", true, RuleMetachars}, {"underscore", "my_pod", false, ""}, - {"leading hyphen", "-bad", true, RuleFormat}, {"leading dot", ".hidden", true, RuleFormat}, } for _, tt := range tests { diff --git a/test/integration/snapshot_show_test.go b/test/integration/snapshot_show_test.go index 200c13bc..d51e04e5 100644 --- a/test/integration/snapshot_show_test.go +++ b/test/integration/snapshot_show_test.go @@ -110,12 +110,15 @@ func TestSnapshotShowWithoutResources(t *testing.T) { assert.NotContains(t, stdout, "Resources", "Resources section must be omitted when no counts are available") } -func TestSnapshotShowPodNameWithPeriod(t *testing.T) { +// TestSnapshotShowPodNameLeadingUnderscore covers a name the platform's +// POD_NAME_PATTERN (^[a-zA-Z0-9_-]+$) accepts but older lstk versions +// rejected — the CLI must be able to address any platform-legal pod. +func TestSnapshotShowPodNameLeadingUnderscore(t *testing.T) { t.Parallel() - const name = "release.v1" + const name = "_baseline" var cap showCapture - body := `{"pod_name": "release.v1", "max_version": 1, "versions": [{"version": 1}]}` + body := `{"pod_name": "_baseline", "max_version": 1, "versions": [{"version": 1}]}` srv := mockCloudPodServer(t, name, body, &cap) _, stderr, err := runLstk(t, testContext(t), t.TempDir(), @@ -129,6 +132,26 @@ func TestSnapshotShowPodNameWithPeriod(t *testing.T) { assert.Equal(t, "/v1/cloudpods/"+name, path) } +// TestSnapshotShowRejectsPodNameWithPeriod: dots are outside the platform's +// POD_NAME_PATTERN and would be rejected server-side with HTTP 400, so the +// CLI must fail fast locally without calling the platform. +func TestSnapshotShowRejectsPodNameWithPeriod(t *testing.T) { + t.Parallel() + + var cap showCapture + srv := mockCloudPodServer(t, "release.v1", `{}`, &cap) + + _, stderr, err := runLstk(t, testContext(t), t.TempDir(), + listEnv(t, srv, "test-token"), + "--non-interactive", "snapshot", "show", "pod:release.v1", + ) + require.Error(t, err, "a dotted pod name must be rejected client-side") + assert.Contains(t, stderr, "invalid pod name") + + called, _, _ := cap.get() + assert.False(t, called, "the platform endpoint must not be called for an invalid name") +} + func TestSnapshotShowSendsBasicAuthHeader(t *testing.T) { t.Parallel() From f068e459df7b5d5d353c2a120692c264fb3bdcaf Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Thu, 23 Jul 2026 16:49:46 +0300 Subject: [PATCH 6/6] Fix integration tests asserting stale pod name rules Co-Authored-By: Claude --- test/integration/snapshot_load_test.go | 2 +- test/integration/snapshot_remove_test.go | 2 +- test/integration/snapshot_save_test.go | 3 +-- test/integration/start_snapshot_test.go | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/test/integration/snapshot_load_test.go b/test/integration/snapshot_load_test.go index 70d47874..d58e086d 100644 --- a/test/integration/snapshot_load_test.go +++ b/test/integration/snapshot_load_test.go @@ -149,7 +149,7 @@ func TestSnapshotLoadPodNoAuthToken(t *testing.T) { func TestSnapshotLoadPodInvalidName(t *testing.T) { t.Parallel() - for _, ref := range []string{"pod:", "pod:-bad", "pod:_bad", "pod:my pod"} { + for _, ref := range []string{"pod:", "pod:bad.name", "pod:my pod"} { t.Run(ref, func(t *testing.T) { t.Parallel() ctx := testContext(t) diff --git a/test/integration/snapshot_remove_test.go b/test/integration/snapshot_remove_test.go index a9e546c6..0b720074 100644 --- a/test/integration/snapshot_remove_test.go +++ b/test/integration/snapshot_remove_test.go @@ -79,7 +79,7 @@ func TestSnapshotRemovePodNoAuthToken(t *testing.T) { func TestSnapshotRemovePodInvalidName(t *testing.T) { t.Parallel() - for _, ref := range []string{"pod:", "pod:-bad", "pod:_bad", "pod:my pod"} { + for _, ref := range []string{"pod:", "pod:bad.name", "pod:my pod"} { t.Run(ref, func(t *testing.T) { t.Parallel() ctx := testContext(t) diff --git a/test/integration/snapshot_save_test.go b/test/integration/snapshot_save_test.go index c5b727e8..25d9c815 100644 --- a/test/integration/snapshot_save_test.go +++ b/test/integration/snapshot_save_test.go @@ -372,8 +372,7 @@ func TestSnapshotSavePodInvalidName(t *testing.T) { t.Parallel() for _, dest := range []string{ "pod:", - "pod:-invalid", - "pod:_invalid", + "pod:bad.name", "pod:my pod", } { t.Run(dest, func(t *testing.T) { diff --git a/test/integration/start_snapshot_test.go b/test/integration/start_snapshot_test.go index f35773ba..26bc6986 100644 --- a/test/integration/start_snapshot_test.go +++ b/test/integration/start_snapshot_test.go @@ -30,7 +30,7 @@ func TestStartSnapshotInvalidPodName(t *testing.T) { _, stderr, err := runLstk(t, ctx, t.TempDir(), testEnvWithHome(t.TempDir(), ""), - "--non-interactive", "start", "--snapshot", "pod:_bad", + "--non-interactive", "start", "--snapshot", "pod:bad.name", ) requireExitCode(t, 1, err) assert.Contains(t, stderr, "invalid pod name")