From 1be1a2ac18aaa7bd7bba7a800aa70380ebc6ba4d Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 15 Oct 2026 10:45:27 +0100 Subject: [PATCH 1/2] test: cover curl commands, sensitive variables and logout Add integration tests for security-relevant behavior: - api:curl refuses a path with a host (an absolute or scheme-relative URL) without sending a request, censors the access token in verbose output, and passes -X, --json and -H to curl. - api:curl, project:curl, environment:curl and organization:curl request paths under their own base path. - Sensitive variables at project and environment level: the value is sent on create and update but not displayed by variable:get or variable:list, and a variable can be updated to be sensitive. - auth:logout of the current session revokes its tokens and deletes its files, warns when an API token is set via config, and --other keeps the current session when there are no others. - auth:api-token-login is refused when an API token is set via config. Co-Authored-By: Claude Opus 5.5 --- integration-tests/api_curl_test.go | 167 ++++++++++++++++++ .../auth_api_token_login_test.go | 15 ++ integration-tests/auth_logout_test.go | 56 ++++++ integration-tests/variable_write_test.go | 91 +++++++++- 4 files changed, 328 insertions(+), 1 deletion(-) diff --git a/integration-tests/api_curl_test.go b/integration-tests/api_curl_test.go index 6c715eb0..56599a06 100644 --- a/integration-tests/api_curl_test.go +++ b/integration-tests/api_curl_test.go @@ -2,15 +2,18 @@ package tests import ( "encoding/json" + "io" "net/http" "net/http/httptest" "os/exec" + "slices" "strings" "sync" "testing" "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/upsun/cli/pkg/mockapi" ) @@ -106,3 +109,167 @@ func TestApiCurlCommand(t *testing.T) { assert.Empty(t, stdOut) assert.Equal(t, 2, state.getFetches()) } + +type recordedRequest struct { + Method string + Path string + Header http.Header + Body string +} + +// requestRecorder records the requests a handler receives. +type requestRecorder struct { + mu sync.Mutex + reqs []recordedRequest + next http.Handler +} + +func (r *requestRecorder) ServeHTTP(w http.ResponseWriter, req *http.Request) { + body, _ := io.ReadAll(req.Body) + req.Body = io.NopCloser(strings.NewReader(string(body))) + r.mu.Lock() + r.reqs = append(r.reqs, recordedRequest{ + Method: req.Method, Path: req.URL.Path, Header: req.Header.Clone(), Body: string(body), + }) + r.mu.Unlock() + r.next.ServeHTTP(w, req) +} + +func (r *requestRecorder) requests() []recordedRequest { + r.mu.Lock() + defer r.mu.Unlock() + return slices.Clone(r.reqs) +} + +// requestsTo returns the recorded requests to a path. +func (r *requestRecorder) requestsTo(path string) []recordedRequest { + return slices.DeleteFunc(r.requests(), func(req recordedRequest) bool { return req.Path != path }) +} + +// TestApiCurl_RejectsHost checks that a path with a host is refused, so the access token is not sent to another host. +func TestApiCurl_RejectsHost(t *testing.T) { + t.Parallel() + authServer := mockapi.NewAuthServer(t) + defer authServer.Close() + apiRecorder := &requestRecorder{next: mockapi.NewHandler(t)} + apiServer := httptest.NewServer(apiRecorder) + defer apiServer.Close() + otherRecorder := &requestRecorder{next: http.NotFoundHandler()} + otherServer := httptest.NewServer(otherRecorder) + defer otherServer.Close() + + f := newCommandFactory(t, apiServer.URL, authServer.URL) + otherHost := strings.TrimPrefix(otherServer.URL, "http://") + for _, path := range []string{otherServer.URL + "/steal", "//" + otherHost + "/steal"} { + stdout, stderr, err := f.RunCombinedOutput("api:curl", path) + assertExitCode(t, 1, err) + assert.Empty(t, stdout) + assert.Contains(t, stderr, "Invalid path: "+path) + } + assert.Empty(t, otherRecorder.requests()) + assert.Empty(t, apiRecorder.requests()) +} + +func TestApiCurl_Options(t *testing.T) { + t.Parallel() + authServer := mockapi.NewAuthServer(t) + defer authServer.Close() + apiHandler := mockapi.NewHandler(t) + apiHandler.HandleFunc("/curl-test", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("success")) + }) + apiRecorder := &requestRecorder{next: apiHandler} + apiServer := httptest.NewServer(apiRecorder) + defer apiServer.Close() + + f := newCommandFactory(t, apiServer.URL, authServer.URL) + + assert.Equal(t, "success", f.Run("api:curl", "/curl-test", "-X", "PATCH", + "--json", `{"foo":"bar"}`, "-H", "X-Test: one", "-H", "X-Other: two")) + reqs := apiRecorder.requestsTo("/curl-test") + require.Len(t, reqs, 1) + assert.Equal(t, "PATCH", reqs[0].Method) + assert.JSONEq(t, `{"foo":"bar"}`, reqs[0].Body) + assert.Equal(t, "application/json", reqs[0].Header.Get("Content-Type")) + assert.Equal(t, "application/json", reqs[0].Header.Get("Accept")) + assert.Equal(t, "one", reqs[0].Header.Get("X-Test")) + assert.Equal(t, "two", reqs[0].Header.Get("X-Other")) + assert.Equal(t, "Bearer access-token-1", reqs[0].Header.Get("Authorization")) + + _, stderr, err := f.RunCombinedOutput("api:curl", "/curl-test", "--json", "{invalid") + assert.Error(t, err) + assert.Contains(t, stderr, "The value of --json contains invalid JSON.") + assert.Len(t, apiRecorder.requestsTo("/curl-test"), 1) +} + +// TestApiCurl_CensorsToken checks that the access token is replaced by "[token]" in verbose output. +func TestApiCurl_CensorsToken(t *testing.T) { + t.Parallel() + authServer := mockapi.NewAuthServer(t) + defer authServer.Close() + apiHandler := mockapi.NewHandler(t) + apiHandler.HandleFunc("/curl-test", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("success")) + }) + apiServer := httptest.NewServer(apiHandler) + defer apiServer.Close() + + f := newCommandFactory(t, apiServer.URL, authServer.URL) + + for _, verbosity := range []string{"-v", "-vv"} { + stdout, stderr, err := f.RunCombinedOutput("api:curl", "/curl-test", verbosity) + require.NoError(t, err) + assert.Equal(t, "success", stdout) + assert.Contains(t, stderr, "Running command: curl -H 'Authorization: Bearer [token]'") + assert.NotContains(t, stderr, "access-token-1") + if verbosity == "-vv" { + // The curl --verbose output. + assert.Contains(t, stderr, "> Authorization: Bearer [token]") + } + } +} + +// TestCurlCommands_BasePath checks the base path of each of the curl commands. +func TestCurlCommands_BasePath(t *testing.T) { + t.Parallel() + authServer := mockapi.NewAuthServer(t) + defer authServer.Close() + apiHandler := mockapi.NewHandler(t) + apiRecorder := &requestRecorder{next: apiHandler} + apiServer := httptest.NewServer(apiRecorder) + defer apiServer.Close() + + projectID := mockapi.ProjectID() + envPath := "/projects/" + projectID + "/environments/main" + apiHandler.SetProjects([]*mockapi.Project{{ + ID: projectID, + Links: mockapi.MakeHALLinks( + "self=/projects/"+projectID, + "environments=/projects/"+projectID+"/environments", + ), + DefaultBranch: "main", + }}) + apiHandler.SetEnvironments([]*mockapi.Environment{makeEnv(projectID, "main", "production", "active", nil)}) + apiHandler.SetOrgs([]*mockapi.Org{makeOrg("org-id-1", "org-1", "Org 1", "my-user-id", "flexible")}) + + cases := []struct { + args []string + path string + }{ + {[]string{"api:curl"}, "/curl-test"}, + {[]string{"project:curl", "-p", projectID}, "/projects/" + projectID + "/curl-test"}, + {[]string{"environment:curl", "-p", projectID, "-e", "main"}, envPath + "/curl-test"}, + {[]string{"organization:curl", "-o", "org-1"}, "/organizations/org-id-1/curl-test"}, + } + for _, c := range cases { + apiHandler.HandleFunc(c.path, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("success")) + }) + } + + f := newCommandFactory(t, apiServer.URL, authServer.URL) + for _, c := range cases { + assert.Equal(t, "success", f.Run(append(c.args, "/curl-test")...), c.args[0]) + assert.Len(t, apiRecorder.requestsTo(c.path), 1, c.args[0]) + } +} diff --git a/integration-tests/auth_api_token_login_test.go b/integration-tests/auth_api_token_login_test.go index 8194b48b..e3f7af0c 100644 --- a/integration-tests/auth_api_token_login_test.go +++ b/integration-tests/auth_api_token_login_test.go @@ -70,6 +70,21 @@ func TestAuthAPITokenLogin_CommandAfterLogin(t *testing.T) { assert.Contains(t, out, "acme") } +// TestAuthAPITokenLogin_ConfiguredToken checks that the login is refused when an API token is set via config. +func TestAuthAPITokenLogin_ConfiguredToken(t *testing.T) { + t.Parallel() + authServer := mockapi.NewAuthServer(t) + defer authServer.Close() + apiServer := newMyUserAPI(t) + defer apiServer.Close() + + f := newCommandFactory(t, apiServer.URL, authServer.URL) + _, stderr, err := f.RunInteractive(mockapi.ValidAPITokens[0]+"\n", "auth:api-token-login") + assertExitCode(t, 1, err) + assert.Contains(t, stderr, "An API token is already set via config") + assert.NotContains(t, stderr, "Please enter an API token") +} + func TestAuthAPITokenLogin_NonInteractive(t *testing.T) { t.Parallel() f := newCommandFactory(t, "", "") diff --git a/integration-tests/auth_logout_test.go b/integration-tests/auth_logout_test.go index 84e84e33..12388439 100644 --- a/integration-tests/auth_logout_test.go +++ b/integration-tests/auth_logout_test.go @@ -2,6 +2,7 @@ package tests import ( "net/http/httptest" + "os" "path/filepath" "testing" "time" @@ -26,6 +27,61 @@ func TestAuthLogout_Single(t *testing.T) { _, stderr, err := f.RunCombinedOutput("auth:logout") require.NoError(t, err) assert.Contains(t, stderr, "logged out") + // The factory sets an API token via the environment, which cannot be removed. + assert.Contains(t, stderr, "Warning: an API token is set via config") +} + +// TestAuthLogout_SingleRevokesSession checks that logging out of the current session revokes its tokens and deletes +// its files. +func TestAuthLogout_SingleRevokesSession(t *testing.T) { + t.Parallel() + authServer := mockapi.NewAuthServer(t) + defer authServer.Close() + + f := newCommandFactory(t, "", authServer.URL) + f.extraEnv = append(f.extraEnv, EnvPrefix+"TOKEN=") + writeOAuthSession(t, f.home, "default", map[string]any{ + "accessToken": "default-access", "refreshToken": "default-refresh", "tokenType": "bearer", + "expires": time.Now().Add(time.Hour).Unix(), + }) + sessDir := filepath.Join(f.home, ".platform-test-cli", ".session") + certFile := filepath.Join(sessDir, "sess-cli-default", "ssh", "id_ed25519-cert.pub") + require.NoError(t, os.WriteFile(certFile, []byte("cert"), 0o600)) + + _, stderr, err := f.RunCombinedOutput("auth:logout") + require.NoError(t, err, "stderr: %s", stderr) + assert.Contains(t, stderr, "You are now logged out.") + assert.NotContains(t, stderr, "Warning: an API token is set via config") + assert.NotContains(t, stderr, "Other sessions exist") + + assert.ElementsMatch(t, []string{"default-access", "default-refresh"}, authServer.RevokedTokens()) + assert.NoFileExists(t, filepath.Join(sessDir, "sess-default", "sess-default.json")) + assert.NoFileExists(t, certFile) + assert.NoFileExists(t, filepath.Join(f.home, ".platform-test-cli", "auth", "default.json")) + + _, _, err = f.RunCombinedOutput("auth:token", "--no-warn") + assertExitCode(t, 3, err) +} + +// TestAuthLogout_OtherWithoutOtherSessions checks that --other keeps the current session. +func TestAuthLogout_OtherWithoutOtherSessions(t *testing.T) { + t.Parallel() + authServer := mockapi.NewAuthServer(t) + defer authServer.Close() + + f := newCommandFactory(t, "", authServer.URL) + f.extraEnv = append(f.extraEnv, EnvPrefix+"TOKEN=") + writeOAuthSession(t, f.home, "default", map[string]any{ + "accessToken": "default-access", "refreshToken": "default-refresh", "tokenType": "bearer", + "expires": time.Now().Add(time.Hour).Unix(), + }) + + _, stderr, err := f.RunCombinedOutput("auth:logout", "--other") + require.NoError(t, err, "stderr: %s", stderr) + assert.Contains(t, stderr, "The current session ID is: default") + assert.Contains(t, stderr, "No other sessions exist.") + assert.Empty(t, authServer.RevokedTokens()) + assert.Equal(t, "default-access", f.Run("auth:token", "--no-warn")) } // TestAuthLogout_OtherSessionsHint: when other sessions still exist after a single logout, diff --git a/integration-tests/variable_write_test.go b/integration-tests/variable_write_test.go index ed7a4ddb..5ae54901 100644 --- a/integration-tests/variable_write_test.go +++ b/integration-tests/variable_write_test.go @@ -1,10 +1,14 @@ package tests import ( + "encoding/json" + "fmt" "net/http/httptest" + "slices" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/upsun/cli/pkg/mockapi" ) @@ -14,6 +18,7 @@ type variableTestSetup struct { authServer *mockapi.AuthServer apiServer *httptest.Server apiHandler *mockapi.Handler + recorder *requestRecorder projectID string mainEnv *mockapi.Environment factory *cmdFactory @@ -25,7 +30,8 @@ func setupVariableTest(t *testing.T) *variableTestSetup { t.Cleanup(authServer.Close) apiHandler := mockapi.NewHandler(t) - apiServer := httptest.NewServer(apiHandler) + recorder := &requestRecorder{next: apiHandler} + apiServer := httptest.NewServer(recorder) t.Cleanup(apiServer.Close) projectID := mockapi.ProjectID() @@ -47,6 +53,7 @@ func setupVariableTest(t *testing.T) *variableTestSetup { authServer: authServer, apiServer: apiServer, apiHandler: apiHandler, + recorder: recorder, projectID: projectID, mainEnv: mainEnv, factory: newCommandFactory(t, apiServer.URL, authServer.URL), @@ -251,3 +258,85 @@ func TestVariableCreateWithAppScopeNoDeployment(t *testing.T) { out := f.Run("var:get", "-p", p, "-l", "p", "env:ANY_APP", "-P", "application_scope") assert.Contains(t, out, "anyapp") } + +// TestVariableSensitive checks that sensitive values are sent to the API, but not displayed. +func TestVariableSensitive(t *testing.T) { + t.Parallel() + cases := []struct { + level string + path string + args []string + }{ + {"project", "/projects/%s/variables", []string{"-l", "p"}}, + {"environment", "/projects/%s/environments/main/variables", []string{"-l", "e", "-e", "main"}}, + } + for _, c := range cases { + t.Run(c.level, func(t *testing.T) { + t.Parallel() + s := setupVariableTest(t) + s.apiHandler.SetEnvironments([]*mockapi.Environment{s.mainEnv}) + f, p := s.factory, s.projectID + path := fmt.Sprintf(c.path, p) + args := append([]string{"-p", p}, c.args...) + + stdout, stderr, err := f.RunCombinedOutput(append([]string{"var:create", "env:SECRET", + "--value", "secret-value-1", "--sensitive", "true"}, args...)...) + require.NoError(t, err, "stderr: %s", stderr) + assert.NotContains(t, stdout+stderr, "secret-value-1") + reqs := slices.DeleteFunc(s.recorder.requestsTo(path), func(r recordedRequest) bool { return r.Method != "POST" }) + require.Len(t, reqs, 1) + assert.JSONEq(t, `true`, jsonField(t, reqs[0].Body, "is_sensitive")) + assert.JSONEq(t, `"secret-value-1"`, jsonField(t, reqs[0].Body, "value")) + + stdout, stderr, err = f.RunCombinedOutput(append([]string{"var:update", "env:SECRET", + "--value", "secret-value-2"}, args...)...) + require.NoError(t, err, "stderr: %s", stderr) + assert.NotContains(t, stdout+stderr, "secret-value") + reqs = slices.DeleteFunc(s.recorder.requestsTo(path+"/env:SECRET"), + func(r recordedRequest) bool { return r.Method != "PATCH" }) + require.Len(t, reqs, 1) + assert.JSONEq(t, `"secret-value-2"`, jsonField(t, reqs[0].Body, "value")) + + assertTrimmed(t, "true", f.Run(append([]string{"var:get", "env:SECRET", "-P", "is_sensitive"}, args...)...)) + + stdout, stderr, err = f.RunCombinedOutput(append([]string{"var:get", "env:SECRET"}, args...)...) + require.NoError(t, err, "stderr: %s", stderr) + assert.Contains(t, stdout, "env:SECRET") + assert.NotContains(t, stdout+stderr, "secret-value") + + _, stderr, err = f.RunCombinedOutput(append([]string{"var:get", "env:SECRET", "-P", "value"}, args...)...) + assertExitCode(t, 1, err) + assert.Contains(t, stderr, "The variable is sensitive, so its value cannot be read.") + + assertTrimmed(t, "env:SECRET,"+c.level+",", + f.Run("var:list", "-p", p, "-e", "main", "--format", "csv", "--columns", "name,level,value", "--no-header")) + }) + } +} + +// TestVariableUpdateToSensitive checks that an existing variable can be made sensitive. +func TestVariableUpdateToSensitive(t *testing.T) { + t.Parallel() + s := setupVariableTest(t) + s.apiHandler.SetEnvironments([]*mockapi.Environment{s.mainEnv}) + f, p := s.factory, s.projectID + + f.Run("var:create", "-p", p, "-l", "p", "env:TOKEN", "--value", "token-value") + assertTrimmed(t, "token-value", f.Run("var:get", "-p", p, "-l", "p", "env:TOKEN", "-P", "value")) + + f.Run("var:update", "-p", p, "-l", "p", "env:TOKEN", "--sensitive", "true") + assertTrimmed(t, "true", f.Run("var:get", "-p", p, "-l", "p", "env:TOKEN", "-P", "is_sensitive")) + _, stderr, err := f.RunCombinedOutput("var:get", "-p", p, "-l", "p", "env:TOKEN", "-P", "value") + assertExitCode(t, 1, err) + assert.Contains(t, stderr, "The variable is sensitive") + assert.NotContains(t, f.Run("var:list", "-p", p, "-e", "main", "--format", "tsv"), "token-value") +} + +// jsonField returns the JSON encoding of a field of a JSON object. +func jsonField(t *testing.T, data, field string) string { + t.Helper() + var m map[string]json.RawMessage + require.NoError(t, json.Unmarshal([]byte(data), &m)) + require.Contains(t, m, field) + return string(m[field]) +} From c37bca7cd0a7d12fbae443f42e1ad950109b9adf Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 15 Oct 2026 10:49:10 +0100 Subject: [PATCH 2/2] test(api:curl): drop the other-host server from the host check test The path is appended to the API URL, so curl would never contact another server even without the host check. Assert only that no request is sent. Co-Authored-By: Claude Opus 5.5 --- integration-tests/api_curl_test.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/integration-tests/api_curl_test.go b/integration-tests/api_curl_test.go index 56599a06..3e31d012 100644 --- a/integration-tests/api_curl_test.go +++ b/integration-tests/api_curl_test.go @@ -146,7 +146,8 @@ func (r *requestRecorder) requestsTo(path string) []recordedRequest { return slices.DeleteFunc(r.requests(), func(req recordedRequest) bool { return req.Path != path }) } -// TestApiCurl_RejectsHost checks that a path with a host is refused, so the access token is not sent to another host. +// TestApiCurl_RejectsHost checks that a path with a host is refused without sending a request. +// The path is appended to the API URL, so the host check is a second safeguard against sending the token elsewhere. func TestApiCurl_RejectsHost(t *testing.T) { t.Parallel() authServer := mockapi.NewAuthServer(t) @@ -154,19 +155,14 @@ func TestApiCurl_RejectsHost(t *testing.T) { apiRecorder := &requestRecorder{next: mockapi.NewHandler(t)} apiServer := httptest.NewServer(apiRecorder) defer apiServer.Close() - otherRecorder := &requestRecorder{next: http.NotFoundHandler()} - otherServer := httptest.NewServer(otherRecorder) - defer otherServer.Close() f := newCommandFactory(t, apiServer.URL, authServer.URL) - otherHost := strings.TrimPrefix(otherServer.URL, "http://") - for _, path := range []string{otherServer.URL + "/steal", "//" + otherHost + "/steal"} { + for _, path := range []string{"https://example.com/steal", "//example.com/steal"} { stdout, stderr, err := f.RunCombinedOutput("api:curl", path) assertExitCode(t, 1, err) assert.Empty(t, stdout) assert.Contains(t, stderr, "Invalid path: "+path) } - assert.Empty(t, otherRecorder.requests()) assert.Empty(t, apiRecorder.requests()) }