From 57834b40b8108bf97dae08fb9190570b7620bee6 Mon Sep 17 00:00:00 2001 From: Zongqi Chen <137198879+zongqichen@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:25:12 +0100 Subject: [PATCH 1/2] test: reproduce seed race under context lock --- internal/app/app_linux_test.go | 115 +++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 internal/app/app_linux_test.go diff --git a/internal/app/app_linux_test.go b/internal/app/app_linux_test.go new file mode 100644 index 0000000..f351822 --- /dev/null +++ b/internal/app/app_linux_test.go @@ -0,0 +1,115 @@ +package app + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/zongqichen/cloud-foundry-cli-contexts/internal/lock" + "github.com/zongqichen/cloud-foundry-cli-contexts/internal/store" + "github.com/zongqichen/cloud-foundry-cli-contexts/internal/workspace" + "golang.org/x/sys/unix" +) + +func TestShimPreservesConfigurationCreatedWhileWaitingForLock(t *testing.T) { + fakeCF := writeFakeCF(t) + stateRoot := canonicalTestPath(t, t.TempDir()) + configureTestEnvironment(t, fakeCF, stateRoot) + t.Setenv("CFS_LOCK_TIMEOUT", "5s") + globalHome := t.TempDir() + t.Setenv("HOME", globalHome) + writeCFConfig(t, globalHome, []byte(`{"Target":"https://synthetic-global.invalid"}`)) + root := markerWorkspace(t) + + ws, err := workspace.Resolve(root) + if err != nil { + t.Fatal(err) + } + stateStore := store.New(stateRoot) + ctx, err := stateStore.ContextFor(ws) + if err != nil { + t.Fatal(err) + } + if err := stateStore.Prepare(ctx); err != nil { + t.Fatal(err) + } + held, err := lock.Acquire(ctx.LockPath, 0) + if err != nil { + t.Fatal(err) + } + lockHeld := true + t.Cleanup(func() { + if lockHeld { + _ = held.Release() + } + }) + + watch, err := unix.InotifyInit1(unix.IN_CLOEXEC | unix.IN_NONBLOCK) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = unix.Close(watch) }) + if _, err := unix.InotifyAddWatch(watch, ctx.LockPath, unix.IN_OPEN); err != nil { + t.Fatal(err) + } + + previous, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(root); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(previous) }) + + result := make(chan commandResult, 1) + go func() { + var stdout bytes.Buffer + var stderr bytes.Buffer + code := Run(Options{Args: []string{"cf", "version"}, Stdout: &stdout, Stderr: &stderr}) + result <- commandResult{code: code, stdout: stdout.String(), stderr: stderr.String()} + }() + + events := []unix.PollFd{{Fd: int32(watch), Events: unix.POLLIN}} + ready, err := unix.Poll(events, 5_000) + if err != nil { + t.Fatal(err) + } + if ready != 1 || events[0].Revents&unix.POLLIN == 0 { + t.Fatal("shim did not reach the held context lock") + } + buffer := make([]byte, unix.SizeofInotifyEvent+unix.NAME_MAX+1) + if _, err := unix.Read(watch, buffer); err != nil { + t.Fatal(err) + } + + want := []byte(`{"ConfigVersion":4}`) + writeCFConfig(t, ctx.CFHome, want) + if err := held.Release(); err != nil { + t.Fatal(err) + } + lockHeld = false + + var got commandResult + select { + case got = <-result: + case <-time.After(10 * time.Second): + t.Fatal("shim did not finish after the context lock was released") + } + if got.code != exitOK { + t.Fatalf("result = %#v", got) + } + actual, err := os.ReadFile(filepath.Join(ctx.CFHome, ".cf", "config.json")) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(actual, want) { + t.Fatalf("workspace configuration = %s, want %s", actual, want) + } + if strings.Contains(got.stderr, "seeded this workspace") { + t.Fatalf("stderr = %q", got.stderr) + } +} From 7b91b7cc1f40a57faf0e778976ce1c27092b1163 Mon Sep 17 00:00:00 2001 From: Zongqi Chen <137198879+zongqichen@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:26:44 +0100 Subject: [PATCH 2/2] fix: inspect seed state under context lock --- CHANGELOG.md | 3 +++ README.md | 10 +++++----- SECURITY.md | 12 ++++++------ docs/design.md | 31 ++++++++++++++---------------- internal/app/app.go | 17 ++++++----------- internal/app/app_unix_test.go | 36 +++++++++++++++++++++++++++++++++++ 6 files changed, 70 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf99d83..7f223d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ contract may still change while cfs is pre-1.0. ### Fixed +- The `cf` shim now checks for an existing default workspace configuration while + it holds the context lock. Automatic login copying no longer overwrites + configuration created by a concurrent command. - `cfs doctor` no longer reports that a saved local target proves current authentication. - Application tests no longer inherit or read the caller's global Cloud Foundry diff --git a/README.md b/README.md index d3351b9..d3a90f2 100644 --- a/README.md +++ b/README.md @@ -91,9 +91,9 @@ names fail without creating state. An ordinary managed `cf` command can copy your saved global API and authentication configuration into the workspace's fresh default context without prompting. -This happens only when its managed `.cf/config.json` is absent at command start -and the global configuration has a nonblank API `Target`. The copy clears the -organization and space, so choose them in the workspace: +This happens only when its managed `.cf/config.json` is absent while the shim +holds the context lock and the global configuration has a nonblank API `Target`. +The copy clears the organization and space, so choose them in the workspace: ```sh cd ~/work/orders @@ -101,8 +101,8 @@ cf target -o commerce -s development ``` The shim skips automatic copying if it finds an existing workspace configuration -at command start, even if the configuration has no API target. Named invocations -such as `cfs -c prod ...` require explicit import or login. +while holding the context lock, even if the configuration has no API target. +Named invocations such as `cfs -c prod ...` require explicit import or login. To copy the global organization and space as well, run `cfs import` in the workspace. To import into a named context, create the context first: diff --git a/SECURITY.md b/SECURITY.md index 3f05f3a..fc73144 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -36,12 +36,12 @@ from other local users, but the files are not encrypted by `cfs`; backups and host administrators may still access them. An ordinary managed `cf` command automatically copies `$HOME/.cf/config.json` -when the default workspace configuration is absent at command start and the -source has a nonblank API `Target`. This copy requires no confirmation, -preserves the saved API and authentication configuration, and clears the -organization and space. The shim skips this copy if it finds an existing -workspace configuration at command start. Named invocations require explicit -import or login. +when the default workspace configuration is absent while the shim holds the +context lock and the source has a nonblank API `Target`. This copy requires no +confirmation, preserves the saved API and authentication configuration, and +clears the organization and space. The shim skips this copy if it finds an +existing workspace configuration while holding the context lock. Named +invocations require explicit import or login. `cfs import` copies the global configuration into the default context or an existing named context, including the saved organization and space. It requires diff --git a/docs/design.md b/docs/design.md index b708d79..ec070be 100644 --- a/docs/design.md +++ b/docs/design.md @@ -106,19 +106,15 @@ There is no setup step for the default context and no mutable `cfs use` selection. On the first managed `cf` invocation in a workspace, `cfs` creates a private CF -home. If its `.cf/config.json` is absent at command start and the global -configuration has a nonblank API `Target`, `cfs` attempts to copy the saved API -and authentication configuration without confirmation. It clears +home. If its `.cf/config.json` is absent while the shim holds the context lock +and the global configuration has a nonblank API `Target`, `cfs` attempts to copy +the saved API and authentication configuration without confirmation. It clears `OrganizationFields` and `SpaceFields`, so the user chooses the workspace's organization and space. -The shim skips copying if it finds an existing configuration at command start, -even without an API target. If no usable source exists, the official CLI starts -with the workspace's fresh home. Future shells and coding tools in that worktree -resolve the same home. - -The absence check currently runs before acquiring the context lock. A command -waiting for that lock can overwrite configuration created while it waits. -Initialize a fresh default context before starting concurrent commands there. +The shim skips copying if it finds an existing configuration while holding the +context lock, even without an API target. If no usable source exists, the +official CLI starts with the workspace's fresh home. Future shells and coding +tools in that worktree resolve the same home. An existing global context can be copied explicitly: @@ -289,9 +285,10 @@ For each invocation of `cf `: context ID from both workspace identity and name. 6. Validate and create the private state directory. 7. Acquire the selected context's process lock. -8. Construct the child environment. -9. For the ordinary shim path, attempt automatic copying if the workspace - configuration was absent and a saved global API target is available. +8. For the ordinary shim path, inspect the workspace configuration while holding + the lock. +9. Construct the child environment. Attempt automatic copying if the + configuration is absent and a saved global API target is available. 10. Start the official CLI with the original argument vector. 11. Forward standard input, output, error, terminal behavior, and signals. 12. Release the lock and return the official CLI exit status. @@ -303,10 +300,10 @@ func runCF(args []string) int { realCF := config.RealCFPath scope := resolveScope(os.Getwd(), os.Environ()) context := store.Open(scope) - configMissing := !context.HasCFConfig() lock := context.Lock() defer lock.Release() + configMissing := !context.HasCFConfig() env := cloneEnvironment() env.Set("CF_HOME", context.CFHome()) @@ -591,8 +588,8 @@ The MVP is complete only when all of the following are demonstrated: 13. Unknown or malformed context names cannot create state or fall back to the default or global CF home. 14. A fresh default workspace can copy saved global API and authentication - configuration without inheriting the organization or space. Configuration - present at command start and the global source remain unchanged. + configuration without inheriting the organization or space. Configuration that + exists while the context lock is held and the global source remain unchanged. 15. Named invocations do not copy global configuration automatically. The automated coverage for these criteria is documented in diff --git a/internal/app/app.go b/internal/app/app.go index 283b54d..a34cbf6 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -99,12 +99,6 @@ func runShim(options Options, args []string) int { reportWorkspaceError(options, err) return exitUnavailable } - workspaceWasEmpty, err := workspaceConfigMissing(managed.Context.CFHome) - if err != nil { - fprintf(options.Stderr, "cfs: inspect workspace CF state: %v\n", err) - return exitError - } - timeout, err := lockTimeout() if err != nil { fprintf(options.Stderr, "cfs: %v\n", err) @@ -121,13 +115,14 @@ func runShim(options Options, args []string) int { return exitError } + workspaceWasEmpty, err := workspaceConfigMissing(managed.Context.CFHome) + if err != nil { + fprintf(options.Stderr, "cfs: inspect workspace CF state: %v\n", errors.Join(err, workspaceLock.Release())) + return exitError + } + env := managed.environment(os.Environ()) - // Share one login across workspaces: a brand-new workspace inherits the - // global CF login automatically instead of forcing a fresh `cf login` (or a - // manual `cfs import`) in every worktree. Only an empty workspace is seeded, - // so a target the caller already set here is never overwritten. Auth is - // shared; org/space stay per-workspace. seeded := false if workspaceWasEmpty { if home, available, gerr := globalTargetAvailable(); gerr == nil && available { diff --git a/internal/app/app_unix_test.go b/internal/app/app_unix_test.go index ce5a8c5..7b6efa2 100644 --- a/internal/app/app_unix_test.go +++ b/internal/app/app_unix_test.go @@ -179,6 +179,42 @@ func TestShimRejectsConcurrentCommandInSameWorkspace(t *testing.T) { } } +func TestShimReleasesLockWhenWorkspaceStateInspectionFails(t *testing.T) { + fakeCF := writeFakeCF(t) + stateRoot := canonicalTestPath(t, t.TempDir()) + configureTestEnvironment(t, fakeCF, stateRoot) + root := markerWorkspace(t) + + ws, err := workspace.Resolve(root) + if err != nil { + t.Fatal(err) + } + stateStore := store.New(stateRoot) + ctx, err := stateStore.ContextFor(ws) + if err != nil { + t.Fatal(err) + } + if err := stateStore.Prepare(ctx); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(ctx.CFHome, ".cf", "config.json"), 0o700); err != nil { + t.Fatal(err) + } + + result := runFromDirectory(t, root, []string{"cf", "apps"}) + if result.code != exitError || !strings.Contains(result.stderr, "inspect workspace CF state") { + t.Fatalf("result = %#v", result) + } + + acquired, err := lock.Acquire(ctx.LockPath, 0) + if err != nil { + t.Fatalf("context lock remained held: %v", err) + } + if err := acquired.Release(); err != nil { + t.Fatal(err) + } +} + func TestShimForwardsInteractiveInputAndArguments(t *testing.T) { fakeCF := writeFakeCF(t) configureTestEnvironment(t, fakeCF, t.TempDir())