[fix] skip symlinks in a backup and say which ones - #140
Merged
Conversation
A link in the data dir was stored as a link entry while its contents were never included — Walk does not follow links — so pointing projects/ at another disk produced a backup that looked complete and omitted the projects. Silence was the worst part: nothing said so until a restore. Links are now skipped and named in the CLI output, alongside the total size of what went in, so an unexpectedly large or unexpectedly small backup is visible when it is taken rather than when it is needed. Restore refuses any symlink entry. Backups no longer produce them, so one in an archive is old or crafted; a symlink is a write path out of the data dir and that class of bug has already appeared here once. Not creating them is cheaper than jailing them correctly. Hard links are deliberately untouched: a hard link IS the file, so its data belongs in the backup like any other file's.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes misleading backups when the data dir contains symlinks by skipping symlink entries during backup creation, reporting the skipped paths (so operators know what was omitted), and hardening restore to refuse any symlink entries in an archive.
Changes:
- Add
backup.Reportand changebackup.Createto return(*Report, error)with total included-bytes and skipped symlink paths. - Skip symlinks during archive creation and refuse symlink entries during restore; add unit/pentests to lock this down.
- Update CLI output, deployment docs, and changelog to reflect the new behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/backup/backup.go | Introduces Report, skips symlinks during backup, refuses symlink entries on restore, and changes Create return type. |
| internal/backup/backup_test.go | Updates callers for new Create signature and adds regression + pentest coverage for symlink handling. |
| docs/deployment.md | Updates operator-facing documentation about symlink backup/restore behavior. |
| cmd/dockercmd/main.go | Prints backup size + skipped symlink paths and adds a byte-size formatter helper. |
| CHANGELOG.md | Records the user-facing backup/restore behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
524
to
542
| // loadOrCreateJWTSecret returns a persistent signing secret, generating one on | ||
| // first run. Keeping it stable means sessions survive restarts. | ||
| // humanBytes renders a size the way an operator reads one. | ||
| func humanBytes(n int64) string { | ||
| const unit = 1024 | ||
| if n < unit { | ||
| return fmt.Sprintf("%d B", n) | ||
| } | ||
| div, exp := int64(unit), 0 | ||
| for n/div >= unit && exp < 3 { | ||
| div *= unit | ||
| exp++ | ||
| } | ||
| return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGT"[exp]) | ||
| } | ||
|
|
||
| func loadOrCreateJWTSecret(ctx context.Context, st *store.Store) ([]byte, error) { | ||
| return loadOrCreateSecret(ctx, st, "jwt_secret") | ||
| } |
Comment on lines
344
to
346
| instance by accident. Archive entries are jailed to the data dir, so a tampered | ||
| backup can't write elsewhere on the filesystem — including through a **symlink**, | ||
| whose target is checked as well. |
Comment on lines
101
to
+105
| // 1. Consistent database snapshot. | ||
| dbSnapshot := filepath.Join(tmpDir, dbFileName) | ||
| if db != nil { | ||
| if err := db.BackupTo(dbSnapshot); err != nil { | ||
| return fmt.Errorf("snapshot database: %w", err) | ||
| return nil, fmt.Errorf("snapshot database: %w", err) |
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
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
Your instinct on this was right, and the current behaviour was worse than either
of us said.
A symlink in the data dir was stored as a link entry, while its contents were
never included —
filepath.Walkdoes not follow links. So someone who pointedprojects/at a bigger disk got a backup that looked complete and silentlyomitted their projects. The failure surfaces at restore time, which is the worst
possible moment to learn it.
Now: links are skipped and named in the output, next to the total size of
what did go in.
The size is there for the other half of your point: if someone drops something
enormous in the data dir, that shows up when the backup is taken rather than when
the disk fills.
Restore refuses any symlink entry. Backups no longer produce them, so one in
an archive is either old or crafted — and a symlink is a write path out of the
data dir, the class of bug that already appeared here once (#127). Not creating
them at all is cheaper than jailing them correctly, which is the same reasoning
that deleted the extra check in #127.
Hard links are deliberately untouched. A hard link is the file — same
inode, another name — so its data belongs in the backup like any other file's.
Skipping them would omit real data, which is the bug this PR fixes, not a
stricter version of it.
Type of change
Checklist
go test -short ./...andgo vet ./...passgofmtgate is clean (gofmt -l $(git ls-files '*.go')after staging)web/srcchange)web/dist— N/A (noweb/srcchange)docs/and added aCHANGELOG.mdentry for user-facing changesNotes for reviewers
API change:
backup.Createnow returns(*Report, error). The report carriesthe skipped links and the byte count — the CLI is the only caller.
Mutation-tested:
the link should be named in the report, got []SECURITY: a symlink entry was restoredThe restore test uses a well-behaved relative link that stays inside the data
dir — it is refused too, because the rule is "no links", not "no escaping
links". A test built around an escaping link would have passed on the older,
weaker rule and told us nothing new.
One thing this does not do: warn about a large regular file. The size line
makes it visible, but nothing refuses it — a big file in the data dir is the
operator's business, and a cap would break restores of installations that
legitimately hold a lot.