mirror of
https://github.com/multica-ai/multica.git
synced 2026-06-17 03:38:32 +02:00
Revert "MUL-3132: harden /uploads/* (auth, no listing, nosniff, tight CSP) (#…"
This reverts commit 8ff68502fc.
This commit is contained in:
LinYushen
@@ -449,42 +449,11 @@ func NewRouterWithOptions(pool *pgxpool.Pool, hub *realtime.Hub, bus *events.Bus
|
||||
realtime.HandleWebSocket(hub, mc, pr, slugResolver, w, r)
|
||||
})
|
||||
|
||||
// Local file serving (when using local storage).
|
||||
//
|
||||
// The disclosure (security-findings-2026-06-02) explicitly flagged this
|
||||
// route as a layer in the SVG-XSS chain: it was unauthenticated, served
|
||||
// directory listings, and lacked nosniff/CSP. The primary fix (PR #3023)
|
||||
// broke the inline-render step by forcing Content-Disposition: attachment
|
||||
// for non-media types, but the layered defenses here stay open until we
|
||||
// add them.
|
||||
//
|
||||
// Auth dispatch is two-track:
|
||||
// - Signed-query (?exp=&sig=): the route bypasses middleware.Auth and
|
||||
// ServeLocalUpload validates the HMAC signature itself. This is the
|
||||
// path used by token-auth clients (Desktop, legacy-token Web,
|
||||
// mobile) for inline <img>/<video>/<iframe> resource loads —
|
||||
// browsers do not attach Authorization headers to those, so a
|
||||
// server-side mediated short-lived signed URL is the only way to
|
||||
// keep them working after we authenticate /uploads/*. Mirror of
|
||||
// S3 + CloudFront's presigned-URL flow.
|
||||
// - Bearer / cookie: middleware.Auth runs first, then
|
||||
// ServeLocalUpload enforces workspace membership.
|
||||
// Local file serving (when using local storage)
|
||||
if local, ok := store.(*storage.LocalStorage); ok {
|
||||
inner := h.ServeLocalUpload(local)
|
||||
authed := middleware.Auth(queries, patCache, cloudPATVerifier)(inner)
|
||||
r.Get("/uploads/*", func(w http.ResponseWriter, r *http.Request) {
|
||||
// If the request carries BOTH signed-query parameters,
|
||||
// route it to the inner handler with no Auth wrapper —
|
||||
// the signature itself is the authority. ServeLocalUpload
|
||||
// fails the request closed if either is malformed or
|
||||
// expired, so a partial signature is not a downgrade
|
||||
// path to "no auth."
|
||||
exp, sig := storage.LocalUploadSignatureFromQuery(r.URL.Query())
|
||||
if exp != "" && sig != "" {
|
||||
inner.ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
authed.ServeHTTP(w, r)
|
||||
file := strings.TrimPrefix(r.URL.Path, "/uploads/")
|
||||
local.ServeFile(w, r, file)
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -15,7 +15,6 @@ import (
|
||||
"github.com/go-chi/chi/v5"
|
||||
"github.com/google/uuid"
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
"github.com/multica-ai/multica/server/internal/auth"
|
||||
"github.com/multica-ai/multica/server/internal/storage"
|
||||
db "github.com/multica-ai/multica/server/pkg/db/generated"
|
||||
)
|
||||
@@ -31,67 +30,6 @@ var extContentTypes = map[string]string{
|
||||
".wasm": "application/wasm",
|
||||
}
|
||||
|
||||
// uploadDeniedExtensions are file types the upload endpoint rejects outright.
|
||||
//
|
||||
// The defense is layered: the SVG-XSS chain documented in the
|
||||
// security-findings-2026-06-02 disclosure is already broken by the
|
||||
// Content-Disposition: attachment fix shipped in PR #3023 — every entry
|
||||
// here would be served as an attachment download anyway, never inline.
|
||||
// We still reject these at the upload edge because:
|
||||
//
|
||||
// - HTML-family content has no legitimate use case as an issue
|
||||
// attachment in this product (no rich-text export targets it),
|
||||
// yet it is the highest-leverage primitive an attacker would
|
||||
// reach for if a future regression weakens the disposition path.
|
||||
// - Rejecting at upload time gives operators a clean, auditable
|
||||
// "we don't accept this" signal in the request log instead of
|
||||
// silently storing the bytes and relying on the serve path to
|
||||
// stay safe forever.
|
||||
//
|
||||
// We intentionally do NOT reject .js / .svg here. Source-code
|
||||
// attachments preview as text/plain via /api/attachments/{id}/content
|
||||
// (see isTextPreviewable), and SVG logos / diagrams remain a common
|
||||
// legitimate upload — the existing SVG fix neutralizes them. Adding
|
||||
// either to this list would break those flows without adding meaningful
|
||||
// security on top of the disposition fix.
|
||||
var uploadDeniedExtensions = map[string]struct{}{
|
||||
".html": {},
|
||||
".htm": {},
|
||||
".xhtml": {},
|
||||
".shtml": {},
|
||||
".xht": {},
|
||||
".phtml": {},
|
||||
}
|
||||
|
||||
// uploadDeniedContentTypes mirrors uploadDeniedExtensions for the cases
|
||||
// where the extension is benign but the sniffed media type is not. Keeping
|
||||
// both gates means a renamed payload (logo.png that sniffs as text/html)
|
||||
// is still refused.
|
||||
var uploadDeniedContentTypes = map[string]struct{}{
|
||||
"text/html": {},
|
||||
"application/xhtml+xml": {},
|
||||
}
|
||||
|
||||
// isUploadDenied reports whether a multipart upload should be rejected based
|
||||
// on its filename extension or sniffed content type. Both gates are checked
|
||||
// because either alone is bypassable (rename a .html to .png and the
|
||||
// extension passes; ship literal HTML in a .png and the sniffer catches it).
|
||||
func isUploadDenied(filename, contentType string) bool {
|
||||
ext := strings.ToLower(path.Ext(filename))
|
||||
if _, denied := uploadDeniedExtensions[ext]; denied {
|
||||
return true
|
||||
}
|
||||
// Strip parameters (e.g. "text/html; charset=utf-8") before lookup.
|
||||
mediaType := strings.ToLower(strings.TrimSpace(contentType))
|
||||
if i := strings.IndexByte(mediaType, ';'); i >= 0 {
|
||||
mediaType = strings.TrimSpace(mediaType[:i])
|
||||
}
|
||||
if _, denied := uploadDeniedContentTypes[mediaType]; denied {
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
const maxUploadSize = 100 << 20 // 100 MB
|
||||
|
||||
const defaultAttachmentDownloadURLTTL = 30 * time.Minute
|
||||
@@ -149,27 +87,6 @@ func (h *Handler) attachmentToResponse(a db.Attachment) AttachmentResponse {
|
||||
if h.CFSigner != nil {
|
||||
resp.DownloadURL = h.CFSigner.SignedURL(a.Url, time.Now().Add(h.attachmentDownloadURLTTL()))
|
||||
}
|
||||
// LocalStorage backend: append HMAC signed-query params so the URL is
|
||||
// directly fetchable by native <img>/<video>/<iframe> resource loads
|
||||
// from token-auth clients (Desktop, legacy-token Web, mobile). After
|
||||
// MUL-3132 hardened /uploads/* with middleware.Auth, those clients
|
||||
// would otherwise see 401 on inline image rendering — they cannot
|
||||
// attach Authorization headers to native resource loads. We sign
|
||||
// over the storage key (not the URL host) so the value matches what
|
||||
// ServeLocalUpload re-derives from the request path. TTL mirrors
|
||||
// CloudFront mode (defaultAttachmentDownloadURLTTL = 30 min).
|
||||
if local, ok := h.Storage.(*storage.LocalStorage); ok {
|
||||
key := local.KeyFromURL(a.Url)
|
||||
expiry := time.Now().Add(h.attachmentDownloadURLTTL())
|
||||
signed := storage.SignLocalUploadURL(a.Url, key, auth.JWTSecret(), expiry)
|
||||
resp.URL = signed
|
||||
// /api/attachments/{id}/download still goes through the
|
||||
// authenticated proxy path, so we don't sign DownloadURL —
|
||||
// it requires Bearer/cookie like every other JSON API
|
||||
// endpoint. The signed URL above is what unblocks resource
|
||||
// loads; explicit downloads still use the authenticated
|
||||
// route.
|
||||
}
|
||||
if a.IssueID.Valid {
|
||||
s := uuidToString(a.IssueID)
|
||||
resp.IssueID = &s
|
||||
@@ -310,18 +227,6 @@ func (h *Handler) UploadFile(w http.ResponseWriter, r *http.Request) {
|
||||
if ct, ok := extContentTypes[strings.ToLower(path.Ext(header.Filename))]; ok {
|
||||
contentType = ct
|
||||
}
|
||||
|
||||
// Reject HTML-family uploads at the edge. The existing
|
||||
// Content-Disposition: attachment fix already prevents these from
|
||||
// rendering as documents, but there's no legitimate use for an
|
||||
// uploaded .html as an attachment in this product, so we refuse the
|
||||
// bytes outright rather than store them and rely on the serve path
|
||||
// staying perfectly correct forever. See uploadDeniedExtensions for
|
||||
// the full rationale.
|
||||
if isUploadDenied(header.Filename, contentType) {
|
||||
writeError(w, http.StatusUnsupportedMediaType, "this file type is not allowed")
|
||||
return
|
||||
}
|
||||
// Seek back so the full file is uploaded.
|
||||
if _, err := file.Seek(0, io.SeekStart); err != nil {
|
||||
writeError(w, http.StatusInternalServerError, "failed to read file")
|
||||
|
||||
@@ -1,140 +0,0 @@
|
||||
package handler
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"github.com/multica-ai/multica/server/internal/auth"
|
||||
"github.com/multica-ai/multica/server/internal/storage"
|
||||
)
|
||||
|
||||
// ServeLocalUpload returns an http.HandlerFunc that authorizes a request for
|
||||
// /uploads/<key> on the local-storage backend.
|
||||
//
|
||||
// Two acceptable auth paths:
|
||||
//
|
||||
// 1. Signed query string (?exp=&sig=) — minted by attachmentToResponse for
|
||||
// LocalStorage and bound to one specific key with a short TTL.
|
||||
// This is the only path that works for token-auth clients (Desktop,
|
||||
// legacy-token Web sessions, native mobile) because browsers do not
|
||||
// attach Authorization headers to native <img>/<video>/<iframe>
|
||||
// resource loads. Mirror of the S3 + CloudFront presigned-URL flow.
|
||||
// When valid, the request is served straight away — no further
|
||||
// workspace membership lookup, the signature itself is the authority
|
||||
// (the original metadata request that minted it already enforced
|
||||
// membership).
|
||||
//
|
||||
// 2. Bearer / cookie via middleware.Auth — used for direct fetches from
|
||||
// authenticated clients (server-side rendering, CLI, cookie-mode Web).
|
||||
// middleware.Auth must be applied to the chain before this handler;
|
||||
// here we rely on X-User-ID being set, then run the workspace /
|
||||
// user-prefix membership check.
|
||||
//
|
||||
// Path layout follows handler.UploadFile:
|
||||
//
|
||||
// - workspaces/{workspaceID}/{filename} → membership-gated read
|
||||
// - users/{userID}/{filename} → any authenticated user
|
||||
//
|
||||
// Anything else is rejected with 404. The disclosure
|
||||
// (security-findings-2026-06-02) called out that /uploads/* being
|
||||
// unauthenticated was one of the layers that made the SVG-XSS chain
|
||||
// weaponizable end-to-end; this handler closes that gap while preserving
|
||||
// inline image rendering for token-auth clients via the signed-query path.
|
||||
func (h *Handler) ServeLocalUpload(local *storage.LocalStorage) http.HandlerFunc {
|
||||
return func(w http.ResponseWriter, r *http.Request) {
|
||||
key := strings.TrimPrefix(r.URL.Path, "/uploads/")
|
||||
// Reject empty / directory-style paths up front. The storage
|
||||
// layer will also catch this; rejecting here keeps the 404
|
||||
// shape identical to the unrelated 404s on non-existent
|
||||
// keys, denying the directory-existence oracle.
|
||||
if key == "" || strings.HasSuffix(key, "/") {
|
||||
http.NotFound(w, r)
|
||||
return
|
||||
}
|
||||
|
||||
// (1) Signed-query auth path. If the client included exp / sig
|
||||
// query params, treat that as their chosen authentication
|
||||
// method and fail closed if either is missing or invalid.
|
||||
// We deliberately do NOT fall through to Bearer / cookie on a
|
||||
// broken signature: a leaked URL with an expired sig should
|
||||
// not silently start working again because the leaker also
|
||||
// happens to be authenticated. The frontend should re-fetch
|
||||
// the attachment metadata and get a fresh URL.
|
||||
if exp, sig := storage.LocalUploadSignatureFromQuery(r.URL.Query()); exp != "" || sig != "" {
|
||||
if exp == "" || sig == "" {
|
||||
http.Error(w, `{"error":"missing exp or sig"}`, http.StatusUnauthorized)
|
||||
return
|
||||
}
|
||||
if !storage.VerifyLocalUploadSignature(key, exp, sig, auth.JWTSecret(), time.Now()) {
|
||||
http.Error(w, `{"error":"signed URL expired or invalid"}`, http.StatusUnauthorized)
|
||||
return
|
||||
}
|
||||
local.ServeFile(w, r, key)
|
||||
return
|
||||
}
|
||||
|
||||
// (2) Bearer / cookie auth path. middleware.Auth has already
|
||||
// stamped X-User-ID by the time we get here.
|
||||
userID, ok := requireUserID(w, r)
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
|
||||
switch {
|
||||
case strings.HasPrefix(key, "workspaces/"):
|
||||
rest := strings.TrimPrefix(key, "workspaces/")
|
||||
slash := strings.Index(rest, "/")
|
||||
if slash <= 0 {
|
||||
// "workspaces/" or "workspaces/<id>" with no file —
|
||||
// directory listing in disguise.
|
||||
http.NotFound(w, r)
|
||||
return
|
||||
}
|
||||
workspaceID := rest[:slash]
|
||||
if !h.canReadWorkspaceUpload(r, userID, workspaceID) {
|
||||
// 404 rather than 403 so the absence of a workspace
|
||||
// and the lack of membership look identical from the
|
||||
// outside — denies the IDOR oracle.
|
||||
http.NotFound(w, r)
|
||||
return
|
||||
}
|
||||
|
||||
case strings.HasPrefix(key, "users/"):
|
||||
// Avatars and similar user-scoped assets. Any
|
||||
// authenticated user can read these — they are
|
||||
// routinely embedded in cross-workspace surfaces (member
|
||||
// lists, inbox items, mention chips). The auth gate
|
||||
// above is the access boundary; we don't gate on a
|
||||
// userID match.
|
||||
|
||||
default:
|
||||
// Unknown prefix — don't serve. New upload key shapes
|
||||
// must opt in here explicitly so they can't inherit a
|
||||
// relaxed policy by accident.
|
||||
http.NotFound(w, r)
|
||||
return
|
||||
}
|
||||
|
||||
local.ServeFile(w, r, key)
|
||||
}
|
||||
}
|
||||
|
||||
// canReadWorkspaceUpload returns true when the user is a member of the
|
||||
// workspace whose ID is embedded in a /uploads/workspaces/{id}/* path.
|
||||
// Uses the membership cache when available so every image fetch on a
|
||||
// busy issue page doesn't hit Postgres. The cache itself nil-handles,
|
||||
// so the explicit checks below are only for the empty-string inputs.
|
||||
func (h *Handler) canReadWorkspaceUpload(r *http.Request, userID, workspaceID string) bool {
|
||||
if workspaceID == "" || userID == "" {
|
||||
return false
|
||||
}
|
||||
if h.MembershipCache.Get(r.Context(), userID, workspaceID) {
|
||||
return true
|
||||
}
|
||||
if _, err := h.getWorkspaceMember(r.Context(), userID, workspaceID); err != nil {
|
||||
return false
|
||||
}
|
||||
h.MembershipCache.Set(r.Context(), userID, workspaceID)
|
||||
return true
|
||||
}
|
||||
@@ -1,621 +0,0 @@
|
||||
package handler
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"mime/multipart"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
"github.com/multica-ai/multica/server/internal/auth"
|
||||
"github.com/multica-ai/multica/server/internal/storage"
|
||||
db "github.com/multica-ai/multica/server/pkg/db/generated"
|
||||
)
|
||||
|
||||
// dbAttachmentForLocalSign builds a synthetic db.Attachment row pointing
|
||||
// at a /uploads/ URL — used by the LocalStorage signed-URL test to drive
|
||||
// attachmentToResponse without going through a full upload round-trip.
|
||||
func dbAttachmentForLocalSign(t *testing.T, key string) db.Attachment {
|
||||
t.Helper()
|
||||
id, err := uuid.NewV7()
|
||||
if err != nil {
|
||||
t.Fatalf("uuid.NewV7: %v", err)
|
||||
}
|
||||
wsUUID, err := uuid.Parse(testWorkspaceID)
|
||||
if err != nil {
|
||||
t.Fatalf("parse testWorkspaceID: %v", err)
|
||||
}
|
||||
uploaderUUID, err := uuid.Parse(testUserID)
|
||||
if err != nil {
|
||||
t.Fatalf("parse testUserID: %v", err)
|
||||
}
|
||||
return db.Attachment{
|
||||
ID: pgtype.UUID{Bytes: id, Valid: true},
|
||||
WorkspaceID: pgtype.UUID{Bytes: wsUUID, Valid: true},
|
||||
UploaderType: "member",
|
||||
UploaderID: pgtype.UUID{Bytes: uploaderUUID, Valid: true},
|
||||
Filename: "x.png",
|
||||
Url: "/uploads/" + key,
|
||||
ContentType: "image/png",
|
||||
SizeBytes: 4,
|
||||
CreatedAt: pgtype.Timestamptz{Time: time.Now(), Valid: true},
|
||||
}
|
||||
}
|
||||
|
||||
// TestIsUploadDenied is a pure-function check on the denylist used by
|
||||
// UploadFile. No DB / handler fixture required — runs in any environment.
|
||||
func TestIsUploadDenied(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
filename string
|
||||
contentType string
|
||||
want bool
|
||||
}{
|
||||
// Allowed shapes — these are the everyday legitimate uploads.
|
||||
{"png is allowed", "logo.png", "image/png", false},
|
||||
{"pdf is allowed", "report.pdf", "application/pdf", false},
|
||||
{"plain text is allowed", "notes.txt", "text/plain", false},
|
||||
// SVG is allowed at upload time — the SVG-XSS chain is broken
|
||||
// at the serve path (Content-Disposition: attachment) and SVG
|
||||
// logos / diagrams are a common legitimate upload.
|
||||
{"svg is allowed", "logo.svg", "image/svg+xml", false},
|
||||
// JS is allowed because source-code attachments preview as
|
||||
// text/plain via /api/attachments/{id}/content. Blocking it
|
||||
// here would break the preview feature without adding security
|
||||
// on top of the disposition fix.
|
||||
{"js source upload is allowed", "snippet.js", "application/javascript", false},
|
||||
|
||||
// Denied: HTML family by extension.
|
||||
{".html denied", "evil.html", "text/plain", true},
|
||||
{".htm denied", "evil.htm", "text/plain", true},
|
||||
{".xhtml denied", "evil.xhtml", "text/plain", true},
|
||||
{".shtml denied", "evil.shtml", "text/plain", true},
|
||||
{".xht denied", "evil.xht", "text/plain", true},
|
||||
{".phtml denied", "evil.phtml", "text/plain", true},
|
||||
|
||||
// Denied: HTML by sniffed content type even if extension is benign.
|
||||
// This is the renamed-payload case — logo.png that is actually
|
||||
// HTML must still be refused.
|
||||
{"text/html under image extension", "logo.png", "text/html", true},
|
||||
{"text/html with charset param", "logo.png", "text/html; charset=utf-8", true},
|
||||
{"application/xhtml+xml", "diagram.svg", "application/xhtml+xml", true},
|
||||
|
||||
// Case-insensitive on extension and content type.
|
||||
{"upper-case extension", "evil.HTML", "text/plain", true},
|
||||
{"upper-case content-type", "logo.png", "TEXT/HTML", true},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
if got := isUploadDenied(tc.filename, tc.contentType); got != tc.want {
|
||||
t.Errorf("isUploadDenied(%q, %q) = %v, want %v",
|
||||
tc.filename, tc.contentType, got, tc.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestUploadFile_RejectsHTMLByExtension verifies the upload-edge gate fires
|
||||
// when a caller tries to upload a .html file. Defense-in-depth on top of
|
||||
// the Content-Disposition: attachment fix from PR #3023.
|
||||
func TestUploadFile_RejectsHTMLByExtension(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
origStorage := testHandler.Storage
|
||||
testHandler.Storage = &mockStorage{}
|
||||
defer func() { testHandler.Storage = origStorage }()
|
||||
|
||||
var body bytes.Buffer
|
||||
writer := multipart.NewWriter(&body)
|
||||
part, err := writer.CreateFormFile("file", "evil.html")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
part.Write([]byte("<script>alert(1)</script>"))
|
||||
writer.Close()
|
||||
|
||||
req := httptest.NewRequest("POST", "/api/upload-file", &body)
|
||||
req.Header.Set("Content-Type", writer.FormDataContentType())
|
||||
req.Header.Set("X-User-ID", testUserID)
|
||||
req.Header.Set("X-Workspace-ID", testWorkspaceID)
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
testHandler.UploadFile(w, req)
|
||||
|
||||
if w.Code != http.StatusUnsupportedMediaType {
|
||||
t.Fatalf("expected 415 for .html upload, got %d: %s", w.Code, w.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestUploadFile_RejectsHTMLByContentType verifies the sniffer-side gate.
|
||||
// A caller renames an HTML payload to logo.png — the extension check
|
||||
// passes, but http.DetectContentType returns "text/html" so the
|
||||
// content-type denylist refuses the upload.
|
||||
func TestUploadFile_RejectsHTMLByContentType(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
origStorage := testHandler.Storage
|
||||
testHandler.Storage = &mockStorage{}
|
||||
defer func() { testHandler.Storage = origStorage }()
|
||||
|
||||
var body bytes.Buffer
|
||||
writer := multipart.NewWriter(&body)
|
||||
// Disguise as PNG — extension passes, content sniffs as text/html.
|
||||
part, err := writer.CreateFormFile("file", "logo.png")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
// Leading "<!DOCTYPE html" is the strongest text/html sniff signal.
|
||||
part.Write([]byte("<!DOCTYPE html><html><body><script>alert(1)</script></body></html>"))
|
||||
writer.Close()
|
||||
|
||||
req := httptest.NewRequest("POST", "/api/upload-file", &body)
|
||||
req.Header.Set("Content-Type", writer.FormDataContentType())
|
||||
req.Header.Set("X-User-ID", testUserID)
|
||||
req.Header.Set("X-Workspace-ID", testWorkspaceID)
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
testHandler.UploadFile(w, req)
|
||||
|
||||
if w.Code != http.StatusUnsupportedMediaType {
|
||||
t.Fatalf("expected 415 for renamed HTML payload, got %d: %s", w.Code, w.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestUploadFile_AllowsLegitimateImage is a regression guard: the new
|
||||
// denylist must not start refusing routine image uploads.
|
||||
func TestUploadFile_AllowsLegitimateImage(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
origStorage := testHandler.Storage
|
||||
testHandler.Storage = &mockStorage{}
|
||||
defer func() { testHandler.Storage = origStorage }()
|
||||
|
||||
var body bytes.Buffer
|
||||
writer := multipart.NewWriter(&body)
|
||||
part, err := writer.CreateFormFile("file", "logo.png")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
// Real PNG signature — DetectContentType returns image/png.
|
||||
part.Write([]byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A})
|
||||
writer.Close()
|
||||
|
||||
req := httptest.NewRequest("POST", "/api/upload-file", &body)
|
||||
req.Header.Set("Content-Type", writer.FormDataContentType())
|
||||
req.Header.Set("X-User-ID", testUserID)
|
||||
req.Header.Set("X-Workspace-ID", testWorkspaceID)
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
testHandler.UploadFile(w, req)
|
||||
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("expected 200 for legitimate PNG, got %d: %s", w.Code, w.Body.String())
|
||||
}
|
||||
|
||||
// Clean up the attachment row so this test is rerunnable.
|
||||
if _, err := testPool.Exec(
|
||||
context.Background(),
|
||||
`DELETE FROM attachment WHERE workspace_id = $1 AND filename = $2`,
|
||||
testWorkspaceID,
|
||||
"logo.png",
|
||||
); err != nil {
|
||||
t.Fatalf("cleanup attachment: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// TestServeLocalUpload_RequiresAuth verifies the handler refuses a request
|
||||
// where the upstream Auth middleware did not stamp X-User-ID. Auth is the
|
||||
// outer gate; this assertion confirms the inner handler does not have a
|
||||
// "default open" mode if ever reached without it.
|
||||
func TestServeLocalUpload_RequiresAuth(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/uploads/workspaces/"+testWorkspaceID+"/anything.png", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
|
||||
if rec.Code != http.StatusUnauthorized {
|
||||
t.Fatalf("expected 401 without X-User-ID, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestServeLocalUpload_MemberCanRead is the happy path: a workspace member
|
||||
// hitting their own workspace's upload bytes gets 200 + the file body.
|
||||
func TestServeLocalUpload_MemberCanRead(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
key := "workspaces/" + testWorkspaceID + "/abc.png"
|
||||
if _, err := local.Upload(context.Background(), key, []byte("body-bytes"), "image/png", "logo.png"); err != nil {
|
||||
t.Fatalf("Upload: %v", err)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/uploads/"+key, nil)
|
||||
req.Header.Set("X-User-ID", testUserID)
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
|
||||
if rec.Code != http.StatusOK {
|
||||
t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
if !strings.Contains(rec.Body.String(), "body-bytes") {
|
||||
t.Errorf("body did not match: %q", rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestServeLocalUpload_NonMemberDenied verifies that an authenticated user
|
||||
// hitting a workspace they do NOT belong to gets 404 (not 403, to avoid an
|
||||
// IDOR oracle that would let them probe for workspace IDs they have no
|
||||
// business knowing).
|
||||
func TestServeLocalUpload_NonMemberDenied(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
// Foreign workspace ID — testUserID is not a member.
|
||||
foreignWorkspaceID := "00000000-0000-0000-0000-000000000099"
|
||||
key := "workspaces/" + foreignWorkspaceID + "/abc.png"
|
||||
if _, err := local.Upload(context.Background(), key, []byte("foreign-body"), "image/png", "logo.png"); err != nil {
|
||||
t.Fatalf("Upload: %v", err)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/uploads/"+key, nil)
|
||||
req.Header.Set("X-User-ID", testUserID)
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
|
||||
if rec.Code != http.StatusNotFound {
|
||||
t.Fatalf("expected 404 for non-member, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
// 404 must not leak the bytes.
|
||||
if strings.Contains(rec.Body.String(), "foreign-body") {
|
||||
t.Errorf("response body leaked file contents: %q", rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestServeLocalUpload_RejectsDirectoryInPath verifies the handler refuses
|
||||
// requests whose path resolves to a directory or workspace root, even for
|
||||
// legitimately-authenticated members. This is the disclosure's
|
||||
// "directory listing" vector applied at the route layer.
|
||||
func TestServeLocalUpload_RejectsDirectoryInPath(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
// Seed two files so a listing would have something to leak.
|
||||
if _, err := local.Upload(context.Background(), "workspaces/"+testWorkspaceID+"/a.png", []byte("a"), "image/png", "a.png"); err != nil {
|
||||
t.Fatalf("Upload a: %v", err)
|
||||
}
|
||||
if _, err := local.Upload(context.Background(), "workspaces/"+testWorkspaceID+"/b.png", []byte("b"), "image/png", "b.png"); err != nil {
|
||||
t.Fatalf("Upload b: %v", err)
|
||||
}
|
||||
|
||||
cases := []string{
|
||||
"/uploads/workspaces/" + testWorkspaceID + "/",
|
||||
"/uploads/workspaces/" + testWorkspaceID,
|
||||
"/uploads/",
|
||||
"/uploads/workspaces/",
|
||||
}
|
||||
for _, path := range cases {
|
||||
t.Run(path, func(t *testing.T) {
|
||||
req := httptest.NewRequest(http.MethodGet, path, nil)
|
||||
req.Header.Set("X-User-ID", testUserID)
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
|
||||
if rec.Code == http.StatusOK {
|
||||
t.Errorf("status = 200, want 404 (directory request must not return 200)")
|
||||
}
|
||||
if strings.Contains(rec.Body.String(), "a.png") || strings.Contains(rec.Body.String(), "b.png") {
|
||||
t.Errorf("body leaked filenames: %q", rec.Body.String())
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestServeLocalUpload_UnknownPrefixDenied verifies the explicit-allowlist
|
||||
// behavior: a key prefix the handler doesn't know about must 404 instead
|
||||
// of falling through to the storage layer with no auth.
|
||||
func TestServeLocalUpload_UnknownPrefixDenied(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
if _, err := local.Upload(context.Background(), "secrets/admin.png", []byte("secret"), "image/png", "x.png"); err != nil {
|
||||
t.Fatalf("Upload: %v", err)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/uploads/secrets/admin.png", nil)
|
||||
req.Header.Set("X-User-ID", testUserID)
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
|
||||
if rec.Code != http.StatusNotFound {
|
||||
t.Fatalf("expected 404 for unknown prefix, got %d", rec.Code)
|
||||
}
|
||||
if strings.Contains(rec.Body.String(), "secret") {
|
||||
t.Errorf("body leaked file contents: %q", rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestServeLocalUpload_UserPrefixAllowsAnyAuthedUser confirms that the
|
||||
// /uploads/users/{userID}/* path is reachable by any authenticated user,
|
||||
// matching the avatar-display use case (member lists / inbox items
|
||||
// reference avatars across workspace boundaries).
|
||||
func TestServeLocalUpload_UserPrefixAllowsAnyAuthedUser(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
// Owner-by-someone-else avatar — the testUserID is reading
|
||||
// somebody else's avatar bytes.
|
||||
otherUserID := "00000000-0000-0000-0000-000000000088"
|
||||
key := "users/" + otherUserID + "/avatar.png"
|
||||
if _, err := local.Upload(context.Background(), key, []byte("avatar-body"), "image/png", "avatar.png"); err != nil {
|
||||
t.Fatalf("Upload: %v", err)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/uploads/"+key, nil)
|
||||
req.Header.Set("X-User-ID", testUserID)
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
|
||||
if rec.Code != http.StatusOK {
|
||||
t.Fatalf("expected 200 for /uploads/users/*, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
// TestServeLocalUpload_SignedQueryBypassesAuth verifies the new auth path:
|
||||
// a request that carries valid ?exp=&sig= query params is served WITHOUT
|
||||
// any X-User-ID header. This is what unblocks token-auth clients (Desktop,
|
||||
// legacy-token Web, mobile) on inline <img>/<video> resource loads.
|
||||
func TestServeLocalUpload_SignedQueryBypassesAuth(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
key := "workspaces/" + testWorkspaceID + "/signed.png"
|
||||
if _, err := local.Upload(context.Background(), key, []byte("signed-body"), "image/png", "x.png"); err != nil {
|
||||
t.Fatalf("Upload: %v", err)
|
||||
}
|
||||
|
||||
signed := storage.SignLocalUploadURL("/uploads/"+key, key, auth.JWTSecret(), time.Now().Add(5*time.Minute))
|
||||
req := httptest.NewRequest(http.MethodGet, signed, nil)
|
||||
// Deliberately NO X-User-ID — proves the signed query is the only
|
||||
// authority.
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
|
||||
if rec.Code != http.StatusOK {
|
||||
t.Fatalf("signed URL: expected 200, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
if !strings.Contains(rec.Body.String(), "signed-body") {
|
||||
t.Errorf("body did not match: %q", rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestServeLocalUpload_SignedQueryRejectsExpired verifies that a signed URL
|
||||
// past its expiry is refused even on the legitimate route. Otherwise leaked
|
||||
// URLs would last forever.
|
||||
func TestServeLocalUpload_SignedQueryRejectsExpired(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
key := "workspaces/" + testWorkspaceID + "/expired.png"
|
||||
if _, err := local.Upload(context.Background(), key, []byte("body"), "image/png", "x.png"); err != nil {
|
||||
t.Fatalf("Upload: %v", err)
|
||||
}
|
||||
|
||||
expired := storage.SignLocalUploadURL("/uploads/"+key, key, auth.JWTSecret(), time.Now().Add(-1*time.Minute))
|
||||
req := httptest.NewRequest(http.MethodGet, expired, nil)
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
|
||||
if rec.Code != http.StatusUnauthorized {
|
||||
t.Fatalf("expired signed URL: expected 401, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestServeLocalUpload_SignedQueryRejectsTampered confirms that flipping
|
||||
// any byte in the signature breaks verification.
|
||||
func TestServeLocalUpload_SignedQueryRejectsTampered(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
key := "workspaces/" + testWorkspaceID + "/tampered.png"
|
||||
if _, err := local.Upload(context.Background(), key, []byte("body"), "image/png", "x.png"); err != nil {
|
||||
t.Fatalf("Upload: %v", err)
|
||||
}
|
||||
|
||||
signed := storage.SignLocalUploadURL("/uploads/"+key, key, auth.JWTSecret(), time.Now().Add(5*time.Minute))
|
||||
// Flip a byte in the sig parameter without renormalizing.
|
||||
tampered := signed[:len(signed)-1] + "X"
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, tampered, nil)
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
|
||||
if rec.Code != http.StatusUnauthorized {
|
||||
t.Fatalf("tampered signed URL: expected 401, got %d", rec.Code)
|
||||
}
|
||||
}
|
||||
|
||||
// TestServeLocalUpload_SignedQueryBoundToOneKey is the IDOR check: a sig
|
||||
// minted for key A must not authorize a request for key B even when both
|
||||
// belong to the same workspace.
|
||||
func TestServeLocalUpload_SignedQueryBoundToOneKey(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
keyA := "workspaces/" + testWorkspaceID + "/a.png"
|
||||
keyB := "workspaces/" + testWorkspaceID + "/b.png"
|
||||
if _, err := local.Upload(context.Background(), keyA, []byte("body-a"), "image/png", "a.png"); err != nil {
|
||||
t.Fatalf("Upload a: %v", err)
|
||||
}
|
||||
if _, err := local.Upload(context.Background(), keyB, []byte("body-b"), "image/png", "b.png"); err != nil {
|
||||
t.Fatalf("Upload b: %v", err)
|
||||
}
|
||||
|
||||
// Sign for A, request B with A's signature.
|
||||
signed := storage.SignLocalUploadURL("/uploads/"+keyA, keyA, auth.JWTSecret(), time.Now().Add(5*time.Minute))
|
||||
parts := strings.SplitN(signed, "?", 2)
|
||||
if len(parts) != 2 {
|
||||
t.Fatalf("signed URL has no query: %s", signed)
|
||||
}
|
||||
bWithASig := "/uploads/" + keyB + "?" + parts[1]
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, bWithASig, nil)
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
|
||||
if rec.Code != http.StatusUnauthorized {
|
||||
t.Fatalf("cross-key sig: expected 401, got %d", rec.Code)
|
||||
}
|
||||
if strings.Contains(rec.Body.String(), "body-b") {
|
||||
t.Errorf("body leaked: %q", rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestServeLocalUpload_PartialSignedQueryFailsClosed: if exactly one of
|
||||
// exp or sig is present, the handler must reject rather than fall back to
|
||||
// "no signed query attempted."
|
||||
func TestServeLocalUpload_PartialSignedQueryFailsClosed(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
local := storage.NewLocalStorageFromEnv()
|
||||
if local == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
key := "workspaces/" + testWorkspaceID + "/partial.png"
|
||||
if _, err := local.Upload(context.Background(), key, []byte("body"), "image/png", "x.png"); err != nil {
|
||||
t.Fatalf("Upload: %v", err)
|
||||
}
|
||||
|
||||
cases := []string{
|
||||
"/uploads/" + key + "?exp=1700000000",
|
||||
"/uploads/" + key + "?sig=AAAA",
|
||||
}
|
||||
for _, urlStr := range cases {
|
||||
t.Run(urlStr, func(t *testing.T) {
|
||||
req := httptest.NewRequest(http.MethodGet, urlStr, nil)
|
||||
// Note: NOT setting X-User-ID. The handler reads
|
||||
// signed-query first; partial sig must fail-closed
|
||||
// here, not fall through to "user is unauthenticated"
|
||||
// which would also be 401 but for a different reason.
|
||||
rec := httptest.NewRecorder()
|
||||
testHandler.ServeLocalUpload(local)(rec, req)
|
||||
if rec.Code != http.StatusUnauthorized {
|
||||
t.Errorf("partial signed query: expected 401, got %d", rec.Code)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestAttachmentToResponse_LocalStorageMintsSignedURL is the integration
|
||||
// check: when the storage backend is LocalStorage, the URL surfaced in
|
||||
// the JSON response carries valid exp/sig query params. Without this
|
||||
// every <img src=attachment.url> in token-auth clients would 401.
|
||||
func TestAttachmentToResponse_LocalStorageMintsSignedURL(t *testing.T) {
|
||||
if testHandler == nil {
|
||||
t.Skip("test database not available")
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
origStorage := testHandler.Storage
|
||||
testHandler.Storage = storage.NewLocalStorageFromEnv()
|
||||
defer func() { testHandler.Storage = origStorage }()
|
||||
|
||||
// Build a synthetic Attachment row with a /uploads/ URL.
|
||||
att := dbAttachmentForLocalSign(t, "workspaces/"+testWorkspaceID+"/abc.png")
|
||||
resp := testHandler.attachmentToResponse(att)
|
||||
|
||||
if !strings.Contains(resp.URL, "exp=") || !strings.Contains(resp.URL, "sig=") {
|
||||
t.Fatalf("LocalStorage URL did not carry signed-query params: %s", resp.URL)
|
||||
}
|
||||
u, err := url.Parse(resp.URL)
|
||||
if err != nil {
|
||||
t.Fatalf("parse resp.URL: %v", err)
|
||||
}
|
||||
exp, sig := storage.LocalUploadSignatureFromQuery(u.Query())
|
||||
if !storage.VerifyLocalUploadSignature("workspaces/"+testWorkspaceID+"/abc.png", exp, sig, auth.JWTSecret(), time.Now()) {
|
||||
t.Errorf("minted URL did not verify: %s", resp.URL)
|
||||
}
|
||||
}
|
||||
@@ -156,24 +156,6 @@ func (s *LocalStorage) GetFilePath(key string) string {
|
||||
return filepath.Join(s.uploadDir, key)
|
||||
}
|
||||
|
||||
// servedUploadCSP is the per-response CSP override for /uploads/*.
|
||||
//
|
||||
// The application-wide CSP from middleware.ContentSecurityPolicy is
|
||||
// designed for the application origin (script-src 'self', style-src
|
||||
// allowlist, etc.). Uploads, however, are user-supplied content that
|
||||
// happens to be served from the same origin in the local-storage
|
||||
// deployment. A tight per-response CSP overrides the global one for
|
||||
// these responses so that even if a browser were coaxed into
|
||||
// rendering an upload as a document (via attempted MIME confusion,
|
||||
// content-type override, or future regression), the document cannot
|
||||
// run scripts, load remote resources, or be framed.
|
||||
//
|
||||
// `default-src 'none'` blocks every fetch directive, `sandbox`
|
||||
// strips origin privileges (no cookies, no DOM access for embedders),
|
||||
// and `frame-ancestors 'none'` makes the response unframable as a
|
||||
// belt-and-suspenders against clickjacking-style chains.
|
||||
const servedUploadCSP = "default-src 'none'; sandbox; frame-ancestors 'none'"
|
||||
|
||||
func (s *LocalStorage) ServeFile(w http.ResponseWriter, r *http.Request, filename string) {
|
||||
// The sidecar is an implementation detail of the local backend; refuse
|
||||
// to serve it directly so /uploads/<key>.meta.json doesn't become a
|
||||
@@ -184,18 +166,6 @@ func (s *LocalStorage) ServeFile(w http.ResponseWriter, r *http.Request, filenam
|
||||
return
|
||||
}
|
||||
|
||||
// Reject anything that would resolve to a directory listing. The
|
||||
// disclosure recommendation is explicit: even with UUID filenames,
|
||||
// enumerating every upload in a workspace ("/uploads/workspaces/{ws}/")
|
||||
// shouldn't be free. http.ServeFile would happily render such a
|
||||
// request as an HTML index when no index.html is present, so we cut
|
||||
// it off before the disk hit. An empty filename ("/uploads/") and a
|
||||
// trailing slash both indicate a directory request from the client.
|
||||
if filename == "" || strings.HasSuffix(filename, "/") {
|
||||
http.NotFound(w, r)
|
||||
return
|
||||
}
|
||||
|
||||
filePath := filepath.Join(s.uploadDir, filename)
|
||||
// filepath.Join cleans the path but doesn't enforce containment, so a
|
||||
// caller passing "../etc/passwd" lands outside uploadDir. http.ServeFile
|
||||
@@ -206,29 +176,8 @@ func (s *LocalStorage) ServeFile(w http.ResponseWriter, r *http.Request, filenam
|
||||
http.NotFound(w, r)
|
||||
return
|
||||
}
|
||||
// Stat the resolved path so we can refuse directories explicitly.
|
||||
// http.ServeFile would render an HTML index for a directory hit; this
|
||||
// guard runs first so the index is never generated.
|
||||
if info, err := os.Stat(filePath); err == nil && info.IsDir() {
|
||||
http.NotFound(w, r)
|
||||
return
|
||||
}
|
||||
slog.Info("serving file", "filename", filename, "filepath", filePath)
|
||||
|
||||
// Hardening headers applied to every successful upload response:
|
||||
// - X-Content-Type-Options: nosniff
|
||||
// Locks the browser to the Content-Type we declare, so an
|
||||
// uploaded .txt cannot be re-interpreted as a script via
|
||||
// content-type sniffing.
|
||||
// - Content-Security-Policy: tight per-response policy
|
||||
// (default-src 'none'; sandbox; frame-ancestors 'none').
|
||||
// Belt-and-suspenders defense if a future regression in the
|
||||
// Content-Disposition: attachment path lets a browser render
|
||||
// an upload as a document — the document still cannot
|
||||
// execute scripts or load anything cross-origin.
|
||||
w.Header().Set("X-Content-Type-Options", "nosniff")
|
||||
w.Header().Set("Content-Security-Policy", servedUploadCSP)
|
||||
|
||||
// Mirror the S3 Upload path: when sidecar metadata exists for this key,
|
||||
// set Content-Disposition with the original uploaded filename. Without
|
||||
// it, browsers download the file under the storage-key basename (the
|
||||
|
||||
@@ -1,130 +0,0 @@
|
||||
package storage
|
||||
|
||||
import (
|
||||
"crypto/hmac"
|
||||
"crypto/sha256"
|
||||
"encoding/base64"
|
||||
"net/url"
|
||||
"strconv"
|
||||
"strings"
|
||||
"time"
|
||||
)
|
||||
|
||||
// Signed-query auth for /uploads/* on the local-storage backend.
|
||||
//
|
||||
// The S3 + CloudFront deployment path lets clients fetch attachments
|
||||
// directly via a presigned URL — `<img src="...?Signature=...">` works
|
||||
// from any browser context because the credentials are inside the URL,
|
||||
// not in an Authorization header. The local-storage backend has no
|
||||
// equivalent: a self-hosted deployment with `/uploads/*` behind
|
||||
// `middleware.Auth` works for cookie-auth web clients (browser auto-sends
|
||||
// the cookie on `<img>` loads) but fails for token-auth clients (Desktop's
|
||||
// default mode, legacy localStorage Web sessions, the mobile app, anything
|
||||
// using `Authorization: Bearer ...`) because browsers do not attach the
|
||||
// Authorization header to native `<img>` / `<video>` resource loads.
|
||||
//
|
||||
// To close that gap without giving up the auth wins, we sign the upload
|
||||
// URLs the same way CloudFront does: append `?exp=<unix>&sig=<HMAC>` and
|
||||
// let `ServeLocalUpload` accept that signature as an alternative to the
|
||||
// Bearer / cookie path. The sig is HMAC-SHA256 over `key|exp` with the
|
||||
// server's JWTSecret. Each signed URL is bound to one specific key, has
|
||||
// a short TTL (we reuse `defaultAttachmentDownloadURLTTL` = 30 min, the
|
||||
// same TTL as CloudFront-mode), and cannot be widened to expose other
|
||||
// files in the bucket — exactly the same security model as the S3 path.
|
||||
//
|
||||
// The handler API surface is `SignLocalUploadURL` (callable from
|
||||
// attachmentToResponse to mint a fresh URL each time the metadata is
|
||||
// served) and `VerifyLocalUploadSignature` (callable from the request
|
||||
// path to authorize the read). The functions take the key and the secret
|
||||
// as parameters so the storage package does not depend on the auth
|
||||
// package — the caller injects `auth.JWTSecret()`.
|
||||
const (
|
||||
signedURLExpParam = "exp"
|
||||
signedURLSigParam = "sig"
|
||||
)
|
||||
|
||||
// SignLocalUploadURL returns rawURL with `?exp=<unix>&sig=<HMAC>` query
|
||||
// params appended. The signature is HMAC-SHA256 over `key|exp` with the
|
||||
// supplied secret.
|
||||
//
|
||||
// `key` MUST be the storage key (e.g. "workspaces/abc/file.png"), not
|
||||
// the full URL — `ServeLocalUpload` re-derives the key from the request
|
||||
// path and runs the same HMAC, so they have to match exactly. Picking
|
||||
// the key keeps the signature stable across local vs base-URL builds and
|
||||
// avoids canonicalization bugs with the URL host / scheme.
|
||||
//
|
||||
// `expiry` is an absolute time; the verifier compares it against
|
||||
// time.Now() with no skew tolerance because the signer and verifier are
|
||||
// the same process.
|
||||
//
|
||||
// If rawURL already has query parameters, they are preserved.
|
||||
func SignLocalUploadURL(rawURL, key string, secret []byte, expiry time.Time) string {
|
||||
exp := strconv.FormatInt(expiry.Unix(), 10)
|
||||
sig := computeLocalUploadSignature(key, exp, secret)
|
||||
q := signedQueryParams(exp, sig)
|
||||
|
||||
if idx := strings.IndexByte(rawURL, '?'); idx >= 0 {
|
||||
// Preserve existing query but ensure ours wins on collision.
|
||||
existing, err := url.ParseQuery(rawURL[idx+1:])
|
||||
if err == nil {
|
||||
existing.Del(signedURLExpParam)
|
||||
existing.Del(signedURLSigParam)
|
||||
merged := existing.Encode()
|
||||
if merged != "" {
|
||||
return rawURL[:idx+1] + merged + "&" + q
|
||||
}
|
||||
}
|
||||
return rawURL[:idx+1] + q
|
||||
}
|
||||
return rawURL + "?" + q
|
||||
}
|
||||
|
||||
// VerifyLocalUploadSignature returns true when (expStr, sigStr) is a
|
||||
// well-formed, unexpired signature for `key` produced by
|
||||
// SignLocalUploadURL with the same secret. Constant-time comparison and
|
||||
// hard parse-failure rejection ensure that a malformed input cannot fall
|
||||
// through to an "unsigned-but-trusted" branch.
|
||||
func VerifyLocalUploadSignature(key, expStr, sigStr string, secret []byte, now time.Time) bool {
|
||||
if key == "" || expStr == "" || sigStr == "" {
|
||||
return false
|
||||
}
|
||||
expUnix, err := strconv.ParseInt(expStr, 10, 64)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
if now.Unix() >= expUnix {
|
||||
return false
|
||||
}
|
||||
want := computeLocalUploadSignature(key, expStr, secret)
|
||||
// Both `want` and `sigStr` are URL-safe base64 strings of fixed
|
||||
// length, so a byte-wise constant-time compare is enough. A malformed
|
||||
// sigStr will simply not equal `want` and return false.
|
||||
return hmac.Equal([]byte(want), []byte(sigStr))
|
||||
}
|
||||
|
||||
func computeLocalUploadSignature(key, expStr string, secret []byte) string {
|
||||
mac := hmac.New(sha256.New, secret)
|
||||
// `|` is not part of the URL-safe base64 alphabet and cannot appear
|
||||
// in a Unix timestamp, so it is a safe separator and there is no
|
||||
// collision risk between (key, expStr) and (key', expStr') when
|
||||
// concatenated.
|
||||
mac.Write([]byte(key))
|
||||
mac.Write([]byte{'|'})
|
||||
mac.Write([]byte(expStr))
|
||||
return base64.RawURLEncoding.EncodeToString(mac.Sum(nil))
|
||||
}
|
||||
|
||||
func signedQueryParams(exp, sig string) string {
|
||||
v := url.Values{}
|
||||
v.Set(signedURLExpParam, exp)
|
||||
v.Set(signedURLSigParam, sig)
|
||||
return v.Encode()
|
||||
}
|
||||
|
||||
// LocalUploadSignatureFromQuery extracts the (exp, sig) pair from a
|
||||
// URL query string. Empty strings indicate "no signature present" —
|
||||
// callers should treat that as "fall through to other auth paths",
|
||||
// not as a verification failure.
|
||||
func LocalUploadSignatureFromQuery(q url.Values) (exp, sig string) {
|
||||
return q.Get(signedURLExpParam), q.Get(signedURLSigParam)
|
||||
}
|
||||
@@ -1,162 +0,0 @@
|
||||
package storage
|
||||
|
||||
import (
|
||||
"net/url"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// TestSignAndVerifyLocalUploadURL_RoundTrip is the happy-path sanity check:
|
||||
// a URL signed with secret S verifies under the same secret, before expiry.
|
||||
func TestSignAndVerifyLocalUploadURL_RoundTrip(t *testing.T) {
|
||||
secret := []byte("test-jwt-secret")
|
||||
key := "workspaces/abc/foo.png"
|
||||
rawURL := "/uploads/" + key
|
||||
expiry := time.Now().Add(30 * time.Minute)
|
||||
|
||||
signed := SignLocalUploadURL(rawURL, key, secret, expiry)
|
||||
u, err := url.Parse(signed)
|
||||
if err != nil {
|
||||
t.Fatalf("parse signed URL: %v", err)
|
||||
}
|
||||
exp, sig := LocalUploadSignatureFromQuery(u.Query())
|
||||
if exp == "" || sig == "" {
|
||||
t.Fatalf("signed URL missing exp/sig: %s", signed)
|
||||
}
|
||||
if !VerifyLocalUploadSignature(key, exp, sig, secret, time.Now()) {
|
||||
t.Fatalf("freshly signed URL did not verify: %s", signed)
|
||||
}
|
||||
}
|
||||
|
||||
// TestVerifyLocalUploadSignature_RejectsExpired confirms that we treat the
|
||||
// expiry as a hard cutoff. Without this, signed URLs would survive
|
||||
// indefinitely once leaked.
|
||||
func TestVerifyLocalUploadSignature_RejectsExpired(t *testing.T) {
|
||||
secret := []byte("test-jwt-secret")
|
||||
key := "workspaces/abc/foo.png"
|
||||
expired := time.Now().Add(-1 * time.Minute)
|
||||
|
||||
signed := SignLocalUploadURL("/uploads/"+key, key, secret, expired)
|
||||
u, _ := url.Parse(signed)
|
||||
exp, sig := LocalUploadSignatureFromQuery(u.Query())
|
||||
|
||||
if VerifyLocalUploadSignature(key, exp, sig, secret, time.Now()) {
|
||||
t.Fatalf("expired URL must not verify: %s", signed)
|
||||
}
|
||||
}
|
||||
|
||||
// TestVerifyLocalUploadSignature_RejectsTamperedSig confirms that any byte
|
||||
// flip in the signature value breaks verification. HMAC + constant-time
|
||||
// compare guarantees this; the test enforces it as a regression guard.
|
||||
func TestVerifyLocalUploadSignature_RejectsTamperedSig(t *testing.T) {
|
||||
secret := []byte("test-jwt-secret")
|
||||
key := "workspaces/abc/foo.png"
|
||||
|
||||
signed := SignLocalUploadURL("/uploads/"+key, key, secret, time.Now().Add(5*time.Minute))
|
||||
u, _ := url.Parse(signed)
|
||||
exp, sig := LocalUploadSignatureFromQuery(u.Query())
|
||||
|
||||
// Flip the last char of the sig.
|
||||
tampered := sig[:len(sig)-1] + flipChar(sig[len(sig)-1])
|
||||
if VerifyLocalUploadSignature(key, exp, tampered, secret, time.Now()) {
|
||||
t.Fatalf("tampered sig must not verify: %s", tampered)
|
||||
}
|
||||
|
||||
// Empty / random sigs must not verify.
|
||||
if VerifyLocalUploadSignature(key, exp, "", secret, time.Now()) {
|
||||
t.Errorf("empty sig must not verify")
|
||||
}
|
||||
if VerifyLocalUploadSignature(key, exp, "AAAAAAAAAAAA", secret, time.Now()) {
|
||||
t.Errorf("random sig must not verify")
|
||||
}
|
||||
}
|
||||
|
||||
// TestVerifyLocalUploadSignature_BoundToKey is the security-critical check:
|
||||
// a signature minted for key A must not authorize a request for key B. This
|
||||
// is what guarantees that leaking one signed URL does not grant the holder
|
||||
// access to the entire workspace's uploads.
|
||||
func TestVerifyLocalUploadSignature_BoundToKey(t *testing.T) {
|
||||
secret := []byte("test-jwt-secret")
|
||||
keyA := "workspaces/abc/file-a.png"
|
||||
keyB := "workspaces/abc/file-b.png"
|
||||
|
||||
signed := SignLocalUploadURL("/uploads/"+keyA, keyA, secret, time.Now().Add(5*time.Minute))
|
||||
u, _ := url.Parse(signed)
|
||||
exp, sig := LocalUploadSignatureFromQuery(u.Query())
|
||||
|
||||
// Sig was minted for keyA; it must NOT verify for keyB even though
|
||||
// the URL came from the same workspace and is unexpired.
|
||||
if VerifyLocalUploadSignature(keyB, exp, sig, secret, time.Now()) {
|
||||
t.Fatalf("sig for %q must not verify against %q", keyA, keyB)
|
||||
}
|
||||
// Sanity: it does verify for the original key.
|
||||
if !VerifyLocalUploadSignature(keyA, exp, sig, secret, time.Now()) {
|
||||
t.Fatalf("sig for %q failed to verify against itself", keyA)
|
||||
}
|
||||
}
|
||||
|
||||
// TestVerifyLocalUploadSignature_RejectsWrongSecret confirms that a signed
|
||||
// URL minted under a different secret does NOT verify. Protects against
|
||||
// the case where the JWT_SECRET env var changes (e.g. operator rotation):
|
||||
// in-flight signed URLs become invalid, never silently fall back to
|
||||
// "trust anyway."
|
||||
func TestVerifyLocalUploadSignature_RejectsWrongSecret(t *testing.T) {
|
||||
key := "workspaces/abc/foo.png"
|
||||
signed := SignLocalUploadURL("/uploads/"+key, key, []byte("old-secret"), time.Now().Add(5*time.Minute))
|
||||
u, _ := url.Parse(signed)
|
||||
exp, sig := LocalUploadSignatureFromQuery(u.Query())
|
||||
|
||||
if VerifyLocalUploadSignature(key, exp, sig, []byte("new-secret"), time.Now()) {
|
||||
t.Fatalf("URL signed under old secret must not verify under new secret")
|
||||
}
|
||||
}
|
||||
|
||||
// TestSignLocalUploadURL_PreservesExistingQuery is a regression guard: if
|
||||
// for any reason the stored URL already had query params (e.g. a future
|
||||
// CDN-facing URL with a cache-buster), signing must not drop them. Our
|
||||
// signed params win on collision so a stale exp/sig in the input gets
|
||||
// replaced.
|
||||
func TestSignLocalUploadURL_PreservesExistingQuery(t *testing.T) {
|
||||
secret := []byte("test-jwt-secret")
|
||||
key := "workspaces/abc/foo.png"
|
||||
rawURL := "/uploads/" + key + "?v=42"
|
||||
|
||||
signed := SignLocalUploadURL(rawURL, key, secret, time.Now().Add(5*time.Minute))
|
||||
if !strings.Contains(signed, "v=42") {
|
||||
t.Errorf("signed URL must preserve existing query params: %s", signed)
|
||||
}
|
||||
u, err := url.Parse(signed)
|
||||
if err != nil {
|
||||
t.Fatalf("parse signed URL: %v", err)
|
||||
}
|
||||
exp, sig := LocalUploadSignatureFromQuery(u.Query())
|
||||
if !VerifyLocalUploadSignature(key, exp, sig, secret, time.Now()) {
|
||||
t.Fatalf("signed URL with pre-existing query failed to verify")
|
||||
}
|
||||
}
|
||||
|
||||
// TestLocalUploadSignatureFromQuery_EmptyOnAbsence ensures the helper
|
||||
// returns "" for missing keys rather than panicking — callers
|
||||
// distinguish "no signed-query auth attempted" from "broken signed-query
|
||||
// auth" by both being empty.
|
||||
func TestLocalUploadSignatureFromQuery_EmptyOnAbsence(t *testing.T) {
|
||||
v := url.Values{}
|
||||
exp, sig := LocalUploadSignatureFromQuery(v)
|
||||
if exp != "" || sig != "" {
|
||||
t.Errorf("empty query must yield empty exp/sig, got %q/%q", exp, sig)
|
||||
}
|
||||
|
||||
v.Set("exp", "1700000000")
|
||||
exp, sig = LocalUploadSignatureFromQuery(v)
|
||||
if exp != "1700000000" || sig != "" {
|
||||
t.Errorf("partial query: exp=%q sig=%q", exp, sig)
|
||||
}
|
||||
}
|
||||
|
||||
func flipChar(c byte) string {
|
||||
if c == 'A' {
|
||||
return "B"
|
||||
}
|
||||
return "A"
|
||||
}
|
||||
@@ -7,7 +7,6 @@ import (
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
@@ -519,89 +518,3 @@ func TestLocalStorage_GetReader_MissingKey(t *testing.T) {
|
||||
t.Fatal("GetReader should error on missing key")
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
// TestLocalStorage_ServeFile_RejectsDirectoryListing verifies that a request
|
||||
// for /uploads/<dir> or /uploads/<dir>/ does NOT return an HTML index.
|
||||
// http.ServeFile renders such paths as a directory listing when no
|
||||
// index.html is present, which leaks every UUID filename in the workspace —
|
||||
// directly enabling the IDOR-by-enumeration step of the chain documented in
|
||||
// security-findings-2026-06-02.
|
||||
func TestLocalStorage_ServeFile_RejectsDirectoryListing(t *testing.T) {
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
|
||||
store := NewLocalStorageFromEnv()
|
||||
if store == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
// Seed two real upload-style files inside a workspace dir so a
|
||||
// listing would have something to leak.
|
||||
ctx := context.Background()
|
||||
if _, err := store.Upload(ctx, "workspaces/ws-1/aaa.png", []byte("body"), "image/png", "logo.png"); err != nil {
|
||||
t.Fatalf("Upload aaa.png: %v", err)
|
||||
}
|
||||
if _, err := store.Upload(ctx, "workspaces/ws-1/bbb.svg", []byte("body"), "image/svg+xml", "diagram.svg"); err != nil {
|
||||
t.Fatalf("Upload bbb.svg: %v", err)
|
||||
}
|
||||
|
||||
cases := []struct {
|
||||
name string
|
||||
key string
|
||||
}{
|
||||
{"empty key", ""},
|
||||
{"trailing slash on workspace dir", "workspaces/ws-1/"},
|
||||
{"workspace dir without slash", "workspaces/ws-1"},
|
||||
{"trailing slash on uploads root", ""},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
req := httptest.NewRequest(http.MethodGet, "/uploads/"+tc.key, nil)
|
||||
rec := httptest.NewRecorder()
|
||||
store.ServeFile(rec, req, tc.key)
|
||||
|
||||
// A directory listing would be 200 + an HTML body that
|
||||
// embeds the UUID filenames. Anything else is fine; we
|
||||
// assert specifically that the body does NOT mention any
|
||||
// of the seeded keys.
|
||||
body := rec.Body.String()
|
||||
if strings.Contains(body, "aaa.png") || strings.Contains(body, "bbb.svg") {
|
||||
t.Fatalf("body leaked directory contents: %q", body)
|
||||
}
|
||||
if rec.Code == http.StatusOK {
|
||||
t.Errorf("status = 200, want 404 (directory listing must not return 200)")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestLocalStorage_ServeFile_HardeningHeaders verifies that every successful
|
||||
// upload response carries the X-Content-Type-Options: nosniff header and a
|
||||
// tight per-response Content-Security-Policy. These are the two header
|
||||
// recommendations from the disclosure's hardening list.
|
||||
func TestLocalStorage_ServeFile_HardeningHeaders(t *testing.T) {
|
||||
tmpDir := t.TempDir()
|
||||
t.Setenv("LOCAL_UPLOAD_DIR", tmpDir)
|
||||
|
||||
store := NewLocalStorageFromEnv()
|
||||
if store == nil {
|
||||
t.Fatal("NewLocalStorageFromEnv returned nil")
|
||||
}
|
||||
|
||||
ctx := context.Background()
|
||||
if _, err := store.Upload(ctx, "workspaces/ws-1/abc.png", []byte("body"), "image/png", "logo.png"); err != nil {
|
||||
t.Fatalf("Upload: %v", err)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/uploads/workspaces/ws-1/abc.png", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
store.ServeFile(rec, req, "workspaces/ws-1/abc.png")
|
||||
|
||||
if got := rec.Header().Get("X-Content-Type-Options"); got != "nosniff" {
|
||||
t.Errorf("X-Content-Type-Options = %q, want %q", got, "nosniff")
|
||||
}
|
||||
if got := rec.Header().Get("Content-Security-Policy"); got != servedUploadCSP {
|
||||
t.Errorf("Content-Security-Policy = %q, want %q", got, servedUploadCSP)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user