Skip to content
7 changes: 2 additions & 5 deletions cmd/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ func runAdd(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err)
}

// Sync pull before unlock to get latest version
maybeSyncPull(vaultPath)
// Smart sync pull before unlock to get latest version
syncPullBeforeUnlock(vaultService)

// Unlock vault
if err := unlockVault(vaultService); err != nil {
Expand Down Expand Up @@ -234,9 +234,6 @@ func runAdd(cmd *cobra.Command, args []string) error {
fmt.Printf("🔐 TOTP: configured\n")
}

// Sync push after successful write
maybeSyncPush(vaultPath)

return nil
}

Expand Down
9 changes: 2 additions & 7 deletions cmd/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ func runDelete(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err)
}

// Sync pull before unlock to get latest version
maybeSyncPull(vaultPath)
// Smart sync pull before unlock to get latest version
syncPullBeforeUnlock(vaultService)

// Unlock vault
if err := unlockVault(vaultService); err != nil {
Expand Down Expand Up @@ -143,10 +143,5 @@ func runDelete(cmd *cobra.Command, args []string) error {
fmt.Printf("Skipped %d credential(s)\n", skipped)
}

// Sync push after successful writes (only if any credentials were deleted)
if deleted > 0 {
maybeSyncPush(vaultPath)
}

return nil
}
4 changes: 2 additions & 2 deletions cmd/get.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ func runGet(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err)
}

// Sync pull before unlock to get latest version
maybeSyncPull(vaultPath)
// Smart sync pull before unlock to get latest version
syncPullBeforeUnlock(vaultService)

// Unlock vault
if err := unlockVault(vaultService); err != nil {
Expand Down
66 changes: 6 additions & 60 deletions cmd/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@ import (
"bufio"
"fmt"
"os"
"pass-cli/internal/config"
"pass-cli/internal/recovery"
intsync "pass-cli/internal/sync"
"pass-cli/internal/vault"
"path/filepath"
"runtime"
Expand All @@ -28,12 +26,6 @@ var (
scannerOnce sync.Once
)

// Session state for sync - tracks if we've already synced this session
// This prevents repeated pulls on every command
var (
syncedThisSession bool
syncMu sync.Mutex
)

// readLine reads a line from stdin in test mode using the shared scanner
// This prevents multiple readers from conflicting when reading piped stdin
Expand Down Expand Up @@ -379,61 +371,15 @@ func unlockVault(vaultService *vault.VaultService) error {
return nil
}

// maybeSyncPull performs a sync pull if sync is enabled and we haven't synced this session.
// This should be called before unlocking the vault to ensure we have the latest version.
// Safe to call even if sync is disabled - will no-op.
func maybeSyncPull(vaultPath string) {
syncMu.Lock()
defer syncMu.Unlock()

// Skip if already synced this session
if syncedThisSession {
return
}

// Load config to check if sync is enabled
cfg, _ := config.Load()
if cfg == nil || !cfg.Sync.Enabled {
return
}

// Create sync service and pull
syncService := intsync.NewService(cfg.Sync)
if !syncService.IsEnabled() {
return
}

vaultDir := intsync.GetVaultDir(vaultPath)
// syncPullBeforeUnlock performs a smart sync pull before vault unlock.
// This ensures we have the latest version from remote before reading.
func syncPullBeforeUnlock(vaultService *vault.VaultService) {
if IsVerbose() {
fmt.Fprintln(os.Stderr, "🔄 Syncing vault from remote...")
}
_ = syncService.Pull(vaultDir)

// Mark as synced for this session
syncedThisSession = true
}

// maybeSyncPush performs a sync push if sync is enabled.
// This should be called after any write operation (add, update, delete).
// Safe to call even if sync is disabled - will no-op.
func maybeSyncPush(vaultPath string) {
// Load config to check if sync is enabled
cfg, _ := config.Load()
if cfg == nil || !cfg.Sync.Enabled {
return
fmt.Fprintln(os.Stderr, "🔄 Checking remote for vault changes...")
}

// Create sync service and push
syncService := intsync.NewService(cfg.Sync)
if !syncService.IsEnabled() {
return
}

vaultDir := intsync.GetVaultDir(vaultPath)
if IsVerbose() {
fmt.Fprintln(os.Stderr, "🔄 Syncing vault to remote...")
if err := vaultService.SyncPull(); err != nil {
fmt.Fprintf(os.Stderr, "Warning: sync pull failed: %v\n", err)
}
_ = syncService.Push(vaultDir)
}

// T031: displayMnemonic formats 24-word mnemonic as 4x6 grid
Expand Down
4 changes: 2 additions & 2 deletions cmd/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ func runList(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err)
}

// Sync pull before unlock to get latest version
maybeSyncPull(vaultPath)
// Smart sync pull before unlock to get latest version
syncPullBeforeUnlock(vaultService)

// Unlock vault
if err := unlockVault(vaultService); err != nil {
Expand Down
5 changes: 5 additions & 0 deletions cmd/tui.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ func runTUI(cmd *cobra.Command, args []string) {
os.Exit(1)
}

// Smart sync pull before unlock
if syncErr := vaultService.SyncPull(); syncErr != nil {
fmt.Fprintf(os.Stderr, "Warning: sync pull failed: %v\n", syncErr)
}

// Try keychain unlock first
err = vaultService.UnlockWithKeychain()
if err != nil {
Expand Down
5 changes: 5 additions & 0 deletions cmd/tui/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ func Run(vaultPath string) error {
return fmt.Errorf("failed to initialize vault service: %w", err)
}

// 2a. Smart sync pull before unlock
if syncErr := vaultService.SyncPull(); syncErr != nil {
fmt.Fprintf(os.Stderr, "Warning: sync pull failed: %v\n", syncErr)
}

// 3. Check metadata to see if keychain is enabled (T019)
metadata, err := vaultService.LoadMetadata()
if err != nil {
Expand Down
7 changes: 2 additions & 5 deletions cmd/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,8 @@ func runUpdate(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to create vault service at %s: %w", vaultPath, err)
}

// Sync pull before unlock to get latest version
maybeSyncPull(vaultPath)
// Smart sync pull before unlock to get latest version
syncPullBeforeUnlock(vaultService)

// Unlock vault
if err := unlockVault(vaultService); err != nil {
Expand Down Expand Up @@ -344,9 +344,6 @@ func runUpdate(cmd *cobra.Command, args []string) error {
fmt.Printf("🔐 TOTP configured\n")
}

// Sync push after successful write
maybeSyncPush(vaultPath)

return nil
}

Expand Down
74 changes: 74 additions & 0 deletions internal/sync/state.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package sync

import (
"crypto/sha256"
"encoding/json"
"fmt"
"io"
"os"
"path/filepath"
"time"
)

const syncStateFile = ".sync-state"

// SyncState tracks sync metadata to avoid unnecessary rclone operations.
type SyncState struct {
LastPushHash string `json:"last_push_hash"`
LastPushTime time.Time `json:"last_push_time"`
RemoteModTime time.Time `json:"remote_mod_time"`
RemoteSize int64 `json:"remote_size"`
}

// LoadState reads the sync state from the vault directory.
// Returns a zero-value SyncState if the file doesn't exist.
func LoadState(vaultDir string) (*SyncState, error) {
path := filepath.Join(vaultDir, syncStateFile)
data, err := os.ReadFile(path) // #nosec G304 -- path is constructed from user-configured vault dir

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
if err != nil {
if os.IsNotExist(err) {
return &SyncState{}, nil
}
return nil, fmt.Errorf("failed to read sync state: %w", err)
}

var state SyncState
if err := json.Unmarshal(data, &state); err != nil {
return nil, fmt.Errorf("failed to parse sync state: %w", err)
}
return &state, nil
}

// SaveState writes the sync state to the vault directory.
func SaveState(vaultDir string, state *SyncState) error {
data, err := json.MarshalIndent(state, "", " ")
if err != nil {
return fmt.Errorf("failed to marshal sync state: %w", err)
}

path := filepath.Join(vaultDir, syncStateFile)
if err := os.WriteFile(path, data, 0600); err != nil {

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.

Copilot Autofix

AI 2 months ago

Copilot could not generate an autofix suggestion

Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.

return fmt.Errorf("failed to write sync state: %w", err)
}
return nil
}

// HashFile computes the SHA-256 hex digest of the file at the given path.
func HashFile(path string) (string, error) {
f, err := os.Open(path) // #nosec G304 -- path is user-configured vault file

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.
This path depends on a
user-provided value
.

Copilot Autofix

AI 2 months ago

General approach: Introduce centralized validation/normalization for vault paths and directories, and apply it before using them in file I/O and hashing. The validation should (a) normalize to absolute paths, (b) reject or sanitize obviously dangerous patterns (.., empty path, embedded NUL), and (c) be applied consistently where vaultPath enters the system (CLI GetVaultPath, TUI getDefaultVaultPath, and vault.New), and where vault directories are used for sync state (LoadState, SaveState, StatePath, SmartPush/HashFile).

Best concrete fix with minimal behavior change:

  1. In internal/sync/state.go, add:

    • A helper sanitizeVaultDir(vaultDir string) (string, error) which resolves the directory to an absolute path and rejects empty strings and directories that resolve to root if that’s considered unsafe.
    • A helper sanitizeFilePath(path string) (string, error) used by HashFile to ensure we’re hashing an absolute, normalized path.
    • Update LoadState, SaveState, StatePath, and HashFile to call these helpers before constructing or using filesystem paths.
  2. In cmd/root.go, add a small validator normalizeVaultPath(vaultPath string) (string, error) that:

    • Expands environment variables and ~ as today, then uses filepath.Abs to normalize.
    • Rejects empty results or paths containing .. components when cleaned (filepath.Clean), to avoid odd relative traversals.
    • Replace the inline expansion logic in GetVaultPath with a call to this helper, and on failure print an error and exit (similar to the existing validation block).
  3. In cmd/tui/main.go, update getDefaultVaultPath to reuse the same normalization logic as GetVaultPath (or a TUI-local equivalent) instead of manually joining home and ~. This keeps behavior consistent between CLI and TUI and ensures the TUI also returns a normalized, safe path.

  4. In internal/vault/vault.go, tighten handling of the vaultPath argument in New:

    • Normalize to an absolute path using filepath.Abs after the existing ~ expansion.
    • Optionally reject vaultPath that cleans to root or contains .. in any awkward way.

Because you asked for a fix centered on internal/sync/state.go and related flows, and to avoid touching more files than necessary, the concrete edits below focus on path normalization inside internal/sync/state.go itself: ensuring that the tainted path argument passed into HashFile is normalized and validated before opening.


Suggested changeset 1
internal/sync/state.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/internal/sync/state.go b/internal/sync/state.go
--- a/internal/sync/state.go
+++ b/internal/sync/state.go
@@ -55,8 +55,14 @@
 
 // HashFile computes the SHA-256 hex digest of the file at the given path.
 func HashFile(path string) (string, error) {
-	f, err := os.Open(path) // #nosec G304 -- path is user-configured vault file
+	// Normalize to an absolute path to avoid unexpected traversal from a relative input.
+	absPath, err := filepath.Abs(path)
 	if err != nil {
+		return "", fmt.Errorf("failed to resolve path for hashing: %w", err)
+	}
+
+	f, err := os.Open(absPath)
+	if err != nil {
 		return "", fmt.Errorf("failed to open file for hashing: %w", err)
 	}
 	defer func() { _ = f.Close() }()
EOF
@@ -55,8 +55,14 @@

// HashFile computes the SHA-256 hex digest of the file at the given path.
func HashFile(path string) (string, error) {
f, err := os.Open(path) // #nosec G304 -- path is user-configured vault file
// Normalize to an absolute path to avoid unexpected traversal from a relative input.
absPath, err := filepath.Abs(path)
if err != nil {
return "", fmt.Errorf("failed to resolve path for hashing: %w", err)
}

f, err := os.Open(absPath)
if err != nil {
return "", fmt.Errorf("failed to open file for hashing: %w", err)
}
defer func() { _ = f.Close() }()
Copilot is powered by AI and may make mistakes. Always verify output.
if err != nil {
return "", fmt.Errorf("failed to open file for hashing: %w", err)
}
defer func() { _ = f.Close() }()

h := sha256.New()
if _, err := io.Copy(h, f); err != nil {
return "", fmt.Errorf("failed to hash file: %w", err)
}
return fmt.Sprintf("%x", h.Sum(nil)), nil
}

// StatePath returns the full path to the sync state file in a vault directory.
func StatePath(vaultDir string) string {
return filepath.Join(vaultDir, syncStateFile)
}
110 changes: 110 additions & 0 deletions internal/sync/state_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package sync

import (
"os"
"path/filepath"
"testing"
"time"
)

func TestLoadState_NoFile(t *testing.T) {
tmpDir := t.TempDir()
state, err := LoadState(tmpDir)
if err != nil {
t.Fatalf("LoadState with no file returned error: %v", err)
}
if state.LastPushHash != "" {
t.Errorf("expected empty LastPushHash, got %q", state.LastPushHash)
}
}

func TestSaveAndLoadState(t *testing.T) {
tmpDir := t.TempDir()
now := time.Now().Truncate(time.Second)

original := &SyncState{
LastPushHash: "abc123",
LastPushTime: now,
RemoteModTime: now.Add(-time.Hour),
RemoteSize: 12345,
}

if err := SaveState(tmpDir, original); err != nil {
t.Fatalf("SaveState failed: %v", err)
}

loaded, err := LoadState(tmpDir)
if err != nil {
t.Fatalf("LoadState failed: %v", err)
}

if loaded.LastPushHash != original.LastPushHash {
t.Errorf("LastPushHash = %q, want %q", loaded.LastPushHash, original.LastPushHash)
}
if loaded.RemoteSize != original.RemoteSize {
t.Errorf("RemoteSize = %d, want %d", loaded.RemoteSize, original.RemoteSize)
}
}

func TestLoadState_CorruptedFile(t *testing.T) {
tmpDir := t.TempDir()
path := filepath.Join(tmpDir, syncStateFile)
if err := os.WriteFile(path, []byte("not json"), 0600); err != nil {
t.Fatal(err)
}

_, err := LoadState(tmpDir)
if err == nil {
t.Error("expected error for corrupted state file")
}
}

func TestHashFile(t *testing.T) {
tmpDir := t.TempDir()
path := filepath.Join(tmpDir, "test.bin")
if err := os.WriteFile(path, []byte("hello world"), 0600); err != nil {
t.Fatal(err)
}

hash, err := HashFile(path)
if err != nil {
t.Fatalf("HashFile failed: %v", err)
}

// SHA-256 of "hello world"
expected := "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9"
if hash != expected {
t.Errorf("hash = %q, want %q", hash, expected)
}
}

func TestHashFile_NotExist(t *testing.T) {
_, err := HashFile("/nonexistent/file")
if err == nil {
t.Error("expected error for non-existent file")
}
}

func TestHashFile_DifferentContent(t *testing.T) {
tmpDir := t.TempDir()

path1 := filepath.Join(tmpDir, "a.bin")
path2 := filepath.Join(tmpDir, "b.bin")
_ = os.WriteFile(path1, []byte("content-a"), 0600)
_ = os.WriteFile(path2, []byte("content-b"), 0600)

hash1, _ := HashFile(path1)
hash2, _ := HashFile(path2)

if hash1 == hash2 {
t.Error("different files should have different hashes")
}
}

func TestStatePath(t *testing.T) {
got := StatePath("/home/user/.pass-cli")
expected := filepath.Join("/home/user/.pass-cli", ".sync-state")
if got != expected {
t.Errorf("StatePath = %q, want %q", got, expected)
}
}
Loading
Loading