diff --git a/cmd/entire/cli/checkpoint/remote/util.go b/cmd/entire/cli/checkpoint/remote/util.go index aad8668cd7..235eba47e6 100644 --- a/cmd/entire/cli/checkpoint/remote/util.go +++ b/cmd/entire/cli/checkpoint/remote/util.go @@ -127,18 +127,38 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) { return checkpointURL, nil } +// ForkOwnerMismatchError reports that a checkpoint_remote is configured but the +// push remote's owner differs from the checkpoint target's owner — i.e. the user +// is pushing from a fork. Callers must NOT fall back to pushing checkpoints to +// the push remote (the fork): that could publish session transcripts to a public +// repository. The push should be skipped and the user told how to opt in. +type ForkOwnerMismatchError struct { + PushOwner string // owner of the push remote (the fork) + TargetOwner string // owner of the configured checkpoint target + TargetRepo string // full "owner/repo" of the configured checkpoint target +} + +func (e *ForkOwnerMismatchError) Error() string { + return fmt.Sprintf( + "checkpoints not pushed: push remote is a fork (owner %q differs from checkpoint target owner %q)", + e.PushOwner, e.TargetOwner, + ) +} + // PushURL returns the effective checkpoint push URL for the current repository. // Unlike FetchURL: // - it derives protocol from the requested push remote, not always origin -// - it skips checkpoint remote use when the push remote owner differs -// from the configured checkpoint remote owner +// - when the push remote owner differs from the configured checkpoint target +// owner (a fork), it returns a *ForkOwnerMismatchError instead of a URL, so +// the caller skips the push rather than leaking checkpoints to the fork // // If ENTIRE_CHECKPOINT_TOKEN is set, HTTPS is forced so the token can be used // even when the push remote is configured via SSH. // // The boolean return value reports whether a dedicated checkpoint_remote is -// configured and should be used for push. When false, the returned URL is the -// repository's origin URL as a fallback. +// configured and should be used for push. When false (and err is nil), the +// returned URL is the repository's origin URL as a benign fallback (no +// checkpoint_remote configured, or a non-fork derivation failure). func PushURL(ctx context.Context, pushRemoteName string) (string, bool, error) { originURL := "" if resolvedOriginURL, err := GetRemoteURL(ctx, originRemote); err == nil { @@ -212,11 +232,16 @@ func PushURL(ctx context.Context, pushRemoteName string) (string, bool, error) { checkpointOwner := config.Owner() if pushInfo.Owner != "" && checkpointOwner != "" && !strings.EqualFold(pushInfo.Owner, checkpointOwner) { - fallbackURL, fallbackErr := resolvePushFallbackURL(ctx, pushRemoteName, originURL) - if fallbackErr != nil { - return "", false, fmt.Errorf("no push URL found: %w", fallbackErr) + // The push remote is a fork: its owner differs from the configured + // checkpoint target owner. Report this distinctly rather than falling + // back to the push remote (the fork) — pushing checkpoints there could + // publish session transcripts to a public repository. The caller skips + // the push and tells the user how to opt in. See ForkOwnerMismatchError. + return "", false, &ForkOwnerMismatchError{ + PushOwner: pushInfo.Owner, + TargetOwner: checkpointOwner, + TargetRepo: config.Repo, } - return fallbackURL, false, nil } if withToken && pushInfo.Protocol == ProtocolEntire { diff --git a/cmd/entire/cli/checkpoint/remote/util_test.go b/cmd/entire/cli/checkpoint/remote/util_test.go index a49d4533cd..0264bc322e 100644 --- a/cmd/entire/cli/checkpoint/remote/util_test.go +++ b/cmd/entire/cli/checkpoint/remote/util_test.go @@ -2,6 +2,7 @@ package remote import ( "context" + "errors" "os" "os/exec" "path/filepath" @@ -222,15 +223,17 @@ func TestFetchURL_EdgeCases(t *testing.T) { func TestPushURL(t *testing.T) { tests := []struct { - name string - originURL string - pushRemote string - pushURL string - settingsJSON string - token string - wantURL string - wantEnabled bool - wantErr bool + name string + originURL string + pushRemote string + pushURL string + settingsJSON string + token string + wantURL string + wantEnabled bool + wantErr bool + wantForkPushOwn string // expected ForkOwnerMismatchError.PushOwner; "" means no fork error expected + wantForkTgtOwn string // expected ForkOwnerMismatchError.TargetOwner }{ { name: "no checkpoint remote falls back to origin https url and reports disabled", @@ -310,12 +313,12 @@ func TestPushURL(t *testing.T) { wantEnabled: true, }, { - name: "different push remote owner disables checkpoint push url", - originURL: "https://github.com/fork/app.git", - pushRemote: "origin", - settingsJSON: `{"enabled":true,"strategy_options":{"checkpoint_remote":{"provider":"github","repo":"acme/checkpoints"}}}`, - wantURL: "https://github.com/fork/app.git", - wantEnabled: false, + name: "different push remote owner reports a fork owner mismatch", + originURL: "https://github.com/fork/app.git", + pushRemote: "origin", + settingsJSON: `{"enabled":true,"strategy_options":{"checkpoint_remote":{"provider":"github","repo":"acme/checkpoints"}}}`, + wantForkPushOwn: "fork", + wantForkTgtOwn: "acme", }, { name: "entire:// origin derives mirror checkpoint url on same cluster", @@ -334,12 +337,12 @@ func TestPushURL(t *testing.T) { wantEnabled: true, }, { - name: "entire:// origin with different owner disables checkpoint push url", - originURL: "entire://app.entire.io/gh/fork/app", - pushRemote: "origin", - settingsJSON: `{"enabled":true,"strategy_options":{"checkpoint_remote":{"provider":"github","repo":"acme/checkpoints"}}}`, - wantURL: "entire://app.entire.io/gh/fork/app", - wantEnabled: false, + name: "entire:// origin with different owner reports a fork owner mismatch", + originURL: "entire://app.entire.io/gh/fork/app", + pushRemote: "origin", + settingsJSON: `{"enabled":true,"strategy_options":{"checkpoint_remote":{"provider":"github","repo":"acme/checkpoints"}}}`, + wantForkPushOwn: "fork", + wantForkTgtOwn: "acme", }, { name: "file:// origin routes to provider checkpoint url (ssh default)", @@ -401,6 +404,19 @@ func TestPushURL(t *testing.T) { } gotURL, gotEnabled, err := PushURL(context.Background(), tt.pushRemote) + if tt.wantForkPushOwn != "" { + var forkErr *ForkOwnerMismatchError + if !errors.As(err, &forkErr) { + t.Fatalf("PushURL() error = %v, want *ForkOwnerMismatchError", err) + } + if forkErr.PushOwner != tt.wantForkPushOwn { + t.Fatalf("ForkOwnerMismatchError.PushOwner = %q, want %q", forkErr.PushOwner, tt.wantForkPushOwn) + } + if forkErr.TargetOwner != tt.wantForkTgtOwn { + t.Fatalf("ForkOwnerMismatchError.TargetOwner = %q, want %q", forkErr.TargetOwner, tt.wantForkTgtOwn) + } + return + } if tt.wantErr { if err == nil { t.Fatal("PushURL() error = nil, want error") diff --git a/cmd/entire/cli/integration_test/remote_operations_test.go b/cmd/entire/cli/integration_test/remote_operations_test.go index 15cb2afb8e..868b45a1f0 100644 --- a/cmd/entire/cli/integration_test/remote_operations_test.go +++ b/cmd/entire/cli/integration_test/remote_operations_test.go @@ -221,9 +221,10 @@ func TestPrePush_CheckpointURLDerivationFailureFallsBackToOrigin(t *testing.T) { // Configure checkpoint_remote with a different owner than origin. // Since our bare remote is a local path (not a URL), resolvePushSettings cannot - // parse it via remote.ParseURL and falls back to origin. The unit test - // TestResolvePushSettings_ForkDetection in checkpoint_remote_test.go validates - // the exact fork detection logic with real URL parsing. + // parse it via remote.ParseURL and falls back to origin — a benign derivation + // failure, NOT the fork owner-mismatch branch (which requires a parseable URL + // and is skipped, not fallen back). The fork owner-mismatch logic is unit-tested + // in checkpoint_remote_test.go:TestResolvePushSettings_ForkDetection. env.PatchSettings(map[string]any{ "strategy_options": map[string]any{ "checkpoint_remote": map[string]any{ diff --git a/cmd/entire/cli/strategy/checkpoint_remote.go b/cmd/entire/cli/strategy/checkpoint_remote.go index 5779567714..cb2cfe90ba 100644 --- a/cmd/entire/cli/strategy/checkpoint_remote.go +++ b/cmd/entire/cli/strategy/checkpoint_remote.go @@ -2,6 +2,7 @@ package strategy import ( "context" + "errors" "fmt" "log/slog" "strings" @@ -28,6 +29,15 @@ type pushSettings struct { checkpointURL string // pushDisabled is true if push_sessions is explicitly set to false. pushDisabled bool + // skipCheckpointPush is true when a checkpoint_remote is configured but the + // push remote is a fork (different owner). Pushing would otherwise fall back + // to the fork, risking public exposure of session transcripts, so the push + // is skipped and the user is warned. Distinct from pushDisabled (an explicit + // opt-out) and from an empty checkpointURL (a benign origin fallback when no + // checkpoint target is configured). forkMismatch carries the details for the + // warning. + skipCheckpointPush bool + forkMismatch *remote.ForkOwnerMismatchError } // pushTarget returns the target to use for git push/fetch commands for checkpoint branches. @@ -68,6 +78,14 @@ func resolvePushSettings(ctx context.Context, pushRemoteName string) pushSetting return ps } checkpointURL, enabled, err := remote.PushURL(ctx, pushRemoteName) + var forkErr *remote.ForkOwnerMismatchError + if errors.As(err, &forkErr) { + // Fork push remote with a configured private target: skip the push rather + // than fall back to the fork (which could publish transcripts publicly). + ps.skipCheckpointPush = true + ps.forkMismatch = forkErr + return ps + } if err != nil { logging.Warn(ctx, "checkpoint-remote: could not derive URL from push remote", slog.String("remote", pushRemoteName), diff --git a/cmd/entire/cli/strategy/checkpoint_remote_test.go b/cmd/entire/cli/strategy/checkpoint_remote_test.go index a5ad3c7120..fa55c502ac 100644 --- a/cmd/entire/cli/strategy/checkpoint_remote_test.go +++ b/cmd/entire/cli/strategy/checkpoint_remote_test.go @@ -330,10 +330,15 @@ func TestResolvePushSettings_ForkDetection(t *testing.T) { t.Chdir(localDir) ps := resolvePushSettings(ctx, "origin") - // Should fall back to origin since the remote owner differs (alice != org). + // A configured private checkpoint target + a fork push remote (alice != org) + // must NOT silently fall back to pushing checkpoints to the fork. The push is + // skipped so session transcripts are never published to the fork. + assert.True(t, ps.skipCheckpointPush, "fork push must be skipped, not redirected to origin") assert.False(t, ps.hasCheckpointURL()) - assert.Equal(t, "origin", ps.pushTarget()) assert.False(t, ps.pushDisabled) + require.NotNil(t, ps.forkMismatch) + assert.Equal(t, "alice", ps.forkMismatch.PushOwner) + assert.Equal(t, "org", ps.forkMismatch.TargetOwner) } // Not parallel: uses t.Chdir() @@ -576,9 +581,10 @@ func TestFetchURL_IgnoresOwnerMismatchCheck(t *testing.T) { require.NoError(t, err) assert.Equal(t, "git@github.com:org/checkpoints.git", url) - // Contrast: push settings should reject the same config + // Contrast: push settings should reject the same config and skip the push ps := resolvePushSettings(ctx, "origin") assert.False(t, ps.hasCheckpointURL(), "resolvePushSettings should reject an origin with a different owner") + assert.True(t, ps.skipCheckpointPush, "resolvePushSettings should skip pushing for a fork owner mismatch") } // Not parallel: uses t.Chdir() diff --git a/cmd/entire/cli/strategy/manual_commit_push.go b/cmd/entire/cli/strategy/manual_commit_push.go index 5ab8070ca6..7ba44269a8 100644 --- a/cmd/entire/cli/strategy/manual_commit_push.go +++ b/cmd/entire/cli/strategy/manual_commit_push.go @@ -75,6 +75,15 @@ func (s *ManualCommitStrategy) prePush(ctx context.Context, remote string, prote return nil } + // Fork push remote + a configured private checkpoint target: skip the push + // rather than fall back to the fork, which could publish session transcripts + // to a public repository. Warn once (git shows stderr during pre-push) with + // the opt-in instructions. Covers both storage backends since it precedes them. + if ps.skipCheckpointPush { + warnForkCheckpointSkip(ps) + return nil + } + // git-refs primary: push the per-checkpoint refs recorded in the push queue // instead of the single v1 branch. Those refs live under refs/entire/, not // refs/heads/, so a forge can never pick them as a repository's default @@ -177,6 +186,34 @@ func (s *ManualCommitStrategy) prePush(ctx context.Context, remote string, prote return nil } +// warnForkCheckpointSkip prints a one-time, user-facing explanation that +// checkpoints were skipped because the push remote is a fork, plus the two ways +// to opt in. Written to stderr, which git surfaces during the pre-push hook. +func warnForkCheckpointSkip(ps pushSettings) { + if msg := forkCheckpointSkipMessage(ps.forkMismatch); msg != "" { + fmt.Fprint(os.Stderr, msg) + } +} + +// forkCheckpointSkipMessage builds the user-facing skip explanation. Returns the +// empty string when fm is nil so callers can skip printing entirely. +func forkCheckpointSkipMessage(fm *checkpointremote.ForkOwnerMismatchError) string { + if fm == nil { + return "" + } + return fmt.Sprintf( + "[entire] Checkpoints were NOT pushed.\n"+ + "This repo sends checkpoints to a private target (%s), but your push remote is a\n"+ + "fork (owner %q). Pushing to your fork could publish session transcripts, so the\n"+ + "push was skipped.\n"+ + "\n"+ + "To re-enable, edit %s and either:\n"+ + " • point checkpoint_remote at your own private repo (owner %q), or\n"+ + " • set \"checkpoint_remote\": null to accept your fork as the target (it may be public).\n", + fm.TargetRepo, fm.PushOwner, settings.EntireSettingsLocalFile, fm.PushOwner, + ) +} + // deferCheckpointPushOnEmptyRemote reports whether publication of the git-branch // v1 metadata should be held back because the push remote may be brand new. // @@ -302,6 +339,12 @@ func PushQueuedCheckpointRefs(ctx context.Context, repo *git.Repository, remote if ps.pushDisabled { return 0, true, nil } + if ps.skipCheckpointPush { + // Fork push remote + a configured private target: refuse to push to the + // fork (which could publish transcripts). Surface it as an error here — + // unlike the fail-soft pre-push path, this is a foreground command. + return 0, false, ps.forkMismatch + } syncCheckpointPolicyForPrePush(ctx, repo, ps) if !checkpointPolicyAllowsGitHook(ctx, repo) { return 0, false, errors.New("checkpoint policy does not allow pushing checkpoint refs; refs stay queued") diff --git a/cmd/entire/cli/strategy/manual_commit_push_test.go b/cmd/entire/cli/strategy/manual_commit_push_test.go index 75485f2ed4..586e70d261 100644 --- a/cmd/entire/cli/strategy/manual_commit_push_test.go +++ b/cmd/entire/cli/strategy/manual_commit_push_test.go @@ -5,11 +5,31 @@ import ( "os/exec" "testing" + checkpointremote "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" "github.com/entireio/cli/cmd/entire/cli/testutil" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestForkCheckpointSkipMessage(t *testing.T) { + t.Parallel() + + assert.Empty(t, forkCheckpointSkipMessage(nil), "nil mismatch must produce no message") + + msg := forkCheckpointSkipMessage(&checkpointremote.ForkOwnerMismatchError{ + PushOwner: "alice", + TargetOwner: "acme", + TargetRepo: "acme/checkpoints", + }) + assert.Contains(t, msg, "NOT pushed", "must state checkpoints were not pushed") + assert.Contains(t, msg, "acme/checkpoints", "must name the configured private target") + assert.Contains(t, msg, `"alice"`, "must name the fork owner") + assert.Contains(t, msg, ".entire/settings.local.json", "must point at the local settings file") + assert.Contains(t, msg, `"checkpoint_remote": null`, "must show the clear-target escape hatch") + assert.Contains(t, msg, "point checkpoint_remote at your own private repo", "must show the repoint escape hatch") +} + // TestDeferCheckpointPushOnEmptyRemote_UsesLocalTrackingRefs verifies the guard // decides purely from local remote-tracking refs, with no network access: a // remote with no refs/remotes//* is treated as possibly-empty (defer),