Skip to content

Commit f3dbb06

Browse files
fix(cli): T16 P1 + T10 — BugBash 2026-05-20
Wave 2 cli-repo P1 fixes from BugHunt 2026-05-20. Each is paired with a hermetic regression test in cmd/bughunt_p1_test.go (CI-runnable, race-clean). - T16 P1-1 Move API key off plaintext disk into the OS keychain via internal/secretstore (macOS Keychain / Linux libsecret / Windows Credential Manager). Fall back to ~/.instant-config mode 0600 with a one-time stderr warning when the keychain is unavailable. Truncate any displayed key at 8 chars + '…' (whoami no longer prints 16 chars of a long-lived credential). Tests: TestBugHunt_T16_P1_1_WhoamiTruncatesKey, TestBugHunt_T16_P1_1_NoPlaintextKeyInConfigFile, TestSave_NoPlaintextAPIKeyOnDisk_KeychainBackend, TestSave_FallbackOnDiskWhenKeychainMissing, TestLegacyAPIKeyMigratesOnNextSave, TestTruncateForDisplay_NeverLeaksFullKey. - T16 P1-2 Uniform 401 handling across resources / up / direct provisioning. Authenticated 401 -> errSessionExpired() -> exit code 3. Anonymous 401 -> errAuthRequired() -> exit 3. Was: resources exited 0 (silent), up swallowed (P1-4 symptom), provisioning surfaced raw body. Test: TestBugHunt_T16_P1_2_UniformExitCodeFor401. - T16 P1-3 Implement the documented up exit-code contract via the new cmd.ExitCodeError type and cmd.ExitCodeFor extractor in main.go. 0 ok / 1 generic / 2 ExitResourceFailed / 3 ExitAuthRequired. Tests: TestBugHunt_T16_P1_3_ExitCodes_UpManifestParseError, TestBugHunt_T16_P1_3_ExitCodes_UpResourceFailed, TestBugHunt_T16_P1_3_ExitCodes_UpAuthRequiredForNonProd. - T16 P1-4 fetchExistingResources now returns (items, error) and a non-nil error aborts the reconcile with ExitResourceFailed (or ExitAuthRequired on 401-with-saved-token). Previous behaviour swallowed every non-200 and re-provisioned blind, burning money/quota on transient 429/5xx and silently re-provisioning on stale-token 401. Tests: TestBugHunt_T16_P1_4_UpAbortsOnListFetch_5xx, TestBugHunt_T16_P1_4_UpAbortsOnListFetch_429, TestBugHunt_T16_P1_4_UpAbortsOnListFetch_401_AuthenticatedSession. - T16 P1-5 --emit-env values are POSIX shell-quoted (single quotes with embedded-quote escaping), not Go-%q'd. Resource names with spaces / dots / slashes are sanitized into valid shell identifiers via sanitizeExportName(); names that can't be sanitized are rejected with a clear error rather than emitting a broken 'export NAME=…' line. Tests: TestShellQuote_BasicCases, TestShellQuote_EvalRoundTrip, TestShellQuote_NoCommandInjection, TestShellQuote_EvalAssignsToVariable, TestSanitizeExportName, TestIsValidShellIdentifier, TestBugHunt_T16_P1_5_EmitEnvShellQuotesHostileValues, TestBugHunt_T16_P1_5_EmitEnvSanitizesExportName. - T10 Scrub any remaining wrong-domain references in the CLI repo and pin the scan with a registry-iterating regression test that greps the entire tree. Test: TestBugHunt_T10_NoStaleInstantDevDomain. Process: go build ./... OK go vet ./... OK go test ./... -race -count=1 PASS (101 subtests) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c669817 commit f3dbb06

18 files changed

Lines changed: 1767 additions & 77 deletions

cmd/bughunt_p1_test.go

Lines changed: 524 additions & 0 deletions
Large diffs are not rendered by default.

cmd/discover.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,23 @@ Use 'instant status' to see resources tracked locally (no login required).
3434
}
3535
defer resp.Body.Close()
3636

37+
// T16 P1-2 — uniform 401 handling. Three different code paths used
38+
// to do three different things on 401 (resources: exit 0, up: silent
39+
// re-provision, provision: bare error). Now:
40+
// - anonymous caller: print 'not logged in' hint, exit 3 (auth req)
41+
// - authenticated caller (stale token): print 'session expired',
42+
// exit 3 (auth req) — same code so agents have one branch.
3743
if resp.StatusCode == http.StatusUnauthorized {
44+
if haveAuth() {
45+
return errSessionExpired()
46+
}
3847
fmt.Fprintln(os.Stderr, "Not logged in. Run `instant login` first.")
39-
return nil
48+
return errAuthRequired("authentication required — run `instant login` first")
4049
}
4150

4251
raw, _ := io.ReadAll(resp.Body)
4352
if resp.StatusCode != http.StatusOK {
44-
return fmt.Errorf("server returned %d: %s", resp.StatusCode, raw)
53+
return fmt.Errorf("server returned %d: %s", resp.StatusCode, truncate(string(raw), 200))
4554
}
4655

4756
var result struct {

cmd/errors.go

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
package cmd
2+
3+
// errors.go — typed CLI errors that carry an exit-code contract.
4+
//
5+
// The CLI advertises specific exit codes in its `up` help text (and intends
6+
// the same discipline platform-wide):
7+
//
8+
// ExitOK 0 success
9+
// ExitGeneric 1 generic / unknown failure (manifest parse, I/O, ...)
10+
// ExitResourceFailed 2 one or more resources failed to provision / reconcile
11+
// ExitAuthRequired 3 authentication required for the requested action
12+
// (also used for 401 from a previously-valid session
13+
// — see ExitSessionExpired below, which folds into 3)
14+
// ExitSessionExpired 3 same exit code as ExitAuthRequired: the server
15+
// rejected our credentials, so re-login is required.
16+
//
17+
// main.go inspects the returned error for an *ExitCodeError; if present the
18+
// embedded code is the process exit code. RunE handlers return one of the
19+
// helpers below (errResourceFailed, errAuthRequired, errSessionExpired) and
20+
// wrap any extra context with %w if needed.
21+
//
22+
// Pre-existing surface preserved: a plain `error` still exits 1 (errors.go's
23+
// extractExitCode default), matching today's behaviour for legacy code paths
24+
// that have not been converted to the typed-error contract.
25+
26+
import (
27+
"errors"
28+
"fmt"
29+
)
30+
31+
// Exit codes — the public contract the CLI promises.
32+
const (
33+
ExitOK = 0
34+
ExitGeneric = 1
35+
ExitResourceFailed = 2
36+
ExitAuthRequired = 3
37+
// ExitSessionExpired collapses to the same value as ExitAuthRequired: an
38+
// agent script only needs one branch ("any auth issue → re-login") and
39+
// the documented contract advertises 0/1/2/3.
40+
ExitSessionExpired = ExitAuthRequired
41+
)
42+
43+
// ExitCodeError carries both a wrapped cause and the documented exit code
44+
// the CLI should terminate with. main.go uses errors.As to extract it.
45+
type ExitCodeError struct {
46+
Code int
47+
Err error
48+
}
49+
50+
// Error implements error.
51+
func (e *ExitCodeError) Error() string {
52+
if e == nil || e.Err == nil {
53+
return fmt.Sprintf("exit %d", e.codeOrDefault())
54+
}
55+
return e.Err.Error()
56+
}
57+
58+
// Unwrap supports errors.Is / errors.As.
59+
func (e *ExitCodeError) Unwrap() error {
60+
if e == nil {
61+
return nil
62+
}
63+
return e.Err
64+
}
65+
66+
func (e *ExitCodeError) codeOrDefault() int {
67+
if e == nil || e.Code == 0 {
68+
return ExitGeneric
69+
}
70+
return e.Code
71+
}
72+
73+
// withExitCode wraps an error with the requested exit code. Nil err returns
74+
// nil so callers can chain it on the happy path.
75+
func withExitCode(code int, err error) error {
76+
if err == nil {
77+
return nil
78+
}
79+
return &ExitCodeError{Code: code, Err: err}
80+
}
81+
82+
// errResourceFailed wraps an error with ExitResourceFailed (2). Used by `up`
83+
// when one or more resources failed to reconcile.
84+
func errResourceFailed(err error) error {
85+
return withExitCode(ExitResourceFailed, err)
86+
}
87+
88+
// errAuthRequired returns a 'login required' error (exit code 3). The message
89+
// is a single uniform string so agents can branch on it deterministically.
90+
func errAuthRequired(detail string) error {
91+
if detail == "" {
92+
detail = "authentication required — run `instant login` or set INSTANT_TOKEN to a Personal Access Token"
93+
}
94+
return &ExitCodeError{Code: ExitAuthRequired, Err: errors.New(detail)}
95+
}
96+
97+
// errSessionExpired is the uniform "your saved session is no longer valid"
98+
// error. Emitted whenever the server returns 401 for a request that did send
99+
// a bearer token. Tests assert on this exact wording so the contract is
100+
// stable for downstream agents.
101+
//
102+
// IMPORTANT: keep the literal phrase "session expired" in the message — the
103+
// hermetic suite (and the project's "shipped ≠ verified" rules) grep for it.
104+
func errSessionExpired() error {
105+
return &ExitCodeError{
106+
Code: ExitSessionExpired,
107+
Err: errors.New("session expired — run `instant login` to re-authenticate"),
108+
}
109+
}
110+
111+
// ExitCodeFor returns the exit code for an error: 0 for nil, the embedded
112+
// code if it is an *ExitCodeError, else ExitGeneric (1). main.go uses this
113+
// to translate any error from the cobra tree into os.Exit(n).
114+
func ExitCodeFor(err error) int {
115+
if err == nil {
116+
return ExitOK
117+
}
118+
var ec *ExitCodeError
119+
if errors.As(err, &ec) {
120+
return ec.codeOrDefault()
121+
}
122+
return ExitGeneric
123+
}

cmd/integration_test.go

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import (
3434
"testing"
3535
"time"
3636

37+
"github.com/instant-dev/cli/internal/secretstore"
3738
"github.com/instant-dev/cli/internal/tokens"
3839
"github.com/spf13/cobra"
3940
)
@@ -58,6 +59,11 @@ func TestMain(m *testing.M) {
5859
// Never let an ambient token leak into the hermetic suite.
5960
os.Unsetenv("INSTANT_TOKEN")
6061
os.Unsetenv("INSTANT_API_URL")
62+
// Pin the secret backend to an in-memory store so cliconfig.Save /
63+
// .Load never touch the developer's OS keychain. Also disable the
64+
// real-keychain probe (no DBus / Security framework calls).
65+
os.Setenv("INSTANT_DISABLE_KEYCHAIN", "1")
66+
secretstore.UseMemoryBackend()
6167

6268
code := m.Run()
6369
_ = os.RemoveAll(tmpHome)
@@ -88,6 +94,9 @@ func newITContext(t *testing.T) *itContext {
8894
home, _ := os.UserHomeDir()
8995
_ = os.Remove(filepath.Join(home, ".instant-tokens"))
9096
_ = os.Remove(filepath.Join(home, ".instant-config"))
97+
// Reset the in-memory secret store so a token saved by a previous test
98+
// doesn't leak into "I'm anonymous" assertions.
99+
secretstore.UseMemoryBackend()
91100

92101
t.Cleanup(func() {
93102
APIBaseURL, HTTPClient = prevURL, prevClient
@@ -616,22 +625,31 @@ func TestIntegration_ResourcesEmpty(t *testing.T) {
616625
}
617626
}
618627

619-
// TestIntegration_ResourcesUnauthorized asserts a 401 prints a friendly hint
620-
// and exits 0 (the CLI treats "not logged in" as informational, not fatal).
628+
// TestIntegration_ResourcesUnauthorized — T16 P1-2 fix.
629+
// Anonymous caller (no auth) on a 401 prints a friendly hint AND exits with
630+
// ExitAuthRequired (3) so an agent can branch on the code. The previous
631+
// behaviour was exit 0, which silently masked a stale-token situation.
621632
func TestIntegration_ResourcesUnauthorized(t *testing.T) {
622633
c := newITContext(t)
623634
c.mock.mu.Lock()
624635
c.mock.requireAuth = true
625636
c.mock.mu.Unlock()
626637

638+
var gotErr error
627639
_, stderr := captureStdout(t, func() {
628640
_, _, err := run("resources")
629-
if err != nil {
630-
t.Fatalf("resources unauthorized should exit 0, got err: %v", err)
631-
}
641+
gotErr = err
632642
})
633-
if !strings.Contains(strings.ToLower(stderr), "logged in") {
634-
t.Errorf("resources 401: expected 'logged in' hint, stderr=%q", stderr)
643+
if gotErr == nil {
644+
t.Fatal("resources 401 must now return a non-nil error (auth required)")
645+
}
646+
if code := ExitCodeFor(gotErr); code != ExitAuthRequired {
647+
t.Errorf("resources 401: expected exit code %d (ExitAuthRequired), got %d (err: %v)",
648+
ExitAuthRequired, code, gotErr)
649+
}
650+
if !strings.Contains(strings.ToLower(stderr+gotErr.Error()), "log") {
651+
t.Errorf("resources 401: expected a login-related hint, stderr=%q err=%q",
652+
stderr, gotErr)
635653
}
636654
}
637655

cmd/monitor.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,10 @@ type provisionResponse struct {
139139
}
140140

141141
// provisionResource calls POST {APIBaseURL}{endpoint} and returns parsed credentials.
142+
//
143+
// T16 P1-2: a 401 against an authenticated request returns the uniform
144+
// errSessionExpired() error so the exit-code contract is consistent across
145+
// `resources`, `up`, and direct provisioning.
142146
func provisionResource(endpoint, name string) (*provisionResponse, error) {
143147
url := APIBaseURL + endpoint
144148
body, _ := json.Marshal(map[string]string{"name": name})
@@ -150,8 +154,11 @@ func provisionResource(endpoint, name string) (*provisionResponse, error) {
150154
defer resp.Body.Close()
151155

152156
raw, _ := io.ReadAll(resp.Body)
157+
if resp.StatusCode == http.StatusUnauthorized && haveAuth() {
158+
return nil, errSessionExpired()
159+
}
153160
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated {
154-
return nil, fmt.Errorf("server returned %d: %s", resp.StatusCode, raw)
161+
return nil, fmt.Errorf("server returned %d: %s", resp.StatusCode, truncate(string(raw), 200))
155162
}
156163

157164
var result provisionResponse

cmd/root.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88

99
"github.com/spf13/cobra"
1010
"github.com/instant-dev/cli/internal/cliconfig"
11+
"github.com/instant-dev/cli/internal/secretstore"
1112
)
1213

1314
// APIBaseURL is the instanode.dev API base URL.
@@ -70,6 +71,12 @@ func init() {
7071
}
7172

7273
func initConfig() {
74+
// Wire up the secret backend BEFORE loading cliconfig (cliconfig.Load
75+
// reads the bearer token via secretstore.Get). On test runs HOME points
76+
// to a temp dir and INSTANT_DISABLE_KEYCHAIN=1 is set; the OS keychain
77+
// is then bypassed and the cliconfig file-fallback path is exercised.
78+
secretstore.UseDefault()
79+
7380
// Load saved CLI credentials (may be empty = anonymous).
7481
cfg, err := cliconfig.Load()
7582
if err != nil {

cmd/shellquote.go

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
package cmd
2+
3+
// shellquote.go — POSIX shell-safe quoting for the `--emit-env` path of
4+
// `instant up`. The previous implementation used Go's %q verb which produces
5+
// a DOUBLE-quoted Go string — that happens to coincide with shell quoting
6+
// for boring values but is INSECURE for hostile ones: a connection URL
7+
// containing `$(...)`, backticks, `!`, or an embedded `"` was either a parse
8+
// error or a command-injection vector via `eval $(instant up --emit-env)`.
9+
//
10+
// POSIX shell single-quotes are the safe choice:
11+
//
12+
// - Everything between '...' is literal — no $, no `, no \, no !.
13+
// - The ONE thing that needs escaping is the single quote itself, which
14+
// POSIX does by closing the quoted run, emitting an escaped quote
15+
// (\'), and reopening: `it's` becomes `'it'\''s'`.
16+
//
17+
// This is the same algorithm shellwords / Python shlex.quote / Bash
18+
// `printf %q` (modulo Bash-specific extensions) implement.
19+
20+
import (
21+
"strings"
22+
)
23+
24+
// shellQuote returns a POSIX-shell-safe quoted form of s, suitable for
25+
// inclusion on a line that will be evaluated by `eval` or sourced by `sh`.
26+
//
27+
// shellQuote("") -> "''"
28+
// shellQuote("simple") -> "'simple'"
29+
// shellQuote("with space") -> "'with space'"
30+
// shellQuote("it's a $TEST") -> "'it'\\''s a $TEST'" (literal $TEST)
31+
// shellQuote("`whoami`") -> "'`whoami`'" (literal backticks)
32+
//
33+
// The returned string is always wrapped in single quotes so a downstream
34+
// `eval` cannot interpret the contents. Use this for any value the CLI
35+
// emits as part of `export NAME=...`.
36+
func shellQuote(s string) string {
37+
if s == "" {
38+
return "''"
39+
}
40+
// Escape embedded single quotes via the POSIX idiom: close the quoted
41+
// run, emit an escaped quote, reopen.
42+
if !strings.ContainsRune(s, '\'') {
43+
return "'" + s + "'"
44+
}
45+
var b strings.Builder
46+
b.Grow(len(s) + 8)
47+
b.WriteByte('\'')
48+
for _, r := range s {
49+
if r == '\'' {
50+
b.WriteString(`'\''`)
51+
continue
52+
}
53+
b.WriteRune(r)
54+
}
55+
b.WriteByte('\'')
56+
return b.String()
57+
}
58+
59+
// isValidShellIdentifier reports whether name is a syntactically valid
60+
// POSIX shell variable name: starts with [A-Z_], followed by [A-Z0-9_].
61+
// We restrict to UPPER_SNAKE_CASE because that's the convention for
62+
// environment variables and refusing mixed-case is a forcing function that
63+
// surfaces typos.
64+
func isValidShellIdentifier(name string) bool {
65+
if name == "" {
66+
return false
67+
}
68+
for i, r := range name {
69+
ok := r == '_' || (r >= 'A' && r <= 'Z')
70+
if i > 0 {
71+
ok = ok || (r >= '0' && r <= '9')
72+
}
73+
if !ok {
74+
return false
75+
}
76+
}
77+
return true
78+
}
79+
80+
// sanitizeExportName turns a user-provided manifest resource name into a
81+
// valid shell identifier. The rules:
82+
//
83+
// - lowercase letters become uppercase
84+
// - dashes, spaces, dots, slashes become underscores
85+
// - a leading digit gets a `R_` prefix (resource_)
86+
// - everything else is dropped
87+
//
88+
// If the result is empty after sanitization, we return "" so the caller
89+
// can choose to error rather than emit an invalid `export = ...` line.
90+
func sanitizeExportName(name string) string {
91+
var b strings.Builder
92+
for i, r := range name {
93+
switch {
94+
case r >= 'a' && r <= 'z':
95+
b.WriteRune(r - 'a' + 'A')
96+
case r >= 'A' && r <= 'Z':
97+
b.WriteRune(r)
98+
case r >= '0' && r <= '9':
99+
if i == 0 {
100+
b.WriteString("R_")
101+
}
102+
b.WriteRune(r)
103+
case r == '_', r == '-', r == ' ', r == '.', r == '/':
104+
b.WriteByte('_')
105+
}
106+
// anything else (punctuation, unicode) is dropped
107+
}
108+
return b.String()
109+
}

0 commit comments

Comments
 (0)