Skip to content

Commit 51c826b

Browse files
committed
fix(snapshot): address Copilot review findings
- Guard handleList/handleDownload/handleDelete against a nil manager (only reachable if Routes were ever registered while disabled), matching the existing handleStatus/handleTrigger pattern instead of panicking. - Add Cache-Control: no-store / Pragma: no-cache to the download response, since the zip contains users.db (password hashes, TOTP secrets). - triggerNow() now reloads status/list in a finally block so a failed trigger (e.g. 409 already-running) doesn't leave the UI stale.
1 parent f9e304b commit 51c826b

3 files changed

Lines changed: 71 additions & 4 deletions

File tree

internal/wiki/snapshot/routes.go

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,18 @@ func (r *Routes) handleStatus(c *gin.Context) {
6767
})
6868
}
6969

70+
// respondNotEnabled writes the standard 503 response for handlers that need
71+
// a non-nil manager/scheduler, which is only nil if snapshots are disabled.
72+
func (r *Routes) respondNotEnabled(c *gin.Context) {
73+
respondWithSnapshotStatusError(c, http.StatusServiceUnavailable, ErrCodeSnapshotNotEnabled, "Snapshot backup is not enabled", "snapshot backup not enabled")
74+
}
75+
7076
// handleList returns all finished snapshots, newest first.
7177
func (r *Routes) handleList(c *gin.Context) {
78+
if r.manager == nil {
79+
r.respondNotEnabled(c)
80+
return
81+
}
7282
entries, err := r.manager.List()
7383
if err != nil {
7484
respondWithSnapshotError(c, err)
@@ -80,7 +90,7 @@ func (r *Routes) handleList(c *gin.Context) {
8090
// handleTrigger triggers an immediate snapshot and returns 202 Accepted.
8191
func (r *Routes) handleTrigger(c *gin.Context) {
8292
if r.scheduler == nil {
83-
respondWithSnapshotStatusError(c, http.StatusServiceUnavailable, ErrCodeSnapshotNotEnabled, "Snapshot backup is not enabled", "snapshot backup not enabled")
93+
r.respondNotEnabled(c)
8494
return
8595
}
8696
if !r.scheduler.TriggerNow() {
@@ -92,6 +102,10 @@ func (r *Routes) handleTrigger(c *gin.Context) {
92102

93103
// handleDownload streams the ZIP file for a given snapshot id as an attachment.
94104
func (r *Routes) handleDownload(c *gin.Context) {
105+
if r.manager == nil {
106+
r.respondNotEnabled(c)
107+
return
108+
}
95109
id := c.Param("id")
96110
zipPath, err := r.manager.SnapshotZipPath(id)
97111
if err != nil {
@@ -118,12 +132,20 @@ func (r *Routes) handleDownload(c *gin.Context) {
118132
disposition = "attachment"
119133
}
120134
c.Header("Content-Disposition", disposition)
135+
// The zip contains users.db (password hashes, TOTP secrets) — never let
136+
// proxies/browsers cache it.
137+
c.Header("Cache-Control", "no-store")
138+
c.Header("Pragma", "no-cache")
121139
c.Writer.Header().Set("Content-Type", "application/zip")
122140
http.ServeContent(c.Writer, c.Request, filename, stat.ModTime(), f)
123141
}
124142

125143
// handleDelete removes a snapshot's ZIP and sidecar metadata.
126144
func (r *Routes) handleDelete(c *gin.Context) {
145+
if r.manager == nil {
146+
r.respondNotEnabled(c)
147+
return
148+
}
127149
id := c.Param("id")
128150
if err := r.manager.Delete(id); err != nil {
129151
respondWithSnapshotError(c, err)

internal/wiki/snapshot/routes_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,42 @@ func TestHandleStatus_Enabled(t *testing.T) {
127127
}
128128
}
129129

130+
func TestHandleList_NotEnabledWhenManagerNil(t *testing.T) {
131+
router := newTestRouter(&Routes{})
132+
133+
w := httptest.NewRecorder()
134+
req, _ := http.NewRequest(http.MethodGet, "/snapshot", nil)
135+
router.ServeHTTP(w, req)
136+
137+
if w.Code != http.StatusServiceUnavailable {
138+
t.Errorf("expected 503, got %d", w.Code)
139+
}
140+
}
141+
142+
func TestHandleDownload_NotEnabledWhenManagerNil(t *testing.T) {
143+
router := newTestRouter(&Routes{})
144+
145+
w := httptest.NewRecorder()
146+
req, _ := http.NewRequest(http.MethodGet, "/snapshot/snapshot-20260101-000000/download", nil)
147+
router.ServeHTTP(w, req)
148+
149+
if w.Code != http.StatusServiceUnavailable {
150+
t.Errorf("expected 503, got %d", w.Code)
151+
}
152+
}
153+
154+
func TestHandleDelete_NotEnabledWhenManagerNil(t *testing.T) {
155+
router := newTestRouter(&Routes{})
156+
157+
w := httptest.NewRecorder()
158+
req, _ := http.NewRequest(http.MethodDelete, "/snapshot/snapshot-20260101-000000", nil)
159+
router.ServeHTTP(w, req)
160+
161+
if w.Code != http.StatusServiceUnavailable {
162+
t.Errorf("expected 503, got %d", w.Code)
163+
}
164+
}
165+
130166
func TestHandleTrigger_NotEnabledWhenSchedulerNil(t *testing.T) {
131167
router := newTestRouter(&Routes{})
132168

@@ -212,6 +248,9 @@ func TestHandleDownload_ReturnsZip(t *testing.T) {
212248
if ct := w.Header().Get("Content-Type"); ct != "application/zip" {
213249
t.Errorf("expected Content-Type application/zip, got %q", ct)
214250
}
251+
if cc := w.Header().Get("Cache-Control"); cc != "no-store" {
252+
t.Errorf("expected Cache-Control no-store (snapshot contains users.db), got %q", cc)
253+
}
215254
disposition := w.Header().Get("Content-Disposition")
216255
if disposition == "" {
217256
t.Error("expected Content-Disposition header to be set")

ui/leafwiki-ui/src/stores/snapshot.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,15 @@ export const useSnapshotStore = create<SnapshotState>((set, get) => ({
7272
},
7373

7474
triggerNow: async () => {
75-
await triggerSnapshot()
76-
await get().loadStatus()
77-
await get().loadList()
75+
try {
76+
await triggerSnapshot()
77+
} finally {
78+
// Reload even on failure (e.g. 409 because a run is already in
79+
// progress) so the UI reflects current server state instead of
80+
// staying stale behind the error.
81+
await get().loadStatus()
82+
await get().loadList()
83+
}
7884
},
7985

8086
remove: async (id: string) => {

0 commit comments

Comments
 (0)