Conversation
- Add network_apply_additional_test.go with extensive tests for rollback (arm/disarm), NIC repair CLI (overrides/conflicts), snapshot IP parsing, command selection, and error paths. - Use FakeFS/FakeCommandRunner plus PATH stubs to deterministically exercise both success and failure branches. - Bring network_apply.go coverage to ~99% with no production code changes.
…164) Bumps the actions-updates group with 2 updates in the / directory: [goreleaser/goreleaser-action](https://github.com/goreleaser/goreleaser-action) and [actions/attest-build-provenance](https://github.com/actions/attest-build-provenance). Updates `goreleaser/goreleaser-action` from 6 to 7 - [Release notes](https://github.com/goreleaser/goreleaser-action/releases) - [Commits](goreleaser/goreleaser-action@v6...v7) Updates `actions/attest-build-provenance` from 3 to 4 - [Release notes](https://github.com/actions/attest-build-provenance/releases) - [Changelog](https://github.com/actions/attest-build-provenance/blob/main/RELEASE.md) - [Commits](actions/attest-build-provenance@v3...v4) --- updated-dependencies: - dependency-name: goreleaser/goreleaser-action dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-updates - dependency-name: actions/attest-build-provenance dependency-version: '4' 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>
Make PVE datastore skip messages user-friendly: log disabled storages as SKIP, offline storages as actionable WARNING, and always emit DEBUG skip details with active/enabled/status and an explicit reason. Fix detected datastores report header to match output column order. Extend unit tests to cover runtime flag parsing and the new skip logging, and make Makefile coverage targets honor the go.mod toolchain via GOTOOLCHAIN.
Adds per-endpoint opt-in Discord content text (WEBHOOK_<ENDPOINT>_DISCORD_CONTENT_ENABLED/..._DISCORD_CONTENT) to avoid “empty” messages when embeds aren’t rendered. Default remains unchanged (embed-only). Includes 2000-char truncation, docs/template updates, and tests.
…ly messages" This reverts commit fc0aed4.
Derive restore compatibility, cluster gating, and hostname warnings from archive-backed facts instead of candidate manifest metadata. Add restore archive analysis for internal backup metadata and category-based fallback, update restore planning and compatibility checks to consume trusted decision inputs, and add regression tests covering manifest spoofing for backup type and cluster mode.
Require checksum verification for staged backup archives before any restore or decrypt operation proceeds. Add strict SHA256 parsing and normalization, enforce checksum source agreement between sidecar and manifest metadata, reject raw backup candidates with no verifiable integrity source, centralize shared prepare-path verification for bundle and raw inputs, preserve source artifact checksum separately from the prepared plain archive checksum, and update regression tests to cover missing, mismatched, and legacy checksum scenarios.
Use Lstat in network staged apply so symlink entries are handled as symlinks instead of being dereferenced and copied as regular file contents. Reject symlinked staged network directories, extend the FS abstraction with Lstat, and add regression tests covering preserved symlinks and invalid symlinked stage roots.
…ink escapes Replace the restore path safety checks with a shared FS-aware resolver that walks paths component by component using Lstat and Readlink, correctly handling missing tails and rejecting intermediate symlink escapes. Apply the new validation to regular restore, safety restore, symlink and hardlink handling, and add regression tests for broken escaping symlinks, symlink loops, and symlinked restore roots.
Introduce a shared path-safe key for PBS datastore names and PVE storage names when building collector output paths, preventing path traversal and filename collisions from raw config values. Keep raw names unchanged in metadata and command invocations, update all affected PBS/PVE collector call sites, and add regression tests covering unsafe names while preserving the existing layout for already-safe names.
Prevent unbounded goroutine accumulation in timeout/cancel wrappers by limiting in-flight safefs operations and reusing a single in-flight read per reader/file descriptor in the input package. Keep public APIs unchanged, preserve current timeout semantics, and add regression coverage for limiter saturation, repeated timeouts, and race-safe cleanup of timed operations.
…lidation Teach the restore path resolver to distinguish security violations from local operational failures, so only real root-escape conditions are reported as illegal paths. Keep broken-symlink and traversal hardening intact, allow in-root permission and ENOTDIR failures to surface through the normal restore flow, and add regression tests covering the new security vs operational error classification.
- add explicit PBS datastore metadata for source, CLI identity and output key - derive stable path-based output keys for PBS_DATASTORE_PATH overrides - keep existing output names for real CLI-discovered PBS datastores - skip datastore show/status CLI calls for override-only entries - use filesystem-only namespace discovery for override paths - keep PXAR outputs isolated for colliding override basenames - preserve distinct override entries in PBS datastore inventory - exclude override-only inventory entries from datastore.cfg fallback restore - add regression coverage for basename collisions and restore safety - update PBS_DATASTORE_PATH documentation to reflect scan-root semantics
…tive links resolvePathRelativeToBaseWithinRootFS now canonicalizes baseDir by resolving existing symlinks before validating relative link targets. This closes the bypass where an archive could create an apparently harmless parent symlink and then install a relative symlink that escapes destRoot once materialized by the kernel.
Make safefs timeout tests failure-safe by draining blocked workers from cleanup and separating worker cleanup from global state restore. Also capture limiter and fs operation hooks locally in runLimited/Stat/ReadDir/Statfs to avoid races with global resets while async work is still in flight.
Keep completed in-flight line and password reads attached to their state until a caller consumes the buffered result, instead of clearing the inflight state from the producer goroutine. This fixes the race where a read could complete after a timeout, be cleaned up before the next retry started, and leave the completed input unreachable. Add deterministic tests for both retry-while-pending and completion-before-retry cases to lock the behavior down.
Add a PBS-specific output-key resolver to guarantee stable, unique datastore filenames and directories across auto-detected datastores and PBS_DATASTORE_PATH overrides. This fixes real collisions between CLI datastore names and path-derived override keys, makes pbsDatastore.pathKey() source-aware when OutputKey is unset, and keeps inventory output_key values aligned with the actual files written by PBS collectors. Includes regression tests for CLI-vs-override collisions, override fallback behavior, and inventory consistency.
The new tests verify that line and password inflight reads remain attached after timeout until the completed result is reused, and that state.inflight is cleared immediately after that consumer receives the result.
Dependency ReviewThe following issues were found:
License Issues.github/workflows/release.yml
OpenSSF Scorecard
Scanned Files
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds extensive FS-aware path-security and symlink-safe restore logic, PBS datastore path normalization with deterministic output keys, archive-based restore analysis and decision flow, staged integrity verification and bundle preparation, per-reader input inflight tracking, safeFS operation limiting, numerous PBS/PVE collector updates, tests, and minor build/doc/toolchain bumps. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Workflow as Restore Workflow
participant Archive as Archive
participant Analyzer as Archive Analyzer
participant Decision as Decision Info
participant Plan as Restore Plan
User->>Workflow: Start restore with bundle
Workflow->>Archive: Open prepared archive
Workflow->>Analyzer: AnalyzeRestoreArchive(archivePath, logger)
Analyzer->>Archive: Read tar entries & internal metadata
Analyzer->>Decision: Build RestoreDecisionInfo (type, cluster, hostname)
Decision-->>Analyzer: Return decision & categories
Analyzer-->>Workflow: Return decision & available categories
Workflow->>Plan: PlanRestore(decision.ClusterPayload, selectedCategories, systemType, mode)
Plan-->>Workflow: Return restore plan
Workflow->>User: Present prompts & proceed
sequenceDiagram
participant Extract as Extract Operation
participant SafeFS as Path Security FS
participant Resolver as Path Resolver
participant Validator as Symlink Validator
Extract->>SafeFS: sanitizeRestoreEntryTargetWithFS(destRoot, entry)
SafeFS->>Resolver: resolvePathWithinRootFS(destRoot, entry)
Resolver->>Validator: Walk path components, detect symlinks
Validator->>Validator: Resolve symlinks (hop limit)
Validator->>Validator: Ensure resolved path inside root
Validator-->>Resolver: Return resolved path or security/operational error
Resolver-->>SafeFS: Return validation result
SafeFS-->>Extract: Allow or reject extraction
sequenceDiagram
participant Reader as Reader
participant Input as Input Handler
participant State as Inflight State Map
participant Goroutine as Read Goroutine
Reader->>Input: ReadLineWithContext(ctx)
Input->>State: Check inflight for reader
alt no existing inflight
Input->>Goroutine: Spawn goroutine to perform ReadString
Goroutine->>State: Deliver result via channel
State->>Input: Store inflight op
else reuse inflight
Input->>State: Wait on existing inflight channel
end
Goroutine-->>Input: Return result/error
Input->>State: Clear inflight
Input-->>Reader: Return line or error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
internal/input/input.go (2)
90-106: Minor: Redundant Load before LoadOrStore.The initial
Loadcheck is unnecessary sinceLoadOrStoreperforms an atomic load-or-store operation that handles both cases. This is a minor optimization opportunity.🔧 Suggested simplification
func getLineState(reader *bufio.Reader) *lineState { - if state, ok := lineStates.Load(reader); ok { - return state.(*lineState) - } state := &lineState{} actual, _ := lineStates.LoadOrStore(reader, state) return actual.(*lineState) } func getPasswordState(fd int) *passwordState { - if state, ok := passwordStates.Load(fd); ok { - return state.(*passwordState) - } state := &passwordState{} actual, _ := passwordStates.LoadOrStore(fd, state) return actual.(*passwordState) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/input/input.go` around lines 90 - 106, Both getLineState and getPasswordState perform an unnecessary initial Load before calling LoadOrStore; remove the redundant Load and simply create the new state (lineState/passwordState), call lineStates.LoadOrStore(reader, state) / passwordStates.LoadOrStore(fd, state), then return the actual value cast to *lineState or *passwordState respectively so the atomic load-or-store handles both existing and new cases.
125-152: Mutex held during blocking select limits concurrent consumption.The mutex is held across the entire
selectblock, which means if multiple goroutines callReadLineWithContexton the same reader concurrently, they will serialize on the mutex rather than all waiting on the same in-flight result. This appears intentional per the docstring (at-most-one in-flight read for sequential retries), but it's worth noting this design doesn't support multiple concurrent consumers efficiently.Additionally, entries in
lineStatesare never removed after use. For long-running processes with many transient readers, this could lead to gradual memory accumulation. Consider adding a cleanup mechanism if readers are short-lived.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/input/input.go` around lines 125 - 152, The mutex (state.mu) is held while blocking on the select, serializing concurrent callers; fix by creating/assigning the lineInflight under lock but releasing state.mu before the select so multiple goroutines can wait on the same inflight.done channel concurrently (capture a local inflight variable while holding the lock, then unlock). Also add cleanup to avoid unbounded growth of lineStates: when clearing state.inflight (where currently you set state.inflight = nil after receiving res), remove the corresponding entry from the container map (lineStates) if the reader is transient so entries are not retained forever; do this under the same mutex to avoid races (e.g., in the branch that sets state.inflight = nil, lock, remove the map entry, then unlock).internal/orchestrator/compatibility_test.go (1)
19-19: Make this test prove the new API is argument-driven.Because the fake host is also set up as PVE, this still passes if
ValidateCompatibilityaccidentally readsDetectCurrentSystem()instead of honoring thecurrentSystemargument. Passing a deliberately conflicting value here would guard the refactor much better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/compatibility_test.go` at line 19, The test currently passes because the fake host is PVE; update the call to ValidateCompatibility so it supplies a deliberately conflicting currentSystem argument (e.g., pass SystemTypePBS as the currentSystem while the host is set up as PVE) to prove the API uses the passed arguments rather than calling DetectCurrentSystem(); modify the test assertion accordingly to expect an error from ValidateCompatibility(SystemTypePBS, SystemTypePVE) (or the inverse conflict) so the refactor is guarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/backup/collector_pbs_datastore.go`:
- Around line 762-795: The current logic gates adding c.config.PBSDatastorePaths
on successful CLI enumeration (getDatastoreList) so overrides never get applied
when the proxmox-backup-manager CLI is missing or datastore list fails; change
the flow so CLI enumeration is best-effort (catch/log errors from
getDatastoreList but do not return early) and always run the override-appending
block that iterates c.config.PBSDatastorePaths (using normalizePBSDatastorePath,
buildPBSOverrideDisplayName, buildPBSOverrideOutputKey) and then call
assignUniquePBSDatastoreOutputKeys(datastores); alternatively move the
override-appending block out of any early-return path so overrides are appended
regardless of getDatastoreList outcome.
In `@internal/backup/collector_pbs.go`:
- Around line 350-353: The status snapshot filename is still generated from
ds.pathKey(), so assignUniquePBSDatastoreOutputKeys(datastores) has no effect
and later entries overwrite earlier ones; update the status snapshot generation
(where you currently call ds.pathKey()) to use the datastore's assigned output
key set by assignUniquePBSDatastoreOutputKeys (the field or accessor populated
by that function, e.g., datastore.OutputKey or datastore.outputKey()) when
creating the datastore_*_status.json filenames so collisions are avoided; apply
the same change to the other occurrences mentioned (lines ~400-403) and keep
clonePBSDatastores(datastores) + assignUniquePBSDatastoreOutputKeys(datastores)
as-is.
In `@internal/orchestrator/backup_sources.go`:
- Around line 263-267: The current guard only checks presence of checksum
strings (item.remoteChecksum and manifest.SHA256) but not their validity; update
the validation inside the candidate-filtering logic (the block referencing
item.remoteChecksum, manifest.SHA256, and logWarning) to normalize and parse the
checksum material before accepting a candidate: for manifest.SHA256,
trim/normalize and verify it is a valid 64-hex SHA256 string (reject and
increment integrityMissing/logWarning if malformed), and for local sidecar
checksum (item.remoteChecksum or wherever the .sha256 is read) parse/normalize
the value similarly (reject non-hex or wrong-length values); ensure the same
normalized checksum value and its source tag are stored on the candidate so
downstream staged integrity resolution only sees pre-validated checksums—apply
the identical validation logic to the other occurrence noted (lines around the
second block that checks these fields).
In `@internal/orchestrator/compatibility.go`:
- Around line 64-73: parseSystemTypeString currently only matches acronyms and
hyphenated names, causing inputs like "Proxmox VE" or "Proxmox Backup Server" to
return SystemTypeUnknown; update parseSystemTypeString to also detect
space-separated and full names by checking for strings.Contains(normalized,
"proxmox ve"), strings.Contains(normalized, "proxmox backup"), and
strings.Contains(normalized, "proxmox backup server") (and/or a generic "proxmox
backup" check) alongside the existing checks so SystemTypePVE and SystemTypePBS
are returned for those inputs; reference the parseSystemTypeString function and
the SystemTypePVE/SystemTypePBS constants when making the change.
In `@internal/orchestrator/network_staged_apply.go`:
- Around line 185-210: The code currently replays symlink targets verbatim
(restoreFS.Readlink -> restoreFS.Symlink) which can escape the real destination
subtree; change the logic to resolve and validate the link target before
creating it: after reading target from restoreFS.Readlink and before
restoreFS.Symlink, compute the effective absolute path the link would point to
when created under filepath.Dir(dest) (e.g., resolved :=
filepath.Clean(filepath.Join(filepath.Dir(dest), target)) and optionally
EvalSymlinks on the destination FS), ensure that this resolved path is inside
the allowed destination subtree root (reject with an error if
!strings.HasPrefix(resolved, destRoot+string(os.PathSeparator)) or equivalent
safe containment check), and for cases where the original target is absolute or
would escape, either rewrite the target to a safe relative target under the
destination subtree or return an error; apply these checks around the existing
functions (restoreFS.Readlink, ensureDirExistsWithInheritedMeta,
restoreFS.Lstat, restoreFS.Remove, restoreFS.Symlink) so Symlink is only called
with a validated/rewritten target.
In `@internal/orchestrator/restore_decision.go`:
- Around line 139-149: The code reads backup_metadata.txt with
io.ReadAll(tarReader) which can allocate unbounded memory; replace that call by
reading through an io.LimitedReader (or io.LimitReader) with a small configured
max bytes (e.g., a few KB) before calling parseRestoreDecisionMetadata, and if
the reader hits the limit treat it as invalid metadata (set metadataErr and
continue or return as the surrounding logic requires). Update the logic around
parseRestoreDecisionMetadata, metadataErr, and metadata to ensure overflowing
reads are rejected consistently and do not permit parsing of truncated/partial
data.
In `@internal/orchestrator/restore_workflow_ui.go`:
- Around line 79-84: When AnalyzeRestoreArchive returns an error, do not return
early into runFullRestoreWithUI; instead log the error (as already done) but
continue through the existing compatibility and cluster/PBS guard flow by
invoking ValidateCompatibility and the decision-driven checks before any
fallback. Modify the block around AnalyzeRestoreArchive so that on error you set
availableCategories/decisionInfo to nil or an appropriate default, then proceed
to call ValidateCompatibility and the later guard/decision logic, and only call
runFullRestoreWithUI as the final fallback after those checks pass or explicitly
allow it.
In `@internal/safefs/safefs.go`:
- Around line 97-105: The code calls effectiveTimeout and treats timeout==0 as
"no timeout", which also happens when the context deadline already elapsed;
update runLimited to distinguish these cases by checking ctx.Err() after
computing effectiveTimeout: if timeout==0 and ctx.Err()!=nil then return the
zero value and the provided TimeoutError (or wrap/convert ctx.Err() into a
TimeoutError) instead of calling run(), otherwise proceed with no-timeout path.
Reference runLimited, effectiveTimeout, TimeoutError and the run func when
implementing this check.
---
Nitpick comments:
In `@internal/input/input.go`:
- Around line 90-106: Both getLineState and getPasswordState perform an
unnecessary initial Load before calling LoadOrStore; remove the redundant Load
and simply create the new state (lineState/passwordState), call
lineStates.LoadOrStore(reader, state) / passwordStates.LoadOrStore(fd, state),
then return the actual value cast to *lineState or *passwordState respectively
so the atomic load-or-store handles both existing and new cases.
- Around line 125-152: The mutex (state.mu) is held while blocking on the
select, serializing concurrent callers; fix by creating/assigning the
lineInflight under lock but releasing state.mu before the select so multiple
goroutines can wait on the same inflight.done channel concurrently (capture a
local inflight variable while holding the lock, then unlock). Also add cleanup
to avoid unbounded growth of lineStates: when clearing state.inflight (where
currently you set state.inflight = nil after receiving res), remove the
corresponding entry from the container map (lineStates) if the reader is
transient so entries are not retained forever; do this under the same mutex to
avoid races (e.g., in the branch that sets state.inflight = nil, lock, remove
the map entry, then unlock).
In `@internal/orchestrator/compatibility_test.go`:
- Line 19: The test currently passes because the fake host is PVE; update the
call to ValidateCompatibility so it supplies a deliberately conflicting
currentSystem argument (e.g., pass SystemTypePBS as the currentSystem while the
host is set up as PVE) to prove the API uses the passed arguments rather than
calling DetectCurrentSystem(); modify the test assertion accordingly to expect
an error from ValidateCompatibility(SystemTypePBS, SystemTypePVE) (or the
inverse conflict) so the refactor is guarded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 831216f9-eb91-4ba0-a25b-53094afa0e68
📒 Files selected for processing (63)
.github/workflows/release.ymlMakefiledocs/CONFIGURATION.mddocs/RESTORE_GUIDE.mdgo.modinternal/backup/checksum.gointernal/backup/checksum_legacy_test.gointernal/backup/checksum_test.gointernal/backup/collector.gointernal/backup/collector_helpers_extra_test.gointernal/backup/collector_pbs.gointernal/backup/collector_pbs_commands_coverage_test.gointernal/backup/collector_pbs_datastore.gointernal/backup/collector_pbs_datastore_inventory.gointernal/backup/collector_pbs_datastore_inventory_test.gointernal/backup/collector_pbs_test.gointernal/backup/collector_pve.gointernal/backup/collector_pve_parse_test.gointernal/backup/collector_pve_test.gointernal/config/templates/backup.envinternal/input/input.gointernal/input/input_test.gointernal/orchestrator/backup_safety.gointernal/orchestrator/backup_safety_test.gointernal/orchestrator/backup_sources.gointernal/orchestrator/backup_sources_test.gointernal/orchestrator/compatibility.gointernal/orchestrator/compatibility_test.gointernal/orchestrator/decrypt.gointernal/orchestrator/decrypt_integrity.gointernal/orchestrator/decrypt_integrity_test.gointernal/orchestrator/decrypt_integrity_test_helpers_test.gointernal/orchestrator/decrypt_prepare_common.gointernal/orchestrator/decrypt_test.gointernal/orchestrator/decrypt_tui.gointernal/orchestrator/decrypt_tui_test.gointernal/orchestrator/decrypt_workflow_test.gointernal/orchestrator/decrypt_workflow_ui.gointernal/orchestrator/deps.gointernal/orchestrator/deps_test.gointernal/orchestrator/network_apply_additional_test.gointernal/orchestrator/network_staged_apply.gointernal/orchestrator/network_staged_apply_test.gointernal/orchestrator/path_security.gointernal/orchestrator/path_security_test.gointernal/orchestrator/pbs_staged_apply.gointernal/orchestrator/pbs_staged_apply_additional_test.gointernal/orchestrator/restore.gointernal/orchestrator/restore_decision.gointernal/orchestrator/restore_decision_test.gointernal/orchestrator/restore_errors_test.gointernal/orchestrator/restore_plan.gointernal/orchestrator/restore_plan_test.gointernal/orchestrator/restore_test.gointernal/orchestrator/restore_workflow_decision_test.gointernal/orchestrator/restore_workflow_more_test.gointernal/orchestrator/restore_workflow_ui.gointernal/orchestrator/restore_workflow_ui_helpers_test.gointernal/orchestrator/selective.gointernal/pbs/namespaces.gointernal/pbs/namespaces_test.gointernal/safefs/safefs.gointernal/safefs/safefs_test.go
Comment on lines
762
to
+795
| if len(c.config.PBSDatastorePaths) > 0 { | ||
| existing := make(map[string]struct{}, len(datastores)) | ||
| for _, ds := range datastores { | ||
| if ds.Path != "" { | ||
| existing[ds.Path] = struct{}{} | ||
| if normalized := ds.normalizedPath(); normalized != "" { | ||
| existing[normalized] = struct{}{} | ||
| } | ||
| } | ||
| validName := regexp.MustCompile(`^[a-zA-Z0-9_-]+$`) | ||
| for idx, override := range c.config.PBSDatastorePaths { | ||
| override = strings.TrimSpace(override) | ||
| if override == "" { | ||
| continue | ||
| } | ||
| if _, ok := existing[override]; ok { | ||
| normalized := normalizePBSDatastorePath(override) | ||
| if normalized == "" { | ||
| continue | ||
| } | ||
| name := filepath.Base(filepath.Clean(override)) | ||
| if name == "" || name == "." || name == string(os.PathSeparator) || !validName.MatchString(name) { | ||
| name = fmt.Sprintf("datastore_%d", idx+1) | ||
| if _, ok := existing[normalized]; ok { | ||
| continue | ||
| } | ||
| existing[normalized] = struct{}{} | ||
| name := buildPBSOverrideDisplayName(normalized, idx) | ||
| datastores = append(datastores, pbsDatastore{ | ||
| Name: name, | ||
| Path: override, | ||
| Comment: "configured via PBS_DATASTORE_PATH", | ||
| Name: name, | ||
| Path: override, | ||
| Comment: "configured via PBS_DATASTORE_PATH", | ||
| Source: pbsDatastoreSourceOverride, | ||
| NormalizedPath: normalized, | ||
| OutputKey: buildPBSOverrideOutputKey(normalized), | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| assignUniquePBSDatastoreOutputKeys(datastores) | ||
|
|
There was a problem hiding this comment.
Don't gate override scan roots on proxmox-backup-manager.
The override entries added here are never reached when the CLI is missing or datastore list fails, because getDatastoreList returns earlier. That breaks the override-only namespace/PXAR discovery path on exactly the hosts this fallback is meant to support. Make CLI enumeration best-effort, then append c.config.PBSDatastorePaths regardless.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/backup/collector_pbs_datastore.go` around lines 762 - 795, The
current logic gates adding c.config.PBSDatastorePaths on successful CLI
enumeration (getDatastoreList) so overrides never get applied when the
proxmox-backup-manager CLI is missing or datastore list fails; change the flow
so CLI enumeration is best-effort (catch/log errors from getDatastoreList but do
not return early) and always run the override-appending block that iterates
c.config.PBSDatastorePaths (using normalizePBSDatastorePath,
buildPBSOverrideDisplayName, buildPBSOverrideOutputKey) and then call
assignUniquePBSDatastoreOutputKeys(datastores); alternatively move the
override-appending block out of any early-return path so overrides are appended
regardless of getDatastoreList outcome.
Comment on lines
+350
to
+353
| if len(datastores) > 0 { | ||
| datastores = clonePBSDatastores(datastores) | ||
| assignUniquePBSDatastoreOutputKeys(datastores) | ||
| } |
There was a problem hiding this comment.
Use the assigned datastore output key for status snapshots.
assignUniquePBSDatastoreOutputKeys(datastores) is effectively ignored here because the status filename is still derived from ds.pathKey(). If two entries collapse to the same path key, the later datastore_*_status.json overwrites the earlier one, which drops datastore metadata and defeats the collision handling this PR adds.
Also applies to: 400-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/backup/collector_pbs.go` around lines 350 - 353, The status snapshot
filename is still generated from ds.pathKey(), so
assignUniquePBSDatastoreOutputKeys(datastores) has no effect and later entries
overwrite earlier ones; update the status snapshot generation (where you
currently call ds.pathKey()) to use the datastore's assigned output key set by
assignUniquePBSDatastoreOutputKeys (the field or accessor populated by that
function, e.g., datastore.OutputKey or datastore.outputKey()) when creating the
datastore_*_status.json filenames so collisions are avoided; apply the same
change to the other occurrences mentioned (lines ~400-403) and keep
clonePBSDatastores(datastores) + assignUniquePBSDatastoreOutputKeys(datastores)
as-is.
Comment on lines
+79
to
+84
| availableCategories, decisionInfo, err := AnalyzeRestoreArchive(prepared.ArchivePath, logger) | ||
| if err != nil { | ||
| logger.Warning("Could not analyze categories: %v", err) | ||
| logger.Info("Falling back to full restore mode") | ||
| return runFullRestoreWithUI(ctx, ui, candidate, prepared, destRoot, logger, cfg.DryRun) | ||
| } |
There was a problem hiding this comment.
Don't bypass compatibility and cluster safeguards on analysis failure.
Line 83 returns runFullRestoreWithUI immediately. That path never calls ValidateCompatibility and skips the later decision-driven cluster/PBS guard flow in this function, so a partially unreadable archive can fall straight into a raw full restore on the wrong system or with live services still running. Please keep the best-effort compatibility and cluster checks before offering this fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/orchestrator/restore_workflow_ui.go` around lines 79 - 84, When
AnalyzeRestoreArchive returns an error, do not return early into
runFullRestoreWithUI; instead log the error (as already done) but continue
through the existing compatibility and cluster/PBS guard flow by invoking
ValidateCompatibility and the decision-driven checks before any fallback. Modify
the block around AnalyzeRestoreArchive so that on error you set
availableCategories/decisionInfo to nil or an appropriate default, then proceed
to call ValidateCompatibility and the later guard/decision logic, and only call
runFullRestoreWithUI as the final fallback after those checks pass or explicitly
allow it.
update runLimited to re-check ctx.Err() after effectiveTimeout() avoid calling run() when effectiveTimeout() returns 0 because the context deadline already elapsed preserve the no-timeout path only for genuinely disabled timeouts add a regression test for the expired-deadline edge case in internal/safefs
When restore archive analysis fails in the UI workflow, do not jump immediately to full restore. Build a best-effort RestoreDecisionInfo from the manifest, run the existing compatibility check first, and only then fall back to runFullRestoreWithUI. Also add a regression test covering the analysis-error path to ensure the compatibility warning is still shown before the full restore fallback executes.
Reject oversized backup_metadata.txt entries during restore archive inspection by reading through a fixed limit before parsing. This prevents unbounded memory allocation from crafted archives and ensures truncated metadata caused by limit overflow is treated as invalid rather than partially trusted. Add regression tests covering oversized metadata rejection and the public restore analysis path.
Validate symlink targets during staged network apply before recreating them under the destination tree. Reuse the existing path-security resolver to ensure targets stay within the allowed destination root, rewrite safe absolute targets to relative links, and reject absolute or relative targets that escape the subtree. Add regression tests for safe rewrites and escaping symlink targets.
Validate symlink targets in syncDirExact before recreating them under the destination subtree. Reuse the existing path-security resolver to ensure firewall and SDN staged symlinks stay within the allowed destination root, rewrite safe absolute targets to relative links, and reject absolute or relative targets that escape the subtree. Add regression tests for safe rewrites and escaping symlink targets.
Verify and fix parseSystemTypeString in compatibility.go. The parser now recognizes space-separated and full-name variants such as "Proxmox VE", "Proxmox Backup", and "Proxmox Backup Server", in addition to the existing acronym and hyphenated forms. Add regression tests to ensure these values map correctly to SystemTypePVE and SystemTypePBS.
Validate and normalize manifest and sidecar SHA256 values before accepting local or rclone raw backup candidates. Reject candidates when the manifest checksum is malformed, the .sha256 sidecar cannot be parsed, or both checksum sources disagree. Persist the normalized checksum and its source on the candidate so downstream staged integrity verification only works with pre-validated values. Add coverage for local and rclone discovery, malformed and conflicting checksums, and reuse of the candidate integrity expectation during staged verification.
Adds a regression test for collectPBSCommands() covering colliding PBS datastore output keys. The test verifies that assignUniquePBSDatastoreOutputKeys() disambiguates colliding datastore status filenames and that two distinct datastore_*_status.json files are written with the correct datastore-specific content.
Make PBS datastore enumeration best-effort in getDatastoreList so configured PBSDatastorePaths overrides are still appended when proxmox-backup-manager is missing, the datastore list command fails, or its JSON output is invalid. Preserve context cancellation as a fatal error, keep unique output key assignment, and add regression tests for missing CLI, command failure, and parse failure fallback cases.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores