From 92e40ed194ea4954cbb4f8d4f676b318ac27959b Mon Sep 17 00:00:00 2001 From: SlavaKuntsov Date: Thu, 17 Sep 2026 21:27:18 +0300 Subject: [PATCH 1/2] fix(upload): verify video ownership in complete and resumable --- services/upload/README.md | 3 + services/upload/cmd/server/main.go | 6 +- services/upload/internal/client/metadata.go | 40 +++++- .../upload/internal/client/metadata_test.go | 76 +++++++++++ services/upload/internal/handler/ownership.go | 33 +++++ services/upload/internal/handler/presign.go | 22 +++- .../upload/internal/handler/presign_test.go | 119 ++++++++++++++++-- services/upload/internal/handler/resumable.go | 17 ++- .../upload/internal/handler/resumable_test.go | 71 +++++++++-- services/upload/internal/storage/minio.go | 16 ++- 10 files changed, 370 insertions(+), 33 deletions(-) create mode 100644 services/upload/internal/client/metadata_test.go create mode 100644 services/upload/internal/handler/ownership.go diff --git a/services/upload/README.md b/services/upload/README.md index d591e09..f579158 100644 --- a/services/upload/README.md +++ b/services/upload/README.md @@ -23,6 +23,9 @@ open http://localhost:8003/swagger/doc.json ## Лимиты `UPLOAD_MAX_BYTES` (по умолчанию 5GB) ограничивает тело multipart-загрузки и resumable-путей (`PUT /api/v1/videos/{id}/resumable` — и полный PUT, и суммарный размер объекта по чанкам). Превышение → `413 Request Entity Too Large`. На gateway тот же лимит через `maxBytesMw`. +## Ownership (issue #46) +`complete`, `GET/PUT resumable` и presign-пути перед записью в `raw/{id}/*` проверяют владельца через metadata internal-API (`GET /internal/videos/{id}`, заголовок `X-Internal-Token` из `INTERNAL_TOKEN`): чужой id → `403`, несуществующий → `404`, metadata недоступна → `502` (fail-closed). `complete` дополнительно сверяет объект в MinIO: пустой или не-`video/*` content-type → `400`. + ## Линт / формат ```bash make fmt-go diff --git a/services/upload/cmd/server/main.go b/services/upload/cmd/server/main.go index 3ee3323..fd4a55d 100644 --- a/services/upload/cmd/server/main.go +++ b/services/upload/cmd/server/main.go @@ -81,10 +81,10 @@ func main() { } defer pub.Close() - metaCl := client.NewMetadataClient(metadataURL) + metaCl := client.NewMetadataClient(metadataURL, os.Getenv("INTERNAL_TOKEN")) uh := handler.NewUploadHandler(store, pub, metaCl) - ph := handler.NewPresignHandler(store, pub, metaCl) - rh := handler.NewResumableHandler(store) + ph := handler.NewPresignHandler(store, pub, metaCl, metaCl) + rh := handler.NewResumableHandler(store, metaCl) r := chi.NewRouter() r.Use(middleware.RequestID, middleware.Recoverer, mw.RequestLogger) diff --git a/services/upload/internal/client/metadata.go b/services/upload/internal/client/metadata.go index 3e1bf55..cef5ab9 100644 --- a/services/upload/internal/client/metadata.go +++ b/services/upload/internal/client/metadata.go @@ -5,21 +5,28 @@ import ( "encoding/json" "fmt" "net/http" + "net/url" ) type MetadataClient struct { - baseURL string - client *http.Client + baseURL string + internalToken string + client *http.Client } -func NewMetadataClient(baseURL string) *MetadataClient { - return &MetadataClient{baseURL: baseURL, client: &http.Client{}} +func NewMetadataClient(baseURL, internalToken string) *MetadataClient { + return &MetadataClient{baseURL: baseURL, internalToken: internalToken, client: &http.Client{}} } type CreateVideoResponse struct { ID string `json:"id"` } +type internalVideoResponse struct { + ID string `json:"id"` + OwnerID string `json:"owner_id"` +} + func (c *MetadataClient) CreateVideo(token, title, description string) (string, error) { body, _ := json.Marshal(map[string]string{"title": title, "description": description}) req, err := http.NewRequest("POST", c.baseURL+"/api/v1/videos", bytes.NewReader(body)) @@ -45,3 +52,28 @@ func (c *MetadataClient) CreateVideo(token, title, description string) (string, } return out.ID, nil } + +// GetVideoOwner resolves the owner of a video via metadata internal API +// (upload handlers must verify ownership before writing raw/{id}/* — issue #46). +func (c *MetadataClient) GetVideoOwner(videoID string) (string, error) { + req, err := http.NewRequest("GET", c.baseURL+"/internal/videos/"+url.PathEscape(videoID), nil) + if err != nil { + return "", err + } + if c.internalToken != "" { + req.Header.Set("X-Internal-Token", c.internalToken) + } + resp, err := c.client.Do(req) + if err != nil { + return "", fmt.Errorf("metadata owner: %w", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("metadata owner status %d", resp.StatusCode) + } + var out internalVideoResponse + if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + return "", err + } + return out.OwnerID, nil +} diff --git a/services/upload/internal/client/metadata_test.go b/services/upload/internal/client/metadata_test.go new file mode 100644 index 0000000..43adb17 --- /dev/null +++ b/services/upload/internal/client/metadata_test.go @@ -0,0 +1,76 @@ +package client + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestGetVideoOwner(t *testing.T) { + var gotPath, gotToken string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + gotToken = r.Header.Get("X-Internal-Token") + w.Header().Set("Content-Type", "application/json") + if gotToken != "test-internal-token" { + http.Error(w, `{"error":"unauthorized internal"}`, http.StatusUnauthorized) + return + } + switch r.URL.Path { + case "/internal/videos/vid-1": + _, _ = w.Write([]byte(`{"id":"vid-1","owner_id":"owner-1"}`)) + case "/internal/videos/vid-404": + http.Error(w, `{"error":"not found"}`, http.StatusNotFound) + default: + http.Error(w, `{"error":"unauthorized internal"}`, http.StatusUnauthorized) + } + })) + defer srv.Close() + + c := NewMetadataClient(srv.URL, "test-internal-token") + + owner, err := c.GetVideoOwner("vid-1") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if owner != "owner-1" { + t.Fatalf("owner = %q, want owner-1", owner) + } + if gotPath != "/internal/videos/vid-1" { + t.Fatalf("path = %q", gotPath) + } + if gotToken != "test-internal-token" { + t.Fatalf("X-Internal-Token = %q", gotToken) + } + + // unknown video: error must carry "404" so handlers classify it as not-found + _, err = c.GetVideoOwner("vid-404") + if err == nil || !strings.Contains(err.Error(), "404") { + t.Fatalf("err = %v, want it to contain 404", err) + } + + // wrong token -> 401 from metadata, not classified as 404 + cNoToken := NewMetadataClient(srv.URL, "wrong-token") + _, err = cNoToken.GetVideoOwner("vid-1") + if err == nil || strings.Contains(err.Error(), "404") { + t.Fatalf("err = %v, want non-404 for 401", err) + } +} + +func TestGetVideoOwnerOmitsEmptyToken(t *testing.T) { + var sawHeader bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sawHeader = r.Header.Get("X-Internal-Token") != "" + _, _ = w.Write([]byte(`{"id":"v","owner_id":"o"}`)) + })) + defer srv.Close() + + c := NewMetadataClient(srv.URL, "") + if _, err := c.GetVideoOwner("v"); err != nil { + t.Fatalf("unexpected err: %v", err) + } + if sawHeader { + t.Fatal("X-Internal-Token must be omitted when internalToken is empty") + } +} diff --git a/services/upload/internal/handler/ownership.go b/services/upload/internal/handler/ownership.go new file mode 100644 index 0000000..69a9d50 --- /dev/null +++ b/services/upload/internal/handler/ownership.go @@ -0,0 +1,33 @@ +package handler + +import ( + "net/http" + "strings" +) + +// OwnershipChecker resolves the video owner via metadata internal API. +// Required before any write to raw/{id}/* — presign/complete/resumable +// used to trust the video UUID from the URL (IDOR, issue #46). +type OwnershipChecker interface { + GetVideoOwner(videoID string) (string, error) +} + +// requireOwnership verifies userID owns videoID. On failure it writes the +// response (403 wrong owner, 404 unknown video, 502 metadata unavailable — +// fail closed) and returns false. +func requireOwnership(checker OwnershipChecker, videoID, userID string, w http.ResponseWriter) bool { + owner, err := checker.GetVideoOwner(videoID) + if err != nil { + if strings.Contains(err.Error(), "404") { + http.Error(w, `{"error":"video not found"}`, http.StatusNotFound) + return false + } + http.Error(w, `{"error":"metadata unavailable"}`, http.StatusBadGateway) + return false + } + if owner != userID { + http.Error(w, `{"error":"forbidden"}`, http.StatusForbidden) + return false + } + return true +} diff --git a/services/upload/internal/handler/presign.go b/services/upload/internal/handler/presign.go index 2d184e8..840db50 100644 --- a/services/upload/internal/handler/presign.go +++ b/services/upload/internal/handler/presign.go @@ -18,6 +18,7 @@ import ( type PresignStorage interface { PresignedPutObjectExternal(ctx context.Context, key string, expires time.Duration, publicEndpoint string) (string, error) StatObject(ctx context.Context, key string) error + StatObjectInfo(ctx context.Context, key string) (int64, string, error) } // We use context.Context but avoid import cycle; interface matches storage. @@ -41,10 +42,11 @@ type PresignHandler struct { storage PresignStorage publisher Publisher metadata MetadataCreator + owner OwnershipChecker } -func NewPresignHandler(s PresignStorage, p Publisher, m MetadataCreator) *PresignHandler { - return &PresignHandler{storage: s, publisher: p, metadata: m} +func NewPresignHandler(s PresignStorage, p Publisher, m MetadataCreator, o OwnershipChecker) *PresignHandler { + return &PresignHandler{storage: s, publisher: p, metadata: m, owner: o} } // Presign godoc @@ -139,11 +141,25 @@ func (h *PresignHandler) Complete(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"error":"video_id required"}`, 400) return } + // IDOR (issue #46): only the owner may complete an upload for a video id. + if !requireOwnership(h.owner, videoID, ownerID, w) { + return + } s3Key := fmt.Sprintf("raw/%s/original.mp4", videoID) - if err := h.storage.StatObject(r.Context(), s3Key); err != nil { + // Verify the uploaded object matches a real video PUT (issue #46). + size, contentType, err := h.storage.StatObjectInfo(r.Context(), s3Key) + if err != nil { http.Error(w, `{"error":"object not found, upload via presigned URL first"}`, 404) return } + if size == 0 { + http.Error(w, `{"error":"empty object"}`, 400) + return + } + if contentType != "" && !strings.HasPrefix(contentType, "video/") { + http.Error(w, `{"error":"unexpected content type: `+contentType+`"}`, 400) + return + } metrics.UploadBytes.Inc() ev := VideoUploadedEvent{VideoID: videoID, S3Key: s3Key, OwnerID: ownerID} if err := h.publisher.Publish(r.Context(), ev); err != nil { diff --git a/services/upload/internal/handler/presign_test.go b/services/upload/internal/handler/presign_test.go index 8b40f2d..57bcf8a 100644 --- a/services/upload/internal/handler/presign_test.go +++ b/services/upload/internal/handler/presign_test.go @@ -14,10 +14,12 @@ import ( ) type fakePresignStorage struct { - url string - err error - statErr error - presignedKey string + url string + err error + statErr error + statSize int64 + statContentType string + presignedKey string } func (f *fakePresignStorage) PresignedPutObjectExternal(_ context.Context, key string, _ time.Duration, _ string) (string, error) { @@ -37,11 +39,27 @@ func (f *fakePresignStorage) StatObject(_ context.Context, key string) error { return nil } +func (f *fakePresignStorage) StatObjectInfo(_ context.Context, key string) (int64, string, error) { + if f.statErr != nil { + return 0, "", f.statErr + } + return f.statSize, f.statContentType, nil +} + +// fakeOwnership is a stub OwnershipChecker: owner returns the owner id, +// err forces the metadata-unavailable path. +type fakeOwnership struct { + owner string + err error +} + +func (f *fakeOwnership) GetVideoOwner(string) (string, error) { return f.owner, f.err } + func TestPresignSuccess(t *testing.T) { meta := &fakeMeta{id: "vid-123"} st := &fakePresignStorage{} pub := &fakePub{} - h := NewPresignHandler(st, pub, meta) + h := NewPresignHandler(st, pub, meta, &fakeOwnership{owner: "owner1"}) body, _ := json.Marshal(map[string]string{"title": "hello", "filename": "test.mp4", "content_type": "video/mp4"}) req := httptest.NewRequest("POST", "/api/v1/videos/presign", bytes.NewReader(body)) @@ -71,7 +89,7 @@ func TestPresignSuccess(t *testing.T) { func TestPresignInvalidContentType(t *testing.T) { meta := &fakeMeta{id: "vid-1"} - h := NewPresignHandler(&fakePresignStorage{}, &fakePub{}, meta) + h := NewPresignHandler(&fakePresignStorage{}, &fakePub{}, meta, &fakeOwnership{owner: "owner1"}) body, _ := json.Marshal(map[string]string{"title": "x", "content_type": "image/jpeg"}) req := httptest.NewRequest("POST", "/api/v1/videos/presign", bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") @@ -85,9 +103,9 @@ func TestPresignInvalidContentType(t *testing.T) { func TestCompleteSuccess(t *testing.T) { meta := &fakeMeta{id: "vid-123"} - st := &fakePresignStorage{} + st := &fakePresignStorage{statSize: 1024, statContentType: "video/mp4"} pub := &fakePub{} - h := NewPresignHandler(st, pub, meta) + h := NewPresignHandler(st, pub, meta, &fakeOwnership{owner: "owner1"}) r := newChiRouter(h) req := httptest.NewRequest("POST", "/api/v1/videos/vid-123/complete", nil) @@ -109,7 +127,7 @@ func TestCompleteSuccess(t *testing.T) { func TestCompleteNotFound(t *testing.T) { meta := &fakeMeta{id: "vid-1"} st := &fakePresignStorage{statErr: errFake("not found")} - h := NewPresignHandler(st, &fakePub{}, meta) + h := NewPresignHandler(st, &fakePub{}, meta, &fakeOwnership{owner: "owner1"}) r := newChiRouter(h) req := httptest.NewRequest("POST", "/api/v1/videos/vid-1/complete", nil) req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, "owner1")) @@ -126,3 +144,86 @@ func newChiRouter(h *PresignHandler) http.Handler { r.Post("/api/v1/videos/complete", h.Complete) return r } + +// IDOR (issue #46): a foreign video id must be rejected with 403. + +func TestCompleteForeignVideoForbidden(t *testing.T) { + h := NewPresignHandler( + &fakePresignStorage{statSize: 1024, statContentType: "video/mp4"}, + &fakePub{}, &fakeMeta{id: "vid-1"}, + &fakeOwnership{owner: "real-owner"}, + ) + r := newChiRouter(h) + req := httptest.NewRequest("POST", "/api/v1/videos/vid-1/complete", nil) + req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, "attacker")) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code != http.StatusForbidden { + t.Fatalf("want 403 got %d %s", w.Code, w.Body.String()) + } +} + +func TestCompleteUnknownVideo(t *testing.T) { + h := NewPresignHandler( + &fakePresignStorage{}, + &fakePub{}, &fakeMeta{}, + &fakeOwnership{err: errFake("metadata owner status 404")}, + ) + r := newChiRouter(h) + req := httptest.NewRequest("POST", "/api/v1/videos/no-such-video/complete", nil) + req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, "owner1")) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("want 404 got %d %s", w.Code, w.Body.String()) + } +} + +func TestCompleteMetadataUnavailableForbidden(t *testing.T) { + // metadata unavailable -> fail closed (no ownership bypass) + h := NewPresignHandler( + &fakePresignStorage{statSize: 1024}, + &fakePub{}, &fakeMeta{id: "vid-1"}, + &fakeOwnership{err: errFake("connection refused")}, + ) + r := newChiRouter(h) + req := httptest.NewRequest("POST", "/api/v1/videos/vid-1/complete", nil) + req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, "owner1")) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code != http.StatusBadGateway { + t.Fatalf("want 502 got %d %s", w.Code, w.Body.String()) + } +} + +func TestCompleteRejectsEmptyObject(t *testing.T) { + h := NewPresignHandler( + &fakePresignStorage{}, + &fakePub{}, &fakeMeta{id: "vid-1"}, + &fakeOwnership{owner: "owner1"}, + ) + r := newChiRouter(h) + req := httptest.NewRequest("POST", "/api/v1/videos/vid-1/complete", nil) + req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, "owner1")) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("want 400 got %d %s", w.Code, w.Body.String()) + } +} + +func TestCompleteRejectsNonVideoContentType(t *testing.T) { + h := NewPresignHandler( + &fakePresignStorage{statSize: 1024, statContentType: "text/html"}, + &fakePub{}, &fakeMeta{id: "vid-1"}, + &fakeOwnership{owner: "owner1"}, + ) + r := newChiRouter(h) + req := httptest.NewRequest("POST", "/api/v1/videos/vid-1/complete", nil) + req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, "owner1")) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("want 400 got %d %s", w.Code, w.Body.String()) + } +} diff --git a/services/upload/internal/handler/resumable.go b/services/upload/internal/handler/resumable.go index ee587d8..3e3bb82 100644 --- a/services/upload/internal/handler/resumable.go +++ b/services/upload/internal/handler/resumable.go @@ -14,6 +14,8 @@ import ( "strings" "github.com/go-chi/chi/v5" + + mw "flowix/upload/internal/middleware" ) // ResumableStorage extends PresignStorage with size and object access. @@ -26,15 +28,16 @@ type ResumableStorage interface { type ResumableHandler struct { storage ResumableStorage + owner OwnershipChecker } -func NewResumableHandler(s ResumableStorage) *ResumableHandler { - return &ResumableHandler{storage: s} +func NewResumableHandler(s ResumableStorage, o OwnershipChecker) *ResumableHandler { + return &ResumableHandler{storage: s, owner: o} } // statusResponse for GET resumable offset. type statusResponse struct { - Uploaded int64 `json:"uploaded"` + Uploaded int64 `json:"uploaded"` Total *int64 `json:"total,omitempty"` } @@ -52,6 +55,10 @@ func (h *ResumableHandler) Status(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"error":"video_id required"}`, 400) return } + // IDOR (issue #46): only the owner may read the upload offset. + if !requireOwnership(h.owner, videoID, mw.UserIDFromCtx(r.Context()), w) { + return + } key := fmt.Sprintf("raw/%s/original.mp4", videoID) size, err := h.storage.StatObjectSize(r.Context(), key) if err != nil { @@ -103,6 +110,10 @@ func (h *ResumableHandler) Upload(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"error":"video_id required"}`, 400) return } + // IDOR (issue #46): only the owner may append/overwrite the raw object. + if !requireOwnership(h.owner, videoID, mw.UserIDFromCtx(r.Context()), w) { + return + } maxBytes := int64(5 << 30) // 5GB if v := os.Getenv("UPLOAD_MAX_BYTES"); v != "" { if n, err := strconv.ParseInt(v, 10, 64); err == nil && n > 0 { diff --git a/services/upload/internal/handler/resumable_test.go b/services/upload/internal/handler/resumable_test.go index 1e6c47a..d6da1c5 100644 --- a/services/upload/internal/handler/resumable_test.go +++ b/services/upload/internal/handler/resumable_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "testing" + mw "flowix/upload/internal/middleware" "github.com/go-chi/chi/v5" ) @@ -42,7 +43,6 @@ func (f *fakeResumableStorage) PutObject(ctx context.Context, key string, r io.R return nil } - func newResumableRouter(h *ResumableHandler) http.Handler { r := chi.NewRouter() r.Get("/api/v1/videos/{id}/resumable", h.Status) @@ -52,7 +52,7 @@ func newResumableRouter(h *ResumableHandler) http.Handler { func TestResumableStatusEmpty(t *testing.T) { st := newFakeResumable() - h := NewResumableHandler(st) + h := NewResumableHandler(st, &fakeOwnership{}) r := newResumableRouter(h) req := httptest.NewRequest("GET", "/api/v1/videos/vid1/resumable", nil) w := httptest.NewRecorder() @@ -67,7 +67,7 @@ func TestResumableStatusEmpty(t *testing.T) { func TestResumableUploadSingleChunk(t *testing.T) { st := newFakeResumable() - h := NewResumableHandler(st) + h := NewResumableHandler(st, &fakeOwnership{}) r := newResumableRouter(h) // first chunk 0-4/10 body := bytes.Repeat([]byte("a"), 5) @@ -96,7 +96,7 @@ func TestResumableUploadSingleChunk(t *testing.T) { func TestResumableRangeMismatch(t *testing.T) { st := newFakeResumable() st.data["raw/vid1/original.mp4"] = bytes.Repeat([]byte("x"), 5) - h := NewResumableHandler(st) + h := NewResumableHandler(st, &fakeOwnership{}) r := newResumableRouter(h) body := bytes.Repeat([]byte("a"), 5) req := httptest.NewRequest("PUT", "/api/v1/videos/vid1/resumable", bytes.NewReader(body)) @@ -110,7 +110,7 @@ func TestResumableRangeMismatch(t *testing.T) { func TestResumableFullPutNoRange(t *testing.T) { st := newFakeResumable() - h := NewResumableHandler(st) + h := NewResumableHandler(st, &fakeOwnership{}) r := newResumableRouter(h) body := bytes.Repeat([]byte("z"), 7) req := httptest.NewRequest("PUT", "/api/v1/videos/vid1/resumable", bytes.NewReader(body)) @@ -127,7 +127,7 @@ func TestResumableFullPutNoRange(t *testing.T) { func TestResumableFullPutTooLarge(t *testing.T) { t.Setenv("UPLOAD_MAX_BYTES", "16") st := newFakeResumable() - h := NewResumableHandler(st) + h := NewResumableHandler(st, &fakeOwnership{}) r := newResumableRouter(h) body := bytes.Repeat([]byte("z"), 17) req := httptest.NewRequest("PUT", "/api/v1/videos/vid1/resumable", bytes.NewReader(body)) @@ -144,7 +144,7 @@ func TestResumableFullPutTooLarge(t *testing.T) { func TestResumableChunkBodyTooLarge(t *testing.T) { t.Setenv("UPLOAD_MAX_BYTES", "16") st := newFakeResumable() - h := NewResumableHandler(st) + h := NewResumableHandler(st, &fakeOwnership{}) r := newResumableRouter(h) // объявлен chunkSize=16 (= maxBytes), тело 17 байт → MaxBytesError при чтении body := bytes.Repeat([]byte("a"), 17) @@ -163,7 +163,7 @@ func TestResumableChunkBodyTooLarge(t *testing.T) { func TestResumableChunkTooLarge(t *testing.T) { t.Setenv("UPLOAD_MAX_BYTES", "16") st := newFakeResumable() - h := NewResumableHandler(st) + h := NewResumableHandler(st, &fakeOwnership{}) r := newResumableRouter(h) // 8 bytes ok body := bytes.Repeat([]byte("a"), 8) @@ -187,3 +187,58 @@ func TestResumableChunkTooLarge(t *testing.T) { t.Fatalf("want 8 bytes kept got %d", len(st.data["raw/vid1/original.mp4"])) } } + +// IDOR (issue #46): a foreign video id must be rejected with 403. + +func TestResumableStatusForeignVideoForbidden(t *testing.T) { + h := NewResumableHandler(newFakeResumable(), &fakeOwnership{owner: "real-owner"}) + r := newResumableRouter(h) + req := httptest.NewRequest("GET", "/api/v1/videos/vid1/resumable", nil) + req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, "attacker")) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code != http.StatusForbidden { + t.Fatalf("want 403 got %d %s", w.Code, w.Body.String()) + } +} + +func TestResumableUploadForeignVideoForbidden(t *testing.T) { + st := newFakeResumable() + h := NewResumableHandler(st, &fakeOwnership{owner: "real-owner"}) + r := newResumableRouter(h) + req := httptest.NewRequest("PUT", "/api/v1/videos/vid1/resumable", bytes.NewReader([]byte("evil"))) + req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, "attacker")) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code != http.StatusForbidden { + t.Fatalf("want 403 got %d %s", w.Code, w.Body.String()) + } + if len(st.data) != 0 { + t.Fatalf("object must not be stored, got %d keys", len(st.data)) + } +} + +func TestResumableUploadUnknownVideo(t *testing.T) { + h := NewResumableHandler(newFakeResumable(), &fakeOwnership{err: errFake("metadata owner status 404")}) + r := newResumableRouter(h) + req := httptest.NewRequest("PUT", "/api/v1/videos/vid1/resumable", bytes.NewReader([]byte("x"))) + req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, "owner1")) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("want 404 got %d %s", w.Code, w.Body.String()) + } +} + +func TestResumableUploadMetadataUnavailableForbidden(t *testing.T) { + // metadata unavailable -> fail closed (no ownership bypass) + h := NewResumableHandler(newFakeResumable(), &fakeOwnership{err: errFake("connection refused")}) + r := newResumableRouter(h) + req := httptest.NewRequest("PUT", "/api/v1/videos/vid1/resumable", bytes.NewReader([]byte("x"))) + req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, "owner1")) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code != http.StatusBadGateway { + t.Fatalf("want 502 got %d %s", w.Code, w.Body.String()) + } +} diff --git a/services/upload/internal/storage/minio.go b/services/upload/internal/storage/minio.go index d241fd8..1bb182f 100644 --- a/services/upload/internal/storage/minio.go +++ b/services/upload/internal/storage/minio.go @@ -77,9 +77,9 @@ func (m *MinioClient) PresignedPutObjectExternal(ctx context.Context, key string pubEndpoint := pub.Host // If pubEndpoint lacks port, minio.New will default to 443/80; keep as is. tmpClient, err := minio.New(pubEndpoint, &minio.Options{ - Creds: credentials.NewStaticV4(m.accessKey, m.secretKey, ""), - Secure: pubSecure, - Region: "us-east-1", + Creds: credentials.NewStaticV4(m.accessKey, m.secretKey, ""), + Secure: pubSecure, + Region: "us-east-1", BucketLookup: minio.BucketLookupPath, }) if err != nil { @@ -120,6 +120,16 @@ func (m *MinioClient) StatObjectSize(ctx context.Context, key string) (int64, er return info.Size, nil } +// StatObjectInfo returns size and content-type of the object (complete must +// verify the uploaded object matches the presigned upload — issue #46). +func (m *MinioClient) StatObjectInfo(ctx context.Context, key string) (int64, string, error) { + info, err := m.client.StatObject(ctx, m.bucket, key, minio.StatObjectOptions{}) + if err != nil { + return 0, "", fmt.Errorf("stat %s: %w", key, err) + } + return info.Size, info.ContentType, nil +} + func (m *MinioClient) GetObject(ctx context.Context, key string) (io.ReadCloser, error) { obj, err := m.client.GetObject(ctx, m.bucket, key, minio.GetObjectOptions{}) if err != nil { From 3765f994bc38cdb24a37facb0e17a2817df58c8c Mon Sep 17 00:00:00 2001 From: SlavaKuntsov Date: Thu, 17 Sep 2026 21:27:18 +0300 Subject: [PATCH 2/2] chore(infra): pass INTERNAL_TOKEN to upload --- deploy/docker-compose.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/deploy/docker-compose.yml b/deploy/docker-compose.yml index 4d80ddb..c3a2fc4 100644 --- a/deploy/docker-compose.yml +++ b/deploy/docker-compose.yml @@ -217,6 +217,7 @@ services: MINIO_SECRET_KEY: ${MINIO_SECRET_KEY} VIDEO_STORAGE_BUCKET: ${VIDEO_STORAGE_BUCKET:-videos} METADATA_URL: http://metadata:8002 + INTERNAL_TOKEN: ${INTERNAL_TOKEN:-} JWT_SECRET: ${JWT_SECRET} UPLOAD_MAX_BYTES: ${UPLOAD_MAX_BYTES:-5368709120} MINIO_PUBLIC_ENDPOINT: ${MINIO_PUBLIC_ENDPOINT:-http://localhost:9000}