Harden backup runner, env-file reads, and notification clients#175
Open
jonhadfield wants to merge 1 commit into
Open
Harden backup runner, env-file reads, and notification clients#175jonhadfield wants to merge 1 commit into
jonhadfield wants to merge 1 commit into
Conversation
jonhadfield
force-pushed
the
review-fixes
branch
from
6f320c5 to
49debc1
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens soba’s backup execution and notification pathways by reducing risky filesystem operations, improving shutdown behavior for scheduled runs, tightening secret/env-file handling, and avoiding credential leakage through HTTP client logging.
Changes:
- Hardened backup execution: safer working-dir cleanup, graceful SIGINT/SIGTERM shutdown, refactored provider validation to return errors instead of exiting.
- Hardened configuration input: capped
*_FILEreads to 1 MiB and tightened working-dir permissions to0o700. - Hardened outbound notifications: reduced Telegram token exposure, stopped logging response bodies, improved ntfy error handling, and isolated webhook retry configuration from shared clients.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/webhook.go | Uses a dedicated retryablehttp client for webhook retries to avoid mutating the shared provider client. |
| internal/notify.go | Prevents Telegram token leakage via retryablehttp logging; avoids logging response bodies; returns on ntfy request error. |
| internal/envfile.go | Adds a size cap when reading *_FILE env indirection to prevent OOM/device-backed reads. |
| internal/constants.go | Tightens working-dir permissions and introduces maxEnvFileSize. |
| internal/backup.go | Adds safer working-dir cleanup, graceful scheduler shutdown, non-fatal git version probe, and provider-validation refactor. |
| internal/backup_test.go | Removes now-obsolete global reset. |
| go.mod | Bumps dependencies and updates the declared Go version. |
| go.sum | Updates dependency checksums accordingly. |
Comments suppressed due to low confidence (1)
go.mod:3
- The
godirective should not include a patch version (e.g.1.25.10). Thegoline is the language version and is typicallygo 1.25/go 1.25.0; if you need to pin a specific toolchain patch, use the separatetoolchain go1.25.10directive.
go 1.25.1
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+324
to
+335
| if slices.Contains(justTokenProviders, provider) { | ||
| for _, param := range enabledProviderAuth[provider] { | ||
| val, exists := GetEnvOrFile(param) | ||
| if exists { | ||
| if strings.Trim(val, " ") == "" { | ||
| _, _ = fmt.Fprintf(&outputErrs, "%s parameter '%s' is not defined.\n", provider, param) | ||
| } else { | ||
| count++ | ||
| } | ||
| } | ||
| } | ||
| } |
| defer resp.Body.Close() | ||
|
|
||
| buf, err := io.ReadAll(resp.Body) | ||
| _, err = io.ReadAll(resp.Body) |
Comment on lines
+149
to
+151
| // Do not log response body — Telegram error responses can echo | ||
| // chat_id and other metadata that is sensitive in shared deployments. | ||
| logger.Printf("telegram failed to send message - code [%d]", resp.StatusCode) |
Comment on lines
+45
to
49
| b, err := io.ReadAll(io.LimitReader(f, maxEnvFileSize+1)) | ||
| if err != nil { | ||
| logger.Printf("error reading file %s: %v", filePath, err) | ||
|
|
||
| return "", false |
Comment on lines
+136
to
+140
| // cleanupWorkingDir removes the working directory after a backup run. | ||
| // It refuses to remove anything that does not resolve to a path inside | ||
| // backupDir to protect against a misconfigured GIT_WORKING_DIR wiping | ||
| // arbitrary filesystem locations. | ||
| func cleanupWorkingDir(workingDir, backupDir string) { |
Security and robustness fixes from code review: - backup: refuse to remove a working dir outside the backup dir, guarding against a misconfigured GIT_WORKING_DIR wiping arbitrary paths - backup: graceful SIGINT/SIGTERM shutdown so in-flight jobs (and their deferred cleanup) complete before exit - backup: replace logger.Fatal calls in library code with returned errors; split execProviderBackups into a testable runProviderBackups and convert the provider-check closure into checkProvider returning (count, error) - backup: log resolved git version at startup - envfile: cap *_FILE reads at 1 MiB so a device-backed path cannot OOM - notify: stop logging the token-bearing Telegram URL and response bodies; return on ntfy request error - webhook: use a dedicated retryablehttp client instead of mutating the shared one - constants: tighten working dir mode 0o755 -> 0o700; drop the numUserDefinedProviders global Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jonhadfield
force-pushed
the
review-fixes
branch
from
49debc1 to
e8ff254
Compare
|
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
Security and robustness fixes from a code review, across the backup runner, env-file indirection, and notification clients. Builds clean, passes
go vet, and unit tests pass (live provider integration tests still gated on credentials).Changes
cleanupWorkingDirrefuses toRemoveAllany path that doesn't resolve inside the backup directory — guards against a misconfiguredGIT_WORKING_DIRwiping arbitrary filesystem locations.SIGINT/SIGTERMshutdown viawaitForShutdown, so an in-flight job (and its deferred cleanup) finishes before the process exits.logger.Fatal*calls in library code with returned errors; splitexecProviderBackupsinto a testablerunProviderBackups; converted thecheckProviderFactoryclosure intocheckProviderreturning(count, error)instead of callingFatallnfrom a library function.*_FILEreads at 1 MiB (maxEnvFileSize) so a device-backed path like/dev/zerocannot OOM the process.retryablehttpclient instead of mutating the shared one used by provider calls.0o755→0o700; drop thenumUserDefinedProviderspackage global.Test plan
go build ./...— cleango vet ./...— cleango test ./...— passes with provider credentials unset (live integration tests skip)🤖 Generated with Claude Code