Conversation
…group (#238) ci: bump actions/checkout in the actions-updates group Bumps the actions-updates group with 1 update: [actions/checkout](https://github.com/actions/checkout). Updates `actions/checkout` from 6.0.3 to 7.0.0 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@df4cb1c...9c091bb) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 7.0.0 dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-updates ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…laky test
TestCreateArchiveWithDifferentCompressionTypes/pigz failed intermittently
in CI with "archive/tar: write too long". The compression tests placed the
output archive inside the very tempDir they archived, so filepath.Walk
reached the archiver's own still-growing output file, snapshotted its size
into the tar header, then overran it as the (async, external) compressor
kept appending. The archiver correctly fails closed in that case
("archive incomplete"), but the scenario is self-inflicted and never occurs
in production, where the backup destination is always outside the source.
Route every compression test's output through a new archiveOutputPath
helper that allocates a separate t.TempDir(), eliminating the whole class
of latent flake across all 9 sites (only the pigz table subtest had tripped
CI, but the direct create*Archive tests shared the same pattern).
Verified: 60x all compression tests under heavy CPU load -> 0 failures
(was ~18/40 on pigz alone); full package + -race green; gofmt/vet clean.
#239) * fix(restore): clear chattr +i mount-guard fallback in --cleanup-guards During restore, when a storage mountpoint is offline, ProxSave applies a read-only bind-mount guard; if the bind mount fails it falls back to `chattr +i` on the mountpoint directory so PVE/PBS cannot silently write into the root filesystem. `--cleanup-guards` previously only undid the bind-mount guards: the immutable flag was left set, persists across reboots, and had to be cleared by hand. Record each chattr-guarded mountpoint in a host-side index (/var/lib/proxsave/guards/chattr-targets) and have --cleanup-guards reverse it: - record on a successful `chattr +i` at both apply sites (PBS + PVE), written atomically on the real filesystem, deduped, with strict target validation (datastore mount root only; reject newline/control chars). - clearImmutableGuards reads the index and runs `chattr -i`, but only on targets that are NOT currently mounted (a live mount shadows the flagged dir; clearing it would touch the wrong inode), mirroring how hidden bind-mount guards are left in place. Symlinks are resolved and the allowlist re-checked before clearing so a parent-component symlink cannot escape /mnt|/media|/run/media. - keep the guard directory + index while any immutable target is still pending (mounted/unresolved/failed) so a later run can finish the job; remove them only when nothing is pending and no bind-mount guards remain. - surface the skip-while-mounted case at Info level and point users at the manual recovery (lsattr -d / chattr -i) when the guard dir was deleted. PVE guard apply now uses the existing mountGuardMkdirAll / mountGuardIsPathOnRootFilesystem seams (default behavior unchanged) so the record wiring can be tested without a writable /mnt/pve. Adversarially reviewed (correctness/security/test-rigor), mutation-proven. * docs(restore): document --cleanup-guards clearing the chattr +i fallback Correct the mount-guard docs to match the new cleanup behavior: - distinguish the two guard types: a bind-mount guard is shadowed by a later real mount and discarded on reboot / by --cleanup-guards, whereas the `chattr +i` fallback sets an immutable flag on the directory inode that persists across reboots and is now recorded under /var/lib/proxsave/guards/chattr-targets for cleanup to reverse. - fix the "real mount overlays the guard" wording, which was only true for the bind-mount guard. - explain that --cleanup-guards clears recorded immutable flags only on not-currently-mounted targets, the unmount→cleanup→remount procedure for mounted ones, and the manual lsattr/chattr -i recovery when the guard directory was deleted by hand. - add a TROUBLESHOOTING entry for "mountpoint read-only after restore". Touches RESTORE_GUIDE.md, RESTORE_TECHNICAL.md, CLI_REFERENCE.md, TROUBLESHOOTING.md. * fix(orchestrator): resolve gosec G301/G304 on the chattr guard index Address the two github-advanced-security (gosec) findings on mount_guard_chattr_index.go structurally, without #nosec: - G301: tighten the guard base dir from 0o755 to 0o750. It is root-owned host state (the 0o600 index + bind-mount guard subdirs); nothing non-root needs to traverse it. - G304: read the index through an *os.Root on its directory (root.ReadFile(base)), mirroring readLockFileContent / normalizeTextFile. This confines the read to the guard directory at the syscall level so a symlink or `..` in the basename cannot redirect it, and removes the raw variable sink. Behavior is unchanged (missing/unreadable index -> nil).
The Exit Codes table listed 0-7 with wrong meanings; the code (internal/types/exit_codes.go) defines 0-15. Rewrite the table to match exactly (e.g. 3=environment, 5=storage, 7=permission, 14=security, 15=encryption). Also fix the cloud note: cloud storage is non-critical, so an upload failure does not abort with a storage error (5); it is recorded as a warning and the run exits non-zero (1), not 0.
Cloud upload verification only compared file size, although the docs promised SHA256 checksum validation and the .sha256 sidecar was uploaded but never used. VerifyUpload now escalates from the size pre-check to a real SHA256 comparison: it asks the remote for its hash via 'rclone hashsum sha256' and compares it against the checksum computed fresh from the local file (the exact uploaded bytes -- raw archive, bundle, or sidecar -- never metadata.Checksum, which is the inner-archive hash and would not match a bundle). Behavior is gated by CLOUD_VERIFY_CHECKSUM (default true). On backends without a native SHA256 (S3, B2, Wasabi, ...) it falls back to the size-only verdict, logged at debug (not warning, so a successful run is not turned into exit code 1); CLOUD_VERIFY_DOWNLOAD=true forces a download-and-hash on such backends. A genuine hash mismatch fails the upload; transport errors stay fatal while 'hash type not supported' degrades to the size-only fallback. Adds 'hashsum' to the rclone allowlist and a table-driven test suite covering match/mismatch/no-native-hash/download/transport-error/wrong-file/ malformed-hash, the full Store() path, and a regression lock-in that the no-native-hash fallback emits no WARNING. Docs and the env template updated to match.
The Environment Variables section showed CONFIG_FILE=/path ./build/proxsave, but no such env var is read: the config file location comes only from the -c/--config flag (internal/cli/args.go), and the env-override mechanism (loadEnvOverrides) runs after the file is already loaded, so it cannot select it. Replace the example with the real flag and a note. The neighboring DRY_RUN/DEBUG_LEVEL/USE_COLOR examples are genuine env overrides and stay.
…USER_HOMES toggle BACKUP_USER_HOMES was a ghost toggle: read only in config.go (default true), absent from the env template and all docs, so no real user could set it. User home directories are part of a system backup, so collection is now unconditional. The brickSystemUserHomes brick always calls collectUserHomes (the if c.config.BackupUserHomes guard is gone). The BackupUserHomes field is removed from config.Config and CollectorConfig, along with its copy in the orchestrator and its default in GetDefaultCollectorConfig. Existing exclusions are unchanged: the global blacklist/exclude patterns still apply, and .ssh is still gated by BACKUP_SSH_KEYS. The Validate() 'at least one backup option must be enabled' guard (and its hasCollectionOptionEnabled/collectionOptionFlags helpers) is removed: the system recipe always collects a baseline (kernel/hardware report info plus /home), so the precondition is no longer accurate. Tests: the empty-config assertion is inverted (an empty config is now valid), the user-homes table case is dropped, and TestUserHomesBrickCollectsUncondi- tionally locks in that the brick collects /home with no config gate.
…and docs CLOUD_UPLOAD_MODE fell back to sequential when the key was absent or blank, but the shipped template sets it to parallel and every doc states the default is parallel (with CLOUD_PARALLEL_MAX_JOBS=2). Align the code with the documented/shipped default: unset or blank now selects parallel; only an explicit sequential (or any other non-parallel value) selects sequential. Adds TestCloudUploadModeDefault covering absent/blank/explicit/unknown.
After /home collection became unconditional, the RunGoBackup end-to-end tests backed up the real root filesystem and pulled in the CI runner's /home/<user>/go module cache (~1.9 GiB), making the archive phase exceed the 2-minute timeout (the Race Detector job failed on TestRunGoBackupFallback- Compression). These tests previously avoided /home only because the struct-literal config left the old BackupUserHomes flag at its false zero value. Set SystemRootPrefix to an isolated temp dir in setSmallBackupTestConfig and in the dry-run/bundle RunGoBackup tests so collection walks a controlled root. go test -race -count=1 ./... now passes; the RunGoBackup tests drop from 120s+ to ~2s.
…functions restore.go was split from a ~1000-line monolith into ~30 focused files, but RESTORE_TECHNICAL.md still cited restore.go:NNN with line numbers that no longer exist in the 55-line stub. Replace the 22 stale file:line citations with file + function references (approach B: no line numbers, which rot on every refactor), e.g. extractRegularFile() -> restore_archive_entries.go, unmountEtcPVE() -> restore_services.go, createDecompressionReader() -> restore_decompression.go, the workflow phases -> restore_workflow_ui_*.go. Clarify the 'File: restore.go' section header to note restore.go is the entry stub and the workflow body lives in restore_workflow_ui_run.go. Accurate references (the file table row, the RunRestoreWorkflow entry) are unchanged.
BACKUP_PXAR_FILES was listed under 'Unchanged (same name)' as SAME, but the code reads it as a legacy fallback alias: config.go uses getBoolWithFallback(["PXAR_SCAN_ENABLE", "BACKUP_PXAR_FILES"], true), so PXAR_SCAN_ENABLE is the canonical name and BACKUP_PXAR_FILES is only accepted for backward compatibility. Move it to the Renamed section with the doc's own convention: RENAMED(PXAR_SCAN_ENABLE).
…plate and docs The code defaulted CLOUD_PARALLEL_VERIFICATION to false when the key was absent, but the shipped template sets it to true and every doc states the default is true. Align the code with the documented/shipped default (same pattern as the earlier CLOUD_UPLOAD_MODE fix). Adds TestCloudParallelVerifi- cationDefault (absent->true, explicit false->false, explicit true->true).
--new-install preserves build/, env/ and identity/ (cmd/proxsave/new_install.go newInstallPreservedEntries), but several places said only env/identity, omitting build -- including the shipped --help text. Correct the --help string (internal/cli/args.go) plus README, EXAMPLES, CLI_REFERENCE and TROUBLESHOOTING so every mention names build/env/identity.
CONFIGURATION.md listed the built-in defaults without psimon and as 'various regex patterns', and its example value presented non-default regexes as if they were defaults. The setting is additive (user entries are merged with built-in defaults). List the real defaults from config.go (incl. psimon and the ^kvm-pit/ and ^worker/...-drbd_as_pm- regexes), note the additive behavior, and make the example match the template. Also add psimon to the template summary.
The intro cited reference/env/backup.env as the legacy Bash source, but neither that file nor the reference/ directory exists in this repo (it referred to the old pre-migration project). Reword to describe the legacy Bash origin without a broken path.
…e' for build-from-source proxsave installs as root: the binary lives at /opt/proxsave/build/proxsave and the installer creates the PATH symlink /usr/local/bin/proxsave, so the canonical command for an installed user is just 'proxsave' (matching the tool's own 'Next steps' output). The docs used './build/proxsave' everywhere, which only works from the repo dir. Convert usage examples to 'proxsave', keeping './build/proxsave' only in genuine build-from-source/test sections (INSTALL Building-from-Source + wizard, CLOUD_STORAGE Testing, all of MIGRATION_GUIDE which is build-from-source only). Cron examples keep the absolute /opt/proxsave/build/proxsave path (cron needs an absolute path).
The release-assets list omitted SHA256SUMS.sig, but the install and upgrade flows refuse to proceed without a verifiable release signature (install.sh downloads+verifies it; upgrade.go does the same) and release.yml validates the signing key before publishing. Add it to the asset list with a mandatory note.
INSTALL.md and CLOUD_STORAGE.md stated a v1.50+ minimum while the README badge declares 1.60+. Align the stated minimum to v1.60+ everywhere (the 'Recommended: latest v1.65+' note is unchanged).
go.mod ships with the repo, so 'go mod init github.com/tis24dev/proxsave' fails
('go.mod already exists'). For 'cannot find main module' the fix is to run from
the project root; keep cd + go mod tidy + make build and correct the cause.
…ions claim parseRecipientString accepts ssh-ed25519/ssh-rsa public keys (agessh) for both AGE_RECIPIENT and AGE_RECIPIENT_FILE, but the docs only mentioned X25519 and passphrase recipients. Document SSH recipients. Also fix the overclaim 'Enforces 0700/0600': files are created 0700/0600 and the security check verifies/warns, auto-fixing only when AUTO_FIX_PERMISSIONS is enabled.
…tches) The documented snippet used strings.HasPrefix(target, "/etc/pve") (which would also match /etc/pvexyz) and showed 'return nil'. Replace it with the real logic from restore_archive_entries.go shouldSkipRestoreEntryTarget: early-return when the dest root isn't '/', then block exactly /etc/pve or anything under /etc/pve/, returning true,nil with the real warning text.
RESTORE_DIAGRAMS.md never mentioned the mount-guard subsystem. Add a section + mermaid flow: when a datastore mount is offline during PBS restore, the mount-point (on the root FS) is bind-mounted read-only (fallback chattr +i) to prevent writes from landing on / and creating a ghost datastore; on remount the real storage shadows the guard; --cleanup-guards removes them. References mount_guard.go / mount_guard_apply.go / guards_cleanup.go.
The shipped template (internal/config/templates/backup.env) is the source of truth for defaults, but four code getBool fallbacks (used when a key is absent) diverged from it. Align them so an absent key behaves like the template: ENABLE_DEDUPLICATION, ENABLE_PREFILTER, AUTO_FIX_PERMISSIONS false->true; BACKUP_CEPH_CONFIG true->false. Template-based installs ship all four keys so they are unaffected; only configs that omit a key change. Flip the config_test AUTO_FIX assertion and add TestOptimizationAndSecurityDefaults- MatchTemplate to lock the four defaults to the template.
…y Sections The 10-step 'Key Sections' list under RunRestoreWorkflow cited bare line ranges (e.g. '(Lines 28-66)') that pointed into the old monolithic restore.go and drifted on every edit. Replace each with the actual implementing function(s)+file, and reorder phases 2-4 to match real execution: PlanRestore/splitRestoreCategories (mode+plan build) runs before the cluster SAFE/RECOVERY prompt, and confirmRestorePlan is the final pre-write confirmation. Fix the phase 9 chain to runClusterSafeApply -> runSafeClusterApplyWithUI (the standalone runSafeClusterApply is the unused CLI wrapper). Relabel the 'Extract Engine' ASCII cell from restore.go to restore_archive* (extraction moved out of the stub).
…from RESTORE_TECHNICAL The Module Structure / Execution Flow / Implementation Details sections still described the pre-refactor architecture: ~28 source line-number citations (file.go:NNN and '(Lines NNN)'), several function names that no longer exist or were renamed (AllCategories, ConfirmRestorePlan, GetCategoriesForModeOrCustom, splitExportCategories, SelectAndPrepareBackup, DiscoverBackups, SelectSpecificBackup, DecryptIfNeeded), a phantom file (bash.go), a phantom helper (isSecurePath), the PreparedBackup/CleanupFunc type, and stale signatures (extractArchiveNative, extractSelectiveArchive, createDecompressionReader). Replace every line-number citation with a greppable function+file reference; correct phantom/renamed symbols to their real current names (GetAllCategories, ShowRestoreModeMenuWithReader/ShowCategorySelectionMenuWithReader, ConfirmRestoreOperationWithReader, preparedBundle, prepareRestoreBundleWithUI/ selectBackupCandidateWithUI/discoverBackupCandidates/preparePlainBundleCommon, verifyStagedArchiveIntegrity, standaloneClusterMode); fix the entry point to main_restore_decrypt.go; update stale code-block signatures to the real opts-struct/ctx forms; align the /etc/pve guard snippet to the real exact-or-prefix check; and relabel the path-traversal pseudocode as a simplified illustration of ensureRestoreTargetWithinRoot(). Verified by adversarial refuters against the code until HOLD.
…de allowlist escape) The restore-time PBS/PVE mount guards bind-mounted / chattr +i'd a guard target validated only by a string-prefix allowlist check (isConfirmableDatastoreMountRoot: /mnt, /media, /run/media) on a non-symlink-resolved path. A parent-component symlink (e.g. /mnt/data -> /etc) passed the prefix check, so the guard — and especially the reboot-surviving chattr +i fallback — could land outside the allowlist on a live OS directory. The cleanup path already resolved symlinks and re-checked the allowlist; the two apply paths did not (the divergence that caused this). Add a single shared helper resolveGuardTargetWithinAllowlist() (mount_guard.go) that resolves the target through symlinks (deepest-existing-ancestor walk for a not-yet-created mountpoint leaf) and re-checks the allowlist on the resolved path, and route all three sites through it: PBS apply (before MkdirAll), PVE apply (before MkdirAll), and cleanup (replacing the inline resolve+recheck; the cleanupResolveTarget seam is renamed resolveGuardTarget). On an escape or resolution error the apply paths skip fail-safe and never act outside the allowlist; on success they operate on and record the RESOLVED path so cleanup can match it. The deepest-ancestor resolution before MkdirAll also prevents MkdirAll from materializing directories through a pre-existing symlinked parent. Normal (no-symlink) targets are unchanged. Tests: unit table for the helper plus PBS+PVE integration cases for symlink escape (refused), allowlist->allowlist (acts on/records the resolved path), and generic resolve-error fail-safe. go test ./... and -race green.
… guard visibility The restore-time mount guards used a chattr +i immutable-flag fallback when the read-only bind-mount guard could not be created. That flag survived reboots and became a latent landmine: once the storage was later unmounted it silently re-blocked the mountpoint, and it could only be cleared while unmounted (a real mount on top shadows the inode). The guard's 'auto-restore on remount' was only overlay shadowing, never removal, so the chattr case left a persistent footgun. (c) Downgrade the fallback to WARN-ONLY at both apply sites (mount_guard_apply.go, pve_staged_apply.go): when the bind mount fails, log a loud, actionable warning and proceed without setting any persistent flag. This is safe because proxsave's own directory recreation is already skipped by the storage-mount preflight (shouldSkipUnmountedStorageMount), the config-only restore never extracts into datastore mountpoints, and PVE/PBS services are stopped during the guarded window; only external writers are unblocked in the rare bind-impossible case. (b) Add visibility: warn at restore start about any LEGACY chattr +i flags still recorded (warnLegacyImmutableGuards), and print a summary at the end of --cleanup-guards (bind-unmounted / guards-remaining / immutable-cleared / immutable-pending / guard-dir state). Backward-compat: the legacy-flag cleanup path (clearImmutableGuards + the chattr index) is retained, so hosts upgraded from a chattr-setting version are still remediated by --cleanup-guards. Docs (RESTORE_TECHNICAL/DIAGRAMS/GUIDE, CLI_REFERENCE, TROUBLESHOOTING) reframe chattr as legacy/removed. Tests updated to assert warn-only (no chattr, empty index) plus new reader/warner/summary tests; go test ./... and -race green. Built from 3 constructor analyses and validated by 4 adversarial refuters to HOLD.
The final command summary printed after every run (including --restore) did not mention --cleanup-guards, so an operator finishing a restore that applied offline mount guards had no on-screen pointer to the command that removes them. Add a concise entry right after --restore.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
📝 WalkthroughWalkthroughThis PR removes the ChangesCore Feature Changes
CI Workflow Updates
Documentation Updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1151
to
+1156
| // Match by basename so we never compare against a different object when | ||
| // more than one line is returned. The hash is always the first field; the | ||
| // path is the last (paths may contain spaces). | ||
| if want != "" && path.Base(fields[len(fields)-1]) != want { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The comment on line 1152 says "paths may contain spaces," but strings.Fields splits on whitespace, so a remote path like "remote:dir/file with spaces.tar" yields fields[len(fields)-1] == "spaces.tar", while want holds the full basename "file with spaces.tar". The comparison fails, the loop finds no matching line, and the function returns "", false, nil — silently downgrading to size-only verification with no error or warning. In practice, proxsave archive names don't typically contain spaces, but the code's own comment suggests this case should be handled.
Comment on lines
+301
to
+313
| mounted, mErr := isMounted(target) | ||
| if mErr != nil { | ||
| logger.Warning("Guard cleanup: cannot determine mount status of %s: %v; leaving immutable flag (clear manually with: chattr -i %s)", target, mErr, target) | ||
| pending++ | ||
| continue | ||
| } | ||
| if mounted { | ||
| // The real storage is mounted on top, so the immutable flag is on the | ||
| // shadowed underlying directory; clearing here would touch the live mount | ||
| // instead. Left intact (mirrors how a hidden bind-mount guard is kept). | ||
| logger.Info("Guard cleanup: %s is currently mounted; its immutable flag is on the shadowed directory and was left intact (to clear it: unmount the storage, run --cleanup-guards again, then remount)", target) | ||
| pending++ | ||
| continue |
There was a problem hiding this comment.
isMounted uses the unresolved path; a symlinked mountpoint can be missed
isMounted(target) compares against /proc/self/mountinfo which records the kernel-resolved mountpoint path. If a legacy index entry holds /mnt/link where /mnt/link is a symlink to /mnt/real, mountinfo records /mnt/real — not /mnt/link — so isMounted("/mnt/link") returns false even though storage IS mounted. The code then proceeds past the guard, resolves the symlink to /mnt/real (which passes isConfirmableDatastoreMountRoot because it is still under /mnt), and runs chattr -i /mnt/real while the datastore is mounted there, touching the live mount's root inode instead of the shadowed underlying directory. This is limited to legacy index entries (the new apply code no longer creates chattr +i guards), and symlinked Proxmox datastore paths are unusual, but the failure mode (chattr on a live mount) is the exact scenario the mounted-check was designed to prevent.
Comment on lines
+84
to
+95
| // recordImmutableGuardTarget appends target to the immutable-guard index after a | ||
| // successful `chattr +i`, so CleanupMountGuards can later clear exactly the | ||
| // directories ProxSave itself made immutable. Best-effort: any failure is logged | ||
| // and swallowed so a recording problem never aborts the restore. The write is | ||
| // atomic and de-duplicated. | ||
| func recordImmutableGuardTarget(logger *logging.Logger, target string) { | ||
| clean, ok := isRecordableImmutableTarget(target) | ||
| if !ok { | ||
| if logger != nil { | ||
| logger.Warning("Guard chattr index: refusing to record unsafe immutable target %q", target) | ||
| } | ||
| return |
There was a problem hiding this comment.
recordImmutableGuardTarget has no production callsite
With the chattr +i fallback removed from both applyDatastoreBlock (warnOfflineTargetUnguarded replaced protectOfflineTargetWithChattr) and the PVE staged-apply path, recordImmutableGuardTarget is no longer called from any production code — only from test files. The index is now purely a legacy artifact; the function (and writeImmutableGuardIndex) can either be removed or have a comment added noting they are retained for potential re-enablement.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
docs/CLI_REFERENCE.md (1)
833-837: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the environment variable override note.
The comment on Line 833 states "Override config file location (CLI flag, NOT an env var)" but the example immediately below uses
DRY_RUN=true proxsavewhich is an environment variable override. The note should distinguish that--config/-cis a CLI flag, whileDRY_RUNis an env var, to avoid implying that no env vars can override config.Current text:
# Override config file location (CLI flag, NOT an env var) proxsave -c /etc/pbs/prod.env # Force dry-run mode DRY_RUN=true proxsaveConsider rephrasing to:
# Override config file location (must use CLI flag; no env var exists for this) proxsave -c /etc/pbs/prod.env # Override dry-run via environment variable DRY_RUN=true proxsave🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/CLI_REFERENCE.md` around lines 833 - 837, Clarify the CLI/env var distinction in the CLI reference example: the note for the config path should explicitly say that proxsave uses the --config/-c CLI flag and does not have an env var equivalent, while the DRY_RUN example should be described as an environment variable override. Update the nearby markdown text in CLI_REFERENCE.md so the comments for the proxsave examples accurately distinguish config-file selection from dry-run environment overrides.internal/orchestrator/mount_guard_symlink_apply_test.go (1)
188-195: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert fail-safe paths perform no mount action.
These tests would still pass if a regression called
mounton the escaped/unresolved target and then skippedchattr/index recording. Add assertions that no mount attempt is made for the refused target.Also applies to: 265-272, 341-346, 406-411
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/orchestrator/mount_guard_symlink_apply_test.go` around lines 188 - 195, The fail-safe path tests currently only verify that chattr and guard-index recording are skipped, so they can miss a regression where a refused target still gets mounted. Update the affected cases in mount_guard_symlink_apply_test to also assert that no mount command is issued for the escaped or unresolved target by checking cmd.calls for mount alongside the existing chattr and readGuardIndexLines checks. Apply this to each fail-safe scenario mentioned in the test blocks so all refused targets explicitly verify zero mount attempts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/dependency-review.yml:
- Around line 19-22: The Checkout repository step in the dependency review
workflow is still using the default credential persistence, which should be
disabled for consistency and hardening. Update the actions/checkout step in this
workflow to set persist-credentials to false, matching the other workflows and
preventing Git credentials from being left in the workspace.
In `@internal/orchestrator/guards_cleanup.go`:
- Around line 301-345: The guard cleanup flow in cleanupGuardTargets uses target
before resolving it, which can make mount checks and dry-run behavior act on the
wrong path. Update the ordering so resolveGuardTargetWithinAllowlist is called
first, then use the resolved path for isMounted and the dryRun branch, while
keeping the existing allowlist and leafExists checks before running chattr in
cleanupGuardTargets.
- Around line 187-194: The guard cleanup flow is failing open when the
verification reread of /proc/self/mountinfo cannot be completed, because
remaining stays at 0 and the later removal path can proceed without
confirmation. Update the cleanup logic in guards_cleanup.go around the mountinfo
reread and summary.guardsRemaining handling so that a failed
cleanupReadFile("/proc/self/mountinfo") call preserves the guard state by
treating the verification as indeterminate, keeping mountGuardBaseDir and the
index instead of removing them. Use the existing guardMountpointsFromMountinfo,
summary.guardsRemaining, and logger.Warning flow to ensure cleanup only
finalizes when the final mount state is successfully verified.
In `@internal/storage/cloud_verify_checksum_test.go`:
- Around line 32-34: The doc comment is stale and refers to a non-existent
verifyChecksumCase helper instead of the test function it documents. Update the
comment above TestCloudVerifyUploadChecksum so it accurately describes that
test’s behavior and matches the function name, keeping the checksum/lsl-path
details aligned with TestCloudVerifyUploadChecksum.
---
Nitpick comments:
In `@docs/CLI_REFERENCE.md`:
- Around line 833-837: Clarify the CLI/env var distinction in the CLI reference
example: the note for the config path should explicitly say that proxsave uses
the --config/-c CLI flag and does not have an env var equivalent, while the
DRY_RUN example should be described as an environment variable override. Update
the nearby markdown text in CLI_REFERENCE.md so the comments for the proxsave
examples accurately distinguish config-file selection from dry-run environment
overrides.
In `@internal/orchestrator/mount_guard_symlink_apply_test.go`:
- Around line 188-195: The fail-safe path tests currently only verify that
chattr and guard-index recording are skipped, so they can miss a regression
where a refused target still gets mounted. Update the affected cases in
mount_guard_symlink_apply_test to also assert that no mount command is issued
for the escaped or unresolved target by checking cmd.calls for mount alongside
the existing chattr and readGuardIndexLines checks. Apply this to each fail-safe
scenario mentioned in the test blocks so all refused targets explicitly verify
zero mount attempts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b758078c-5605-4281-a4d6-d913e6a45fd6
📒 Files selected for processing (48)
.github/scripts/release-policy.sh.github/workflows/codecov.yml.github/workflows/codeql.yml.github/workflows/dependency-review.yml.github/workflows/post-merge-release.yml.github/workflows/race.yml.github/workflows/release-guard.yml.github/workflows/release-intake.yml.github/workflows/release.yml.github/workflows/security-ultimate.ymlREADME.mdcmd/proxsave/main_footer.godocs/BACKUP_ENV_MAPPING.mddocs/CLI_REFERENCE.mddocs/CLOUD_STORAGE.mddocs/CLUSTER_RECOVERY.mddocs/CONFIGURATION.mddocs/ENCRYPTION.mddocs/EXAMPLES.mddocs/INSTALL.mddocs/RELEASE-PROCESS.mddocs/RESTORE_DIAGRAMS.mddocs/RESTORE_GUIDE.mddocs/RESTORE_TECHNICAL.mddocs/TROUBLESHOOTING.mdinternal/backup/archiver_compression_test.gointernal/backup/collector.gointernal/backup/collector_bricks_system.gointernal/backup/collector_bricks_test.gointernal/backup/collector_config_extra_test.gointernal/cli/args.gointernal/config/config.gointernal/config/config_test.gointernal/config/templates/backup.envinternal/orchestrator/guards_cleanup.gointernal/orchestrator/mount_guard.gointernal/orchestrator/mount_guard_apply.gointernal/orchestrator/mount_guard_chattr_index.gointernal/orchestrator/mount_guard_chattr_index_test.gointernal/orchestrator/mount_guard_more_test.gointernal/orchestrator/mount_guard_symlink_apply_test.gointernal/orchestrator/orchestrator.gointernal/orchestrator/orchestrator_test.gointernal/orchestrator/pve_staged_apply.gointernal/orchestrator/pve_staged_apply_additional_test.gointernal/orchestrator/restore_workflow_ui_plan.gointernal/storage/cloud.gointernal/storage/cloud_verify_checksum_test.go
💤 Files with no reviewable changes (1)
- internal/orchestrator/orchestrator.go
Comment on lines
19
to
22
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add persist-credentials: false to the checkout step.
This workflow leaves Git credentials persisted in the workspace (default persist-credentials: true), which creates unnecessary exposure if the dependency-review-action or any later step is compromised. The other workflows in this PR already set persist-credentials: false; align this one for defense in depth.
- name: Checkout repository
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| steps: | |
| - name: Checkout repository | |
| uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 | |
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 | |
| steps: | |
| - name: Checkout repository | |
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 20-21: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/dependency-review.yml around lines 19 - 22, The Checkout
repository step in the dependency review workflow is still using the default
credential persistence, which should be disabled for consistency and hardening.
Update the actions/checkout step in this workflow to set persist-credentials to
false, matching the other workflows and preventing Git credentials from being
left in the workspace.
Source: Linters/SAST tools
Comment on lines
+187
to
+194
| remaining := 0 | ||
| if after, rerr := cleanupReadFile("/proc/self/mountinfo"); rerr == nil { | ||
| _, _, remaining = guardMountpointsFromMountinfo(string(after)) | ||
| } | ||
| summary.guardsRemaining = remaining | ||
| if remaining > 0 || pending > 0 { | ||
| logger.Warning("Guard cleanup: %d guard mount(s) and %d immutable target(s) still present; not removing %s", remaining, pending, mountGuardBaseDir) | ||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep guard state when the verification reread fails.
remaining defaults to 0 if the second /proc/self/mountinfo read fails, so cleanup may remove mountGuardBaseDir without confirming hidden or failed guard mounts are gone. Fail closed here and keep the directory/index when the reread cannot verify the final state.
Proposed fail-closed handling
// If any guard mounts remain (for example hidden under a real mount), or any
// immutable guard target is still pending, avoid removing the directory/index.
- remaining := 0
- if after, rerr := cleanupReadFile("/proc/self/mountinfo"); rerr == nil {
- _, _, remaining = guardMountpointsFromMountinfo(string(after))
+ after, rerr := cleanupReadFile("/proc/self/mountinfo")
+ if rerr != nil {
+ summary.guardsRemaining = totalGuardMounts - unmounted
+ if summary.guardsRemaining < 0 {
+ summary.guardsRemaining = 0
+ }
+ logger.Warning("Guard cleanup: unable to verify remaining guard mounts: %v; not removing %s", rerr, mountGuardBaseDir)
+ return nil
}
+ _, _, remaining := guardMountpointsFromMountinfo(string(after))
summary.guardsRemaining = remaining📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| remaining := 0 | |
| if after, rerr := cleanupReadFile("/proc/self/mountinfo"); rerr == nil { | |
| _, _, remaining = guardMountpointsFromMountinfo(string(after)) | |
| } | |
| summary.guardsRemaining = remaining | |
| if remaining > 0 || pending > 0 { | |
| logger.Warning("Guard cleanup: %d guard mount(s) and %d immutable target(s) still present; not removing %s", remaining, pending, mountGuardBaseDir) | |
| return nil | |
| after, rerr := cleanupReadFile("/proc/self/mountinfo") | |
| if rerr != nil { | |
| summary.guardsRemaining = totalGuardMounts - unmounted | |
| if summary.guardsRemaining < 0 { | |
| summary.guardsRemaining = 0 | |
| } | |
| logger.Warning("Guard cleanup: unable to verify remaining guard mounts: %v; not removing %s", rerr, mountGuardBaseDir) | |
| return nil | |
| } | |
| _, _, remaining := guardMountpointsFromMountinfo(string(after)) | |
| summary.guardsRemaining = remaining | |
| if remaining > 0 || pending > 0 { | |
| logger.Warning("Guard cleanup: %d guard mount(s) and %d immutable target(s) still present; not removing %s", remaining, pending, mountGuardBaseDir) | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/orchestrator/guards_cleanup.go` around lines 187 - 194, The guard
cleanup flow is failing open when the verification reread of
/proc/self/mountinfo cannot be completed, because remaining stays at 0 and the
later removal path can proceed without confirmation. Update the cleanup logic in
guards_cleanup.go around the mountinfo reread and summary.guardsRemaining
handling so that a failed cleanupReadFile("/proc/self/mountinfo") call preserves
the guard state by treating the verification as indeterminate, keeping
mountGuardBaseDir and the index instead of removing them. Use the existing
guardMountpointsFromMountinfo, summary.guardsRemaining, and logger.Warning flow
to ensure cleanup only finalizes when the final mount state is successfully
verified.
Comment on lines
+301
to
+345
| mounted, mErr := isMounted(target) | ||
| if mErr != nil { | ||
| logger.Warning("Guard cleanup: cannot determine mount status of %s: %v; leaving immutable flag (clear manually with: chattr -i %s)", target, mErr, target) | ||
| pending++ | ||
| continue | ||
| } | ||
| if mounted { | ||
| // The real storage is mounted on top, so the immutable flag is on the | ||
| // shadowed underlying directory; clearing here would touch the live mount | ||
| // instead. Left intact (mirrors how a hidden bind-mount guard is kept). | ||
| logger.Info("Guard cleanup: %s is currently mounted; its immutable flag is on the shadowed directory and was left intact (to clear it: unmount the storage, run --cleanup-guards again, then remount)", target) | ||
| pending++ | ||
| continue | ||
| } | ||
|
|
||
| if dryRun { | ||
| logger.Info("DRY RUN: would clear immutable flag (chattr -i) on %s", target) | ||
| cleared++ | ||
| continue | ||
| } | ||
|
|
||
| // Resolve symlinks and re-check the allowlist so a parent-component symlink | ||
| // cannot make chattr -i escape the datastore roots (shared with the apply | ||
| // paths via resolveGuardTargetWithinAllowlist). A path that no longer exists | ||
| // has nothing to clear (not pending). If a datastore root itself is a symlink | ||
| // that resolves outside the allowlist (rare on Proxmox/Debian), the target is | ||
| // refused and left pending — fail-safe: it never escapes and is never data | ||
| // loss; the operator can clear it manually with chattr -i. | ||
| resolved, leafExists, ok, rErr := resolveGuardTargetWithinAllowlist(target) | ||
| if rErr != nil { | ||
| logger.Warning("Guard cleanup: cannot resolve %s: %v; leaving immutable flag (clear manually with: chattr -i %s)", target, rErr, target) | ||
| pending++ | ||
| continue | ||
| } | ||
| if !leafExists { | ||
| logger.Debug("Guard cleanup: immutable target %s no longer exists; nothing to clear", target) | ||
| continue | ||
| } | ||
| if !ok { | ||
| logger.Warning("Guard cleanup: %s resolves outside the datastore roots (%s); refusing to clear it automatically", target, resolved) | ||
| pending++ | ||
| continue | ||
| } | ||
|
|
||
| if _, err := cleanupRunCmd(ctx, "chattr", "-i", resolved); err != nil { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve before checking mount status or dry-run outcomes.
isMounted(target) and the dry-run branch use the raw index path before resolveGuardTargetWithinAllowlist. If an indexed path resolves to another allowed datastore path that is currently mounted, cleanup can run chattr -i against the live mount instead of leaving the shadowed directory pending; dry-run can also report a clear that the real path would later refuse. Resolve first, then check isMounted(resolved), then dry-run or run chattr.
Proposed ordering fix
- mounted, mErr := isMounted(target)
- if mErr != nil {
- logger.Warning("Guard cleanup: cannot determine mount status of %s: %v; leaving immutable flag (clear manually with: chattr -i %s)", target, mErr, target)
- pending++
- continue
- }
- if mounted {
- // The real storage is mounted on top, so the immutable flag is on the
- // shadowed underlying directory; clearing here would touch the live mount
- // instead. Left intact (mirrors how a hidden bind-mount guard is kept).
- logger.Info("Guard cleanup: %s is currently mounted; its immutable flag is on the shadowed directory and was left intact (to clear it: unmount the storage, run --cleanup-guards again, then remount)", target)
- pending++
- continue
- }
-
- if dryRun {
- logger.Info("DRY RUN: would clear immutable flag (chattr -i) on %s", target)
- cleared++
- continue
- }
-
// Resolve symlinks and re-check the allowlist so a parent-component symlink
// cannot make chattr -i escape the datastore roots (shared with the apply
// paths via resolveGuardTargetWithinAllowlist). A path that no longer exists
@@
if !ok {
logger.Warning("Guard cleanup: %s resolves outside the datastore roots (%s); refusing to clear it automatically", target, resolved)
pending++
continue
}
+
+ mounted, mErr := isMounted(resolved)
+ if mErr != nil {
+ logger.Warning("Guard cleanup: cannot determine mount status of %s: %v; leaving immutable flag (clear manually with: chattr -i %s)", resolved, mErr, resolved)
+ pending++
+ continue
+ }
+ if mounted {
+ logger.Info("Guard cleanup: %s is currently mounted; its immutable flag is on the shadowed directory and was left intact (to clear it: unmount the storage, run --cleanup-guards again, then remount)", resolved)
+ pending++
+ continue
+ }
+
+ if dryRun {
+ logger.Info("DRY RUN: would clear immutable flag (chattr -i) on %s", resolved)
+ cleared++
+ continue
+ }
if _, err := cleanupRunCmd(ctx, "chattr", "-i", resolved); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mounted, mErr := isMounted(target) | |
| if mErr != nil { | |
| logger.Warning("Guard cleanup: cannot determine mount status of %s: %v; leaving immutable flag (clear manually with: chattr -i %s)", target, mErr, target) | |
| pending++ | |
| continue | |
| } | |
| if mounted { | |
| // The real storage is mounted on top, so the immutable flag is on the | |
| // shadowed underlying directory; clearing here would touch the live mount | |
| // instead. Left intact (mirrors how a hidden bind-mount guard is kept). | |
| logger.Info("Guard cleanup: %s is currently mounted; its immutable flag is on the shadowed directory and was left intact (to clear it: unmount the storage, run --cleanup-guards again, then remount)", target) | |
| pending++ | |
| continue | |
| } | |
| if dryRun { | |
| logger.Info("DRY RUN: would clear immutable flag (chattr -i) on %s", target) | |
| cleared++ | |
| continue | |
| } | |
| // Resolve symlinks and re-check the allowlist so a parent-component symlink | |
| // cannot make chattr -i escape the datastore roots (shared with the apply | |
| // paths via resolveGuardTargetWithinAllowlist). A path that no longer exists | |
| // has nothing to clear (not pending). If a datastore root itself is a symlink | |
| // that resolves outside the allowlist (rare on Proxmox/Debian), the target is | |
| // refused and left pending — fail-safe: it never escapes and is never data | |
| // loss; the operator can clear it manually with chattr -i. | |
| resolved, leafExists, ok, rErr := resolveGuardTargetWithinAllowlist(target) | |
| if rErr != nil { | |
| logger.Warning("Guard cleanup: cannot resolve %s: %v; leaving immutable flag (clear manually with: chattr -i %s)", target, rErr, target) | |
| pending++ | |
| continue | |
| } | |
| if !leafExists { | |
| logger.Debug("Guard cleanup: immutable target %s no longer exists; nothing to clear", target) | |
| continue | |
| } | |
| if !ok { | |
| logger.Warning("Guard cleanup: %s resolves outside the datastore roots (%s); refusing to clear it automatically", target, resolved) | |
| pending++ | |
| continue | |
| } | |
| if _, err := cleanupRunCmd(ctx, "chattr", "-i", resolved); err != nil { | |
| // Resolve symlinks and re-check the allowlist so a parent-component symlink | |
| // cannot make chattr -i escape the datastore roots (shared with the apply | |
| // paths via resolveGuardTargetWithinAllowlist). A path that no longer exists | |
| // has nothing to clear (not pending). If a datastore root itself is a symlink | |
| // that resolves outside the allowlist (rare on Proxmox/Debian), the target is | |
| // refused and left pending — fail-safe: it never escapes and is never data | |
| // loss; the operator can clear it manually with chattr -i. | |
| resolved, leafExists, ok, rErr := resolveGuardTargetWithinAllowlist(target) | |
| if rErr != nil { | |
| logger.Warning("Guard cleanup: cannot resolve %s: %v; leaving immutable flag (clear manually with: chattr -i %s)", target, rErr, target) | |
| pending++ | |
| continue | |
| } | |
| if !leafExists { | |
| logger.Debug("Guard cleanup: immutable target %s no longer exists; nothing to clear", target) | |
| continue | |
| } | |
| if !ok { | |
| logger.Warning("Guard cleanup: %s resolves outside the datastore roots (%s); refusing to clear it automatically", target, resolved) | |
| pending++ | |
| continue | |
| } | |
| mounted, mErr := isMounted(resolved) | |
| if mErr != nil { | |
| logger.Warning("Guard cleanup: cannot determine mount status of %s: %v; leaving immutable flag (clear manually with: chattr -i %s)", resolved, mErr, resolved) | |
| pending++ | |
| continue | |
| } | |
| if mounted { | |
| logger.Info("Guard cleanup: %s is currently mounted; its immutable flag is on the shadowed directory and was left intact (to clear it: unmount the storage, run --cleanup-guards again, then remount)", resolved) | |
| pending++ | |
| continue | |
| } | |
| if dryRun { | |
| logger.Info("DRY RUN: would clear immutable flag (chattr -i) on %s", resolved) | |
| cleared++ | |
| continue | |
| } | |
| if _, err := cleanupRunCmd(ctx, "chattr", "-i", resolved); err != nil { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/orchestrator/guards_cleanup.go` around lines 301 - 345, The guard
cleanup flow in cleanupGuardTargets uses target before resolving it, which can
make mount checks and dry-run behavior act on the wrong path. Update the
ordering so resolveGuardTargetWithinAllowlist is called first, then use the
resolved path for isMounted and the dryRun branch, while keeping the existing
allowlist and leafExists checks before running chattr in cleanupGuardTargets.
Comment on lines
+32
to
+34
| // verifyChecksumCase drives VerifyUpload through the primary (lsl) path with the | ||
| // checksum step enabled and a stubbed hashsum response. | ||
| func TestCloudVerifyUploadChecksum(t *testing.T) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale doc comment. The comment names a verifyChecksumCase helper, but it documents TestCloudVerifyUploadChecksum. Align the comment with the function name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/storage/cloud_verify_checksum_test.go` around lines 32 - 34, The doc
comment is stale and refers to a non-existent verifyChecksumCase helper instead
of the test function it documents. Update the comment above
TestCloudVerifyUploadChecksum so it accurately describes that test’s behavior
and matches the function name, keeping the checksum/lsl-path details aligned
with TestCloudVerifyUploadChecksum.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated release PR for
v0.26.0.Greptile Summary
This release adds two major features: (1) a symlink-safe guard-target resolution gate shared across the PBS/PVE mount-guard apply and cleanup paths, replacing the persistent
chattr +ioffline fallback with a warn-only path and a legacy-cleanup flow via a new immutable-guard index; (2) a two-step cloud upload verification that layers a SHA256 content check (viarclone hashsum) on top of the existing size pre-check, with best-effort fallback when the backend has no native hash. Several config defaults were also aligned with the shippedbackup.envtemplate.resolveGuardTargetWithinAllowlist(ancestor-walk + allowlist re-check after symlink resolution) is now applied before anymkdir/mount/chattron both the PBS and PVE apply paths, and thechattr +ioffline fallback — which survived reboots and re-blocked writes after storage was later unmounted — was replaced by a warning.CleanupMountGuardsgains a symmetricclearImmutableGuardsstep that reads a new per-target index to reverse only the flags ProxSave itself set.VerifyUploadnow runsrclone hashsum sha256after the size pre-check whenCLOUD_VERIFY_CHECKSUM=true(new default). Backends without native SHA256 fall back to size-only with a debug log;CLOUD_VERIFY_DOWNLOAD=trueforces a download-and-hash for end-to-end verification at full egress cost.ENABLE_DEDUPLICATION,ENABLE_PREFILTER,AUTO_FIX_PERMISSIONS,CLOUD_PARALLEL_VERIFICATION, andCLOUD_UPLOAD_MODEnow default to the values documented in the shipped template;BACKUP_CEPH_CONFIGflips fromtruetofalseto match the template.Confidence Score: 4/5
Safe to merge; the three flagged items are edge-case quality issues with no path to data loss or security bypass.
The core changes — symlink-safe guard resolution, warn-only offline fallback, legacy chattr cleanup, and two-step cloud checksum verification — are well-structured and thoroughly tested. The most notable gap is in clearImmutableGuards: isMounted checks the unresolved path, which can return a false negative when a legacy index entry is a symlink whose resolved path is still within the /mnt//media allowlist, allowing chattr -i to be attempted on a live mount. The remoteSHA256 basename parser silently falls back to size-only verification for filenames with spaces. recordImmutableGuardTarget and writeImmutableGuardIndex have no production callsite now that the chattr fallback was removed.
internal/orchestrator/guards_cleanup.go (isMounted symlink gap) and internal/storage/cloud.go (remoteSHA256 basename matching with spaces) are the two files most worth a second look.
Important Files Changed
recordImmutableGuardTargethas no production callsite since the chattr fallback was removed.clearImmutableGuardsfor legacy chattr +i cleanup and structured summary logging;isMountedcheck uses the unresolved path, which can produce a false negative for symlinked legacy entries.resolveGuardTargetWithinAllowlist(symlink-safe, ancestor-walk for missing leaves) and injectableresolveGuardTargetseam;mountGuardBaseDirpromoted to var for test redirection.protectOfflineTargetWithChattrwith warn-onlywarnOfflineTargetUnguarded; adds symlink resolution gate before any mkdir/mount/guard action on PBS datastores.rclone hashsum; basename matching silently degrades to size-only for filenames containing spaces.BackupUserHomesfield (user homes are now always collected as a baseline) and drops the "at least one option enabled" validation that the system recipe makes unnecessary.resolveGuardTargetWithinAllowlistand the apply paths correctly refuse escaping symlinks and only act on allowlist-confined resolved paths.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant U as VerifyUpload participant P as verifyPrimary/Alternative participant R as remoteSHA256 participant B as backup.GenerateChecksum U->>P: size pre-check (rclone lsl/ls) P-->>U: "ok=false, return early" P-->>U: "ok=true, proceed" alt "CLOUD_VERIFY_CHECKSUM=false" U-->>U: return true (size-only) else "CLOUD_VERIFY_CHECKSUM=true" U->>R: "remoteSHA256(download=false)" R->>R: rclone hashsum sha256 remote alt backend has no native SHA256 R-->>U: "ok=false" alt "CLOUD_VERIFY_DOWNLOAD=true" U->>R: "remoteSHA256(download=true)" R->>R: rclone hashsum sha256 --download remote R-->>U: "hash or ok=false" end alt still no hash U-->>U: debug log, return true (size-only) end else native SHA256 available R-->>U: "remoteHash ok=true" U->>B: GenerateChecksum(localFile) B-->>U: localHash alt hashes match U-->>U: return true else mismatch U-->>U: return false error end end end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant U as VerifyUpload participant P as verifyPrimary/Alternative participant R as remoteSHA256 participant B as backup.GenerateChecksum U->>P: size pre-check (rclone lsl/ls) P-->>U: "ok=false, return early" P-->>U: "ok=true, proceed" alt "CLOUD_VERIFY_CHECKSUM=false" U-->>U: return true (size-only) else "CLOUD_VERIFY_CHECKSUM=true" U->>R: "remoteSHA256(download=false)" R->>R: rclone hashsum sha256 remote alt backend has no native SHA256 R-->>U: "ok=false" alt "CLOUD_VERIFY_DOWNLOAD=true" U->>R: "remoteSHA256(download=true)" R->>R: rclone hashsum sha256 --download remote R-->>U: "hash or ok=false" end alt still no hash U-->>U: debug log, return true (size-only) end else native SHA256 available R-->>U: "remoteHash ok=true" U->>B: GenerateChecksum(localFile) B-->>U: localHash alt hashes match U-->>U: return true else mismatch U-->>U: return false error end end endReviews (1): Last reviewed commit: "docs(cli): list --cleanup-guards in the ..." | Re-trigger Greptile
Summary by CodeRabbit
New Features
Bug Fixes
Documentation