Skip to content

fix(backup): contain restore file paths within the handler root#9760

Open
alhudz wants to merge 1 commit into
dgraph-io:mainfrom
alhudz:backup-handler-path-containment
Open

fix(backup): contain restore file paths within the handler root#9760
alhudz wants to merge 1 commit into
dgraph-io:mainfrom
alhudz:backup-handler-path-containment

Conversation

@alhudz

@alhudz alhudz commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Description

The path field of a backup manifest is read back from the backup location (readMasterManifest) and used as-is during restore.

  1. restore_map.go joins manifest.Path with the backup file name and streams it through fileHandler.Stream -> os.Open(filepath.Join(rootDir, prefix, path)).
  2. getManifestsToRestore joins the same manifest.Path for FileExists, and the s3/minio handler builds object keys the same way in getObjectPath.

JoinPath/getObjectPath do not contain the relative path, so a manifest left in the backup target with path set to ../../.. steers those reads outside the handler root. The fix sits in the handlers because that is the single choke point every backup/restore file op goes through.

Repro: worker/backup_handler_test.go (fails before, passes after).
Expected: the resolved path stays under the handler root.
Actual: JoinPath("../../../../etc/passwd") returns /etc/passwd; getObjectPath returns ../../etc/passwd.
Fix: cleanRelPath anchors the path at the root and drops a leading separator / .. before the join; JoinPath (file + s3) and getObjectPath all route through it.

Checklist

  • The PR title follows the
    Conventional Commits syntax, leading
    with fix:, feat:, chore:, ci:, etc.
  • Code compiles correctly and linting (via trunk) passes locally
  • Tests added for new functionality, or regression tests for bug fixes added as applicable

@alhudz alhudz requested a review from a team as a code owner June 20, 2026 15:16
@alhudz

alhudz commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

gentle ping

@shiva-istari shiva-istari force-pushed the backup-handler-path-containment branch from 992162b to cf75bad Compare July 6, 2026 12:08

@shiva-istari shiva-istari left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, @alhudz !

I went through the changes and they look good to me. It would be nice to add a table-driven test for cleanRelPath covering a few edge cases like "", ".", a/../b, absolute paths, and paths with trailing or duplicate separators. I think that would make future regressions less likely.

In addition, an integration test would be valuable to verify the end-to-end behavior and help prevent regressions in the future.

Other than that, LGTM. Happy to see this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants