Repository navigation
Fix server account login to use protected HTTP - #123
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change replaces SSH, SCP, and gcloud credential transfer with protected HTTP account import. It adds hosted Stack authentication, tenant routing, atomic credential persistence, secure token provisioning, generation-aware account loading, GCP and Docker deployment assets, and runtime safeguards. ChangesProtected account import and hosted routing
Deployment and runtime controls
Sequence Diagram(s)sequenceDiagram
participant CLI
participant StackAuth
participant TenantProxy
participant AccountStore
participant Scheduler
CLI->>StackAuth: Start and poll hosted login
StackAuth-->>CLI: Session and team credentials
CLI->>TenantProxy: Exchange authenticated Stack token
TenantProxy->>AccountStore: Create tenant and persist account
AccountStore-->>TenantProxy: Reloaded account snapshot and generation
TenantProxy-->>CLI: Hosted URL and tenant-scoped credentials
AccountStore->>Scheduler: Publish generation-aware account state
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
cmd/subrouter/install_systemd.go (1)
169-177: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExtend atomic writes to the unit and socket files.
writeFileAtomicnow protects the defaults file from partial writes. The unit file (Line 172) and socket file (Line 175) still use plainos.WriteFile. A crash or disk-full condition mid-write can leave a truncatedsubrouter.serviceorsubrouter.socketfile, which then failssystemctl daemon-reloadon next boot.Use
writeFileAtomicfor these writes too.♻️ Proposed fix
- if err := os.WriteFile(systemdUnitPath(config), []byte(unit), 0o644); err != nil { + if err := writeFileAtomic(systemdUnitPath(config), []byte(unit), 0o644); err != nil { return err } - if err := os.WriteFile(systemdSocketPath(config), []byte(socketUnit), 0o644); err != nil { + if err := writeFileAtomic(systemdSocketPath(config), []byte(socketUnit), 0o644); err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/subrouter/install_systemd.go` around lines 169 - 177, Replace the os.WriteFile calls for the systemd unit and socket files in the install flow with writeFileAtomic, preserving their existing paths, contents, and 0o644 permissions while retaining the current error returns.internal/proxy/account_import_test.go (3)
337-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReference
maxAccountImportAccountsinstead of a literal.The test is in package
proxy, so it can use the production constant. A future cap change then updates this test automatically.♻️ Proposed change
- const accountLimit = 256 + const accountLimit = maxAccountImportAccounts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/account_import_test.go` around lines 337 - 338, Update TestAccountImportCapsDistinctAccountsButAllowsCredentialRotation to use the production maxAccountImportAccounts constant instead of defining the literal accountLimit value, so the test follows future cap changes automatically.
20-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two preflight tables into one.
TestAccountImportPreflightRequiresProtectedRemoteAccessandTestAccountImportPreflightRequiresAdminTokenFromLoopbackdiffer only inRemoteAddr. Add aremoteAddrfield to the case struct and run both addresses from one table. This keeps the loopback regression guard while removing the duplicated fixture setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/account_import_test.go` around lines 20 - 78, Merge TestAccountImportPreflightRequiresProtectedRemoteAccess and TestAccountImportPreflightRequiresAdminTokenFromLoopback into one table-driven test, adding a remoteAddr field to each case to cover both protected remote and loopback addresses. Reuse the shared fixture setup and set req.RemoteAddr from the case, preserving all existing status expectations and loopback coverage.
313-335: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case that exercises
http.MaxBytesReader.
httptest.NewRequestwith abytes.ReadersetsContentLength, so the oversized case returns 413 from ther.ContentLength > maxAccountImportBodyBytescheck at proxy.go line 1243. Thehttp.MaxBytesErrorbranch at proxy.go lines 1252-1256 stays uncovered. Add a case with an unknown length, for example a request whoseContentLengthis set to -1, to cover the streaming limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/account_import_test.go` around lines 313 - 335, Add a streaming oversized-body case to TestAccountImportBoundsAndStrictlyParsesCredentialBodies by constructing the import request with ContentLength set to -1, ensuring it reaches the http.MaxBytesReader limit instead of the declared-length check and expects http.StatusRequestEntityTooLarge. Preserve the existing oversized, unknown-field, and trailing-document cases.cmd/subrouter/sr_server.go (1)
1026-1054: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGenerate the token only after the lookup succeeds.
The function assigns
server.AdminTokenat line 1034, then discovers at line 1047 that the server is absent from the file. It returns the mutated copy together with the error. The current caller checks the error first and discards the copy, so behavior is correct today. Moving the lookup before the generation removes the trap for a future caller that ignores the error.♻️ Proposed refactor
if strings.TrimSpace(server.AdminToken) != "" { return server, nil } + file, err := store.load() + if err != nil { + return server, err + } + index := -1 + for i := range file.Servers { + if file.Servers[i].Name == server.Name { + index = i + break + } + } + if index < 0 { + return server, fmt.Errorf("server %q not found", server.Name) + } raw := make([]byte, 32) if _, err := rand.Read(raw); err != nil { return server, fmt.Errorf("generate server control token: %w", err) } server.AdminToken = base64.RawURLEncoding.EncodeToString(raw) - file, err := store.load() - if err != nil { - return server, err - } - found := false - for i := range file.Servers { - if file.Servers[i].Name == server.Name { - file.Servers[i].AdminToken = server.AdminToken - found = true - break - } - } - if !found { - return server, fmt.Errorf("server %q not found", server.Name) - } + file.Servers[index].AdminToken = server.AdminToken if err := store.save(file); err != nil { return server, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/subrouter/sr_server.go` around lines 1026 - 1054, Update ensureServerAdminToken so it loads the server file and confirms server.Name exists before generating random bytes or assigning server.AdminToken. Preserve the existing not-found error and only mutate the server and persist the token after a successful lookup.internal/proxy/proxy.go (2)
1306-1315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
errors.Isfor the EOF check.
json.Decoder.Decodereturns a bareio.EOFtoday, so the comparison works.errors.Iskeeps the check correct if the error is ever wrapped.♻️ Proposed refactor
func rejectTrailingJSON(decoder *json.Decoder) error { var trailing any - if err := decoder.Decode(&trailing); err != io.EOF { + if err := decoder.Decode(&trailing); !errors.Is(err, io.EOF) { if err == nil { return errors.New("trailing JSON value") } return err } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/proxy.go` around lines 1306 - 1315, Update rejectTrailingJSON to use errors.Is when checking whether decoder.Decode returned io.EOF, while preserving the existing handling for a trailing JSON value and other errors.
1338-1369: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated stored-account import block.
The Claude case repeats lines 1327-1337 verbatim at lines 1356-1366. Only the Claude OAuth payload needs its own path. After that early branch returns or breaks, the remaining Claude flow can share the stored-account path with Codex, Kimi, and ZAI.
♻️ Proposed refactor sketch
var accountID string + provider := input.Provider + if provider == accounts.ProviderClaude && input.Claude != nil && input.Codex == nil { + name, credential, err := validateClaudeAccountImport(*input.Claude) + if err != nil { + return "", err + } + if err := s.ensureAccountImportCapacity(name, true); err != nil { + return "", err + } + if err := s.AccountRef.claudeStore.ImportProfileCredential(name, credential); err != nil { + return "", err + } + accountID = name + } else { + switch provider { + case accounts.ProviderCodex, accounts.ProviderKimi, accounts.ProviderZAI, accounts.ProviderClaude: + // single shared stored-account path + default: + return "", invalidAccountImport("unsupported account provider") + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/proxy.go` around lines 1338 - 1369, Refactor the account import switch so the Claude-specific credential path in validateClaudeAccountImport remains isolated, while the stored-account flow is handled once after that branch. Reuse the existing validateStoredAccountImport, ensureAccountImportCapacity, SaveStored, and accountID assignment logic for Claude, Codex, Kimi, and ZAI, preserving the payload validation and unsupported-provider behavior.internal/proxy/multitenant.go (1)
253-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the gate for the new write path.
The
reload-accountsentry records its loopback gate inline.account-importis the second write-capable entry in this map. Add a short comment stating thatrequireAccountImportAuthaccepts it because the tenant key was already validated. This keeps the allowlist self-documenting for future reviewers.♻️ Proposed change
"/_subrouter/reload-accounts": true, // loopback-only inside the Server handler - "/_subrouter/account-import": true, + "/_subrouter/account-import": true, // tenant-key authorized inside the Server handler }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/multitenant.go` around lines 253 - 254, Document the authentication gate for the "/_subrouter/account-import" entry in the allowlist map by adding a short inline comment stating that requireAccountImportAuth accepts it because the tenant key was already validated. Keep the existing route and value unchanged.cmd/subrouter/sr_server_account_import_test.go (1)
164-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
t.Context()for consistency.The other tests in this file use
t.Context(). Line 164 usescontext.Background().t.Context()cancels on test completion and matches the file's convention.♻️ Proposed change
- err := runner.serverLoginOne(context.Background(), srServerConfig{ + err := runner.serverLoginOne(t.Context(), srServerConfig{Drop the
"context"import if no other reference remains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/subrouter/sr_server_account_import_test.go` around lines 164 - 168, Update the serverLoginOne invocation in the relevant test to pass t.Context() instead of context.Background(), preserving the existing test behavior and consistency with the surrounding tests. Remove the context import if it is no longer referenced elsewhere in the file.cmd/subrouter/sr_server_account_import.go (2)
157-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local variable
copy.Line 158 shadows the builtin
copyfor the rest of the function. The function does not use the builtin, so behavior is correct. Rename it toclientfor readability.♻️ Proposed rename
- copy := *base - copy.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + client := *base + client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse } parsed, err := url.Parse(rawURL) if err != nil { return nil, err } if parsed.Scheme != "http" { - return ©, nil + return &client, nil }Apply the same rename to the remaining
copyreferences at Lines 170, 179, 183, and 184.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/subrouter/sr_server_account_import.go` around lines 157 - 168, Rename the local variable copy to client throughout securedServerAccountImportClient, including its declaration and all remaining references, while preserving the existing redirect and URL-handling behavior.
142-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider keeping the error chain after redaction.
errors.New(message)discards the wrapped error.errors.Ischecks forcontext.DeadlineExceededorcontext.Canceledthen fail for any future caller. No current caller inspects the chain, so this is defensive only. A redacting wrapper type preserves both properties.♻️ Optional refactor
type redactedError struct { message string cause error } func (e *redactedError) Error() string { return e.message } func (e *redactedError) Unwrap() error { return e.cause }Then return
&redactedError{message: message, cause: err}.Error()still exposes only the redacted text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/subrouter/sr_server_account_import.go` around lines 142 - 155, Update redactServerAccountImportError to preserve the original error chain while returning the redacted message: introduce or reuse a wrapper implementing Error and Unwrap, and return it with message and err as the cause instead of errors.New(message). Keep the existing redaction behavior and ensure Error exposes only the sanitized text.internal/agents/claude/store.go (1)
314-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the first registry lookup.
Lines 315 and 338 look up
data.Profiles[name]twice. Keep the first result and reuse it.♻️ Proposed refactor
data := s.readProfiles() dir := importedProfileDir(name) - if profile, ok := data.Profiles[name]; ok { - if profile.Dir != "" { - dir = profile.Dir + profile, exists := data.Profiles[name] + if exists { + if profile.Dir != "" { + dir = profile.Dir } else { // Preserve the legacy implicit directory for registries written before // every profile carried an explicit Dir field. dir = sanitizeName(name) } } @@ - profile, ok := data.Profiles[name] - if !ok { + if !exists { profile = Profile{Name: name, CreatedAt: time.Now().UTC().Format(time.RFC3339)} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agents/claude/store.go` around lines 314 - 344, Reuse the initial data.Profiles[name] lookup by retaining its profile and existence result for the later profile initialization in the surrounding store operation. Remove the second lookup before creating a new Profile, while preserving the existing behavior for both existing and missing profiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/subrouter/sr_claude_upload_test.go`:
- Around line 19-71: Update the httptest handler in the Claude upload test to
replace every t.Fatalf call with t.Errorf followed by return so handler failures
report safely without aborting the goroutine; avoid including credential or
token values in assertion messages. Add sync/atomic and change preflightRequests
and importRequests to atomic.Int32 counters, increment them atomically in the
handler, and load them atomically for the final assertions.
In `@cmd/subrouter/sr_server_test.go`:
- Around line 659-661: Synchronize test state shared with httptest handler
goroutines in cmd/subrouter/sr_server_test.go at lines 659-661 and 762-769:
protect preflightRequests, importRequests, and imported with a mutex in the
handler and around reads at lines 727-732, and protect postCount similarly or
convert it to atomic.Int32 and use Load() at line 791.
- Around line 359-367: Replace t.Fatalf calls inside the httptest server
handlers, including the handler near serverAccountImportPath and the newer
handler around lines 661-689, with t.Errorf followed by an error HTTP status
response and return. Ensure each invalid path or Authorization request
terminates the handler without writing the success response.
- Line 1135: Update the installer argument assertion in the relevant test to
require the exact “--admin-token-stdin” flag rather than the “--admin-token”
prefix, ensuring the secure stdin-based form is validated and the secret is not
passed as a process argument.
In `@internal/agents/claude/store.go`:
- Around line 313-348: Serialize the entire read-modify-write sequence in
ImportProfileCredential, including readProfiles, credential file creation,
profile updates, and writeProfiles, using the existing store-level
synchronization mechanism or a process-level mutex. Ensure concurrent imports
cannot overwrite each other’s registry entries; use a lock file instead if the
store may be accessed by multiple processes.
In `@internal/proxy/proxy.go`:
- Around line 1402-1406: Update validateStoredAccountImport to reject any
control character in account.Email, not only the explicitly listed characters,
while preserving the existing trimming, length, separator, and
invalidAccountImport behavior. Use the same validation rule as
agentclaude.ValidateProfileNameAllowEmail so terminal control sequences such as
\x1b cannot pass.
---
Nitpick comments:
In `@cmd/subrouter/install_systemd.go`:
- Around line 169-177: Replace the os.WriteFile calls for the systemd unit and
socket files in the install flow with writeFileAtomic, preserving their existing
paths, contents, and 0o644 permissions while retaining the current error
returns.
In `@cmd/subrouter/sr_server_account_import_test.go`:
- Around line 164-168: Update the serverLoginOne invocation in the relevant test
to pass t.Context() instead of context.Background(), preserving the existing
test behavior and consistency with the surrounding tests. Remove the context
import if it is no longer referenced elsewhere in the file.
In `@cmd/subrouter/sr_server_account_import.go`:
- Around line 157-168: Rename the local variable copy to client throughout
securedServerAccountImportClient, including its declaration and all remaining
references, while preserving the existing redirect and URL-handling behavior.
- Around line 142-155: Update redactServerAccountImportError to preserve the
original error chain while returning the redacted message: introduce or reuse a
wrapper implementing Error and Unwrap, and return it with message and err as the
cause instead of errors.New(message). Keep the existing redaction behavior and
ensure Error exposes only the sanitized text.
In `@cmd/subrouter/sr_server.go`:
- Around line 1026-1054: Update ensureServerAdminToken so it loads the server
file and confirms server.Name exists before generating random bytes or assigning
server.AdminToken. Preserve the existing not-found error and only mutate the
server and persist the token after a successful lookup.
In `@internal/agents/claude/store.go`:
- Around line 314-344: Reuse the initial data.Profiles[name] lookup by retaining
its profile and existence result for the later profile initialization in the
surrounding store operation. Remove the second lookup before creating a new
Profile, while preserving the existing behavior for both existing and missing
profiles.
In `@internal/proxy/account_import_test.go`:
- Around line 337-338: Update
TestAccountImportCapsDistinctAccountsButAllowsCredentialRotation to use the
production maxAccountImportAccounts constant instead of defining the literal
accountLimit value, so the test follows future cap changes automatically.
- Around line 20-78: Merge
TestAccountImportPreflightRequiresProtectedRemoteAccess and
TestAccountImportPreflightRequiresAdminTokenFromLoopback into one table-driven
test, adding a remoteAddr field to each case to cover both protected remote and
loopback addresses. Reuse the shared fixture setup and set req.RemoteAddr from
the case, preserving all existing status expectations and loopback coverage.
- Around line 313-335: Add a streaming oversized-body case to
TestAccountImportBoundsAndStrictlyParsesCredentialBodies by constructing the
import request with ContentLength set to -1, ensuring it reaches the
http.MaxBytesReader limit instead of the declared-length check and expects
http.StatusRequestEntityTooLarge. Preserve the existing oversized,
unknown-field, and trailing-document cases.
In `@internal/proxy/multitenant.go`:
- Around line 253-254: Document the authentication gate for the
"/_subrouter/account-import" entry in the allowlist map by adding a short inline
comment stating that requireAccountImportAuth accepts it because the tenant key
was already validated. Keep the existing route and value unchanged.
In `@internal/proxy/proxy.go`:
- Around line 1306-1315: Update rejectTrailingJSON to use errors.Is when
checking whether decoder.Decode returned io.EOF, while preserving the existing
handling for a trailing JSON value and other errors.
- Around line 1338-1369: Refactor the account import switch so the
Claude-specific credential path in validateClaudeAccountImport remains isolated,
while the stored-account flow is handled once after that branch. Reuse the
existing validateStoredAccountImport, ensureAccountImportCapacity, SaveStored,
and accountID assignment logic for Claude, Codex, Kimi, and ZAI, preserving the
payload validation and unsupported-provider behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61a98ca5-2906-4888-9346-75c9901530cc
📒 Files selected for processing (23)
README.mdcmd/subrouter/deploy_scripts_test.gocmd/subrouter/install_systemd.gocmd/subrouter/install_systemd_test.gocmd/subrouter/sr_claude_upload.gocmd/subrouter/sr_claude_upload_test.gocmd/subrouter/sr_server.gocmd/subrouter/sr_server_account_import.gocmd/subrouter/sr_server_account_import_test.gocmd/subrouter/sr_server_install_security_test.gocmd/subrouter/sr_server_test.godeploy/gcp/README.mddeploy/gcp/subrouter-verify.shdocs/production.mdinstall.shinternal/agents/claude/store.gointernal/agents/claude/store_test.gointernal/proxy/account_import_test.gointernal/proxy/multitenant.gointernal/proxy/multitenant_test.gointernal/proxy/plugin_endpoints_regression_test.gointernal/proxy/proxy.gointernal/transcript/gcs_sync_test.go
Replaces every user credential upload over SSH, SCP, or gcloud with a bounded, authenticated account-import endpoint. The CLI preflights with GET, performs the OAuth flow locally, then POSTs credentials over HTTPS or a verified Tailscale/loopback HTTP address.
Hardens import authentication, token redaction, profile validation, atomic credential and registry writes, account caps, installer secret handling, and Tailscale setup without SSH. Adds GCP version and unusable-pool verification.
Validation:
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Switches server account login to a protected HTTPS or verified loopback/Tailscale HTTP account‑import flow, adds native hosted onboarding via
stackauth, and gates GCP deploys behind a golden transport observer with rollbackable fronts.New Features
stackauth:sr loginruns a native flow, installs a built‑incmuxremote, supports--hosted-url, enables team routing and credential leasing, supports staging, and preserves hosted/local‑egress modes with a dedicated local proxy secret./_subrouter/account-importover HTTPS or verified loopback/Tailscale HTTP with tenant‑key auth and anAccountImportToken, serialized/atomic imports, and no SSH/SCP/gcloud paths.frontservice andsubrouter-transport-observerrecord HTTPS/WebSocket upgrade evidence, require fresh‑VM acceptance proof, and gate slot cutovers/migrations with rollback.local/teamCompose profiles, secret‑file env and startup secret checks, a health probe and CI smoke tests, and GCP preflight with live‑transport verification, public HTTPS + IAP routing, staging normalization, and a Codex live gate.Bug Fixes
Written for commit 44fe537. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes