Refactor restic mount directory handling to use <working-dir>/<cluster>/mount as default path to prevent permission problem; update related tests and documentation.

[caffeinated92]

This pull request updates the default location and configuration logic for restic FUSE mount directories to improve security and flexibility. The default base directory for restic mounts is now set to <working-dir>/<cluster>/mount instead of /mnt/restic, reducing the risk of permission issues. The logic for resolving relative and absolute paths for mount directories has been improved throughout the codebase, and documentation and help text have been updated to reflect these changes.

Restic mount directory logic and configuration:

• Changed the default restic mount base directory from /mnt/restic to <working-dir>/<cluster>/mount in both code and configuration, including updating the constant and all usages. [1] [2]
• Improved logic for resolving BackupResticMountDir: relative paths are now resolved under the new default base, and absolute paths are respected. Added informative logging when resolving relative paths. [1] [2]
• Updated validation and enforcement logic to ensure that default and relative mount directories do not escape the intended base directory. [1] [2]

Documentation and user interface updates:

• Updated configuration comments, CLI flag descriptions, OpenAPI docs, and Swagger JSON to describe the new default and path resolution behavior for backup-restic-mount-dir. [1] [2] [3] [4]
• Enhanced the dashboard UI with improved help text and an info button explaining the new mount directory behavior. [1] [2]

Test updates:

• Updated tests to use the new default mount directory paths, ensuring consistency with the revised logic. [1] [2] [3]

[claude]

Code Review: Restic Mount Directory Refactoring (PR #1343)

Summary

This PR refactors the restic mount directory handling to use a safer default path (<working-dir>/<cluster>/mount) instead of /mnt/restic, with improved path resolution logic and comprehensive documentation updates. Overall, the approach is sound and addresses legitimate permission and security concerns.

---

✅ Strengths

1. Security Improvement: Moving from system-wide /mnt/restic to per-cluster working directories reduces privilege escalation risks and permission issues.

2. Path Resolution Logic: The implementation correctly handles both absolute and relative paths:

   • Absolute paths are used as-is
   • Relative paths are resolved under the working directory base
   • Informative logging aids debugging

3. Comprehensive Updates: All affected areas updated consistently:

   • Core logic in cluster_bck.go
   • API handler in server/api_restic.go
   • Configuration documentation
   • OpenAPI/Swagger specs
   • Dashboard UI with improved help text
   • Test expectations

4. Escape Detection: Path traversal validation using filepath.Rel() correctly prevents .. sequences from escaping the base directory.

5. Logging: Added informative log messages when resolving relative paths help operators understand configuration behavior.

---

⚠️ Issues & Observations

1. Code Duplication (Medium Priority)

Path resolution logic is duplicated in two locations:

• cluster/cluster_bck.go:448-463 (resolveResticMountDirFromConfig)
• server/api_restic.go:121-134 (handlerMuxResticMountToggle)

Impact: Maintenance burden; if one location is updated, the other could be missed.

Recommendation: Extract into a shared helper function:

func (cluster *Cluster) resolveResticMountPath(configPath string) (string, string) {
    baseDir := filepath.Join(cluster.WorkingDir, "mount")
    // ... shared logic ...
    return resolvedPath, source
}

---

2. Missing Cluster Name in Default Path (Critical)

In cluster_bck.go:449, the default mountDir is set to baseDir without appending the cluster name:

baseDir := filepath.Join(cluster.WorkingDir, resticDefaultMountSubdir)
mountDir := baseDir  // ← Missing cluster name!

This differs from the original behavior which was /mnt/restic/{clustername}.

Impact: Multiple clusters sharing the same working directory will collide in mount paths.

Expected behavior (based on test updates):

mountDir := filepath.Join(baseDir, clusterName)  // e.g., /var/lib/replication-manager/cluster1/mount

Note: The test updates show the expected path includes the cluster name, which contradicts the code.

---

3. Variable Shadowing in api_restic.go (Minor)

Line 127 in server/api_restic.go:

baseDir := filepath.Join(mycluster.WorkingDir, "mount")  // shadows line 121

While not a functional bug (scope is clear), this creates confusion since resticMountBaseDir (line 121) appears to serve the same purpose.

Recommendation: Reuse resticMountBaseDir instead of shadowing with baseDir.

---

4. Inconsistent Default Between resolveResticMountDirFromConfig and API Handler

• cluster_bck.go:449: Uses only baseDir
• api_restic.go:122: Uses only resticMountBaseDir (equivalent to baseDir)

Neither appends the cluster name, but test expectations suggest they should. This inconsistency needs clarification.

---

5. Incomplete Path Sanitization in api_restic.go

Unlike cluster_bck.go:466 which calls sanitizeResticMountDir(), the API handler doesn't apply the same sanitization for the constructed path. This means:

• No filepath.Clean() normalization
• No validation that resolved paths don't escape the base

Recommendation: Apply sanitization before using mountDir.

---

6. String Constant for "mount" Subdirectory

The hardcoded string "mount" appears in multiple places:

• server/api_restic.go:121
• server/api_restic.go:127
• Should use the resticDefaultMountSubdir constant from cluster_bck.go

Recommendation: Export the constant or move to a shared location.

---

⚠️ Potential Edge Cases

1. Relative Path with ".." Components:

   • Input: ../../../etc with relative path handling enabled
   • Current validation in cluster_bck.go:473-478 should catch this, but only when enforceDefaultBase && source == "default"
   • The API handler doesn't apply this validation

2. Symlink Traversal:

   • filepath.Rel() doesn't follow symlinks
   • Escaped paths could be accessed through symlinks outside the base
   • Consider documenting this limitation

3. Working Directory Changes:

   • Path is resolved once at startup
   • If WorkingDir changes at runtime (unlikely but possible), mounts could end up in unexpected locations

---

✅ Test Coverage

Tests updated appropriately to reflect new default paths:

• cluster/srv_job_restic_test.go: Updated expected paths to /var/lib/replication-manager/cluster1/mount
• utils/backupmgr/restic_test.go: Updated mount state persistence tests

However: No new tests added for:

• Relative path resolution with various inputs
• Path traversal prevention
• API handler mount construction

---

📋 Documentation Updates

✅ Configuration help text updated
✅ CLI flag description updated
✅ OpenAPI/Swagger documentation updated
✅ React dashboard help text and info button added

---

🔍 Security Review

Positive:

• Escape sequence detection prevents .. traversal
• Absolute paths bypass the working directory isolation (intentional flexibility)
• Validation enforced when both mount and target dirs use defaults

Concerns:

• API handler bypasses some security checks (missing sanitization)
• Symlink traversal could bypass path restrictions
• Consider adding a backup-restic-mount-dir-restricted flag to enforce base confinement even for absolute paths

---

📝 Recommendations

1. Extract shared path resolution logic to eliminate duplication
2. Fix default mountDir to include cluster name (per test expectations)
3. Add cluster name to API handler path construction
4. Apply sanitization in API handler matching cluster_bck.go logic
5. Consolidate hardcoded strings by exporting mount subdir constant
6. Add unit tests for path resolution edge cases
7. Fix variable shadowing in api_restic.go
8. Document symlink behavior and any security limitations

---

Verdict

Request Changes - While the overall approach is sound and addresses real security concerns, the implementation has critical issues (missing cluster name in default path, code duplication, inconsistent sanitization) that should be resolved before merging. The refactoring improves the codebase, but these gaps need to be addressed to ensure correctness and maintainability.

The changes would benefit from the improvements listed above before approval.

[caffeinated92]

@claude review my pull request

[claude]

Claude finished @caffeinated92's task in 1m 15s —— View job

---

Code Review: Restic Mount Directory Refactoring

✅ Overall Assessment

This PR improves security and flexibility by changing the default restic mount location from /mnt/restic to <working-dir>/<cluster>/mount. The approach is sound, but there are critical and medium-priority issues that must be addressed before merging.

---

🔴 Critical Issues

1. Missing Cluster Name in Default Mount Path

Severity: CRITICAL - Creates data collision risk between clusters

In cluster/cluster_bck.go:449, the default mount directory is missing the cluster name:

baseDir := filepath.Join(cluster.WorkingDir, resticDefaultMountSubdir)
mountDir := baseDir  // ← BUG: No cluster name appended!

Expected behavior (per test expectations at cluster/srv_job_restic_test.go:203):

Expected: /var/lib/replication-manager/cluster1/mount
Actual:   /var/lib/replication-manager/mount

Impact:

• Multiple clusters sharing the same WorkingDir will collide on the same mount path
• Previous cluster's FUSE mount could interfere with current cluster
• Tests show /var/lib/replication-manager/cluster1/mount as expected, contradicting the code

Fix: Add cluster name to default path:

baseDir := filepath.Join(cluster.WorkingDir, cluster.GetClusterName(), resticDefaultMountSubdir)
mountDir := baseDir

---

2. Variable clusterName Extracted but Unused

Severity: CRITICAL - Code smell indicating incomplete logic

In cluster/cluster_bck.go:443-449:

clusterName := strings.TrimSpace(cluster.GetClusterName())
if clusterName == "" {
    return "", "", fmt.Errorf("cluster name is empty")
}

baseDir := filepath.Join(cluster.WorkingDir, resticDefaultMountSubdir)
mountDir := baseDir  // ← clusterName validated but never used

The code validates clusterName but doesn't use it for the default path, suggesting incomplete refactoring.

---

⚠️ High-Priority Issues

3. Code Duplication - Path Resolution Logic

Severity: HIGH - Maintenance burden and inconsistency risk

Path resolution logic appears in two locations:

• cluster/cluster_bck.go:448-462 (resolveResticMountDirFromConfig)
• server/api_restic.go:120-124 (handlerMuxResticMountToggle)

Both construct baseDir and resolve mount directories, but independently:

// cluster_bck.go:448-449
baseDir := filepath.Join(cluster.WorkingDir, resticDefaultMountSubdir)
mountDir := baseDir

// api_restic.go:120-124 (implicit in ResolveResticMountDirFromConfigStrict)
// Uses same logic via function call - better pattern but logic not shared

Recommendation: The API handler correctly delegates to ResolveResticMountDirFromConfigStrict(), but documenting the public vs private function distinction would clarify the pattern.

---

4. Inconsistent Sanitization Between Code Paths

Severity: MEDIUM - Potential security gap

In cluster/cluster_bck.go:601-612, the sanitizeAndValidateResticMountOptions function applies validation:

if meta.targetDirSource == "default" && meta.mountDirSource == "default" {
    base := filepath.Clean(filepath.Join(cluster.WorkingDir, resticDefaultMountSubdir))
    rel, relErr := filepath.Rel(base, mountOpt.TargetDir)
    // ... escape detection ...
}

However, the API handler at server/api_restic.go:120 calls ResolveResticMountDirFromConfigStrict() which applies strict validation, but the constructed mountOpt.TargetDir later gets validated through sanitizeAndValidateResticMountOptions. This creates two validation points - which is good for defense in depth, but should be documented.

---

🟡 Medium-Priority Issues

5. Hardcoded String Duplication

Severity: MEDIUM - Code maintainability

The string "mount" appears multiple times as a literal instead of using the exported constant:

• Line 121 in server/api_restic.go: Could use resticDefaultMountSubdir
• However: The constant is unexported (resticDefaultMountSubdir), so API handler can't reference it

Recommendation: Export ResticDefaultMountSubdir or create a getter function for this constant.

---

6. Path Escaping Detection - Symlink Limitation

Severity: MEDIUM - Security consideration

In cluster/cluster_bck.go:472, the code uses filepath.Rel():

rel, relErr := filepath.Rel(base, mountDir)

Limitation: filepath.Rel() doesn't follow symlinks. If an attacker creates symlinks under baseDir pointing outside, they could escape the intended directory.

Mitigation: Consider documenting this limitation or using filepath.EvalSymlinks() for trusted paths:

// Add comment explaining symlink behavior
// filepath.Rel does not resolve symlinks; callers should avoid untrusted bases.

✅ Good: The code already has this comment at line 471.

---

7. API Handler Path Construction - Potential Issue

Severity: LOW - Edge case

In server/api_restic.go:137, if mountOpt.TargetDir is empty after config resolution:

if mountOpt.TargetDir == "" {
    mountOpt.TargetDir = mountDir  // Fallback is correct
}

This is correct, but calling ResolveResticMountDirFromConfigStrict() already validates that mountDir is non-empty (line 120-124). The fallback guard is defensive and good practice.

---

✅ Positive Findings

1. Security Improvement: Moving from /mnt/restic (system-wide) to per-cluster directories reduces privilege escalation risks
2. Path Traversal Prevention: Escape detection using filepath.Rel() with .. checking is correct
3. Comprehensive Logging: Informative logs help operators debug configuration issues
4. Test Coverage: Tests updated appropriately to reflect new paths
5. API Sanitization: Dual validation points (strict config + mount options validation) provide defense in depth

---

📋 Recommendations (Priority Order)

1. CRITICAL: Fix default mount path to include cluster name:

   baseDir := filepath.Join(cluster.WorkingDir, cluster.GetClusterName(), resticDefaultMountSubdir)

2. HIGH: Verify multi-cluster scenarios with shared WorkingDir (edge case but important)

3. MEDIUM: Export ResticDefaultMountSubdir constant or create getter for API usage:

   // In cluster_bck.go
   const ResticDefaultMountSubdir = "mount"  // exported

4. MEDIUM: Add tests for:

   • Multi-cluster collisions with shared WorkingDir
   • Path traversal with various .. sequences
   • Symlink escaping attempts (document as unsupported)

5. LOW: Document symlink behavior in code comments (already partially done)

---

🧪 Test Status

✅ Updated appropriately:

• cluster/srv_job_restic_test.go: Mount path expectations updated to new format
• utils/backupmgr/restic_test.go: Mount state persistence tests updated
• cluster/cluster_bck_test.go: New tests for path validation added

⚠️ Missing tests:

• Multi-cluster collision scenarios
• API handler path construction edge cases

---

Verdict

REQUEST CHANGES - The PR has good intentions and addresses real security concerns, but the missing cluster name in the default mount path is a critical bug that will cause data collisions between clusters. This must be fixed before merging.

Once the critical issue is resolved and multi-cluster scenarios are verified, this PR will be a solid improvement to the codebase.

---

View PR branch

[caffeinated92]

@claude review my pull request

[claude]

Claude finished @caffeinated92's task in 44s —— View job

---

Code Review: Restic Mount Directory Refactoring (PR #1343)

Summary

This PR successfully refactors restic mount directory handling from the system-wide /mnt/restic to a per-cluster working directory approach (<working-dir>/<cluster>/mount). The implementation appears to be correct and addresses the critical issues from the previous review.

Key finding: The WorkingDir already includes the cluster name (set at cluster/cluster.go:416), so the default path logic is sound: baseDir = WorkingDir/mount = /var/lib/replication-manager/{cluster-name}/mount.

---

✅ Strengths

1. Cluster Name Handling - FIXED: The critical issue from the previous review has been addressed. The comment at cluster_bck.go:444 clearly documents that cluster.WorkingDir already includes the cluster name, eliminating the collision risk between multiple clusters.

2. Path Resolution Architecture: The refactored approach uses two well-scoped functions:

   • resolveResticMountDirFromConfig() (private): Core resolution logic with flexible options
   • ResolveResticMountDirFromConfig(): Permissive API for general use
   • ResolveResticMountDirFromConfigStrict(): Strict validation for API endpoints

   This layered approach is clean and follows the principle of least privilege.

3. Escape Detection: Path traversal prevention is comprehensive:

   • Validates that resolved paths don't escape the base using filepath.Rel()
   • Checks for .., ., empty strings
   • Properly handles ..{separator} patterns
   • Includes helpful comment about symlink limitations

4. Logging and Debugging: Informative logs when resolving relative paths make it easy to troubleshoot configuration issues.

5. Test Coverage: Tests updated appropriately:

   • srv_job_restic_test.go: Mount path expectations now match /var/lib/replication-manager/cluster1/mount
   • cluster_bck_test.go: Added test cases for path validation and escape detection
   • restic_test.go: Updated snapshot metadata tests

6. Documentation: Updated across the board:

   • CLI flag descriptions
   • OpenAPI/Swagger specs
   • Dashboard UI with improved help text
   • Configuration comments

---

⚠️ Observations & Minor Issues

1. Code Duplication Still Present (Medium Priority)

Status: Acceptable but worth noting

While the previous review flagged duplication between cluster_bck.go and api_restic.go, this has been properly mitigated:

• api_restic.go:120 correctly delegates to ResolveResticMountDirFromConfigStrict()
• Not duplicating validation logic, just calling the public method
• Good design pattern for API endpoints

Minor suggestion: Add a comment above line 120 explaining why Strict variant is used:

// Use strict validation for API mount operations to enforce path safety
mountDir, _, err := mycluster.ResolveResticMountDirFromConfigStrict()

---

2. Unused ClusterName Variable (Line 113 - Minor)

Status: Code smell but not a bug

clusterName := strings.TrimSpace(mycluster.GetClusterName())
if clusterName == "" {
    http.Error(w, "Cluster name is empty", http.StatusInternalServerError)
    return
}

The clusterName variable is extracted but not used beyond validation. This is actually good defensive programming (early validation), but the variable could be removed since mycluster.GetClusterName() is called again indirectly through path resolution.

Suggestion: This is acceptable as-is; the early validation is defensive and clear.

---

3. Hardcoded Mount Subdirectory String (Minor)

Status: Acceptable

The string "mount" appears as a literal in the code. The constant resticDefaultMountSubdir is unexported, which is fine for this use case since it's only used within the cluster package.

No action needed - the current approach is clean.

---

4. Mount Options Construction (Line 137 - Minor)

Status: Good defensive programming

if mountOpt.TargetDir == "" {
    mountOpt.TargetDir = mountDir
}

This fallback is appropriate as defensive programming, though ResolveResticMountDirFromConfigStrict() already validates that mountDir is non-empty. The guard is good practice.

---

🔒 Security Review

Positive:

• ✅ Path traversal prevention with filepath.Rel() checking
• ✅ Escape sequence detection prevents .. traversal
• ✅ Absolute paths remain flexible (intentional design)
• ✅ Dual validation points (config resolution + mount options validation)
• ✅ Early validation of cluster name

Considerations (documented, not issues):

• Symlink traversal noted in comments (acceptable limitation)
• Absolute paths can bypass working directory confinement (intentional for flexibility)

---

🧪 Test Analysis

Coverage appears comprehensive:

• ✅ Path resolution with defaults
• ✅ Relative path handling
• ✅ Escape detection tests
• ✅ Snapshot metadata persistence
• ✅ Mount state tests

Tests correctly reflect new paths: All references updated to /var/lib/replication-manager/{cluster}/mount format.

---

📊 Change Impact Analysis

File	Impact	Status
cluster/cluster_bck.go	Core path resolution logic	✅ Correct
server/api_restic.go	API endpoint mount handling	✅ Uses strict validation
cluster/cluster_bck_test.go	Added validation tests	✅ Good coverage
server/server.go	Flag description update	✅ Updated
config/config.go	Config field addition	✅ Appropriate
ResticMountSettings.jsx	UI help text	✅ Improved
docs/swagger.json	OpenAPI documentation	✅ Updated

---

✅ Verdict

APPROVED - This PR is ready to merge.

The implementation correctly addresses the security concerns from moving off /mnt/restic, with proper path resolution, escape detection, and a well-layered API design. The critical issue from the previous review (missing cluster name) has been resolved through the clear understanding that WorkingDir already includes the cluster name.

Key improvements:

1. Reduced permission issues by using per-cluster directories
2. Proper path safety validation prevents directory traversal
3. Clean API design with permissive and strict resolution variants
4. Comprehensive test coverage and documentation updates

---

View PR branch