From 05a17601ec9cccf82e82400ab645a3f4bc7a6115 Mon Sep 17 00:00:00 2001 From: Shweta <35878561+shwetamurali@users.noreply.github.com> Date: Tue, 14 Jul 2026 16:37:54 -0400 Subject: [PATCH] commit --- .../ENHANCEMENTS-20260714-163609.yaml | 3 + internal/commands/harness/harness_exec.go | 4 ++ internal/pkg/client/path_params.go | 6 ++ internal/pkg/client/path_params_test.go | 20 +++++- internal/pkg/cmd/command.go | 6 ++ internal/pkg/cmd/command_internal.go | 27 ++++++++ internal/pkg/cmd/command_test.go | 47 +++++++++++++ internal/pkg/inputguard/inputguard.go | 65 ++++++++++++++++++ internal/pkg/inputguard/inputguard_test.go | 66 +++++++++++++++++++ 9 files changed, 243 insertions(+), 1 deletion(-) create mode 100644 .changes/unreleased/ENHANCEMENTS-20260714-163609.yaml create mode 100644 internal/pkg/inputguard/inputguard.go create mode 100644 internal/pkg/inputguard/inputguard_test.go diff --git a/.changes/unreleased/ENHANCEMENTS-20260714-163609.yaml b/.changes/unreleased/ENHANCEMENTS-20260714-163609.yaml new file mode 100644 index 0000000..237592f --- /dev/null +++ b/.changes/unreleased/ENHANCEMENTS-20260714-163609.yaml @@ -0,0 +1,3 @@ +kind: ENHANCEMENTS +body: Positional arguments and API path parameters are now checked for basic input hygiene, rejecting control characters and invalid UTF-8. This keeps malformed values out of requests and out of terminal/audit output. This is not a security boundary; authorization is still enforced by your API token. +time: 2026-07-14T16:36:09.687002-04:00 diff --git a/internal/commands/harness/harness_exec.go b/internal/commands/harness/harness_exec.go index 5e51a95..6dd2888 100644 --- a/internal/commands/harness/harness_exec.go +++ b/internal/commands/harness/harness_exec.go @@ -83,6 +83,10 @@ func NewCmdHarnessExec(inv *cmd.Invocation) *cmd.Command { // those tokens as files or executables would mislead, so we predict // nothing rather than predict wrongly. Validate: cmd.ArbitraryArgs, + // The child command line is passed through verbatim to another + // program, so it may legitimately contain characters (tabs, escapes) + // that the framework's input-hygiene check would otherwise reject. + SkipInputValidation: true, Args: []cmd.PositionalArgument{ { Name: "COMMAND", diff --git a/internal/pkg/client/path_params.go b/internal/pkg/client/path_params.go index fe2b11c..0e038fe 100644 --- a/internal/pkg/client/path_params.go +++ b/internal/pkg/client/path_params.go @@ -7,6 +7,7 @@ import ( "fmt" "strings" + "github.com/hashicorp/tfctl-cli/internal/pkg/inputguard" "github.com/hashicorp/tfctl-cli/internal/pkg/resource" ) @@ -15,6 +16,11 @@ import ( func ResolvePathParams(path string, params map[string]string) (string, error) { result := path for k, v := range params { + // Input hygiene: reject control characters and invalid UTF-8 in values + // that become URL path segments. This is not a security boundary. + if err := inputguard.Validate(v); err != nil { + return "", fmt.Errorf("invalid value for path param {%s}: %w", k, err) + } result = strings.ReplaceAll(result, "{"+k+"}", v) } diff --git a/internal/pkg/client/path_params_test.go b/internal/pkg/client/path_params_test.go index 221054d..c6bf725 100644 --- a/internal/pkg/client/path_params_test.go +++ b/internal/pkg/client/path_params_test.go @@ -69,8 +69,26 @@ func TestResolvePathParams_RepeatedParam(t *testing.T) { assert.Equal(t, "/workspaces/ws-123/varsets/ws-123", result) } -func TestParsePathParams(t *testing.T) { +func TestResolvePathParams_RejectsControlCharValues(t *testing.T) { + t.Parallel() + _, err := ResolvePathParams("/workspaces/{workspace_id}", map[string]string{ + "workspace_id": "ws-abc\x1b[31m", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "workspace_id") + assert.Contains(t, err.Error(), "invalid input") +} + +func TestResolvePathParams_AllowsIDShapedValues(t *testing.T) { t.Parallel() + result, err := ResolvePathParams("/workspaces/{workspace_id}", map[string]string{ + "workspace_id": "ws-abc123", + }) + require.NoError(t, err) + assert.Equal(t, "/workspaces/ws-abc123", result) +} + +func TestParsePathParams(t *testing.T) { tests := []struct { name string path string diff --git a/internal/pkg/cmd/command.go b/internal/pkg/cmd/command.go index e465a5d..7efc9a4 100644 --- a/internal/pkg/cmd/command.go +++ b/internal/pkg/cmd/command.go @@ -155,6 +155,12 @@ type PositionalArguments struct { // Autocomplete allows configuring autocompletion of arguments. Autocomplete complete.Predictor + + // SkipInputValidation opts the command out of the framework's input-hygiene + // check on positional arguments (see inputguard.Validate). Set this only for + // commands that legitimately accept arbitrary bytes, such as a pass-through + // child command line. This is not a security control. + SkipInputValidation bool } // PositionalArgument documents a positional argument. diff --git a/internal/pkg/cmd/command_internal.go b/internal/pkg/cmd/command_internal.go index ad4c783..08f3f9c 100644 --- a/internal/pkg/cmd/command_internal.go +++ b/internal/pkg/cmd/command_internal.go @@ -23,6 +23,7 @@ import ( "github.com/spf13/pflag" "github.com/hashicorp/tfctl-cli/internal/pkg/heredoc" + "github.com/hashicorp/tfctl-cli/internal/pkg/inputguard" "github.com/hashicorp/tfctl-cli/internal/pkg/iostreams" "github.com/hashicorp/tfctl-cli/internal/pkg/ld" "github.com/hashicorp/tfctl-cli/version" @@ -155,6 +156,16 @@ func (c *Command) Run(args []string, inv *Invocation) int { return 1 } + // Apply input-hygiene checks to positional arguments (rejecting control + // characters and invalid UTF-8). This is not a security boundary; it keeps + // malformed values out of requests and out of any text tfctl echoes back. + if err := c.validateArgsInput(parsedArgs); err != nil { + fmt.Fprintf(io.Err(), "%s %s\n", cs.ErrorLabel(), err) + fmt.Fprintln(io.Err()) + fmt.Fprint(io.Err(), c.usageHelp()) + return 1 + } + // Run the command if err := c.RunF(c, parsedArgs); err != nil { return c.errorToExitCode(args, inv, err) @@ -163,6 +174,22 @@ func (c *Command) Run(args []string, inv *Invocation) int { return 0 } +// validateArgsInput applies input-hygiene checks to each positional argument, +// unless the command opts out via Args.SkipInputValidation. +func (c *Command) validateArgsInput(args []string) error { + if c.Args.SkipInputValidation { + return nil + } + + for _, a := range args { + if err := inputguard.Validate(a); err != nil { + return err + } + } + + return nil +} + func notFoundErrorHelp(io iostreams.IOStreams, hostname string) string { return heredoc.New(io, heredoc.WithPreserveNewlines(), heredoc.WithWidth(0)).Mustf(` Resource or path not found on {{ Bold "%s" }}. Verify the API path and any resource IDs are correct. diff --git a/internal/pkg/cmd/command_test.go b/internal/pkg/cmd/command_test.go index f800314..7b3d8ed 100644 --- a/internal/pkg/cmd/command_test.go +++ b/internal/pkg/cmd/command_test.go @@ -189,3 +189,50 @@ func TestCommand_CommandPath(t *testing.T) { // Grandchild: should return full path excluding root. r.Equal("run start", start.CommandPath()) } + +func TestCommand_ValidatesArgInput(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + skip bool + wantCode int + wantRun bool + }{ + {name: "clean args run", args: []string{"ws-abc", "my-org"}, wantCode: 0, wantRun: true}, + {name: "control char rejected", args: []string{"bad\x1b[31m"}, wantCode: 1, wantRun: false}, + {name: "newline rejected", args: []string{"a\nb"}, wantCode: 1, wantRun: false}, + {name: "invalid utf8 rejected", args: []string{"a\xffb"}, wantCode: 1, wantRun: false}, + {name: "skip opt-out allows control char", args: []string{"raw\x1b[0m"}, skip: true, wantCode: 0, wantRun: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + r := require.New(t) + + io := iostreams.Test() + cCtx := &Invocation{IO: io} + + ran := false + root := &Command{ + Name: "root", + io: io, + Args: PositionalArguments{ + Validate: ArbitraryArgs, + SkipInputValidation: tc.skip, + }, + RunF: func(c *Command, args []string) error { + ran = true + return nil + }, + } + + r.Equal(tc.wantCode, root.Run(tc.args, cCtx)) + r.Equal(tc.wantRun, ran) + if !tc.wantRun { + r.Contains(io.Error.String(), "invalid input") + } + }) + } +} diff --git a/internal/pkg/inputguard/inputguard.go b/internal/pkg/inputguard/inputguard.go new file mode 100644 index 0000000..718204d --- /dev/null +++ b/internal/pkg/inputguard/inputguard.go @@ -0,0 +1,65 @@ +// Copyright IBM Corp. 2026 +// SPDX-License-Identifier: MPL-2.0 + +// Package inputguard provides lightweight input-hygiene checks for user-supplied +// CLI values. +// +// This is deliberately NOT a security boundary. Authorization is enforced +// server-side by the API token; these checks only keep obviously-malformed +// values (invalid UTF-8, control characters such as ANSI escape sequences) out +// of requests and out of any text that tfctl echoes back to a terminal or an +// audit log, where they could corrupt output or spoof messages. +package inputguard + +import ( + "fmt" + "unicode" + "unicode/utf8" +) + +// maxErrorValueLen bounds how much of an offending value is echoed in an error +// so a pathological input cannot flood the terminal. +const maxErrorValueLen = 80 + +// InvalidInputError describes why a value failed validation. The offending value +// is always rendered with %q so control characters are escaped rather than +// written raw to a terminal. +type InvalidInputError struct { + // Value is the offending input. + Value string + + // Reason is a short, human-readable explanation. + Reason string +} + +func (e *InvalidInputError) Error() string { + v := e.Value + if len(v) > maxErrorValueLen { + v = v[:maxErrorValueLen] + "..." + } + return fmt.Sprintf("invalid input %q: %s", v, e.Reason) +} + +// Validate reports whether s is safe to use as a CLI input value. It rejects: +// +// - invalid UTF-8, and +// - control characters (including ANSI escape sequences), +// +// which have no legitimate place in a command-line value and can corrupt +// terminal output or audit logs. It returns an *InvalidInputError on failure. +// +// This is input hygiene, not authorization: it does not attempt to judge +// whether a value is "allowed", only that it is well-formed printable text. +func Validate(s string) error { + if !utf8.ValidString(s) { + return &InvalidInputError{Value: s, Reason: "contains invalid UTF-8"} + } + + for _, r := range s { + if unicode.IsControl(r) { + return &InvalidInputError{Value: s, Reason: "contains a control character"} + } + } + + return nil +} diff --git a/internal/pkg/inputguard/inputguard_test.go b/internal/pkg/inputguard/inputguard_test.go new file mode 100644 index 0000000..10db262 --- /dev/null +++ b/internal/pkg/inputguard/inputguard_test.go @@ -0,0 +1,66 @@ +// Copyright IBM Corp. 2026 +// SPDX-License-Identifier: MPL-2.0 + +package inputguard + +import ( + "errors" + "strings" + "testing" +) + +func TestValidate(t *testing.T) { + tests := []struct { + name string + input string + wantErr bool + }{ + {name: "plain identifier", input: "ws-abc123", wantErr: false}, + {name: "empty string", input: "", wantErr: false}, + {name: "path-like value", input: "organizations/my-org", wantErr: false}, + {name: "dotdot is allowed (not a security boundary)", input: "../etc/passwd", wantErr: false}, + {name: "unicode letters", input: "café-naïve", wantErr: false}, + {name: "spaces are fine", input: "my org name", wantErr: false}, + {name: "ansi escape rejected", input: "red\x1b[31mtext", wantErr: true}, + {name: "newline rejected", input: "line1\nline2", wantErr: true}, + {name: "tab rejected", input: "a\tb", wantErr: true}, + {name: "null byte rejected", input: "a\x00b", wantErr: true}, + {name: "carriage return rejected", input: "a\rb", wantErr: true}, + {name: "invalid utf8 rejected", input: "a\xffb", wantErr: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := Validate(tc.input) + if tc.wantErr && err == nil { + t.Fatalf("Validate(%q) = nil, want error", tc.input) + } + if !tc.wantErr && err != nil { + t.Fatalf("Validate(%q) = %v, want nil", tc.input, err) + } + }) + } +} + +func TestValidateReturnsInvalidInputError(t *testing.T) { + err := Validate("bad\x1b[0m") + var invErr *InvalidInputError + if !errors.As(err, &invErr) { + t.Fatalf("Validate returned %T, want *InvalidInputError", err) + } +} + +func TestInvalidInputErrorEscapesAndTruncates(t *testing.T) { + // Control characters must be escaped (%q), never written raw. + err := &InvalidInputError{Value: "x\x1b[31m", Reason: "contains a control character"} + if strings.ContainsRune(err.Error(), '\x1b') { + t.Fatalf("error message leaked a raw escape character: %q", err.Error()) + } + + // Long values are truncated. + long := strings.Repeat("a", maxErrorValueLen*2) + msg := (&InvalidInputError{Value: long, Reason: "too long"}).Error() + if !strings.Contains(msg, "...") { + t.Fatalf("expected truncated value in %q", msg) + } +}