security(sharing): Freigabelink prüft die Prüfsumme, Gruppenordner teilbar
Beim Lesen von internal/sharing fiel eine ungleiche Zusage auf: Der angemeldete Download prüft seit dem 12.09. die SHA-256-Summe beim Ausliefern und lässt die Verbindung fallen, wenn sie nicht stimmt. Der öffentliche Freigabelink lief durch ein blankes io.Copy – ungeprüft. Ausgerechnet der Weg nach draussen. Wer die Datei angemeldet holt, ist der Eigentümer und merkt, wenn etwas kaputt ist; wer einen Link bekommt, merkt es nicht. In CLAUDE.md steht das unter „nicht verhandelbar": beim Upload berechnen, beim Download verifizieren. Die Regel galt an einer von zwei Stellen. Die Mechanik liegt jetzt in files/verified.go und wird von beiden gerufen. Nachgewiesen mit gekipptem Bit auf der Platte: Über den Freigabelink kommt die Datei ebenfalls nicht mehr vollständig an. Gruppenordner: Create übergab user.ID statt des aufgelösten Prinzipals – eine Datei im Gruppenordner liess sich nicht verlinken, auch nicht vom Mitglied. Marcs Entscheidung: Das soll gehen, der Empfänger sieht ohnehin nur die Datei, nie den Ordner darum. Verlangt wird Schreibrecht: Einen Link zu erzeugen heisst, die Datei öffentlich zu machen; wer sie nur geliehen lesen darf, soll sie nicht weiterreichen können (403). expires_in_hours hatte keine Obergrenze. time.Duration rechnet in Nanosekunden; ab etwa 2,6 Mio Stunden läuft int64 über, die Dauer wird negativ, und der Link ist abgelaufen in dem Moment, in dem er entsteht – ohne Fehlermeldung. Jetzt zehn Jahre, darüber 400. Dazu X-Content-Type-Options: nosniff, das der angemeldete Weg längst setzt. Sechs Go-Tests für internal/sharing, unter Mutation nachgewiesen: Eigentümerprüfung entfernt, Ablaufprüfung entfernt und Token verkürzt machen jeweils genau den zugehörigen Test rot. Selbsttests erweitert: sharing 24 → 31 Prüfungen, pruefsumme 14 → 16. Geprüft: go build/vet sauber, alle Go-Pakete grün, 241 Prüfungen der sieben Selbsttest-Suiten grün, 48 Vitest-Tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
marcandClaude Opus 5 committed
1 parent
a7528d7365
commit
6fab2388ca
9 files changed
+628
-74
No files matched your search
@@ -5,7 +5,6 @@ import (
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"strconv"
|
||||
"time"
|
||||
@@ -13,6 +12,7 @@ import (
|
||||
"github.com/go-chi/chi/v5"
|
||||
"github.com/google/uuid"
|
||||
|
||||
"aether/internal/acl"
|
||||
"aether/internal/audit"
|
||||
"aether/internal/auth"
|
||||
"aether/internal/files"
|
||||
@@ -30,6 +30,10 @@ const (
|
||||
shareMaxFailures = 10
|
||||
shareWindow = 15 * time.Minute
|
||||
shareLockout = 15 * time.Minute
|
||||
|
||||
// Zehn Jahre. Grosszügig gemeint – es geht nicht darum, kurze Fristen zu
|
||||
// erzwingen, sondern den Überlauf abzufangen.
|
||||
maxExpiryHours = 10 * 365 * 24
|
||||
)
|
||||
|
||||
type Handler struct {
|
||||
@@ -37,12 +41,16 @@ type Handler struct {
|
||||
fileStore *files.Store
|
||||
storage storage.Storage
|
||||
auditor *audit.Logger
|
||||
limiter *middleware.FailureLimiter
|
||||
// acl löst auf, in wessen Namen die Datei liegt. Ohne ihn ging nur, was
|
||||
// dem Anfragenden persönlich gehört – siehe Create.
|
||||
acl *acl.Store
|
||||
limiter *middleware.FailureLimiter
|
||||
}
|
||||
|
||||
func NewHandler(store *Store, fileStore *files.Store, st storage.Storage, auditor *audit.Logger) *Handler {
|
||||
func NewHandler(store *Store, fileStore *files.Store, st storage.Storage, auditor *audit.Logger, aclStore *acl.Store) *Handler {
|
||||
return &Handler{
|
||||
store: store, fileStore: fileStore, storage: st, auditor: auditor,
|
||||
acl: aclStore,
|
||||
limiter: middleware.NewFailureLimiter(shareMaxFailures, shareWindow, shareLockout),
|
||||
}
|
||||
}
|
||||
@@ -55,7 +63,21 @@ func (h *Handler) Create(w http.ResponseWriter, r *http.Request) {
|
||||
response.Error(w, http.StatusBadRequest, "invalid file id")
|
||||
return
|
||||
}
|
||||
if _, err := h.fileStore.GetFile(r.Context(), fileID, user.ID); err != nil {
|
||||
// **Prinzipal statt Konto.** Vorher stand hier `user.ID` – damit liess sich
|
||||
// eine Datei in einem Gruppenordner nicht per Link teilen, auch nicht von
|
||||
// einem Mitglied, das sie sehen und herunterladen darf. Die Antwort lautete
|
||||
// „file not found", was zwar sicher, aber schlicht falsch war: Der
|
||||
// Empfänger eines Links sieht ohnehin nur die Datei, nie den Ordner
|
||||
// darum.
|
||||
//
|
||||
// **Schreibrecht, nicht nur Leserecht.** Einen Link zu erzeugen heisst,
|
||||
// die Datei öffentlich zu machen. Wer sie nur geliehen lesen darf, soll
|
||||
// sie nicht weiterreichen können.
|
||||
zugriff, ok := h.resolve(w, r, fileID)
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
if _, err := h.fileStore.GetFile(r.Context(), fileID, zugriff.OwnerID); err != nil {
|
||||
response.Error(w, http.StatusNotFound, "file not found")
|
||||
return
|
||||
}
|
||||
@@ -77,6 +99,16 @@ func (h *Handler) Create(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
var expiresAt *time.Time
|
||||
if body.ExpiresIn != nil && *body.ExpiresIn > 0 {
|
||||
// Obergrenze, weil `time.Duration` in Nanosekunden rechnet: Ab etwa
|
||||
// 2,6 Millionen Stunden läuft int64 über, die Dauer wird **negativ**,
|
||||
// und der Link ist in dem Moment abgelaufen, in dem er entsteht – ohne
|
||||
// Fehlermeldung. Wer eine unsinnige Zahl schickt, soll das erfahren,
|
||||
// statt einen toten Link zu bekommen.
|
||||
if *body.ExpiresIn > maxExpiryHours {
|
||||
response.Error(w, http.StatusBadRequest,
|
||||
"expires_in_hours is too large (at most 10 years)")
|
||||
return
|
||||
}
|
||||
t := time.Now().Add(time.Duration(*body.ExpiresIn) * time.Hour)
|
||||
expiresAt = &t
|
||||
}
|
||||
@@ -238,5 +270,41 @@ func (h *Handler) Download(w http.ResponseWriter, r *http.Request) {
|
||||
w.Header().Set("Content-Type", f.MimeType)
|
||||
w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename=%q`, f.Name))
|
||||
w.Header().Set("Content-Length", fmt.Sprintf("%d", f.Size))
|
||||
_, _ = io.Copy(w, rc)
|
||||
// Kein Typ-Raten. `attachment` trägt die Hauptlast, aber der angemeldete
|
||||
// Weg setzt beides – und dies ist der Weg nach draussen.
|
||||
w.Header().Set("X-Content-Type-Options", "nosniff")
|
||||
|
||||
// **Dieselbe Prüfung wie beim angemeldeten Download.** Bis zum 25.09.2026
|
||||
// lief hier ein blankes io.Copy: Wer die Datei über einen Link bekam,
|
||||
// bekam sie ungeprüft. Ausgerechnet auf dem Weg, auf dem sie das Haus
|
||||
// verlässt – und wo der Empfänger es am wenigsten merken kann.
|
||||
files.ServeVerified(r.Context(), w, rc, f.Checksum, files.VerifyInfo{
|
||||
FileID: f.ID.String(),
|
||||
StorageKey: f.StorageKey,
|
||||
Size: f.Size,
|
||||
})
|
||||
}
|
||||
|
||||
// resolve klärt, in wessen Namen die Datei liegt, und verlangt Schreibrecht.
|
||||
func (h *Handler) resolve(w http.ResponseWriter, r *http.Request, fileID uuid.UUID) (acl.Access, bool) {
|
||||
user := auth.UserFromContext(r.Context())
|
||||
if h.acl == nil {
|
||||
return acl.Access{OwnerID: user.ID, Permission: acl.PermWrite}, true
|
||||
}
|
||||
a, err := h.acl.ResolveFile(r.Context(), user.ID, fileID)
|
||||
switch {
|
||||
case errors.Is(err, acl.ErrNotFound), errors.Is(err, acl.ErrNoAccess):
|
||||
// 404 statt 403, wie überall: Ein „verboten" verriete, dass es die ID
|
||||
// gibt.
|
||||
response.Error(w, http.StatusNotFound, "file not found")
|
||||
return acl.Access{}, false
|
||||
case err != nil:
|
||||
response.Error(w, http.StatusInternalServerError, "database error")
|
||||
return acl.Access{}, false
|
||||
}
|
||||
if !a.CanWrite() {
|
||||
response.Error(w, http.StatusForbidden, "you only have read access to this file")
|
||||
return acl.Access{}, false
|
||||
}
|
||||
return a, true
|
||||
}
|
||||
@@ -0,0 +1,251 @@
|
||||
package sharing
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"os"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/jackc/pgx/v5/pgxpool"
|
||||
)
|
||||
|
||||
// Ein Freigabelink ist der einzige Weg, auf dem eine Datei dieses Haus ohne
|
||||
// Anmeldung verlässt. Wer das Token hat, ist drin – also entscheidet sich hier
|
||||
// alles an zwei Fragen: Gilt der Link noch, und gehört er dem, der ihn
|
||||
// anfasst?
|
||||
//
|
||||
// Beides steckt nicht in einer if-Zeile, sondern in WHERE-Klauseln und einem
|
||||
// Zeitvergleich. Ein Fehler darin fällt beim Lesen nicht auf: Die Abfrage
|
||||
// läuft, sie liefert nur zu viel.
|
||||
//
|
||||
// Läuft nur mit TEST_DSN. Einfachster Weg: ./scripts/db-tests.sh ./internal/sharing/
|
||||
|
||||
func testStore(t *testing.T) (*Store, *pgxpool.Pool, context.Context) {
|
||||
t.Helper()
|
||||
dsn := os.Getenv("TEST_DSN")
|
||||
if dsn == "" {
|
||||
t.Skip("TEST_DSN nicht gesetzt")
|
||||
}
|
||||
ctx := context.Background()
|
||||
pool, err := pgxpool.New(ctx, dsn)
|
||||
if err != nil {
|
||||
t.Fatalf("verbinden: %v", err)
|
||||
}
|
||||
t.Cleanup(pool.Close)
|
||||
return NewStore(pool), pool, ctx
|
||||
}
|
||||
|
||||
func konto(t *testing.T, pool *pgxpool.Pool, ctx context.Context, name string) uuid.UUID {
|
||||
t.Helper()
|
||||
var id uuid.UUID
|
||||
err := pool.QueryRow(ctx,
|
||||
`INSERT INTO users (username, display_name, password, is_admin)
|
||||
VALUES ($1, $1, 'x', false) RETURNING id`, name+"-"+uuid.NewString()[:8],
|
||||
).Scan(&id)
|
||||
if err != nil {
|
||||
t.Fatalf("konto %s: %v", name, err)
|
||||
}
|
||||
t.Cleanup(func() { _, _ = pool.Exec(ctx, `DELETE FROM users WHERE id = $1`, id) })
|
||||
return id
|
||||
}
|
||||
|
||||
func datei(t *testing.T, pool *pgxpool.Pool, ctx context.Context, owner uuid.UUID, name string) uuid.UUID {
|
||||
t.Helper()
|
||||
var id uuid.UUID
|
||||
err := pool.QueryRow(ctx,
|
||||
`INSERT INTO files (owner_id, folder_id, name, mime_type, size, checksum, storage_key)
|
||||
VALUES ($1, NULL, $2, 'application/pdf', 1, 'x', $3) RETURNING id`,
|
||||
owner, name, "k/"+uuid.NewString(),
|
||||
).Scan(&id)
|
||||
if err != nil {
|
||||
t.Fatalf("datei %s: %v", name, err)
|
||||
}
|
||||
return id
|
||||
}
|
||||
|
||||
func TestAbgelaufenerLinkGiltNichtMehr(t *testing.T) {
|
||||
s, pool, ctx := testStore(t)
|
||||
anna := konto(t, pool, ctx, "anna")
|
||||
f := datei(t, pool, ctx, anna, "Rechnung.pdf")
|
||||
|
||||
gestern := time.Now().Add(-1 * time.Hour)
|
||||
abgelaufen, err := s.Create(ctx, f, anna, "", &gestern)
|
||||
if err != nil {
|
||||
t.Fatalf("anlegen: %v", err)
|
||||
}
|
||||
if _, _, err := s.GetByToken(ctx, abgelaufen.Token); !errors.Is(err, ErrExpired) {
|
||||
t.Fatalf("abgelaufener Link muss ErrExpired liefern, kam: %v", err)
|
||||
}
|
||||
|
||||
// Gegenprobe – sonst prüfte der Test nur, dass irgendetwas fehlschlägt.
|
||||
morgen := time.Now().Add(1 * time.Hour)
|
||||
gueltig, err := s.Create(ctx, f, anna, "", &morgen)
|
||||
if err != nil {
|
||||
t.Fatalf("anlegen: %v", err)
|
||||
}
|
||||
if _, _, err := s.GetByToken(ctx, gueltig.Token); err != nil {
|
||||
t.Fatalf("gültiger Link muss durchgehen, kam: %v", err)
|
||||
}
|
||||
|
||||
// Ohne Ablauf gilt er unbegrenzt.
|
||||
ewig, err := s.Create(ctx, f, anna, "", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("anlegen: %v", err)
|
||||
}
|
||||
if _, _, err := s.GetByToken(ctx, ewig.Token); err != nil {
|
||||
t.Fatalf("Link ohne Ablauf muss gelten, kam: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestFremderLinkLaesstSichNichtLoeschen(t *testing.T) {
|
||||
s, pool, ctx := testStore(t)
|
||||
anna := konto(t, pool, ctx, "anna")
|
||||
bert := konto(t, pool, ctx, "bert")
|
||||
f := datei(t, pool, ctx, anna, "Gehaltsabrechnung.pdf")
|
||||
|
||||
link, err := s.Create(ctx, f, anna, "", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("anlegen: %v", err)
|
||||
}
|
||||
|
||||
// Bert kennt die ID – etwa, weil sie einmal irgendwo auftauchte.
|
||||
if err := s.Delete(ctx, link.ID, bert); !errors.Is(err, ErrNotFound) {
|
||||
t.Fatalf("Bert darf Annas Link nicht löschen, kam: %v", err)
|
||||
}
|
||||
// Und er ist wirklich noch da.
|
||||
if _, _, err := s.GetByToken(ctx, link.Token); err != nil {
|
||||
t.Fatalf("Link muss nach dem Fehlversuch noch gelten, kam: %v", err)
|
||||
}
|
||||
// Anna darf.
|
||||
if err := s.Delete(ctx, link.ID, anna); err != nil {
|
||||
t.Fatalf("Anna muss ihren Link löschen dürfen: %v", err)
|
||||
}
|
||||
if _, _, err := s.GetByToken(ctx, link.Token); !errors.Is(err, ErrNotFound) {
|
||||
t.Fatalf("gelöschter Link muss ErrNotFound liefern, kam: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestListeZeigtNurEigeneLinks(t *testing.T) {
|
||||
s, pool, ctx := testStore(t)
|
||||
anna := konto(t, pool, ctx, "anna")
|
||||
bert := konto(t, pool, ctx, "bert")
|
||||
fa := datei(t, pool, ctx, anna, "Annas.pdf")
|
||||
fb := datei(t, pool, ctx, bert, "Berts.pdf")
|
||||
|
||||
if _, err := s.Create(ctx, fa, anna, "", nil); err != nil {
|
||||
t.Fatalf("anlegen: %v", err)
|
||||
}
|
||||
if _, err := s.Create(ctx, fb, bert, "", nil); err != nil {
|
||||
t.Fatalf("anlegen: %v", err)
|
||||
}
|
||||
|
||||
annas, err := s.ListByOwner(ctx, anna)
|
||||
if err != nil {
|
||||
t.Fatalf("liste: %v", err)
|
||||
}
|
||||
if len(annas) != 1 {
|
||||
t.Fatalf("Anna soll genau einen Link sehen, sah %d", len(annas))
|
||||
}
|
||||
if annas[0].FileID != fa {
|
||||
t.Fatalf("Anna sieht einen fremden Link: %v", annas[0].FileID)
|
||||
}
|
||||
}
|
||||
|
||||
func TestPasswortVerlaesstDenSpeicherNichtVersehentlich(t *testing.T) {
|
||||
s, pool, ctx := testStore(t)
|
||||
anna := konto(t, pool, ctx, "anna")
|
||||
f := datei(t, pool, ctx, anna, "Vertrag.pdf")
|
||||
|
||||
mitPw, err := s.Create(ctx, f, anna, "argon2-hash-platzhalter", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("anlegen: %v", err)
|
||||
}
|
||||
// Das Anlegen gibt den Hash **nicht** zurück, nur die Tatsache.
|
||||
if !mitPw.HasPassword {
|
||||
t.Fatal("HasPassword muss true sein")
|
||||
}
|
||||
|
||||
// Beim Abruf über das Token kommt er heraus – dort wird er gebraucht,
|
||||
// um das eingegebene Passwort zu prüfen. Aber nur dort.
|
||||
link, hash, err := s.GetByToken(ctx, mitPw.Token)
|
||||
if err != nil {
|
||||
t.Fatalf("abrufen: %v", err)
|
||||
}
|
||||
if hash != "argon2-hash-platzhalter" {
|
||||
t.Fatalf("Hash muss beim Tokenabruf vorliegen, kam %q", hash)
|
||||
}
|
||||
if !link.HasPassword {
|
||||
t.Fatal("HasPassword muss auch beim Abruf true sein")
|
||||
}
|
||||
|
||||
// Die Liste für die Oberfläche kennt ihn nicht.
|
||||
liste, err := s.ListByOwner(ctx, anna)
|
||||
if err != nil {
|
||||
t.Fatalf("liste: %v", err)
|
||||
}
|
||||
if len(liste) != 1 || !liste[0].HasPassword {
|
||||
t.Fatalf("Liste soll den Link mit HasPassword zeigen, kam %+v", liste)
|
||||
}
|
||||
|
||||
// Ohne Passwort bleibt es false – sonst prüfte das oben nichts.
|
||||
ohnePw, err := s.Create(ctx, f, anna, "", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("anlegen: %v", err)
|
||||
}
|
||||
if ohnePw.HasPassword {
|
||||
t.Fatal("HasPassword muss ohne Passwort false sein")
|
||||
}
|
||||
if _, hash, _ := s.GetByToken(ctx, ohnePw.Token); hash != "" {
|
||||
t.Fatalf("ohne Passwort darf kein Hash kommen, kam %q", hash)
|
||||
}
|
||||
}
|
||||
|
||||
func TestTokenIstNichtZuErraten(t *testing.T) {
|
||||
s, pool, ctx := testStore(t)
|
||||
anna := konto(t, pool, ctx, "anna")
|
||||
f := datei(t, pool, ctx, anna, "Rechnung.pdf")
|
||||
|
||||
gesehen := map[string]bool{}
|
||||
for i := 0; i < 20; i++ {
|
||||
l, err := s.Create(ctx, f, anna, "", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("anlegen: %v", err)
|
||||
}
|
||||
// 24 Zufallsbytes, base64url ohne Auffüllen = 32 Zeichen. Wird das
|
||||
// jemals kürzer, ist das Token ratbar – und der Link ist das einzige
|
||||
// Geheimnis, das ihn schützt.
|
||||
if len(l.Token) != 32 {
|
||||
t.Fatalf("Token soll 32 Zeichen haben, hat %d: %q", len(l.Token), l.Token)
|
||||
}
|
||||
if gesehen[l.Token] {
|
||||
t.Fatalf("Token doppelt vergeben: %q", l.Token)
|
||||
}
|
||||
gesehen[l.Token] = true
|
||||
}
|
||||
}
|
||||
|
||||
func TestZaehlerZaehlt(t *testing.T) {
|
||||
s, pool, ctx := testStore(t)
|
||||
anna := konto(t, pool, ctx, "anna")
|
||||
f := datei(t, pool, ctx, anna, "Rechnung.pdf")
|
||||
|
||||
link, err := s.Create(ctx, f, anna, "", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("anlegen: %v", err)
|
||||
}
|
||||
if link.DownloadCount != 0 {
|
||||
t.Fatalf("frischer Link soll bei 0 stehen, stand bei %d", link.DownloadCount)
|
||||
}
|
||||
s.IncrementDownloads(ctx, link.Token)
|
||||
s.IncrementDownloads(ctx, link.Token)
|
||||
|
||||
nach, _, err := s.GetByToken(ctx, link.Token)
|
||||
if err != nil {
|
||||
t.Fatalf("abrufen: %v", err)
|
||||
}
|
||||
if nach.DownloadCount != 2 {
|
||||
t.Fatalf("Zähler soll 2 sein, ist %d", nach.DownloadCount)
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user