From 31d8b983f3e77d2a2f171c037827d7b691dba5bf Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 13 Oct 2026 12:28:25 +0000 Subject: [PATCH] fix: stop losing, duplicating, and exposing tracked commands Correct command pairing, cursor handling, encryption fail-closed behavior, stale-socket fallback, hook duplicates, and owner-only permissions on tokens and local history. Co-authored-by: Le He --- README.md | 3 +- commands/auth.go | 23 +++++++++-- commands/auth_perm_test.go | 47 ++++++++++++++++++++++ commands/gc.go | 2 +- commands/logger.go | 9 ++++- commands/track.go | 36 ++++++++++++++--- commands/track_cov_test.go | 37 +++++++++++++++++ daemon/handlers.sync.go | 66 ++++++++++++++++++------------- daemon/handlers.track.go | 14 ++++++- daemon/handlers_track_cov_test.go | 28 ++++++++++++- daemon/socket.go | 3 +- daemon/sync_encrypt_cov_test.go | 64 ++++++++++++++++++++++++++++++ docs/CONFIG.md | 22 ++++++++++- model/api.go | 31 +++++++-------- model/api_test.go | 24 +++++++++++ model/circuit_breaker.go | 12 +++++- model/command.go | 45 ++++++++++++++++++--- model/command_test.go | 20 +++++++++- model/config.go | 3 ++ model/config_cov_test.go | 19 +++++++++ model/db.go | 6 ++- model/db_scan_test.go | 42 ++++++++++++++++++++ model/hooks/bash.bash | 3 ++ model/hooks/zsh.zsh | 12 +++++- model/hooks_regression_test.go | 27 +++++++++++++ model/list.go | 4 +- model/list_test.go | 23 +++++++++++ model/shell.go | 13 ++++-- model/shell_install_test.go | 18 ++++++++- model/store_file.go | 11 +++--- model/store_file_test.go | 7 ++++ model/tracking_build.go | 6 ++- model/tracking_build_test.go | 25 ++++++++++++ 33 files changed, 620 insertions(+), 85 deletions(-) create mode 100644 commands/auth_perm_test.go create mode 100644 model/db_scan_test.go create mode 100644 model/hooks_regression_test.go diff --git a/README.md b/README.md index 2a0321c..2f914f6 100644 --- a/README.md +++ b/README.md @@ -240,7 +240,8 @@ For Codex, and for Claude Code sessions without the mod, the ShellTime server lo - **Data masking** (`dataMasking`, on by default) redacts sensitive command content before it leaves your machine. - **Exclusion patterns** skip matching commands entirely, so they are never recorded. -- **Encryption** (`encrypted: true`, set in the config `shelltime auth` creates) makes the daemon encrypt command uploads with your token's public key (RSA + AES-GCM). It only applies in daemon mode, and if the key can't be fetched the upload fails instead of going out unencrypted. +- **Encryption** (`encrypted: true`, set in the config `shelltime auth` creates) makes the daemon encrypt command uploads with your token's public key (RSA + AES-GCM). It only applies in daemon mode. If the key can't be fetched, is empty, or encryption fails, the upload fails instead of going out unencrypted or with a key the server cannot unwrap. +- **`shelltime auth`** writes `~/.shelltime` as mode `0700` and `config.yaml` as mode `0600`. - **AI coding telemetry** includes prompts and tool details unless you turn them off (see [Claude Code and Codex Telemetry](#claude-code-and-codex-telemetry)). `--no-prompts` keeps prompt text out of a backfill. - **Local config overrides** keep secrets like tokens out of your main config file. diff --git a/commands/auth.go b/commands/auth.go index f3962cb..3f755e1 100644 --- a/commands/auth.go +++ b/commands/auth.go @@ -37,10 +37,17 @@ func commandAuth(c *cli.Context) error { defer span.End() configDir := os.ExpandEnv("$HOME/" + model.COMMAND_BASE_STORAGE_FOLDER) if _, err := os.Stat(configDir); os.IsNotExist(err) { - err = os.Mkdir(configDir, 0755) + err = os.Mkdir(configDir, 0700) if err != nil { return fmt.Errorf("failed to create config directory: %w", err) } + } else if err != nil { + return fmt.Errorf("failed to stat config directory: %w", err) + } + // The directory holds the API token. 0755 lets other users on the machine + // traverse it and read a world-readable config.yaml. + if err := os.Chmod(configDir, 0700); err != nil { + return fmt.Errorf("failed to restrict config directory permissions: %w", err) } SetupLogger(configDir) @@ -60,7 +67,7 @@ func commandAuth(c *cli.Context) error { // Prepend $schema for IDE autocompletion support schemaHeader := "# yaml-language-server: $schema=./config-schema.json\n" content = append([]byte(schemaHeader), content...) - err = os.WriteFile(configFile, content, 0644) + err = writeTokenConfig(configFile, content) if err != nil { return fmt.Errorf("failed to create config file: %w", err) } @@ -88,7 +95,7 @@ func commandAuth(c *cli.Context) error { if err != nil { return fmt.Errorf("failed to marshal config: %w", err) } - err = os.WriteFile(configFile, content, 0644) + err = writeTokenConfig(configFile, content) if err != nil { return fmt.Errorf("failed to write config file: %w", err) } @@ -146,6 +153,16 @@ func ApplyTokenByHandshake(_ctx context.Context, config model.ShellTimeConfig) ( } } +// writeTokenConfig writes the config that contains the API token and forces +// owner-only permissions. WriteFile does not change the mode of a file that +// already exists, so a previous 0644 config would stay world-readable. +func writeTokenConfig(path string, content []byte) error { + if err := os.WriteFile(path, content, 0600); err != nil { + return err + } + return os.Chmod(path, 0600) +} + // generateSchemaFile generates the JSON schema file for config autocompletion func generateSchemaFile(path string) error { reflector := &jsonschema.Reflector{ diff --git a/commands/auth_perm_test.go b/commands/auth_perm_test.go new file mode 100644 index 0000000..2b95752 --- /dev/null +++ b/commands/auth_perm_test.go @@ -0,0 +1,47 @@ +package commands + +import ( + "os" + "path/filepath" + "testing" + + "github.com/malamtime/cli/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v2" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/trace/noop" +) + +func TestCommandAuth_RestrictsConfigPermissions(t *testing.T) { + otel.SetTracerProvider(noop.NewTracerProvider()) + prevSkip := SKIP_LOGGER_SETTINGS + SKIP_LOGGER_SETTINGS = true + t.Cleanup(func() { SKIP_LOGGER_SETTINGS = prevSkip }) + + home := t.TempDir() + t.Setenv("HOME", home) + dir := filepath.Join(home, ".shelltime") + require.NoError(t, os.Mkdir(dir, 0o755)) + cfgPath := filepath.Join(dir, "config.yaml") + require.NoError(t, os.WriteFile(cfgPath, []byte("token: old\napiEndpoint: https://api.shelltime.xyz\nwebEndpoint: https://shelltime.xyz\n"), 0o644)) + + orig := configService + configService = model.NewConfigService(dir) + t.Cleanup(func() { configService = orig }) + + app := &cli.App{Name: "t", Commands: []*cli.Command{AuthCommand}} + require.NoError(t, app.Run([]string{"t", "auth", "--token", "secret-token"})) + + info, err := os.Stat(cfgPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + dirInfo, err := os.Stat(dir) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o700), dirInfo.Mode().Perm()) + + data, err := os.ReadFile(cfgPath) + require.NoError(t, err) + assert.Contains(t, string(data), "secret-token") + assert.NotContains(t, string(data), "token: old") +} diff --git a/commands/gc.go b/commands/gc.go index f86d89c..f96ffd3 100644 --- a/commands/gc.go +++ b/commands/gc.go @@ -43,7 +43,7 @@ func backupAndWriteFile(filePath string, content []byte) error { } } - if err := os.WriteFile(filePath, content, 0644); err != nil { + if err := model.WritePrivateFile(filePath, content); err != nil { slog.Warn("failed to write file", slog.String("file", filePath), slog.Any("err", err)) return fmt.Errorf("failed to write file %s: %w", filePath, err) } diff --git a/commands/logger.go b/commands/logger.go index 10ffb86..0c1c70d 100644 --- a/commands/logger.go +++ b/commands/logger.go @@ -17,7 +17,7 @@ func SetupLogger(baseFolder string) { logFilePath := baseFolder + "/log.log" if _, err := os.Stat(logFilePath); os.IsNotExist(err) { - err := os.MkdirAll(baseFolder, 0755) + err := os.MkdirAll(baseFolder, 0700) if err != nil { slog.Error("[ShellTime.xyz] failed to create log directory", slog.Any("err", err)) return @@ -29,8 +29,13 @@ func SetupLogger(baseFolder string) { return } } + // Logs can echo command lines. Tighten a world-readable file once; skip + // when it is already private so the per-command track path stays cheap. + if st, err := os.Stat(logFilePath); err == nil && st.Mode().Perm()&0o077 != 0 { + _ = os.Chmod(logFilePath, 0600) + } - f, err := os.OpenFile(logFilePath, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0755) + f, err := os.OpenFile(logFilePath, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0600) if err != nil { os.Stdout.WriteString(err.Error()) slog.Error("[ShellTime.xyz] on setup logger error", slog.Any("err", err)) diff --git a/commands/track.go b/commands/track.go index 69a1a56..503dc31 100644 --- a/commands/track.go +++ b/commands/track.go @@ -27,7 +27,7 @@ var TrackCommand *cli.Command = &cli.Command{ Name: "sessionId", Aliases: []string{"id"}, Value: 0, - Usage: "unix timestamp of the session", + Usage: "session identifier shared by the pre and post events (hooks pass a per-shell id)", }, &cli.StringFlag{ Name: "command", @@ -99,8 +99,10 @@ func commandTrack(c *cli.Context) error { // default socket, hand it the raw event (fire-and-forget) and return. The daemon // is a long-lived process: it reads config once (cached) and owns the // storage-engine decision (bolt vs txt), exclude filtering, sync and pruning. - if daemon.IsSocketReady(ctx, model.DefaultSocketPath) { - return sendTrackEventToDaemon(ctx, span, model.DefaultSocketPath, cmdPhase, instance, result) + // A socket file left behind by a dead daemon still stats as present; if the + // send fails, fall through and persist locally instead of dropping the command. + if forwarded, err := forwardTrackToDaemon(ctx, span, model.DefaultSocketPath, cmdPhase, instance, result); forwarded || err != nil { + return err } // Slow path: no daemon on the default socket. We can now afford to read config — @@ -119,8 +121,10 @@ func commandTrack(c *cli.Context) error { } // A daemon may still be listening on a non-default socket path. - if config.SocketPath != model.DefaultSocketPath && daemon.IsSocketReady(ctx, config.SocketPath) { - return sendTrackEventToDaemon(ctx, span, config.SocketPath, cmdPhase, instance, result) + if config.SocketPath != model.DefaultSocketPath { + if forwarded, err := forwardTrackToDaemon(ctx, span, config.SocketPath, cmdPhase, instance, result); forwarded || err != nil { + return err + } } // No daemon at all: persist to the local txt store and sync directly over HTTP. @@ -146,6 +150,21 @@ func commandTrack(c *cli.Context) error { return nil } +// forwardTrackToDaemon reports whether the event was accepted by a daemon on +// socketPath. A missing socket is not an error. A present socket that refuses +// the connection (stale socket file) is logged and reported as not forwarded +// so the caller can persist the command itself. +func forwardTrackToDaemon(ctx context.Context, span trace.Span, socketPath, cmdPhase string, instance *model.Command, result int) (bool, error) { + if !daemon.IsSocketReady(ctx, socketPath) { + return false, nil + } + if err := sendTrackEventToDaemon(ctx, span, socketPath, cmdPhase, instance, result); err != nil { + slog.Warn("failed to send track event to daemon; saving locally", slog.String("socket", socketPath), slog.Any("err", err)) + return false, nil + } + return true, nil +} + // sendTrackEventToDaemon forwards a single raw pre/post command event to the // daemon. The daemon owns persistence, exclude filtering, sync and pruning. func sendTrackEventToDaemon(ctx context.Context, span trace.Span, socketPath, cmdPhase string, instance *model.Command, result int) error { @@ -207,6 +226,13 @@ func trySyncLocalToServer( if isDryRun { return nil } + // SendLocalDataToServer returns nil without uploading when no token is + // configured. Advancing the cursor here would permanently skip those + // commands once the user authenticates. + if config.Token == "" && len(config.Endpoints) == 0 { + slog.Debug("no token configured; leaving commands buffered for a later sync") + return nil + } return store.SetCursor(ctx, result.LatestRecordingTime) } diff --git a/commands/track_cov_test.go b/commands/track_cov_test.go index 996fd52..736728e 100644 --- a/commands/track_cov_test.go +++ b/commands/track_cov_test.go @@ -126,6 +126,23 @@ func TestX3CommandTrack_PostSavesAndSyncs(t *testing.T) { assert.GreaterOrEqual(t, atomic.LoadInt32(&calls), int32(1), "post phase should sync the batch over HTTP") } +// TestX3CommandTrack_StaleSocketFallsBackToLocalSave covers a socket path that +// still exists after the daemon has died. Stat succeeds, Dial fails, and the +// command must be written locally instead of dropped. +func TestX3CommandTrack_StaleSocketFallsBackToLocalSave(t *testing.T) { + _, mc := x3SetupTrack(t) + stale := filepath.Join(t.TempDir(), "stale.sock") + require.NoError(t, os.WriteFile(stale, []byte("not a socket"), 0o644)) + mc.On("ReadConfigFile", mock.Anything).Return(model.ShellTimeConfig{SocketPath: stale}, nil) + + app := &cli.App{Name: "t", Commands: []*cli.Command{TrackCommand}} + require.NoError(t, app.Run([]string{"t", "track", "--phase", "pre", "--shell", "bash", "--command", "echo stale", "--id", "3"})) + + data, err := os.ReadFile(filepath.Join(model.GetCommandsStoragePath(), "pre.txt")) + require.NoError(t, err) + assert.Contains(t, string(data), "echo stale") +} + // --- trySyncLocalToServer direct ----------------------------------------------- // x3SeedPair writes a completed pre/post pair into the local file store under @@ -217,6 +234,26 @@ func TestX3TrySyncLocalToServer_DryRunSkipsCursor(t *testing.T) { assert.True(t, os.IsNotExist(statErr), "dry-run must not advance the cursor") } +// TestX3TrySyncLocalToServer_NoTokenDoesNotAdvanceCursor covers an unauthenticated +// sync: SendLocalDataToServer returns nil without uploading, and the cursor must +// stay unset so the commands are still eligible after the user authenticates. +func TestX3TrySyncLocalToServer_NoTokenDoesNotAdvanceCursor(t *testing.T) { + otel.SetTracerProvider(noop.NewTracerProvider()) + SKIP_LOGGER_SETTINGS = true + t.Setenv("HOME", t.TempDir()) + x3SeedPair(t) + + cfg := model.ShellTimeConfig{ + SocketPath: filepath.Join(t.TempDir(), "absent.sock"), + FlushCount: 1, + } + require.NoError(t, trySyncLocalToServer(context.Background(), cfg, syncOptions{isForceSync: true})) + + _, noCursor, err := model.NewFileStore().GetLastCursor(context.Background()) + require.NoError(t, err) + assert.True(t, noCursor, "commands must stay buffered until a token is configured") +} + // TestX3TrySyncLocalToServer_SyncErrorPropagates covers the DoSyncData error // branch: the server returns 500, so the send fails and the error surfaces. func TestX3TrySyncLocalToServer_SyncErrorPropagates(t *testing.T) { diff --git a/daemon/handlers.sync.go b/daemon/handlers.sync.go index 30539c2..3b834db 100644 --- a/daemon/handlers.sync.go +++ b/daemon/handlers.sync.go @@ -4,7 +4,9 @@ import ( "context" "encoding/base64" "encoding/json" + "fmt" "log/slog" + "strings" "time" "github.com/malamtime/cli/model" @@ -80,40 +82,48 @@ func sendTrackArgsToServer(ctx context.Context, syncMsg model.PostTrackArgs) err // configured intent. return err } - if len(ot.PublicKey) > 0 { - rs := model.NewRSAService() - as := model.NewAESGCMService() - - k, _, err := as.GenerateKeys() - - if err != nil { - slog.Error("Failed to generate aes-gcm key", slog.Any("err", err)) - } + if strings.TrimSpace(ot.PublicKey) == "" { + err := fmt.Errorf("encryption is enabled but the open token public key is empty") + slog.Error(err.Error()) + return err + } - encodedKey, _, err := rs.Encrypt(ot.PublicKey, k) + rs := model.NewRSAService() + as := model.NewAESGCMService() - if err != nil { - slog.Error("Failed to encrypt key", slog.Any("err", err)) - } + k, _, err := as.GenerateKeys() + if err != nil { + slog.Error("Failed to generate aes-gcm key", slog.Any("err", err)) + return err + } - buf, err := json.Marshal(payload) + encodedKey, _, err := rs.Encrypt(ot.PublicKey, k) + if err != nil { + slog.Error("Failed to encrypt key", slog.Any("err", err)) + return err + } - if err != nil { - slog.Error("Failed to marshal payload", slog.Any("err", err)) - return err - } + buf, err := json.Marshal(payload) + if err != nil { + slog.Error("Failed to marshal payload", slog.Any("err", err)) + return err + } - encryptedData, nonce, err := as.Encrypt(string(k), buf) - if err != nil { - slog.Error("Failed to encrypt data", slog.Any("err", err)) - return err - } + encryptedData, nonce, err := as.Encrypt(string(k), buf) + if err != nil { + slog.Error("Failed to encrypt data", slog.Any("err", err)) + return err + } + if len(encodedKey) == 0 || len(encryptedData) == 0 || len(nonce) == 0 { + err := fmt.Errorf("encryption produced an empty key, ciphertext, or nonce") + slog.Error(err.Error()) + return err + } - realPayload = model.PostTrackArgs{ - Encrypted: base64.StdEncoding.EncodeToString(encryptedData), - AesKey: base64.StdEncoding.EncodeToString(encodedKey), - Nonce: base64.StdEncoding.EncodeToString(nonce), - } + realPayload = model.PostTrackArgs{ + Encrypted: base64.StdEncoding.EncodeToString(encryptedData), + AesKey: base64.StdEncoding.EncodeToString(encodedKey), + Nonce: base64.StdEncoding.EncodeToString(nonce), } } diff --git a/daemon/handlers.track.go b/daemon/handlers.track.go index 9e77c31..41deacd 100644 --- a/daemon/handlers.track.go +++ b/daemon/handlers.track.go @@ -99,6 +99,14 @@ func handlePubSubTrackPost(ctx context.Context, payload interface{}) error { return nil } + // Nothing would be uploaded. Ack the message (a nack re-runs SavePost and + // duplicates the command) and leave the cursor where it is so a later + // authenticated sync can still send this batch. + if cfg.Token == "" && len(cfg.Endpoints) == 0 { + slog.Debug("no token configured; leaving commands buffered for a later sync") + return nil + } + args := model.PostTrackArgs{ CursorID: result.LatestRecordingTime.UnixNano(), Data: result.Data, @@ -107,8 +115,10 @@ func handlePubSubTrackPost(ctx context.Context, payload interface{}) error { if err := sendTrackArgsToServer(ctx, args); err != nil { slog.Error("Failed to send tracking data from command store", slog.Any("err", err)) - // Leave the data in the store; a later post will retry. Do not advance cursor. - return err + // Leave the data in the store; a later post will retry. Do not advance + // the cursor. Returning an error nacks the message, and the subscriber + // redelivers it — which appends this post command again. + return nil } if err := store.SetCursor(ctx, result.LatestRecordingTime); err != nil { diff --git a/daemon/handlers_track_cov_test.go b/daemon/handlers_track_cov_test.go index 22852b9..d4d86f7 100644 --- a/daemon/handlers_track_cov_test.go +++ b/daemon/handlers_track_cov_test.go @@ -32,6 +32,7 @@ type x3TrackStore struct { setCursorCalls int pruneCalls int + savePostCalls int } func (s *x3TrackStore) SavePre(ctx context.Context, cmd model.Command, rt time.Time) error { @@ -45,6 +46,7 @@ func (s *x3TrackStore) SavePre(ctx context.Context, cmd model.Command, rt time.T } func (s *x3TrackStore) SavePost(ctx context.Context, cmd model.Command, result int, rt time.Time) error { + s.savePostCalls++ if s.savePostErr != nil { return s.savePostErr } @@ -209,8 +211,32 @@ func TestX3TrackPost_SendErrorLeavesCursor(t *testing.T) { require.NoError(t, store.SavePre(context.Background(), cmd, now)) err := handlePubSubTrackPost(context.Background(), TrackEventPayload{Command: cmd, RecordingTimeNano: now.UnixNano()}) - require.Error(t, err) + // Ack the message. A nack redelivers it and SavePost would append the same + // command again. The row stays buffered and the cursor does not move, so a + // later post retries the upload. + require.NoError(t, err) assert.Equal(t, 0, store.setCursorCalls, "cursor must not advance on send failure") + assert.Equal(t, 1, store.savePostCalls, "the failed upload must not be saved twice") +} + +// TestX3TrackPost_NoTokenLeavesCursor covers an unauthenticated daemon: the post +// is stored once, nothing is uploaded, and the cursor does not move. +func TestX3TrackPost_NoTokenLeavesCursor(t *testing.T) { + x3SwapTrackGlobals(t) + store := &x3TrackStore{noCursorExist: true} + commandStore = store + mc := model.NewMockConfigService(t) + mc.On("ReadConfigFile", mock.Anything).Return(model.ShellTimeConfig{FlushCount: 1}, nil) + stConfig = mc + + now := time.Now() + cmd := model.Command{Shell: "bash", SessionID: 1, Command: "ls", Username: "u", Hostname: "h", Time: now} + require.NoError(t, store.SavePre(context.Background(), cmd, now)) + + err := handlePubSubTrackPost(context.Background(), TrackEventPayload{Command: cmd, RecordingTimeNano: now.UnixNano()}) + require.NoError(t, err) + assert.Equal(t, 0, store.setCursorCalls) + assert.Equal(t, 1, store.savePostCalls) } // TestX3TrackPost_SetCursorError covers the SetCursor error branch after a diff --git a/daemon/socket.go b/daemon/socket.go index e0fa92f..38547b9 100644 --- a/daemon/socket.go +++ b/daemon/socket.go @@ -131,7 +131,7 @@ func (p *SocketHandler) Start() error { return err } if err := os.Chmod(p.config.SocketPath, 0777); err != nil { - slog.Error("Failed to change the socket permission to 0755", slog.String("socketPath", p.config.SocketPath)) + slog.Error("Failed to change the socket permission to 0777", slog.String("socketPath", p.config.SocketPath)) return err } p.listener = listener @@ -193,6 +193,7 @@ func (p *SocketHandler) handleConnection(conn net.Conn) { buf, err := json.Marshal(msg) if err != nil { slog.Error("Error encoding message", slog.Any("err", err)) + return } chMsg := message.NewMessage(watermill.NewUUID(), buf) diff --git a/daemon/sync_encrypt_cov_test.go b/daemon/sync_encrypt_cov_test.go index 6ad2b77..1948a21 100644 --- a/daemon/sync_encrypt_cov_test.go +++ b/daemon/sync_encrypt_cov_test.go @@ -79,6 +79,70 @@ func TestX3SendTrackArgsToServer_EncryptedHappyPath(t *testing.T) { assert.Contains(t, sentBody, "encrypted", "payload should carry the encrypted envelope") } +func TestX3SendTrackArgsToServer_EmptyPublicKeyFailsClosed(t *testing.T) { + var syncHit bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/opentoken/publickey") { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(model.OpenTokenPublicKeyResponse{ + Data: model.OpenTokenPublicKey{ID: 1, PublicKey: " "}, + }) + return + } + syncHit = true + w.WriteHeader(http.StatusNoContent) + })) + t.Cleanup(srv.Close) + + enabled := true + mc := model.NewMockConfigService(t) + mc.On("ReadConfigFile", mock.Anything).Return(model.ShellTimeConfig{ + Token: "tok", APIEndpoint: srv.URL, Encrypted: &enabled, + }, nil) + x3SwapStConfig(t, mc) + prevCB := syncCircuitBreaker + syncCircuitBreaker = nil + t.Cleanup(func() { syncCircuitBreaker = prevCB }) + + err := sendTrackArgsToServer(context.Background(), model.PostTrackArgs{ + Data: []model.TrackingData{{Command: "super-secret-command"}}, + }) + require.Error(t, err) + assert.False(t, syncHit, "an empty public key must not fall back to plaintext") +} + +func TestX3SendTrackArgsToServer_InvalidPublicKeyFailsClosed(t *testing.T) { + var syncHit bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/opentoken/publickey") { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(model.OpenTokenPublicKeyResponse{ + Data: model.OpenTokenPublicKey{ID: 1, PublicKey: "not-a-pem"}, + }) + return + } + syncHit = true + w.WriteHeader(http.StatusNoContent) + })) + t.Cleanup(srv.Close) + + enabled := true + mc := model.NewMockConfigService(t) + mc.On("ReadConfigFile", mock.Anything).Return(model.ShellTimeConfig{ + Token: "tok", APIEndpoint: srv.URL, Encrypted: &enabled, + }, nil) + x3SwapStConfig(t, mc) + prevCB := syncCircuitBreaker + syncCircuitBreaker = nil + t.Cleanup(func() { syncCircuitBreaker = prevCB }) + + err := sendTrackArgsToServer(context.Background(), model.PostTrackArgs{ + Data: []model.TrackingData{{Command: "super-secret-command"}}, + }) + require.Error(t, err) + assert.False(t, syncHit, "a public key that cannot wrap the AES key must not be sent") +} + // The public-key-fetch-failure branch is covered by // TestSendTrackArgsToServer_PublicKeyFetchErrorFailsClosed in // handlers_sync_pubkey_test.go (it previously panicked on a nil pointer; the diff --git a/docs/CONFIG.md b/docs/CONFIG.md index b5206d2..6217ff1 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -422,8 +422,8 @@ logCleanup: **Files cleaned:** - `~/.shelltime/log.log` -- `~/.shelltime/heartbeat.log` -- `~/.shelltime/sync-pending.txt` +- `~/.shelltime/coding-heartbeat.data.log` +- `~/.shelltime/sync-pending.jsonl` - `~/.shelltime/logs/shelltime-daemon.log` (macOS) - `~/.shelltime/logs/shelltime-daemon.err` (macOS) @@ -442,6 +442,22 @@ enableMetrics: false **Warning:** Enabling metrics adds overhead to every command. Only use for debugging. +### Local command buffer + +| Option | Type | Default | Description | +|--------|------|---------|-------------| +| `storage.engine` | string | `file` | `file` stores commands in txt files. `bolt` stores them in a database the daemon owns | + +```yaml +# Machine-specific. config.local.yaml overrides the base config. +storage: + engine: bolt +``` + +The bolt engine is opened only by the daemon. The CLI keeps using the txt files when no daemon is running. + +`shelltime auth` writes `~/.shelltime` as mode `0700` and `config.yaml` as mode `0600`, so the API token is not readable by other users on the machine. + --- ## Complete Example @@ -508,6 +524,8 @@ logCleanup: # --- Advanced --- socketPath: "/tmp/shelltime.sock" enableMetrics: false +# storage: +# engine: bolt # file (default) or bolt; bolt is daemon-owned # --- Additional Sync Targets --- # endpoints: diff --git a/model/api.go b/model/api.go index 8e0d595..123c94f 100644 --- a/model/api.go +++ b/model/api.go @@ -25,11 +25,11 @@ type TrackingData struct { } type TrackingMetaData struct { - Hostname string `json:"hostname"` - Username string `json:"username"` - OS string `json:"os"` - OSVersion string `json:"osVersion"` - Shell string `json:"shell"` + Hostname string `json:"hostname"` + Username string `json:"username"` + OS string `json:"os"` + OSVersion string `json:"osVersion"` + Shell string `json:"shell"` Terminal string `json:"terminal,omitempty"` Multiplexer string `json:"multiplexer,omitempty"` @@ -76,23 +76,22 @@ func doSendData(ctx context.Context, endpoint Endpoint, data PostTrackArgs) erro func SendLocalDataToServer(ctx context.Context, config ShellTimeConfig, data PostTrackArgs) error { ctx, span := modelTracer.Start(ctx, "sync.local") defer span.End() - if config.Token == "" { + authPair := make([]Endpoint, 0, len(config.Endpoints)+1) + if config.Token != "" { + authPair = append(authPair, Endpoint{ + Token: config.Token, + APIEndpoint: config.APIEndpoint, + }) + } + authPair = append(authPair, config.Endpoints...) + if len(authPair) == 0 { slog.Debug("no token available. do not sync to server") return nil } var wg sync.WaitGroup - wg.Add(len(config.Endpoints) + 1) - - authPair := make([]Endpoint, len(config.Endpoints)+1) - - authPair[0] = Endpoint{ - Token: config.Token, - APIEndpoint: config.APIEndpoint, - } - - copy(authPair[1:], config.Endpoints) + wg.Add(len(authPair)) errs := make(chan error, len(authPair)) diff --git a/model/api_test.go b/model/api_test.go index a48b874..35fde9f 100644 --- a/model/api_test.go +++ b/model/api_test.go @@ -171,6 +171,30 @@ func (s *apiTestSuite) TestSendLocalDataToServer() { assert.Equal(t, int32(3), requestCount.Load()) // Main endpoint + 2 additional endpoints }) + s.T().Run("endpoints only when primary token is empty", func(t *testing.T) { + var requestCount atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount.Add(1) + assert.Equal(t, "CLI endpoint-token", r.Header.Get("Authorization")) + w.WriteHeader(http.StatusNoContent) + })) + defer server.Close() + + config := ShellTimeConfig{ + Token: "", + Endpoints: []Endpoint{{ + Token: "endpoint-token", + APIEndpoint: server.URL, + }}, + } + err := SendLocalDataToServer(context.Background(), config, PostTrackArgs{ + CursorID: time.Now().UnixNano(), + Data: []TrackingData{{Command: "from-endpoint"}}, + }) + assert.NoError(t, err) + assert.Equal(t, int32(1), requestCount.Load()) + }) + s.T().Run("partial failure", func(t *testing.T) { successServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) diff --git a/model/circuit_breaker.go b/model/circuit_breaker.go index 10e0df9..d968693 100644 --- a/model/circuit_breaker.go +++ b/model/circuit_breaker.go @@ -126,11 +126,14 @@ func (s *CircuitBreakerService) RecordFailure() { func (s *CircuitBreakerService) SaveForRetry(ctx context.Context, payload []byte) error { filePath := os.ExpandEnv(fmt.Sprintf("%s/%s", "$HOME", SYNC_PENDING_FILE)) - file, err := os.OpenFile(filePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) + file, err := os.OpenFile(filePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) if err != nil { return err } defer file.Close() + if err := file.Chmod(0600); err != nil { + return err + } _, err = file.Write(payload) if err != nil { @@ -237,7 +240,7 @@ func (s *CircuitBreakerService) rewriteLogFile(logFilePath string, lines []strin } tempFile := logFilePath + ".tmp" - file, err := os.OpenFile(tempFile, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644) + file, err := os.OpenFile(tempFile, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600) if err != nil { return fmt.Errorf("failed to create temp file: %w", err) } @@ -250,6 +253,11 @@ func (s *CircuitBreakerService) rewriteLogFile(logFilePath string, lines []strin } } + if err := file.Chmod(0600); err != nil { + file.Close() + os.Remove(tempFile) + return fmt.Errorf("failed to restrict pending sync file permissions: %w", err) + } if err := file.Close(); err != nil { os.Remove(tempFile) return fmt.Errorf("failed to close temp file: %w", err) diff --git a/model/command.go b/model/command.go index 80deca6..143edaa 100644 --- a/model/command.go +++ b/model/command.go @@ -45,10 +45,15 @@ type Command struct { RecordingTime time.Time `json:"-"` } +// privateFileMode keeps command history, cursors, and queued sync payloads +// readable only by the owning user. Those files can contain secrets that were +// typed into the shell. +const privateFileMode os.FileMode = 0o600 + func ensureStorageFolder() error { storageFolder := GetCommandsStoragePath() if _, err := os.Stat(storageFolder); os.IsNotExist(err) { - if err := os.MkdirAll(storageFolder, 0755); err != nil { + if err := os.MkdirAll(storageFolder, 0o700); err != nil { return fmt.Errorf("failed to create command storage folder: %v", err) } } @@ -56,6 +61,30 @@ func ensureStorageFolder() error { return nil } +// openPrivateAppend opens path for append, creating it if needed, and forces +// owner-only permissions. OpenFile's mode is masked by umask and does not +// change an existing file. +func openPrivateAppend(path string) (*os.File, error) { + f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY|os.O_CREATE, privateFileMode) + if err != nil { + return nil, err + } + if err := f.Chmod(privateFileMode); err != nil { + f.Close() + return nil, err + } + return f, nil +} + +// WritePrivateFile writes data and forces owner-only permissions, including +// when the file already exists (WriteFile leaves an existing mode unchanged). +func WritePrivateFile(path string, data []byte) error { + if err := os.WriteFile(path, data, privateFileMode); err != nil { + return err + } + return os.Chmod(path, privateFileMode) +} + func (c Command) DoSavePre() error { if err := ensureStorageFolder(); err != nil { return err @@ -67,7 +96,7 @@ func (c Command) DoSavePre() error { } preFile := GetPreCommandFilePath() - f, err := os.OpenFile(preFile, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0644) + f, err := openPrivateAppend(preFile) if err != nil { slog.Error("failed to open pre-command storage file", slog.Any("err", err)) return fmt.Errorf("failed to open pre-command storage file: %v", err) @@ -95,7 +124,7 @@ func (c Command) DoUpdate(result int) error { } postFile := GetPostCommandFilePath() - f, err := os.OpenFile(postFile, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0644) + f, err := openPrivateAppend(postFile) if err != nil { return fmt.Errorf("failed to open post-command storage file: %v", err) } @@ -223,7 +252,7 @@ func (cmd *Command) FromLineBytes(line []byte) (recordingTime time.Time, err err } func (cmd Command) FindClosestCommand(commandList []*Command, withSameKey bool) *Command { - closestPreCommand := new(Command) + var closestPreCommand *Command minTimeDiff := int64(^uint64(0) >> 1) // Max int64 value myKey := cmd.GetUniqueKey() @@ -239,12 +268,18 @@ func (cmd Command) FindClosestCommand(commandList []*Command, withSameKey bool) } } - timeDiff := cmd.Time.Unix() - c.Time.Unix() + // Whole seconds collapse two commands that ran in the same second onto + // the same diff, so the first candidate wins instead of the closest. + timeDiff := cmd.Time.UnixNano() - c.Time.UnixNano() if timeDiff >= 0 && timeDiff < minTimeDiff { minTimeDiff = timeDiff closestPreCommand = c } } + if closestPreCommand == nil { + // Callers historically check IsNil() on a non-nil result. + return new(Command) + } return closestPreCommand } diff --git a/model/command_test.go b/model/command_test.go index 30e6acc..fdb0844 100644 --- a/model/command_test.go +++ b/model/command_test.go @@ -367,7 +367,7 @@ func TestCommand_FindClosestCommand(t *testing.T) { commands := []*Command{ {Shell: "bash", SessionID: 100, Command: "ls", Username: "user", Time: baseTime.Add(-10 * time.Second)}, - {Shell: "bash", SessionID: 100, Command: "ls", Username: "user", Time: baseTime.Add(-5 * time.Second)}, // Closest + {Shell: "bash", SessionID: 100, Command: "ls", Username: "user", Time: baseTime.Add(-5 * time.Second)}, // Closest {Shell: "bash", SessionID: 100, Command: "ls", Username: "user", Time: baseTime.Add(-30 * time.Second)}, nil, // Should be skipped } @@ -383,6 +383,24 @@ func TestCommand_FindClosestCommand(t *testing.T) { } } +func TestCommand_FindClosestCommand_UsesNanoseconds(t *testing.T) { + base := time.Date(2024, 1, 15, 12, 0, 0, 0, time.UTC) + target := Command{ + Shell: "bash", + SessionID: 100, + Command: "ls", + Username: "user", + Time: base.Add(900 * time.Millisecond), + } + earlier := &Command{Shell: "bash", SessionID: 100, Command: "ls", Username: "user", Time: base.Add(100 * time.Millisecond)} + closer := &Command{Shell: "bash", SessionID: 100, Command: "ls", Username: "user", Time: base.Add(700 * time.Millisecond)} + + got := target.FindClosestCommand([]*Command{earlier, closer}, true) + if got == nil || !got.Time.Equal(closer.Time) { + t.Fatalf("expected the pre 200ms earlier, got %#v", got) + } +} + func TestCommand_FindClosestCommand_WithoutSameKey(t *testing.T) { baseTime := time.Date(2024, 1, 15, 12, 0, 0, 0, time.UTC) diff --git a/model/config.go b/model/config.go index d0df7fa..2ec18ea 100644 --- a/model/config.go +++ b/model/config.go @@ -185,6 +185,9 @@ func mergeConfig(base, local *ShellTimeConfig) { if local.Proxy != nil { base.Proxy = local.Proxy } + if local.Storage != nil { + base.Storage = local.Storage + } } func (cs *configService) ReadConfigFile(ctx context.Context, opts ...ReadConfigOption) (config ShellTimeConfig, err error) { diff --git a/model/config_cov_test.go b/model/config_cov_test.go index bdb62cb..6a4a910 100644 --- a/model/config_cov_test.go +++ b/model/config_cov_test.go @@ -40,6 +40,7 @@ func TestMergeConfig_AllOverrides(t *testing.T) { SocketPath: "/tmp/local.sock", CodeTracking: &CodeTracking{Token: "ct"}, Proxy: &ProxyConfig{URL: "socks5://127.0.0.1:1080"}, + Storage: &StorageConfig{Engine: StorageEngineBolt}, } mergeConfig(base, local) @@ -62,6 +63,8 @@ func TestMergeConfig_AllOverrides(t *testing.T) { require.NotNil(t, base.CodeTracking) require.NotNil(t, base.Proxy) assert.Equal(t, "socks5://127.0.0.1:1080", base.Proxy.URL) + require.NotNil(t, base.Storage) + assert.Equal(t, StorageEngineBolt, base.Storage.Engine) } // TestMergeConfig_NoOverrides ensures zero-valued local fields leave base intact. @@ -93,6 +96,22 @@ func TestReadConfigFile_MergesLocalOverBase(t *testing.T) { require.Contains(t, cfg.Exclude, "secret-cmd") } +// TestReadConfigFile_LocalStorageOverridesBase covers config.local.yaml +// replacing the base storage engine. mergeConfig used to drop Storage entirely. +func TestReadConfigFile_LocalStorageOverridesBase(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yaml"), + []byte("token: base-tok\nstorage:\n engine: file\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "config.local.yaml"), + []byte("storage:\n engine: bolt\n"), 0o644)) + + cs := NewConfigService(dir) + cfg, err := cs.ReadConfigFile(context.Background()) + require.NoError(t, err) + require.NotNil(t, cfg.Storage) + assert.Equal(t, StorageEngineBolt, cfg.Storage.Engine) +} + // TestReadConfigFile_InvalidLocalIsIgnored covers the branch where the local // config fails to parse: the warning is logged and base config is still used. func TestReadConfigFile_InvalidLocalIsIgnored(t *testing.T) { diff --git a/model/db.go b/model/db.go index d818122..1851945 100644 --- a/model/db.go +++ b/model/db.go @@ -187,7 +187,11 @@ func GetPostCommands(ctx context.Context) ([][]byte, int, error) { if len(l) == 0 { continue } - nonEmptyContent = append(nonEmptyContent, l) + // Scanner.Bytes shares its buffer. Once the file is larger than that + // buffer, Scan shifts it and every previously stored slice is overwritten. + copied := make([]byte, len(l)) + copy(copied, l) + nonEmptyContent = append(nonEmptyContent, copied) } if err := scanner.Err(); err != nil { diff --git a/model/db_scan_test.go b/model/db_scan_test.go new file mode 100644 index 0000000..21e3641 --- /dev/null +++ b/model/db_scan_test.go @@ -0,0 +1,42 @@ +package model + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestGetPostCommands_CopiesLinesBeyondScannerBuffer locks the scanner-buffer +// aliasing bug: once post.txt is larger than MAX_BUFFER_SIZE, Scan shifts its +// buffer and every previously stored slice is overwritten unless copied. +func TestGetPostCommands_CopiesLinesBeyondScannerBuffer(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + dir := filepath.Join(os.Getenv("HOME"), ".shelltime", "commands") + require.NoError(t, os.MkdirAll(dir, 0o700)) + + const lineCount = 12000 + lines := make([]string, lineCount) + var b strings.Builder + for i := 0; i < lineCount; i++ { + lines[i] = fmt.Sprintf("line-%05d-%s", i, strings.Repeat("x", 40)) + b.WriteString(lines[i]) + b.WriteByte('\n') + } + require.Greater(t, b.Len(), MAX_BUFFER_SIZE) + require.NoError(t, os.WriteFile(filepath.Join(dir, "post.txt"), []byte(b.String()), 0o600)) + + got, n, err := GetPostCommands(context.Background()) + require.NoError(t, err) + require.Equal(t, lineCount, n) + require.Len(t, got, lineCount) + for i, line := range got { + if string(line) != lines[i] { + t.Fatalf("line %d corrupted: got %q want %q", i, line, lines[i]) + } + } +} diff --git a/model/hooks/bash.bash b/model/hooks/bash.bash index 2f4e196..86a6274 100644 --- a/model/hooks/bash.bash +++ b/model/hooks/bash.bash @@ -48,6 +48,9 @@ precmd_invoke_cmd() { local LAST_RESULT=$? # BASH_COMMAND in precmd is the *previous* command local CMD="$LAST_COMMAND" + # Consume it. precmd also runs for an empty prompt (the user pressed + # enter), and leaving LAST_COMMAND set would record the same command again. + LAST_COMMAND="" # Check if command starts with exit, logout, or reboot if [[ "$CMD" =~ ^(exit|logout|reboot) ]]; then return diff --git a/model/hooks/zsh.zsh b/model/hooks/zsh.zsh index fb89c05..828fb40 100644 --- a/model/hooks/zsh.zsh +++ b/model/hooks/zsh.zsh @@ -11,6 +11,11 @@ fi # Create a timestamp for the session when the shell starts SESSION_ID=$(date +%Y%m%d%H%M%S) +# Set by preexec and consumed by the next precmd. Empty means this prompt +# does not follow a command we tracked (shell startup, or enter on a blank +# line). fc -ln -1 would otherwise repeat the previous history entry. +typeset -g _SHELLTIME_LAST_CMD="" + # Define the preexec function preexec() { local CMD=$1 @@ -19,13 +24,18 @@ preexec() { return fi + _SHELLTIME_LAST_CMD=$CMD shelltime track -s=zsh -id=$SESSION_ID -cmd="$CMD" -p=pre --ppid=$PPID &> /dev/null } # Define the postexec function (in zsh, it's called precmd) precmd() { local LAST_RESULT=$? - local CMD=$(fc -ln -1) + local CMD=$_SHELLTIME_LAST_CMD + _SHELLTIME_LAST_CMD="" + if [[ -z $CMD ]]; then + return + fi # Check if command starts with exit, logout, or reboot if [[ $CMD =~ ^(exit|logout|reboot) ]]; then return diff --git a/model/hooks_regression_test.go b/model/hooks_regression_test.go new file mode 100644 index 0000000..4357041 --- /dev/null +++ b/model/hooks_regression_test.go @@ -0,0 +1,27 @@ +package model + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestEmbeddedHooksDropRepeatedEmptyCommands(t *testing.T) { + zsh := string(EmbeddedZshHook) + assert.Contains(t, zsh, "_SHELLTIME_LAST_CMD") + assert.Contains(t, zsh, `_SHELLTIME_LAST_CMD=""`) + + bash := string(EmbeddedBashHook) + // Cleared before the post is recorded so an empty enter does not re-post. + assert.Contains(t, bash, `LAST_COMMAND=""`) + idx := strings.Index(bash, "precmd_invoke_cmd") + if idx < 0 { + t.Fatal("bash hook is missing precmd_invoke_cmd") + } + body := bash[idx:] + clearAt := strings.Index(body, `LAST_COMMAND=""`) + trackAt := strings.Index(body, "shelltime track") + assert.Greater(t, clearAt, 0) + assert.Greater(t, trackAt, clearAt, "LAST_COMMAND must be cleared before the post is sent") +} diff --git a/model/list.go b/model/list.go index cafa1b1..9a78ef7 100644 --- a/model/list.go +++ b/model/list.go @@ -43,7 +43,9 @@ func BuildListedCommands(ctx context.Context, store CommandStore) ([]ListedComma closestPreCommand := postCommand.FindClosestCommand(preCommands, false) startTime := postCommand.Time - if closestPreCommand != nil { + // FindClosestCommand returns a non-nil empty command when nothing matches. + // Treating that as a hit replaces the real end time with the zero time. + if closestPreCommand != nil && !closestPreCommand.IsNil() { startTime = closestPreCommand.Time } diff --git a/model/list_test.go b/model/list_test.go index a0e2ea0..d2daf09 100644 --- a/model/list_test.go +++ b/model/list_test.go @@ -36,3 +36,26 @@ func TestBuildListedCommands(t *testing.T) { require.Equal(t, start.Unix(), listed[0].StartTime.Unix()) require.Equal(t, post.Time.Unix(), listed[0].EndTime.Unix()) } + +func TestBuildListedCommands_SameSecondFuturePreDoesNotWin(t *testing.T) { + store, err := newBoltStore(filepath.Join(t.TempDir(), "commands.db")) + require.NoError(t, err) + defer store.Close() + + ctx := context.Background() + base := time.Date(2024, 6, 1, 12, 0, 0, 0, time.UTC) + postTime := base.Add(100 * time.Millisecond) + preTime := base.Add(500 * time.Millisecond) + cmd := Command{Shell: "bash", SessionID: 1, Command: "make", Username: "u", Hostname: "h", Time: preTime} + require.NoError(t, store.SavePre(ctx, cmd, preTime)) + post := cmd + post.Time = postTime + require.NoError(t, store.SavePost(ctx, post, 0, postTime)) + + listed, err := BuildListedCommands(ctx, store) + require.NoError(t, err) + require.Len(t, listed, 1) + require.True(t, listed[0].StartTime.Equal(postTime), "a pre later in the same second must not replace the start time") + require.False(t, listed[0].StartTime.IsZero()) + require.True(t, listed[0].EndTime.Equal(postTime)) +} diff --git a/model/shell.go b/model/shell.go index 68ff890..9c5456b 100644 --- a/model/shell.go +++ b/model/shell.go @@ -3,6 +3,7 @@ package model import ( "bufio" + "bytes" "fmt" "io" "os" @@ -19,11 +20,17 @@ type ShellHookService interface { ShellName() string } -// ensureHookFile writes the embedded hook script to disk if it doesn't already exist. +// ensureHookFile writes the embedded hook script to disk. // This allows Homebrew-installed users to get hook files without the curl installer. +// An existing file is replaced when its contents differ so `hooks install` picks +// up hook bugfixes. Identical contents are left untouched. func ensureHookFile(hookFilePath string, embeddedContent []byte) error { - if _, err := os.Stat(hookFilePath); err == nil { - return nil // already exists + existing, err := os.ReadFile(hookFilePath) + if err == nil && bytes.Equal(existing, embeddedContent) { + return nil + } + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to read hook file: %w", err) } hookDir := filepath.Dir(hookFilePath) if err := os.MkdirAll(hookDir, 0755); err != nil { diff --git a/model/shell_install_test.go b/model/shell_install_test.go index e4a0c41..f65c938 100644 --- a/model/shell_install_test.go +++ b/model/shell_install_test.go @@ -146,13 +146,27 @@ func TestEnsureHookFile(t *testing.T) { assert.Equal(t, "HOOK", string(b)) }) - t.Run("no-op when present", func(t *testing.T) { + t.Run("refreshes stale contents", func(t *testing.T) { p := filepath.Join(dir, "exists.sh") require.NoError(t, os.WriteFile(p, []byte("ORIGINAL"), 0644)) require.NoError(t, ensureHookFile(p, []byte("REPLACEMENT"))) b, err := os.ReadFile(p) require.NoError(t, err) - assert.Equal(t, "ORIGINAL", string(b), "existing file must not be overwritten") + assert.Equal(t, "REPLACEMENT", string(b), "hooks install must replace a stale hook script") + }) + + t.Run("leaves identical contents", func(t *testing.T) { + p := filepath.Join(dir, "same.sh") + require.NoError(t, os.WriteFile(p, []byte("SAME"), 0644)) + before, err := os.Stat(p) + require.NoError(t, err) + require.NoError(t, ensureHookFile(p, []byte("SAME"))) + b, err := os.ReadFile(p) + require.NoError(t, err) + assert.Equal(t, "SAME", string(b)) + after, err := os.Stat(p) + require.NoError(t, err) + assert.True(t, before.ModTime().Equal(after.ModTime()), "identical hook file must not be rewritten") }) } diff --git a/model/store_file.go b/model/store_file.go index 7c9f03e..4f89f7c 100644 --- a/model/store_file.go +++ b/model/store_file.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "log/slog" - "os" "sort" "time" ) @@ -24,7 +23,7 @@ func (s *fileStore) appendLine(path string, cmd Command, recordingTime time.Time if err != nil { return err } - f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0644) + f, err := openPrivateAppend(path) if err != nil { return fmt.Errorf("failed to open command storage file %s: %w", path, err) } @@ -80,7 +79,7 @@ func (s *fileStore) SetCursor(ctx context.Context, cursor time.Time) error { if err := ensureStorageFolder(); err != nil { return err } - cursorFile, err := os.OpenFile(GetCursorFilePath(), os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0644) + cursorFile, err := openPrivateAppend(GetCursorFilePath()) if err != nil { return err } @@ -146,13 +145,13 @@ func (s *fileStore) Prune(ctx context.Context, cursor time.Time) error { postBuf.Write(line) } - if err := os.WriteFile(GetPreCommandFilePath(), preBuf.Bytes(), 0644); err != nil { + if err := WritePrivateFile(GetPreCommandFilePath(), preBuf.Bytes()); err != nil { return err } - if err := os.WriteFile(GetPostCommandFilePath(), postBuf.Bytes(), 0644); err != nil { + if err := WritePrivateFile(GetPostCommandFilePath(), postBuf.Bytes()); err != nil { return err } - return os.WriteFile(GetCursorFilePath(), []byte(fmt.Sprintf("%d", cursor.UnixNano())), 0644) + return WritePrivateFile(GetCursorFilePath(), []byte(fmt.Sprintf("%d", cursor.UnixNano()))) } func (s *fileStore) Engine() string { return StorageEngineFile } diff --git a/model/store_file_test.go b/model/store_file_test.go index 47ddffb..7963bde 100644 --- a/model/store_file_test.go +++ b/model/store_file_test.go @@ -2,6 +2,7 @@ package model import ( "context" + "os" "testing" "time" @@ -18,6 +19,9 @@ func TestFileStoreRoundTrip(t *testing.T) { cmd := Command{Shell: "bash", SessionID: 7, Command: "go build", Username: "u", Hostname: "h", Time: start} require.NoError(t, store.SavePre(ctx, cmd, start)) + preInfo, err := os.Stat(GetPreCommandFilePath()) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), preInfo.Mode().Perm()) _, noCursor, err := store.GetLastCursor(ctx) require.NoError(t, err) @@ -26,6 +30,9 @@ func TestFileStoreRoundTrip(t *testing.T) { post := cmd post.Time = start.Add(time.Second) require.NoError(t, store.SavePost(ctx, post, 0, post.Time)) + postInfo, err := os.Stat(GetPostCommandFilePath()) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), postInfo.Mode().Perm()) tree, err := store.GetPreTree(ctx) require.NoError(t, err) diff --git a/model/tracking_build.go b/model/tracking_build.go index 4847466..faf0da5 100644 --- a/model/tracking_build.go +++ b/model/tracking_build.go @@ -68,7 +68,11 @@ func BuildTrackingData(ctx context.Context, store CommandStore, config ShellTime } recordingTime := postCommand.RecordingTime - if recordingTime.Before(cursor) { + // The cursor is the recording time of the last command included in a + // successful sync. Equality means that command was already sent; Before + // alone would upload it again on every later flush (the file store is + // not pruned on the daemon-less path). + if !recordingTime.After(cursor) { continue } if recordingTime.After(latest) { diff --git a/model/tracking_build_test.go b/model/tracking_build_test.go index f4d3d34..e6965bb 100644 --- a/model/tracking_build_test.go +++ b/model/tracking_build_test.go @@ -111,6 +111,31 @@ func TestBuildTrackingDataExcludes(t *testing.T) { require.Empty(t, res.Data, "excluded command must not be synced") } +func TestBuildTrackingData_SkipsCommandAtCursor(t *testing.T) { + store, err := newBoltStore(filepath.Join(t.TempDir(), "commands.db")) + require.NoError(t, err) + defer store.Close() + + ctx := context.Background() + at := time.Date(2024, 6, 1, 12, 0, 0, 0, time.UTC) + newer := at.Add(time.Second) + + synced := Command{Shell: "bash", SessionID: 1, Command: "already-sent", Username: "u", Hostname: "h", Time: at} + require.NoError(t, store.SavePre(ctx, synced, at)) + require.NoError(t, store.SavePost(ctx, synced, 0, at)) + + fresh := Command{Shell: "bash", SessionID: 2, Command: "still-pending", Username: "u", Hostname: "h", Time: newer} + require.NoError(t, store.SavePre(ctx, fresh, newer)) + require.NoError(t, store.SavePost(ctx, fresh, 0, newer)) + + require.NoError(t, store.SetCursor(ctx, at)) + + res, err := BuildTrackingData(ctx, store, ShellTimeConfig{}) + require.NoError(t, err) + require.Len(t, res.Data, 1) + require.Equal(t, "still-pending", res.Data[0].Command) +} + func TestBuildTrackingDataEmpty(t *testing.T) { store, err := newBoltStore(filepath.Join(t.TempDir(), "commands.db")) require.NoError(t, err)