Skip to content

feat: Add import and export functionality in surge#362

Open
junaid2005p wants to merge 11 commits intomainfrom
export
Open

feat: Add import and export functionality in surge#362
junaid2005p wants to merge 11 commits intomainfrom
export

Conversation

@junaid2005p
Copy link
Copy Markdown
Collaborator

@junaid2005p junaid2005p commented Apr 12, 2026

Greptile Summary

This PR adds import/export (backup/restore) functionality to Surge via new CLI commands (export, import), a TUI Data Transfer panel, HTTP API endpoints (/data/export, /data/import/preview, /data/import/apply), and a backup package with a staged two-phase preview→apply workflow.

The three major security issues raised in the previous review (predictable session ID, missing upload size limit, relative path traversal) are all addressed in this revision.

Confidence Score: 4/5

Safe to merge for most paths; Replace-mode import has a known data-integrity ordering issue from the prior review that remains unresolved.

All three prior security findings (path traversal, upload limit, predictable session ID) are fixed. New findings are P2 only (memory efficiency in verifyManifestFiles, silent marshal error, missing LeavePaused TUI toggle). Score stays at 4 because the non-atomic Replace ordering (clear DB → save settings → restore) is still present and represents a real data-loss risk for Replace-mode imports that abort midway.

internal/backup/backup.go — ApplyImport ordering in Replace mode; verifyManifestFiles memory usage

Important Files Changed

Filename Overview
internal/backup/backup.go Core export/import logic; verifyManifestFiles reads entire zip entries into memory via io.ReadAll for SHA-256, which is wasteful for large partial files; settings are saved before downloads/logs are fully restored in Replace mode (flagged in prior review, still present)
cmd/transfer_api.go HTTP endpoints for export/import with size cap, crypto-random session IDs, and staged session store; json.Marshal error is silently discarded in the export manifest header
internal/backup/path.go Path rebase/sanitization logic with containment checks via pathWithinRoot; previously flagged path traversal is fixed; test coverage added in path_test.go
internal/core/transfer.go Clean TransferService interface with local and remote implementations; remote client correctly ignores src for apply calls; 5-minute HTTP timeout is a reasonable default
internal/tui/update_transfer.go TUI export/import command handlers; LeavePaused option not surfaced despite being supported by ExportOptions; all async commands use context.Background() so they can't be cancelled if TUI exits
cmd/transfer_utils.go CLI helpers for resolve, export, preview, and apply bundle; path cleaning applied consistently; session ID plumbed correctly for remote vs local paths
cmd/export.go New export CLI command; all flags wired correctly; JSON output via shared printJSON helper
cmd/import.go New import CLI command with preview-then-apply flow; SessionID plumbed correctly for remote sessions; options copied correctly from preview to apply
internal/backup/backup_test.go Good test coverage for partial restore, log restore, replace/merge modes, and path escape rejection
Prompt To Fix All With AI
This is a comment left during a code review.
Path: internal/backup/backup.go
Line: 732-748

Comment:
**`verifyManifestFiles` loads full zip entries into memory**

Each entry is read with `io.ReadAll` before being hashed, so a bundle with a large partial file (up to the 512 MB cap) allocates that entire file in RAM. A streaming approach is more efficient:

```go
func verifyManifestFiles(reader *zip.Reader, files []ManifestFile) error {
    for _, mf := range files {
        var found *zip.File
        for _, f := range reader.File {
            if filepath.ToSlash(f.Name) == filepath.ToSlash(mf.Path) {
                found = f
                break
            }
        }
        if found == nil {
            return fmt.Errorf("bundle entry %s not found", mf.Path)
        }
        rc, err := found.Open()
        if err != nil {
            return err
        }
        h := sha256.New()
        size, err := io.Copy(h, rc)
        _ = rc.Close()
        if err != nil {
            return err
        }
        if hex.EncodeToString(h.Sum(nil)) != mf.SHA256 {
            return fmt.Errorf("checksum mismatch for %s", mf.Path)
        }
        if size != mf.Size {
            return fmt.Errorf("size mismatch for %s", mf.Path)
        }
    }
    return nil
}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: cmd/transfer_api.go
Line: 87

Comment:
**Silent marshal error discards manifest header**

If `json.Marshal(manifest)` fails, `manifestBytes` is `nil`, the `X-Surge-Manifest` header is sent as an empty string, and the remote client silently returns `&backup.Manifest{}`. Log or handle the error so callers aren't left with a structureless manifest on unexpected failures.

```suggestion
		manifestBytes, err := json.Marshal(manifest)
		if err != nil {
			http.Error(w, err.Error(), http.StatusInternalServerError)
			return
		}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: internal/tui/update_transfer.go
Line: 28-31

Comment:
**`LeavePaused` not exposed in TUI export**

`ExportOptions.LeavePaused` controls whether paused downloads are automatically resumed after export, and is available in the CLI via `--leave-paused`. The TUI omits this toggle, so interactive users have no way to keep downloads paused. Consider adding a `m.transferLeavePaused` state field alongside the other toggles and wiring it here.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (3): Last reviewed commit: "fix: fixed COdeQL issues" | Re-trigger Greptile

@junaid2005p junaid2005p changed the title feat(backup): add portable backup bundle for import/export feat: Add import and export functionality in surge Apr 12, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 12, 2026

Binary Size Analysis

⚠️ Size Increased

Version Human Readable Raw Bytes
Main 18.98 MB 19898660
PR 19.42 MB 20365604
Difference 456.00 KB 466944

Comment on lines +218 to +237
if opts.Replace {
if err := clearExistingState(); err != nil {
return nil, err
}
}

settingsSaved := false
if payload.Settings != nil {
if payload.Settings.General.DefaultDownloadDir != "" && rootDir != "" {
payload.Settings.General.DefaultDownloadDir = utils.EnsureAbsPath(rootDir)
}
if err := config.SaveSettings(payload.Settings); err != nil {
return nil, err
}
settingsSaved = true
}

logsRestored, err := restoreLogFiles(opened.Reader, manifest, opts.Replace)
if err != nil {
return nil, err
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.

P1 Non-atomic replace leaves Surge in a partially-migrated state

In Replace mode, clearExistingState() wipes the download database (line 219), then settings are written to disk (line 229), and only then are log files and downloads restored. If restoreLogFiles or any iteration of the download-import loop returns an error, the function exits early — leaving the user with a cleared history and new settings applied but zero downloads imported.

Since the database clear is irreversible and happens before any restoration, a transient error (e.g. disk full during partial-file restore) creates unrecoverable data loss for the user. Moving SaveSettings to after all downloads and logs are successfully restored narrows this window significantly:

// Move settings save to after successful restoration
logsRestored, err := restoreLogFiles(opened.Reader, manifest, opts.Replace)
if err != nil {
    return nil, err
}
// ... import loop ...
if payload.Settings != nil {
    // apply settings only after all other steps succeeded
    if err := config.SaveSettings(payload.Settings); err != nil {
        return nil, err
    }
    settingsSaved = true
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/backup/backup.go
Line: 218-237

Comment:
**Non-atomic replace leaves Surge in a partially-migrated state**

In Replace mode, `clearExistingState()` wipes the download database (line 219), then settings are written to disk (line 229), and only then are log files and downloads restored. If `restoreLogFiles` or any iteration of the download-import loop returns an error, the function exits early — leaving the user with a **cleared history and new settings applied but zero downloads imported**.

Since the database clear is irreversible and happens before any restoration, a transient error (e.g. disk full during partial-file restore) creates unrecoverable data loss for the user. Moving `SaveSettings` to after all downloads and logs are successfully restored narrows this window significantly:

```go
// Move settings save to after successful restoration
logsRestored, err := restoreLogFiles(opened.Reader, manifest, opts.Replace)
if err != nil {
    return nil, err
}
// ... import loop ...
if payload.Settings != nil {
    // apply settings only after all other steps succeeded
    if err := config.SaveSettings(payload.Settings); err != nil {
        return nil, err
    }
    settingsSaved = true
}
```

How can I resolve this? If you propose a fix, please make it concise.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants