feat: add filemanager session and crypto version detection#516
feat: add filemanager session and crypto version detection#516moonD4rk merged 10 commits intofeat/v2-hbd-architecturefrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/v2-hbd-architecture #516 +/- ##
============================================================
+ Coverage 46.07% 47.76% +1.68%
============================================================
Files 17 21 +4
Lines 1237 1298 +61
============================================================
+ Hits 570 620 +50
- Misses 536 545 +9
- Partials 131 133 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds new building blocks for the refactor: a filemanager session abstraction for staging browser files into an isolated temp workspace (including a Windows-only locked-file copy path), and a small crypto helper to detect/strip Chromium encryption version prefixes.
Changes:
- Introduces
filemanager.Sessionwith temp-dir lifecycle andAcquire()for files/dirs (+ WAL/SHM companions). - Implements Windows locked-file copy using handle duplication + memory-mapped reads; provides non-Windows stub.
- Adds
crypto.CipherVersiondetection and prefix stripping utilities with unit tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| filemanager/session.go | New session abstraction for temp workspace + file/dir acquisition logic |
| filemanager/session_test.go | Unit tests covering temp dir creation, cleanup, and Acquire behaviors |
| filemanager/copy_windows.go | Windows implementation of locked-file copy via system handle enumeration + mapping |
| filemanager/copy_other.go | Non-Windows stub for locked-file copy |
| crypto/version.go | Detect/strip Chromium encryption version prefix helpers |
| crypto/version_test.go | Unit tests for version detection and prefix stripping |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try normal copy first | ||
| err := fileutil.CopyFile(src, dst) | ||
| if err != nil { |
There was a problem hiding this comment.
Acquire calls fileutil.CopyFile(src, dst) directly, but fileutil.CopyFile uses os.WriteFile and does not create parent directories. If dst includes subdirectories under the session temp dir, this will fail. Consider os.MkdirAll(filepath.Dir(dst), 0o700) (or similar) before copying/writing.
| if err != nil { | ||
| // Normal copy failed, try platform-specific locked file copy | ||
| if err2 := copyLocked(src, dst); err2 != nil { | ||
| return errors.Join( | ||
| fmt.Errorf("copy: %w", err), | ||
| fmt.Errorf("locked copy: %w", err2), | ||
| ) |
There was a problem hiding this comment.
Acquire falls back to copyLocked on any CopyFile error. On non-Windows this always adds a second (unsupported) error, and on Windows it can trigger an expensive system-handle scan even for simple cases like "file not found". Consider gating the fallback to Windows and to specific lock-related errors (e.g., sharing violation) so unrelated failures return the original error cleanly.
| for _, suffix := range []string{"-wal", "-shm"} { | ||
| walSrc := src + suffix | ||
| if fileutil.IsFileExists(walSrc) { | ||
| _ = fileutil.CopyFile(walSrc, dst+suffix) | ||
| } | ||
| } |
There was a problem hiding this comment.
WAL/SHM companion copies ignore errors (_ = fileutil.CopyFile(...)). If a companion file exists but the copy fails, Acquire still returns nil, which can leave an inconsistent SQLite snapshot. Consider returning (or joining) the WAL/SHM copy error when IsFileExists is true.
| for _, suffix := range []string{"-wal", "-shm"} { | |
| walSrc := src + suffix | |
| if fileutil.IsFileExists(walSrc) { | |
| _ = fileutil.CopyFile(walSrc, dst+suffix) | |
| } | |
| } | |
| var walErr error | |
| for _, suffix := range []string{"-wal", "-shm"} { | |
| walSrc := src + suffix | |
| if fileutil.IsFileExists(walSrc) { | |
| if err := fileutil.CopyFile(walSrc, dst+suffix); err != nil { | |
| walErr = errors.Join(walErr, fmt.Errorf("copy companion %s: %w", suffix, err)) | |
| } | |
| } | |
| } | |
| if walErr != nil { | |
| return walErr | |
| } |
| // copyLocked is a no-op on non-Windows platforms. | ||
| // File locking is primarily a Windows issue where Chrome holds exclusive | ||
| // locks on Cookie files via SQLite WAL mode. | ||
| func copyLocked(_, _ string) error { | ||
| return fmt.Errorf("locked file copy not supported on this platform") |
There was a problem hiding this comment.
Comment says copyLocked is a "no-op" on non-Windows, but the implementation returns an error. Update the comment to match behavior (e.g., "unsupported" / "not implemented").
| const headerSize = unsafe.Sizeof(uintptr(0)) * 2 | ||
| entrySize := unsafe.Sizeof(systemHandleTableEntryInfoEx{}) | ||
|
|
||
| entries := make([]systemHandleTableEntryInfoEx, numberOfHandles) | ||
| for i := uintptr(0); i < numberOfHandles; i++ { |
There was a problem hiding this comment.
querySystemHandles reads numberOfHandles as uintptr, but make([]systemHandleTableEntryInfoEx, numberOfHandles) requires an int length and will not compile. Convert to int with overflow checks (and consider validating that headerSize + count*entrySize fits within len(buf) before parsing).
| buf := make([]uint16, 512) | ||
| n, _, err := procGetFinalPathNameByHandleW.Call( | ||
| uintptr(handle), | ||
| uintptr(unsafe.Pointer(&buf[0])), | ||
| uintptr(len(buf)), | ||
| 0, // FILE_NAME_NORMALIZED | ||
| ) | ||
| if n == 0 { | ||
| return "", fmt.Errorf("GetFinalPathNameByHandle: %w", err) | ||
| } | ||
|
|
||
| path := windows.UTF16ToString(buf[:n]) | ||
| // Remove \\?\ prefix added by GetFinalPathNameByHandle | ||
| path = strings.TrimPrefix(path, `\\?\`) | ||
| return path, nil |
There was a problem hiding this comment.
getFinalPathName slices buf[:n], but GetFinalPathNameByHandleW can return a length larger than the provided buffer. If that happens this will panic (slice bounds out of range). Handle the n > len(buf) case by allocating a larger buffer and retrying, or call once with size=0 to get the required length.
| buf := make([]uint16, 512) | |
| n, _, err := procGetFinalPathNameByHandleW.Call( | |
| uintptr(handle), | |
| uintptr(unsafe.Pointer(&buf[0])), | |
| uintptr(len(buf)), | |
| 0, // FILE_NAME_NORMALIZED | |
| ) | |
| if n == 0 { | |
| return "", fmt.Errorf("GetFinalPathNameByHandle: %w", err) | |
| } | |
| path := windows.UTF16ToString(buf[:n]) | |
| // Remove \\?\ prefix added by GetFinalPathNameByHandle | |
| path = strings.TrimPrefix(path, `\\?\`) | |
| return path, nil | |
| size := 512 | |
| for { | |
| buf := make([]uint16, size) | |
| n, _, err := procGetFinalPathNameByHandleW.Call( | |
| uintptr(handle), | |
| uintptr(unsafe.Pointer(&buf[0])), | |
| uintptr(len(buf)), | |
| 0, // FILE_NAME_NORMALIZED | |
| ) | |
| if n == 0 { | |
| return "", fmt.Errorf("GetFinalPathNameByHandle: %w", err) | |
| } | |
| if n > uintptr(len(buf)) { | |
| // Buffer was too small; retry with the required size. | |
| size = int(n) | |
| continue | |
| } | |
| path := windows.UTF16ToString(buf[:n]) | |
| // Remove \\?\ prefix added by GetFinalPathNameByHandle | |
| path = strings.TrimPrefix(path, `\\?\`) | |
| return path, nil | |
| } |
| // Copy mapped memory into a Go byte slice | ||
| data := make([]byte, fileSize) | ||
| copy(data, unsafe.Slice((*byte)(unsafe.Pointer(viewPtr)), fileSize)) | ||
| return data, nil |
There was a problem hiding this comment.
fileSize is an int64, but make([]byte, fileSize) (and passing fileSize to unsafe.Slice) requires an int length and will not compile. Convert to int with bounds checks (e.g., reject sizes > max int) before allocating/copying.
…e+FileMapping fallback
…le, and normal copy scenarios
* feat: add filemanager session and crypto version detection * refactor: move copy logic into filemanager, remove fileutil dependency * fix: apply review suggestions for filemanager * feat: add Windows locked file tests, fix readFileContent with ReadFile+FileMapping fallback * fix: remove self-PID skip in findFileHandle to fix Windows CI test * fix: seek to file start before reading duplicated handle * fix: use full path matching in findFileHandle to avoid cross-app handle collision * test: enhance Windows copyLocked tests with write-then-read, large file, and normal copy scenarios * fix: check all errors in Windows tests, use bytes.Equal for large file comparison * fix: use stable path suffix matching to handle Windows short path names in CI
Summary
Phase 3 of architecture refactoring. New files only — no existing code modified.
Fixes #517
filemanager/
session.go—SessionwithNewSession(),Acquire(),TempDir(),Cleanup()copy.go— internalcopyFile,copyDir,isFileExists(self-contained, no fileutil dependency)copy_windows.go—copyLocked()via DuplicateHandle + FileMapping for Windows locked cookie filescopy_other.go— stub for macOS/Linuxsession_test.go— 6 cross-platform testscopy_windows_test.go— 6 Windows-specific tests with exclusive lock simulationcrypto/
version.go—DetectVersion(),StripPrefix(),CipherVersiontype (v10/v20/DPAPI)version_test.go— 12 testsWindows Cookie Lock Solution
Chrome/Edge use
PRAGMA locking_mode=EXCLUSIVEon the Cookies database, preventingos.ReadFile()from accessing it while the browser runs. Our solution:NtQuerySystemInformation(SystemExtendedHandleInformation)enumerates all system handlesDuplicateHandlecopies the browser's handle into our processCreateFileMapping+MapViewOfFilereads from kernel cache (includes uncommitted data)Verified on Windows 11 ARM64 with Chrome 146 and Edge (both running):
Test plan
go test ./filemanager/... ./crypto/...— all pass (macOS/Linux/Windows)GOOS=windows GOARCH=amd64 go build— compilesgolangci-lint run— 0 issues