diff --git a/internal/api/attachments.go b/internal/api/attachments.go index 04c4c56..862f544 100644 --- a/internal/api/attachments.go +++ b/internal/api/attachments.go @@ -242,6 +242,7 @@ func (a *apiServer) handleCreatePasteMultipart(w http.ResponseWriter, r *http.Re return } + setDeletionTokenCookie(w, created.ID, created.DeletionToken) // #143 resp := map[string]any{ "id": created.ID, "deletion_token": created.DeletionToken, diff --git a/internal/api/burn.go b/internal/api/burn.go index a425ac2..2e108a6 100644 --- a/internal/api/burn.go +++ b/internal/api/burn.go @@ -20,10 +20,12 @@ func (a *apiServer) burnViewerWindow() int { } // handleRedeemDeletion lets a holder of the deletion token hard-delete immediately. -// DELETE /api/pastes/{id}/redeem?token=... +// DELETE /api/pastes/{id}/redeem with the token in the Authorization header +// (#143: the ?token= query path was removed so the secret stays out of +// access logs and browser history). func (a *apiServer) handleRedeemDeletion(w http.ResponseWriter, r *http.Request) { id := chi.URLParam(r, "id") - token := r.URL.Query().Get("token") + token := deletionAuthorization(r) if token == "" { writeErr(w, 400, "token required") return diff --git a/internal/api/burn_test.go b/internal/api/burn_test.go index cdaa2ef..e67c309 100644 --- a/internal/api/burn_test.go +++ b/internal/api/burn_test.go @@ -53,8 +53,9 @@ func TestDeletionTokenRedeem(t *testing.T) { t.Fatal("no deletion token in create response") } - // wrong token - req = httptest.NewRequest("DELETE", "/api/pastes/"+created.ID+"/redeem?token=wrong", nil) + // wrong token (#143: token goes in the Authorization header, not the URL) + req = httptest.NewRequest("DELETE", "/api/pastes/"+created.ID+"/redeem", nil) + req.Header.Set("Authorization", "Bearer wrong") rec = httptest.NewRecorder() h.ServeHTTP(rec, req) if rec.Code != 403 { @@ -62,7 +63,8 @@ func TestDeletionTokenRedeem(t *testing.T) { } // right token: hard delete - req = httptest.NewRequest("DELETE", "/api/pastes/"+created.ID+"/redeem?token="+created.DeletionToken, nil) + req = httptest.NewRequest("DELETE", "/api/pastes/"+created.ID+"/redeem", nil) + req.Header.Set("Authorization", "Bearer "+created.DeletionToken) rec = httptest.NewRecorder() h.ServeHTTP(rec, req) if rec.Code != 200 { diff --git a/internal/api/cookie_set_test.go b/internal/api/cookie_set_test.go new file mode 100644 index 0000000..7311560 --- /dev/null +++ b/internal/api/cookie_set_test.go @@ -0,0 +1,97 @@ +package api + +// #143: create responses must set the short-lived tok_ HttpOnly cookie +// that the paste view reads for the one-time created banner. + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestCreateSetsDeletionTokenCookie(t *testing.T) { + s := testServer(t) + h := s.routes() + + // JSON create + body := `{"content":"hello #143 cookie"}` + req := httptest.NewRequest("POST", "/api/pastes", strings.NewReader(body)) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != 201 { + t.Fatalf("json create: got %d", rec.Code) + } + found := false + for _, c := range rec.Result().Cookies() { + if strings.HasPrefix(c.Name, "tok_") && c.Value != "" { + found = true + if !c.HttpOnly { + t.Error("tok_ cookie not HttpOnly") + } + if c.MaxAge != 60 { + t.Errorf("tok_ cookie MaxAge = %d, want 60", c.MaxAge) + } + } + } + if !found { + t.Error("json create did not set tok_ cookie (#143)") + } + + // multipart create + var buf strings.Builder + boundary := "----qa143" + buf.WriteString("--" + boundary + "\r\n") + buf.WriteString("Content-Disposition: form-data; name=\"content\"\r\n\r\n") + buf.WriteString("multipart #143\r\n") + buf.WriteString("--" + boundary + "--\r\n") + req2 := httptest.NewRequest("POST", "/api/pastes", strings.NewReader(buf.String())) + req2.Header.Set("Content-Type", "multipart/form-data; boundary="+boundary) + rec2 := httptest.NewRecorder() + h.ServeHTTP(rec2, req2) + if rec2.Code != 201 { + t.Fatalf("multipart create: got %d body=%s", rec2.Code, rec2.Body.String()) + } + found = false + for _, c := range rec2.Result().Cookies() { + if strings.HasPrefix(c.Name, "tok_") && c.Value != "" { + found = true + } + } + if !found { + t.Error("multipart create did not set tok_ cookie (#143)") + } +} + +func TestCreatedBannerViaCookie(t *testing.T) { + s := testServer(t) + h := s.routes() + body := `{"content":"banner flow #143"}` + req := httptest.NewRequest("POST", "/api/pastes", strings.NewReader(body)) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != 201 { + t.Fatalf("create: got %d", rec.Code) + } + var id, tok string + for _, c := range rec.Result().Cookies() { + if strings.HasPrefix(c.Name, "tok_") { + id = strings.TrimPrefix(c.Name, "tok_") + tok = c.Value + } + } + if id == "" || tok == "" { + t.Fatal("no tok_ cookie from create") + } + // follow the redirect the browser would make: GET /?created=1 with the cookie + req2 := httptest.NewRequest("GET", "/"+id+"?created=1", nil) + req2.AddCookie(&http.Cookie{Name: "tok_" + id, Value: tok}) + rec2 := httptest.NewRecorder() + h.ServeHTTP(rec2, req2) + if rec2.Code != 200 { + t.Fatalf("paste view: got %d", rec2.Code) + } + if !strings.Contains(rec2.Body.String(), tok) { + t.Error("created banner does not show the deletion token (#143 cookie flow broken)") + } +} diff --git a/internal/api/delete_auth_test.go b/internal/api/delete_auth_test.go index 9581b1d..a2cddde 100644 --- a/internal/api/delete_auth_test.go +++ b/internal/api/delete_auth_test.go @@ -1,9 +1,10 @@ package api -// Regression tests for #63: DELETE /api/pastes/{id} must require the -// deletion token (Authorization header or ?token= query param, constant-time -// compare). Without a token, or with a wrong token, the paste must survive -// and the response must be 403. +// Regression tests for #63 and #143: DELETE /api/pastes/{id} must require the +// deletion token in the Authorization header (constant-time compare). The +// ?token= query parameter is NOT accepted (#143): URL-carried tokens leak +// into access logs and browser history. Without a token, or with a wrong +// token, the paste must survive and the response must be 403. import ( "encoding/json" @@ -68,10 +69,14 @@ func TestDeleteWithWrongTokenForbidden(t *testing.T) { h := s.routes() id, _ := createTestPaste(t, h) - // query param - rec := doReq(t, h, "DELETE", "/api/pastes/"+id+"?token=wrong-token", "", "") + // query param: even the CORRECT token must be rejected now (#143) + id2, tok2 := createTestPaste(t, h) + rec := doReq(t, h, "DELETE", "/api/pastes/"+id2+"?token="+tok2, "", "") if rec.Code != http.StatusForbidden { - t.Fatalf("delete with wrong token (query): got %d want 403", rec.Code) + t.Fatalf("delete with correct token in query: got %d want 403 (#143)", rec.Code) + } + if !pasteExists(t, h, id2) { + t.Fatal("paste was deleted via ?token= query param (#143 regression)") } // header req := httptest.NewRequest("DELETE", "/api/pastes/"+id, nil) @@ -103,14 +108,43 @@ func TestDeleteWithCorrectToken(t *testing.T) { t.Fatal("paste still exists after authorized delete") } - // via query param + // query param: even with the correct token the delete must fail (#143) id, tok = createTestPaste(t, h) rec = doReq(t, h, "DELETE", "/api/pastes/"+id+"?token="+tok, "", "") + if rec.Code != http.StatusForbidden { + t.Fatalf("delete with correct token (query): got %d want 403 (#143)", rec.Code) + } + if !pasteExists(t, h, id) { + t.Fatal("paste was deleted via ?token= query param (#143 regression)") + } +} + +// #143: the deletion token must be accepted via the Authorization header on +// the redeem (hard delete) endpoint too. +func TestRedeemWithCorrectTokenHeader(t *testing.T) { + s := testServer(t) + h := s.routes() + id, tok := createTestPaste(t, h) + + req := httptest.NewRequest("DELETE", "/api/pastes/"+id+"/redeem", nil) + req.Header.Set("Authorization", "Bearer "+tok) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) if rec.Code != 200 { - t.Fatalf("delete with correct token (query): got %d want 200", rec.Code) + t.Fatalf("redeem with correct token (header): got %d want 200: %s", rec.Code, rec.Body.String()) } if pasteExists(t, h, id) { - t.Fatal("paste still exists after authorized delete (query)") + t.Fatal("paste still exists after authorized redeem") + } + + // query param must NOT work on redeem either + id, tok = createTestPaste(t, h) + rec = doReq(t, h, "DELETE", "/api/pastes/"+id+"/redeem?token="+tok, "", "") + if rec.Code != http.StatusBadRequest { + t.Fatalf("redeem via ?token= query: got %d want 400 (#143)", rec.Code) + } + if !pasteExists(t, h, id) { + t.Fatal("paste was hard-deleted via ?token= query param (#143 regression)") } } @@ -161,8 +195,8 @@ func TestDeletionAuthorizationExtract(t *testing.T) { {"bearer tok", "", "tok"}, {"Token tok", "", "tok"}, {"tok", "", "tok"}, - {"", "?token=q", "q"}, - {"Bearer hdr", "?token=q", "hdr"}, // header wins + {"", "?token=q", ""}, // #143: query tokens are never accepted + {"Bearer hdr", "?token=q", "hdr"}, // header only } for _, c := range cases { if got := deletionAuthorization(mk(c.hdr, c.q)); got != c.want { diff --git a/internal/api/server.go b/internal/api/server.go index 24d6924..fb2cd7c 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -174,6 +174,16 @@ func viewerSentCookie(r *http.Request) bool { return !minted } +// #143: hand the deletion token to the creator's browser via a short-lived +// HttpOnly cookie instead of the URL. The paste view reads it once to show +// the one-time created banner; it expires after 60s. +func setDeletionTokenCookie(w http.ResponseWriter, pasteID, token string) { + http.SetCookie(w, &http.Cookie{ + Name: "tok_" + pasteID, Value: token, Path: "/", + MaxAge: 60, HttpOnly: true, SameSite: http.SameSiteLaxMode, + }) +} + func (a *apiServer) handleCreatePaste(w http.ResponseWriter, r *http.Request) { s := a.settings.get() setRateLimitHeaders(w, 1, 5) @@ -241,6 +251,7 @@ func (a *apiServer) handleCreatePaste(w http.ResponseWriter, r *http.Request) { writeErrCode(w, 400, createErrCode(err), err.Error()) return } + setDeletionTokenCookie(w, created.ID, created.DeletionToken) // #143 writeJSON(w, 201, map[string]any{ "id": created.ID, "deletion_token": created.DeletionToken, @@ -311,9 +322,10 @@ func (a *apiServer) handleDeletePaste(w http.ResponseWriter, r *http.Request) { return } // #63: deletion requires authorization. Either the deletion token issued - // at create time (Authorization header or ?token= query param, matching - // the create response's "deletion_token" field), or the creator browser - // itself (client-sent vwr cookie matching the paste's viewer, #37). + // at create time (Authorization header; #143 removed the ?token= query + // path so the bearer secret never lands in access logs or history), or + // the creator browser itself (client-sent vwr cookie matching the + // paste's viewer, #37). if !a.deletionAuthorized(r, row) { writeErr(w, 403, "deletion token required") return @@ -325,9 +337,11 @@ func (a *apiServer) handleDeletePaste(w http.ResponseWriter, r *http.Request) { writeJSON(w, 200, map[string]string{"status": "soft-deleted"}) } -// deletionAuthorization extracts the deletion token from the request: the -// Authorization header ("Bearer ", "Token ", or a bare token) or the -// token query parameter. Returns "" when absent. +// deletionAuthorization extracts the deletion token from the request: +// the Authorization header ("Bearer ", "Token ", or a bare token). +// The ?token= query parameter is deliberately NOT accepted (#143): URL +// query strings end up in proxy access logs and browser history. Returns +// "" when absent. func deletionAuthorization(r *http.Request) string { if h := r.Header.Get("Authorization"); h != "" { for _, prefix := range []string{"Bearer ", "Token "} { @@ -337,7 +351,7 @@ func deletionAuthorization(r *http.Request) string { } return strings.TrimSpace(h) } - return r.URL.Query().Get("token") + return "" } // deletionAuthorized reports whether the request may soft-delete the paste: @@ -551,19 +565,19 @@ func (a *apiServer) renderCan(w http.ResponseWriter, can *store.CanRow) { cards = append(cards, ci) } h.RenderPage(w, "can.html", map[string]any{ - "Page": "can", - "ID": can.ID, - "Title": nullStrOr(can.Title, "Untitled can"), - "Description": can.Description.String, + "Page": "can", + "ID": can.ID, + "Title": nullStrOr(can.Title, "Untitled can"), + "Description": can.Description.String, "HasDescription": can.Description.Valid && can.Description.String != "", - "HasPassword": can.PasswordHash.Valid, - "Items": cards, - "ItemCount": len(cards), - "SizeHuman": web.HumanSize(totalSize), - "CreatedAgo": web.AgoString(can.CreatedAt), - "CreatedAtUnix": can.CreatedAt, - "ExpiresAt": can.ExpiresAt.Valid, - "ExpiresIn": expiryStringIfValid(can.ExpiresAt), + "HasPassword": can.PasswordHash.Valid, + "Items": cards, + "ItemCount": len(cards), + "SizeHuman": web.HumanSize(totalSize), + "CreatedAgo": web.AgoString(can.CreatedAt), + "CreatedAtUnix": can.CreatedAt, + "ExpiresAt": can.ExpiresAt.Valid, + "ExpiresIn": expiryStringIfValid(can.ExpiresAt), }) } diff --git a/internal/web/templates/new.html b/internal/web/templates/new.html index 2edd883..b51e191 100644 --- a/internal/web/templates/new.html +++ b/internal/web/templates/new.html @@ -291,7 +291,9 @@ function finishCreate(data) { setTimeout(() => { copyBtn.classList.remove('ok'); copyBtn.textContent = '⧉'; }, 2000); } catch(e) { toast('Copy failed', 'error'); } }); - const dest = '/' + data.id + '?created=1&token=' + encodeURIComponent(data.deletion_token || ''); + // token carried via sessionStorage, never in the URL (#143) + const dest = '/' + data.id + '?created=1'; + try { sessionStorage.setItem('deletion_token_' + data.id, data.deletion_token || ''); } catch(e) {} // password-protected: unlock now with the password we already have (#26) if ($('haspw').checked && data.id) { const fd = new FormData(); diff --git a/internal/web/templates/paste.html b/internal/web/templates/paste.html index 8ee6053..ba9bae7 100644 --- a/internal/web/templates/paste.html +++ b/internal/web/templates/paste.html @@ -9,7 +9,7 @@ raw copy - {{if .DeletionToken}}delete{{end}} + {{if .DeletionToken}}delete{{end}}
@@ -89,9 +89,12 @@ function copyContent(btn) { toast('Copied', 'success'); } } -function redeem(token) { +function redeem() { if (!confirm('Hard delete this paste immediately?')) return; - fetch('/api/pastes/{{.ID}}/redeem?token=' + encodeURIComponent(token), {method: 'DELETE'}) + let tok = ''; + try { tok = sessionStorage.getItem('deletion_token_{{.ID}}') || ''; } catch(e) {} + if (!tok) { alert('deletion token not available in this browser'); return; } + fetch('/api/pastes/{{.ID}}/redeem', {method: 'DELETE', headers: {'Authorization': 'Bearer ' + tok}}) .then(r => { if (r.ok) location.href = '/history'; else alert('delete failed'); }); } diff --git a/internal/web/web.go b/internal/web/web.go index 2f9a3b1..b3a8166 100644 --- a/internal/web/web.go +++ b/internal/web/web.go @@ -310,7 +310,13 @@ func (h *Handlers) HandlePasteView(w http.ResponseWriter, r *http.Request) { } justCreated := r.URL.Query().Get("created") == "1" - token := r.URL.Query().Get("token") + // #143: the deletion token is no longer round-tripped through the URL + // (?token= leaks into access logs and history). The create flow sets a + // short-lived tok_ cookie; the paste view reads it once from there. + token := "" + if c, err := r.Cookie("tok_" + row.ID); err == nil { + token = c.Value + } if justCreated && token != "" { // one-time display of the deletion token via the created banner http.SetCookie(w, &http.Cookie{Name: "tok_" + row.ID, Value: token, Path: "/", MaxAge: 60, HttpOnly: true, SameSite: http.SameSiteLaxMode})