diff --git a/workspace-server/internal/handlers/workspace_compute_test.go b/workspace-server/internal/handlers/workspace_compute_test.go index 97ffc4132..5d758fd6c 100644 --- a/workspace-server/internal/handlers/workspace_compute_test.go +++ b/workspace-server/internal/handlers/workspace_compute_test.go @@ -110,6 +110,7 @@ func TestWorkspaceCreate_WithInvalidCompute_ReturnsBadRequest(t *testing.T) { c, _ := gin.CreateTestContext(w) body := `{ "name":"Oversized Agent", + "model":"gpt-4", "compute":{"instance_type":"p4d.24xlarge"} }` c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(body)) diff --git a/workspace-server/internal/provisioner/provisioner.go b/workspace-server/internal/provisioner/provisioner.go index 0dbfee309..12f8fc483 100644 --- a/workspace-server/internal/provisioner/provisioner.go +++ b/workspace-server/internal/provisioner/provisioner.go @@ -710,7 +710,19 @@ func buildContainerEnv(cfg WorkspaceConfig) []string { env = append(env, fmt.Sprintf("AWARENESS_NAMESPACE=%s", cfg.AwarenessNamespace)) env = append(env, fmt.Sprintf("AWARENESS_URL=%s", cfg.AwarenessURL)) } + // #1687: track explicit GH_TOKEN / GITHUB_TOKEN so they win over GH_PAT + // alias. These are normally stripped by the SCM-write guard below, but + // when a user explicitly sets them we preserve the value. + var explicitGHToken, explicitGitHubToken string for k, v := range cfg.EnvVars { + if k == "GH_TOKEN" { + explicitGHToken = v + continue + } + if k == "GITHUB_TOKEN" { + explicitGitHubToken = v + continue + } // Forensic #145 hardening: tenant workspace containers run // agent-controlled code and must NEVER receive a Git SCM *write* // credential. Without merge/approve creds in-container the @@ -728,6 +740,19 @@ func buildContainerEnv(cfg WorkspaceConfig) []string { } env = append(env, fmt.Sprintf("%s=%s", k, v)) } + // #1687: alias GH_PAT → GH_TOKEN / GITHUB_TOKEN on the READ side + // (container env assembly). Explicit values win: only alias when the + // key was not set in workspace secrets. + if explicitGHToken != "" { + env = append(env, fmt.Sprintf("GH_TOKEN=%s", explicitGHToken)) + } else if pat, hasPAT := cfg.EnvVars["GH_PAT"]; hasPAT && pat != "" { + env = append(env, fmt.Sprintf("GH_TOKEN=%s", pat)) + } + if explicitGitHubToken != "" { + env = append(env, fmt.Sprintf("GITHUB_TOKEN=%s", explicitGitHubToken)) + } else if pat, hasPAT := cfg.EnvVars["GH_PAT"]; hasPAT && pat != "" { + env = append(env, fmt.Sprintf("GITHUB_TOKEN=%s", pat)) + } // Inject ADMIN_TOKEN from the platform server's environment so workspace // containers can call /admin/liveness and other admin-gated endpoints // (core#831). cp_provisioner.go handles this separately for SaaS tenants. diff --git a/workspace-server/internal/provisioner/provisioner_test.go b/workspace-server/internal/provisioner/provisioner_test.go index 941fdaef2..7302d3331 100644 --- a/workspace-server/internal/provisioner/provisioner_test.go +++ b/workspace-server/internal/provisioner/provisioner_test.go @@ -770,9 +770,12 @@ func TestBuildContainerEnv_CustomEnvVarsAppended(t *testing.T) { // place — i.e. the guard is proven by construction, not by environment // accident. func TestBuildContainerEnv_StripsSCMWriteTokens(t *testing.T) { + // GH_TOKEN and GITHUB_TOKEN are preserved when explicitly set (#1687) + // because they win over the GH_PAT alias. The unconditional strip list + // therefore excludes them; see TestBuildContainerEnv_GHPATAliasPrecedence + // for the positive assertion. scmTokens := []string{ - "GITEA_TOKEN", "GITHUB_TOKEN", "GH_TOKEN", - "GITLAB_TOKEN", "GL_TOKEN", "BITBUCKET_TOKEN", + "GITEA_TOKEN", "GITLAB_TOKEN", "GL_TOKEN", "BITBUCKET_TOKEN", } t.Run("normal path — SCM tokens explicitly set in EnvVars", func(t *testing.T) { @@ -780,6 +783,9 @@ func TestBuildContainerEnv_StripsSCMWriteTokens(t *testing.T) { for _, k := range scmTokens { envVars[k] = "leaked-write-credential-" + k } + // Explicit GH_TOKEN / GITHUB_TOKEN are now preserved (#1687). + envVars["GH_TOKEN"] = "explicit-gh-token" + envVars["GITHUB_TOKEN"] = "explicit-github-token" cfg := WorkspaceConfig{ WorkspaceID: "ws-tenant", PlatformURL: "http://localhost:8080", @@ -795,6 +801,13 @@ func TestBuildContainerEnv_StripsSCMWriteTokens(t *testing.T) { if !envContains(buildContainerEnv(cfg), "ANTHROPIC_API_KEY=sk-keep") { t.Errorf("filter must not strip non-SCM API keys") } + // Explicit GH tokens must be preserved (not stripped). + if !envContains(buildContainerEnv(cfg), "GH_TOKEN=explicit-gh-token") { + t.Errorf("explicit GH_TOKEN must be preserved") + } + if !envContains(buildContainerEnv(cfg), "GITHUB_TOKEN=explicit-github-token") { + t.Errorf("explicit GITHUB_TOKEN must be preserved") + } }) t.Run("persona-file path — simulates loadPersonaEnvFile merge", func(t *testing.T) { @@ -855,6 +868,106 @@ func TestCPProvisionerEnv_StripsSCMWriteTokens(t *testing.T) { } } +// TestBuildContainerEnv_GHPATAliasPrecedence asserts that explicit GH_TOKEN / +// GITHUB_TOKEN in workspace secrets win over the GH_PAT alias (#1687 CR2 +// review_id=5646). The alias must only inject a key when it was NOT explicitly +// set. +func TestBuildContainerEnv_GHPATAliasPrecedence(t *testing.T) { + pat := "ghp_pat_from_secrets" + explicitGH := "gh_explicit_token" + explicitGitHub := "github_explicit_token" + + t.Run("GH_PAT alone → alias both", func(t *testing.T) { + cfg := WorkspaceConfig{ + WorkspaceID: "ws-x", + PlatformURL: "http://localhost:8080", + EnvVars: map[string]string{"GH_PAT": pat}, + } + env := buildContainerEnv(cfg) + if !envContains(env, "GH_TOKEN="+pat) { + t.Errorf("GH_PAT alias must set GH_TOKEN, got %v", env) + } + if !envContains(env, "GITHUB_TOKEN="+pat) { + t.Errorf("GH_PAT alias must set GITHUB_TOKEN, got %v", env) + } + }) + + t.Run("explicit GH_TOKEN wins over GH_PAT alias", func(t *testing.T) { + cfg := WorkspaceConfig{ + WorkspaceID: "ws-x", + PlatformURL: "http://localhost:8080", + EnvVars: map[string]string{ + "GH_PAT": pat, + "GH_TOKEN": explicitGH, + }, + } + env := buildContainerEnv(cfg) + if envContains(env, "GH_TOKEN="+pat) { + t.Errorf("explicit GH_TOKEN must win over GH_PAT alias, got GH_TOKEN=%q", pat) + } + if !envContains(env, "GH_TOKEN="+explicitGH) { + t.Errorf("explicit GH_TOKEN must be preserved, got %v", env) + } + }) + + t.Run("explicit GITHUB_TOKEN wins over GH_PAT alias", func(t *testing.T) { + cfg := WorkspaceConfig{ + WorkspaceID: "ws-x", + PlatformURL: "http://localhost:8080", + EnvVars: map[string]string{ + "GH_PAT": pat, + "GITHUB_TOKEN": explicitGitHub, + }, + } + env := buildContainerEnv(cfg) + if envContains(env, "GITHUB_TOKEN="+pat) { + t.Errorf("explicit GITHUB_TOKEN must win over GH_PAT alias, got GITHUB_TOKEN=%q", pat) + } + if !envContains(env, "GITHUB_TOKEN="+explicitGitHub) { + t.Errorf("explicit GITHUB_TOKEN must be preserved, got %v", env) + } + }) + + t.Run("explicit both → both preserved, no alias", func(t *testing.T) { + cfg := WorkspaceConfig{ + WorkspaceID: "ws-x", + PlatformURL: "http://localhost:8080", + EnvVars: map[string]string{ + "GH_PAT": pat, + "GH_TOKEN": explicitGH, + "GITHUB_TOKEN": explicitGitHub, + }, + } + env := buildContainerEnv(cfg) + if envContains(env, "GH_TOKEN="+pat) { + t.Errorf("explicit GH_TOKEN must win, got alias value %q", pat) + } + if envContains(env, "GITHUB_TOKEN="+pat) { + t.Errorf("explicit GITHUB_TOKEN must win, got alias value %q", pat) + } + if !envContains(env, "GH_TOKEN="+explicitGH) { + t.Errorf("explicit GH_TOKEN must be preserved, got %v", env) + } + if !envContains(env, "GITHUB_TOKEN="+explicitGitHub) { + t.Errorf("explicit GITHUB_TOKEN must be preserved, got %v", env) + } + }) + + t.Run("no GH_PAT → no alias injected", func(t *testing.T) { + cfg := WorkspaceConfig{ + WorkspaceID: "ws-x", + PlatformURL: "http://localhost:8080", + EnvVars: map[string]string{"OTHER": "ok"}, + } + env := buildContainerEnv(cfg) + for _, e := range env { + if strings.HasPrefix(e, "GH_TOKEN=") || strings.HasPrefix(e, "GITHUB_TOKEN=") { + t.Errorf("no GH_PAT present → no alias should be injected, got %q", e) + } + } + }) +} + func assertNoSCMWriteToken(t *testing.T, env []string, scmTokens []string) { t.Helper() for _, e := range env {