diff --git a/internal/api/handlers/management/auth_files_crud.go b/internal/api/handlers/management/auth_files_crud.go index 8b10409408..30fee8dec6 100644 --- a/internal/api/handlers/management/auth_files_crud.go +++ b/internal/api/handlers/management/auth_files_crud.go @@ -17,6 +17,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/router-for-me/CLIProxyAPI/v8/internal/auth/codex" "github.com/router-for-me/CLIProxyAPI/v8/internal/clienterror" "github.com/router-for-me/CLIProxyAPI/v8/internal/util" "github.com/router-for-me/CLIProxyAPI/v8/internal/watcher/synthesizer" @@ -289,7 +290,7 @@ func (h *Handler) writeAuthFile(ctx context.Context, name string, data []byte) e } release := h.beginSIWCFileMutation(ctx, dst) defer release() - if errWrite := os.WriteFile(dst, data, 0o600); errWrite != nil { + if errWrite := h.writeAuthFileBytes(dst, data, auth); errWrite != nil { return fmt.Errorf("failed to write file: %w", errWrite) } if err := h.upsertAuthRecord(ctx, auth); err != nil { @@ -303,6 +304,27 @@ func (h *Handler) writeAuthFile(ctx context.Context, name string, data []byte) e return nil } +func (h *Handler) writeAuthFileBytes(dst string, data []byte, auth *coreauth.Auth) error { + if h.shouldLockCodexAuthFile(auth) { + return codex.WithFileLock(dst, func() error { + return os.WriteFile(dst, data, 0o600) + }) + } + return os.WriteFile(dst, data, 0o600) +} + +func (h *Handler) shouldLockCodexAuthFile(auth *coreauth.Auth) bool { + if coreauth.IsCodexAuthRecord(auth) { + return true + } + if h != nil && h.authManager != nil && auth != nil && auth.ID != "" { + if existing, ok := h.authManager.GetByID(auth.ID); ok && coreauth.IsCodexAuthRecord(existing) { + return true + } + } + return false +} + func requestedAuthFileNamesForDelete(c *gin.Context) ([]string, error) { if c == nil { return nil, nil diff --git a/internal/api/handlers/management/oauth_siwc_test.go b/internal/api/handlers/management/oauth_siwc_test.go index 2f5c012713..b2ffdcff6c 100644 --- a/internal/api/handlers/management/oauth_siwc_test.go +++ b/internal/api/handlers/management/oauth_siwc_test.go @@ -9,6 +9,7 @@ import ( "net/url" "os" "path/filepath" + "runtime" "strings" "sync" "sync/atomic" @@ -542,3 +543,272 @@ func TestSIWCBindRejectsReplacedAccount(t *testing.T) { }) } } + +type siwcCommitWindowContext struct { + context.Context + errCalls atomic.Int32 + onCall int32 + hook func() +} + +func (c *siwcCommitWindowContext) Err() error { + if c.errCalls.Add(1) == c.onCall && c.hook != nil { + c.hook() + } + return c.Context.Err() +} + +type siwcUploadProgressContext struct { + context.Context + valueOnce sync.Once + valueCh chan struct{} + doneOnce sync.Once + doneCh chan struct{} +} + +func (c *siwcUploadProgressContext) Value(key any) any { + c.valueOnce.Do(func() { + close(c.valueCh) + }) + return c.Context.Value(key) +} + +func (c *siwcUploadProgressContext) Done() <-chan struct{} { + c.doneOnce.Do(func() { + close(c.doneCh) + }) + return c.Context.Done() +} + +func TestSIWCBindLegacyUploadDuringCommitWindow(t *testing.T) { + f := setupSIWCFlow(t, false) + f.router.POST("/legacy", f.h.UploadAuthFile) + state, flow := f.start(t) + + uploadedBody := map[string]any{ + "type": "codex", + "access_token": "uploaded-outer", + "refresh_token": "uploaded-refresh", + "account_id": "account", + "note": "uploaded-during-commit", + } + rawBody, errMarshal := json.Marshal(uploadedBody) + if errMarshal != nil { + t.Fatal(errMarshal) + } + + reachedMutation := make(chan struct{}) + reachedUpsert := make(chan struct{}) + uploadDone := make(chan *httptest.ResponseRecorder, 1) + + flow.mu.Lock() + flow.ctx = &siwcCommitWindowContext{ + Context: flow.ctx, + onCall: 6, + hook: func() { + go func() { + req := httptest.NewRequest(http.MethodPost, "/legacy?name=codex.json", strings.NewReader(string(rawBody))) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(&siwcUploadProgressContext{ + Context: req.Context(), + valueCh: reachedMutation, + doneCh: reachedUpsert, + }) + w := httptest.NewRecorder() + f.router.ServeHTTP(w, req) + uploadDone <- w + }() + <-reachedMutation + for range 500 { + select { + case <-reachedUpsert: + return + default: + runtime.Gosched() + } + } + }, + } + flow.mu.Unlock() + + w := siwcRequest(t, f.router, http.MethodPost, "/callback", map[string]any{"provider": "codex", "redirect_url": siwcCallbackURL(state)}) + if w.Code != http.StatusOK { + t.Fatalf("callback: %d %s", w.Code, w.Body.String()) + } + waitSIWCExchange(t, flow) + if status := performOAuthStatusRequest(t, f.router, state); status.Status != "ok" { + t.Fatalf("expected bind commit to finish before locked upload replaced file, got %#v", status) + } + + var wUpload *httptest.ResponseRecorder + select { + case wUpload = <-uploadDone: + case <-time.After(5 * time.Second): + t.Fatal("legacy upload did not finish") + } + if wUpload.Code != http.StatusOK { + t.Fatalf("legacy upload status = %d, body = %s", wUpload.Code, wUpload.Body.String()) + } + + diskData, errRead := os.ReadFile(f.path) + if errRead != nil { + t.Fatal(errRead) + } + var disk map[string]any + if errJSON := json.Unmarshal(diskData, &disk); errJSON != nil { + t.Fatal(errJSON) + } + got, ok := f.manager.GetByID(f.auth.ID) + if !ok || got == nil { + t.Fatal("missing auth in manager after upload") + } + if disk["note"] != "uploaded-during-commit" || disk["access_token"] != "uploaded-outer" { + t.Fatalf("SIWC bind overwrote legacy upload on disk: %#v", disk) + } + if got.Metadata["note"] != "uploaded-during-commit" || got.Metadata["access_token"] != "uploaded-outer" { + t.Fatalf("unexpected in-memory metadata after upload: %#v", got.Metadata) + } + if disk["use_siwc"] != got.Metadata["use_siwc"] { + t.Fatalf("disk and memory disagree on use_siwc: disk=%v memory=%v", disk["use_siwc"], got.Metadata["use_siwc"]) + } +} + +func TestWriteAuthFileWaitsForCodexFileLock(t *testing.T) { + for _, tc := range []struct { + name string + useSymlink bool + uploadedBody map[string]any + }{ + { + name: "codex_upload", + useSymlink: false, + uploadedBody: map[string]any{ + "type": "codex", + "access_token": "uploaded-outer", + "refresh_token": "uploaded-refresh", + "account_id": "account", + "note": "after-lock", + }, + }, + { + name: "non_codex_replacing_existing_codex", + useSymlink: false, + uploadedBody: map[string]any{ + "type": "claude", + "access_token": "claude-access", + "refresh_token": "claude-refresh", + "note": "replaced-provider", + }, + }, + { + name: "symlinked_auth_dir_alias", + useSymlink: true, + uploadedBody: map[string]any{ + "type": "codex", + "access_token": "uploaded-outer", + "refresh_token": "uploaded-refresh", + "account_id": "account", + "note": "via-symlink-dir", + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + f := setupSIWCFlow(t, false) + f.router.POST("/legacy", f.h.UploadAuthFile) + + lockPath := f.path + if tc.useSymlink { + aliasDir := filepath.Join(t.TempDir(), "auths-link") + if errSymlink := os.Symlink(filepath.Dir(f.path), aliasDir); errSymlink != nil { + t.Skipf("symlinks unavailable: %v", errSymlink) + } + f.h.cfg.AuthDir = aliasDir + if realPath, errEval := filepath.EvalSymlinks(f.path); errEval == nil { + lockPath = realPath + } + } + + rawBody, errMarshal := json.Marshal(tc.uploadedBody) + if errMarshal != nil { + t.Fatal(errMarshal) + } + + locked := make(chan struct{}) + releaseLock := make(chan struct{}) + var releaseOnce sync.Once + release := func() { + releaseOnce.Do(func() { + close(releaseLock) + }) + } + t.Cleanup(release) + lockDone := make(chan error, 1) + go func() { + lockDone <- codex.WithFileLock(lockPath, func() error { + close(locked) + <-releaseLock + return nil + }) + }() + <-locked + + reachedMutation := make(chan struct{}) + reachedUpsert := make(chan struct{}) + uploadDone := make(chan *httptest.ResponseRecorder, 1) + go func() { + req := httptest.NewRequest(http.MethodPost, "/legacy?name=codex.json", strings.NewReader(string(rawBody))) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(&siwcUploadProgressContext{ + Context: req.Context(), + valueCh: reachedMutation, + doneCh: reachedUpsert, + }) + w := httptest.NewRecorder() + f.router.ServeHTTP(w, req) + uploadDone <- w + }() + <-reachedMutation + for range 200 { + runtime.Gosched() + } + select { + case <-reachedUpsert: + t.Fatal("writeAuthFile bypassed codex.WithFileLock and reached upsertAuthRecord while lock was held") + default: + } + diskWhileLocked, errRead := os.ReadFile(f.path) + if errRead != nil { + t.Fatal(errRead) + } + if strings.Contains(string(diskWhileLocked), tc.uploadedBody["note"].(string)) { + t.Fatalf("writeAuthFile modified disk while codex.WithFileLock was held: %s", string(diskWhileLocked)) + } + + release() + select { + case errLock := <-lockDone: + if errLock != nil { + t.Fatal(errLock) + } + case <-time.After(5 * time.Second): + t.Fatal("file lock holder did not finish") + } + var wUpload *httptest.ResponseRecorder + select { + case wUpload = <-uploadDone: + case <-time.After(5 * time.Second): + t.Fatal("legacy upload did not finish") + } + if wUpload.Code != http.StatusOK { + t.Fatalf("legacy upload status = %d, body = %s", wUpload.Code, wUpload.Body.String()) + } + diskAfter, errReadAfter := os.ReadFile(f.path) + if errReadAfter != nil { + t.Fatal(errReadAfter) + } + if !strings.Contains(string(diskAfter), tc.uploadedBody["note"].(string)) { + t.Fatalf("expected uploaded content after lock release, got: %s", string(diskAfter)) + } + }) + } +} diff --git a/internal/auth/codex/token.go b/internal/auth/codex/token.go index 15a4eca1e9..6baffb9446 100644 --- a/internal/auth/codex/token.go +++ b/internal/auth/codex/token.go @@ -33,6 +33,11 @@ func canonicalLockPath(authFilePath string) string { if abs, errAbs := filepath.Abs(cleaned); errAbs == nil && strings.TrimSpace(abs) != "" { cleaned = filepath.Clean(abs) } + if real, errEval := filepath.EvalSymlinks(cleaned); errEval == nil && strings.TrimSpace(real) != "" { + cleaned = filepath.Clean(real) + } else if parent, errParent := filepath.EvalSymlinks(filepath.Dir(cleaned)); errParent == nil && strings.TrimSpace(parent) != "" { + cleaned = filepath.Clean(filepath.Join(parent, filepath.Base(cleaned))) + } return cleaned } diff --git a/sdk/cliproxy/auth/codex_siwc_bind.go b/sdk/cliproxy/auth/codex_siwc_bind.go index 221ed70cd5..5d12f8acc3 100644 --- a/sdk/cliproxy/auth/codex_siwc_bind.go +++ b/sdk/cliproxy/auth/codex_siwc_bind.go @@ -1,6 +1,7 @@ package auth import ( + "bytes" "context" "encoding/json" "fmt" @@ -85,12 +86,16 @@ func (m *Manager) BindCodexSIWC(ctx context.Context, id string, expectedEpoch ui } out["siwc"] = json.RawMessage(siwc) out["use_siwc"] = true + if errCtx := ctx.Err(); errCtx != nil { + return errCtx + } latestInfo, errStat := os.Lstat(path) if errStat != nil || !os.SameFile(info, latestInfo) { return fmt.Errorf("bind SIWC: backing file changed during binding") } - if errCtx := ctx.Err(); errCtx != nil { - return errCtx + latestData, errReadLatest := os.ReadFile(path) + if errReadLatest != nil || !bytes.Equal(data, latestData) { + return fmt.Errorf("bind SIWC: backing file changed during binding") } if errWrite := codex.WriteJSONFileAtomic(path, out); errWrite != nil { return fmt.Errorf("bind SIWC: persist binding: %w", errWrite) diff --git a/sdk/cliproxy/auth/codex_siwc_bind_test.go b/sdk/cliproxy/auth/codex_siwc_bind_test.go index b9c366e81d..555742a823 100644 --- a/sdk/cliproxy/auth/codex_siwc_bind_test.go +++ b/sdk/cliproxy/auth/codex_siwc_bind_test.go @@ -476,3 +476,62 @@ func TestBindCodexSIWCRejectsReplacedAccount(t *testing.T) { t.Fatalf("failed binding mutated replaced account on disk: %#v", disk) } } + +type bindCommitHookContext struct { + context.Context + errCalls int + onCall int + hook func() +} + +func (c *bindCommitHookContext) Err() error { + c.errCalls++ + if c.errCalls == c.onCall && c.hook != nil { + c.hook() + } + return c.Context.Err() +} + +func TestBindCodexSIWCRejectsInPlaceRewriteDuringCommit(t *testing.T) { + m, base, path, hook := setupSIWCBinding(t, "auth.json") + rewritten := map[string]any{ + "type": "codex", + "access_token": "uploaded-outer", + "refresh_token": "uploaded-refresh", + "account_id": "account", + "notes": "in-place-upload", + "disabled": false, + } + rewrittenBytes, errMarshal := json.Marshal(rewritten) + if errMarshal != nil { + t.Fatal(errMarshal) + } + beforeInfo, errStat := os.Lstat(path) + if errStat != nil { + t.Fatal(errStat) + } + ctx := &bindCommitHookContext{ + Context: context.Background(), + onCall: 3, + hook: func() { + if errWrite := os.WriteFile(path, rewrittenBytes, 0o600); errWrite != nil { + t.Errorf("in-place write failed: %v", errWrite) + } + afterInfo, errAfter := os.Lstat(path) + if errAfter != nil || !os.SameFile(beforeInfo, afterInfo) { + t.Errorf("expected in-place write to preserve file identity, err=%v", errAfter) + } + }, + } + _, errBind := m.BindCodexSIWC(ctx, base.ID, base.RegistrationEpoch, "account", bindingTokens("bound")) + if errBind == nil { + t.Fatal("binding unexpectedly succeeded and overwrote in-place file update") + } + if hook.updates != 0 { + t.Fatalf("failed binding published update: updates=%d", hook.updates) + } + disk := readBindingDisk(t, path) + if disk["notes"] != "in-place-upload" || disk["access_token"] != "uploaded-outer" || disk["use_siwc"] == true { + t.Fatalf("binding overwrote in-place file rewrite on disk: %#v", disk) + } +}