From b988b4b15e7324f42e544da43b991071f9e4611c Mon Sep 17 00:00:00 2001 From: Huxley Date: Sun, 5 Jul 2026 17:18:19 +0800 Subject: [PATCH] fix(file)!: stream uploads through staged storage - fix: replace multipart form parsing with streaming multipart reads and apply request body limits when max_upload_size is configured. - refactor: route uploads through staging paths before promotion to long-term data paths, keeping incomplete uploads out of durable storage records. - test: cover oversized uploads, unlimited uploads, staged cleanup, and local storage promotion boundaries. - docs: document the staged upload model and multipart parent_id query parameter. --- docs/architecture.md | 6 +-- docs/decisions.md | 19 ++++++++ docs/roadmap.md | 2 +- internal/handler/file.go | 77 ++++++++++++++++++++++++----- internal/handler/file_test.go | 89 ++++++++++++++++++++++++++++++++-- internal/service/file.go | 86 ++++++++++++++++++++++++++++---- internal/service/file_test.go | 55 +++++++++++++++++++-- internal/storage/local.go | 69 +++++++++++++++++++++++++- internal/storage/local_test.go | 86 ++++++++++++++++++++++++++++---- internal/storage/storage.go | 18 ++++--- 10 files changed, 457 insertions(+), 50 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 638a631..b3cf1e9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -15,7 +15,7 @@ Repository (GORM data access) Storage (file I/O) Rules: - Handler has no business logic — parse request, call service, write response. - Service has no HTTP awareness — operates on domain models and interfaces. -- Repository abstracts the database; Storage abstracts where bytes live. +- Repository abstracts the database; Storage abstracts where bytes live, including staged upload promotion. - `internal/server` is the composition root — wires all dependencies together. ## Package Map @@ -34,7 +34,7 @@ Rules: | **Business** | `internal/service` | Business logic: `AuthService` (register, login, refresh, logout, passkey CRUD) | ✅ | | | `internal/model` | Domain types (User, File, Credential, Session), error codes | ✅ | | **Data** | `internal/repository` | Repository interfaces + GORM implementations (User, Session, File, Credential) | ✅ | -| | `internal/storage` | Storage backend interface + local disk impl | 🛠 WIP | +| | `internal/storage` | Storage backend interface + local disk impl, with staging and promotion | 🛠 WIP | | **Util** | `internal/auth` | JWT sign/verify (HS256), token type discrimination (access/refresh), password hashing (bcrypt), app passkey tokens | ✅ | | | `internal/api` | Unified JSON error response helpers | ✅ | @@ -54,7 +54,7 @@ POST /api/v1/account/passkeys DELETE /api/v1/account/passkeys/:id GET /api/v1/files -POST /api/v1/files +POST /api/v1/files # JSON creates directories; multipart uploads files. File upload parent_id is a query parameter. GET /api/v1/files/:id GET /api/v1/files/:id/content PUT /api/v1/files/:id diff --git a/docs/decisions.md b/docs/decisions.md index 2dd4732..5785aa5 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -67,3 +67,22 @@ - Refresh tokens cannot authenticate API requests; access tokens cannot be used to issue new token pairs. - The placeholder JWT secret is safe for local startup, but tokens signed with it are invalidated on restart. - New duration config fields require zero boilerplate — declare as `time.Duration` in the struct. + +## 2026-07-05: Staged File Uploads + +**Context**: Multipart uploads previously used `ParseMultipartForm`, which parses the request before service-level size checks and may spill oversized requests to temporary disk. The file service also wrote directly to the long-term storage path and then attempted compensating deletes on upload failure. + +**Decisions**: + +| Decision | Guidance | +|----------|----------| +| Stream multipart requests | Handlers use `Request.MultipartReader()` instead of `ParseMultipartForm`/`FormFile`, so uploads are streamed into the service. | +| Optional upload limits | `storage.max_upload_size = 0` means unlimited. Positive values enable both HTTP body limiting and service-level file content limiting. | +| Staging before promotion | Storage backends write upload bytes to a staging path first, then promote the object to the long-term data path only after validation succeeds. | +| Promote before DB create | The service promotes the object before creating the active file record, preventing visible DB rows from pointing at missing objects. If DB creation fails after promotion, the service best-effort deletes the promoted object. | +| Upload parent location | Multipart upload `parent_id` is passed as a query parameter, keeping the multipart body focused on the file stream. | + +**Consequences**: +- Interrupted, malformed, and oversized uploads leave only staging objects, which are safe to clean by path prefix and age. +- Local storage can promote with `os.Rename`; future S3 storage can implement promotion with copy/delete while keeping business visibility controlled by the DB row. +- A DB failure after promotion can still leave a long-term orphan object, but it is not visible through the file API and can be cleaned independently. diff --git a/docs/roadmap.md b/docs/roadmap.md index 2c35b87..c4f456c 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -20,7 +20,7 @@ Package-level implementation order (each task includes unit tests): 3. `internal/model` — domain types, error codes ✅ 4. `internal/api` — error response helpers ✅ 5. `internal/auth` — JWT utils ✅ -6. `internal/storage` — backend interface + local fs +6. `internal/storage` — backend interface + local fs with staged upload promotion 7. `internal/repository` — interfaces + GORM/SQLite impl ✅ 8. `internal/service` — auth, file, admin services ✅ (auth done) 9. `internal/middleware` — logger, cors, auth ✅ (auth done) diff --git a/internal/handler/file.go b/internal/handler/file.go index 99eb1b0..f1c14d6 100644 --- a/internal/handler/file.go +++ b/internal/handler/file.go @@ -1,10 +1,12 @@ package handler import ( + "errors" "fmt" "io" "log/slog" "mime" + "mime/multipart" "net/http" "net/url" "strconv" @@ -13,10 +15,15 @@ import ( "github.com/dhao2001/mygo/internal/api" "github.com/dhao2001/mygo/internal/middleware" - "github.com/dhao2001/mygo/internal/model" "github.com/dhao2001/mygo/internal/service" ) +const ( + jsonUploadBodyLimit = 64 << 10 + multipartUploadOverhead = 1 << 20 + multipartFileField = "file" +) + // FileHandler handles file management endpoints. type FileHandler struct { fileService *service.FileService @@ -47,9 +54,15 @@ func (h *FileHandler) Upload(c *gin.Context) { // Directory creation via JSON. mediaType, _, _ := mime.ParseMediaType(contentType) if mediaType == "application/json" { + c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, jsonUploadBodyLimit) + var req createDirRequest if err := c.ShouldBindJSON(&req); err != nil { slog.DebugContext(c.Request.Context(), "create dir bind failed", "error", err) + if isRequestBodyTooLarge(err) { + api.Error(c, http.StatusRequestEntityTooLarge, "request body is too large") + return + } api.Error(c, http.StatusBadRequest, "invalid request body") return } @@ -65,33 +78,45 @@ func (h *FileHandler) Upload(c *gin.Context) { } // File upload via multipart. - if err := c.Request.ParseMultipartForm(32 << 20); err != nil { - slog.DebugContext(c.Request.Context(), "parse multipart form failed", "error", err) - api.Error(c, http.StatusBadRequest, "invalid multipart form") + if mediaType != "multipart/form-data" { + api.Error(c, http.StatusBadRequest, "unsupported content type") return } - parentIDStr := c.PostForm("parent_id") + if maxUploadSize := h.fileService.MaxUploadSize(); maxUploadSize > 0 { + c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, maxUploadSize+multipartUploadOverhead) + } + var parentID *string + parentIDStr := c.Query("parent_id") if parentIDStr != "" { parentID = &parentIDStr } - header, err := c.FormFile("file") + reader, err := c.Request.MultipartReader() if err != nil { - slog.DebugContext(c.Request.Context(), "form file missing", "error", err) - api.Error(c, http.StatusBadRequest, "missing file field") + slog.DebugContext(c.Request.Context(), "create multipart reader failed", "error", err) + if isRequestBodyTooLarge(err) { + api.Error(c, http.StatusRequestEntityTooLarge, "request body is too large") + return + } + api.Error(c, http.StatusBadRequest, "invalid multipart form") return } - file, err := header.Open() + part, fileName, err := nextUploadFilePart(reader) if err != nil { - api.RespondError(c, model.NewInternalError("open uploaded file", err)) + slog.DebugContext(c.Request.Context(), "read multipart file part failed", "error", err) + if isRequestBodyTooLarge(err) { + api.Error(c, http.StatusRequestEntityTooLarge, "request body is too large") + return + } + api.Error(c, http.StatusBadRequest, err.Error()) return } - defer file.Close() + defer part.Close() - info, err := h.fileService.Upload(c.Request.Context(), userID, parentID, header.Filename, file) + info, err := h.fileService.Upload(c.Request.Context(), userID, parentID, fileName, part) if err != nil { api.RespondError(c, err) return @@ -100,6 +125,34 @@ func (h *FileHandler) Upload(c *gin.Context) { c.JSON(http.StatusCreated, info) } +func nextUploadFilePart(reader *multipart.Reader) (*multipart.Part, string, error) { + part, err := reader.NextPart() + if err != nil { + if err == io.EOF { + return nil, "", errors.New("missing file field") + } + return nil, "", err + } + + if part.FormName() != multipartFileField { + part.Close() + return nil, "", fmt.Errorf("unexpected multipart field %q", part.FormName()) + } + + fileName := part.FileName() + if fileName == "" { + part.Close() + return nil, "", errors.New("missing file name") + } + + return part, fileName, nil +} + +func isRequestBodyTooLarge(err error) bool { + var maxBytesErr *http.MaxBytesError + return errors.As(err, &maxBytesErr) +} + // List handles GET /api/v1/files. func (h *FileHandler) List(c *gin.Context) { userID := middleware.MustGetUserID(c) diff --git a/internal/handler/file_test.go b/internal/handler/file_test.go index 3f36118..99dacf2 100644 --- a/internal/handler/file_test.go +++ b/internal/handler/file_test.go @@ -8,6 +8,7 @@ import ( "mime/multipart" "net/http" "net/http/httptest" + "strings" "testing" "github.com/gin-gonic/gin" @@ -29,7 +30,7 @@ func newInMemStore() *inMemStore { return &inMemStore{files: make(map[string][]byte)} } -func (s *inMemStore) Save(_ context.Context, path string, reader io.Reader) (int64, error) { +func (s *inMemStore) SaveStaged(_ context.Context, path string, reader io.Reader) (int64, error) { data, err := io.ReadAll(reader) if err != nil { return 0, err @@ -38,6 +39,16 @@ func (s *inMemStore) Save(_ context.Context, path string, reader io.Reader) (int return int64(len(data)), nil } +func (s *inMemStore) PromoteStaged(_ context.Context, stagedPath, finalPath string) error { + data, ok := s.files[stagedPath] + if !ok { + return io.ErrUnexpectedEOF + } + s.files[finalPath] = data + delete(s.files, stagedPath) + return nil +} + func (s *inMemStore) Open(_ context.Context, path string) (io.ReadCloser, error) { data, ok := s.files[path] if !ok { @@ -46,6 +57,10 @@ func (s *inMemStore) Open(_ context.Context, path string) (io.ReadCloser, error) return io.NopCloser(bytes.NewReader(data)), nil } +func (s *inMemStore) DeleteStaged(ctx context.Context, path string) error { + return s.Delete(ctx, path) +} + func (s *inMemStore) Delete(_ context.Context, path string) error { delete(s.files, path) return nil @@ -58,6 +73,10 @@ func (e *fsNotFoundError) Error() string { return "file not found on disk: " + e var _ storage.StorageBackend = (*inMemStore)(nil) func setupFileHandler(t *testing.T) *FileHandler { + return setupFileHandlerWithMaxUploadSize(t, 0) +} + +func setupFileHandlerWithMaxUploadSize(t *testing.T, maxUploadSize int64) *FileHandler { t.Helper() db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) @@ -70,15 +89,19 @@ func setupFileHandler(t *testing.T) *FileHandler { fileRepo := repository.NewFileRepository(db) store := newInMemStore() - fileService := service.NewFileService(fileRepo, store, 0, nil) + fileService := service.NewFileService(fileRepo, store, maxUploadSize, nil) return NewFileHandler(fileService) } func setupFileRouter(t *testing.T) *gin.Engine { + return setupFileRouterWithMaxUploadSize(t, 0) +} + +func setupFileRouterWithMaxUploadSize(t *testing.T, maxUploadSize int64) *gin.Engine { t.Helper() - handler := setupFileHandler(t) + handler := setupFileHandlerWithMaxUploadSize(t, maxUploadSize) gin.SetMode(gin.TestMode) r := gin.New() @@ -133,6 +156,66 @@ func TestFileHandler_Upload(t *testing.T) { } } +func TestFileHandler_UploadRejectsOversizedFile(t *testing.T) { + r := setupFileRouterWithMaxUploadSize(t, 5) + + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + part, _ := writer.CreateFormFile("file", "too-large.txt") + part.Write([]byte("123456")) + writer.Close() + + req := httptest.NewRequest(http.MethodPost, "/api/v1/files", body) + req.Header.Set("Content-Type", writer.FormDataContentType()) + req.Header.Set("X-User-ID", "user1") + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusRequestEntityTooLarge { + t.Errorf("status = %d, want %d; body = %s", rec.Code, http.StatusRequestEntityTooLarge, rec.Body.String()) + } +} + +func TestFileHandler_UploadAllowsExactMaxUploadSize(t *testing.T) { + r := setupFileRouterWithMaxUploadSize(t, 5) + + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + part, _ := writer.CreateFormFile("file", "exact.txt") + part.Write([]byte("12345")) + writer.Close() + + req := httptest.NewRequest(http.MethodPost, "/api/v1/files", body) + req.Header.Set("Content-Type", writer.FormDataContentType()) + req.Header.Set("X-User-ID", "user1") + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusCreated { + t.Errorf("status = %d, want %d; body = %s", rec.Code, http.StatusCreated, rec.Body.String()) + } +} + +func TestFileHandler_UploadUnlimitedWhenMaxUploadSizeIsZero(t *testing.T) { + r := setupFileRouter(t) + + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + part, _ := writer.CreateFormFile("file", "unlimited.txt") + part.Write([]byte(strings.Repeat("a", 128))) + writer.Close() + + req := httptest.NewRequest(http.MethodPost, "/api/v1/files", body) + req.Header.Set("Content-Type", writer.FormDataContentType()) + req.Header.Set("X-User-ID", "user1") + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusCreated { + t.Errorf("status = %d, want %d; body = %s", rec.Code, http.StatusCreated, rec.Body.String()) + } +} + func TestFileHandler_UploadNoFile(t *testing.T) { r := setupFileRouter(t) diff --git a/internal/service/file.go b/internal/service/file.go index 8a720d4..74bca17 100644 --- a/internal/service/file.go +++ b/internal/service/file.go @@ -41,6 +41,8 @@ type FileList struct { Total int64 `json:"total"` } +var errUploadTooLarge = errors.New("upload exceeds maximum size") + // FileService handles file management business logic. type FileService struct { fileRepo repository.FileRepository @@ -67,6 +69,12 @@ func NewFileService( } } +// MaxUploadSize returns the configured maximum upload size in bytes. +// A value of 0 means uploads are not size-limited by MyGO. +func (s *FileService) MaxUploadSize() int64 { + return s.maxUploadSize +} + // Upload stores a file's content and creates its metadata record. // The MIME type is always detected server-side from the file content. func (s *FileService) Upload(ctx context.Context, userID string, parentID *string, fileName string, reader io.Reader) (*FileInfo, error) { @@ -87,14 +95,16 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin return nil, model.NewInternalError("check name conflict", err) } - // Limit the reader if a max upload size is configured. if s.maxUploadSize > 0 { - reader = io.LimitReader(reader, s.maxUploadSize+1) + reader = &uploadLimitReader{reader: reader, remaining: s.maxUploadSize} } // Detect MIME type from file content. head := make([]byte, 512) n, readErr := io.ReadFull(reader, head) + if isUploadTooLargeError(readErr) { + return nil, uploadTooLargeError(s.maxUploadSize) + } if readErr != nil && readErr != io.ErrUnexpectedEOF && readErr != io.EOF { return nil, model.NewInternalError("read for mime detection", readErr) } @@ -103,21 +113,30 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin reader = io.MultiReader(bytes.NewReader(head), reader) fileID := uuid.NewString() - storagePath := fmt.Sprintf("%s/%s", userID, fileID) + stagedPath := fmt.Sprintf("staging/%s/%s", userID, uuid.NewString()) + storagePath := fmt.Sprintf("data/%s/%s", userID, fileID) - // Compute SHA-256 hash while writing to storage. + // Compute SHA-256 hash while writing to staging storage. hasher := sha256.New() teeReader := io.TeeReader(reader, hasher) - written, err := s.storage.Save(ctx, storagePath, teeReader) + written, err := s.storage.SaveStaged(ctx, stagedPath, teeReader) if err != nil { - return nil, model.NewInternalError("save file", err) + _ = s.storage.DeleteStaged(ctx, stagedPath) + if isUploadTooLargeError(err) { + return nil, uploadTooLargeError(s.maxUploadSize) + } + return nil, model.NewInternalError("save staged file", err) } if s.maxUploadSize > 0 && written > s.maxUploadSize { - // Clean up the oversized file. - _ = s.storage.Delete(ctx, storagePath) - return nil, &model.AppError{Status: http.StatusBadRequest, Message: fmt.Sprintf("file size exceeds the maximum allowed size of %d bytes", s.maxUploadSize)} + _ = s.storage.DeleteStaged(ctx, stagedPath) + return nil, uploadTooLargeError(s.maxUploadSize) + } + + if err := s.storage.PromoteStaged(ctx, stagedPath, storagePath); err != nil { + _ = s.storage.DeleteStaged(ctx, stagedPath) + return nil, model.NewInternalError("promote staged file", err) } file := &model.File{ @@ -134,7 +153,7 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin } if err := s.fileRepo.Create(ctx, file); err != nil { - // Compensate: remove the stored file. + // Compensate: remove the promoted file. _ = s.storage.Delete(ctx, storagePath) if errors.Is(err, model.ErrDuplicate) { return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in this directory"} @@ -145,6 +164,53 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin return modelToFileInfo(file), nil } +type uploadLimitReader struct { + reader io.Reader + remaining int64 +} + +func (r *uploadLimitReader) Read(p []byte) (int, error) { + if len(p) == 0 { + return 0, nil + } + + if r.remaining == 0 { + var one [1]byte + n, err := r.reader.Read(one[:]) + if n > 0 { + p[0] = one[0] + return 1, errUploadTooLarge + } + return 0, err + } + + if int64(len(p)) > r.remaining { + p = p[:r.remaining] + } + + n, err := r.reader.Read(p) + r.remaining -= int64(n) + return n, err +} + +func isUploadTooLargeError(err error) bool { + if err == nil { + return false + } + if errors.Is(err, errUploadTooLarge) { + return true + } + var maxBytesErr *http.MaxBytesError + return errors.As(err, &maxBytesErr) +} + +func uploadTooLargeError(maxUploadSize int64) *model.AppError { + if maxUploadSize > 0 { + return &model.AppError{Status: http.StatusRequestEntityTooLarge, Message: fmt.Sprintf("file size exceeds the maximum allowed size of %d bytes", maxUploadSize)} + } + return &model.AppError{Status: http.StatusRequestEntityTooLarge, Message: "request body is too large"} +} + // Download returns a reader for the file's content and its metadata. // The caller must close the returned ReadCloser. func (s *FileService) Download(ctx context.Context, userID, fileID string) (io.ReadCloser, *FileInfo, error) { diff --git a/internal/service/file_test.go b/internal/service/file_test.go index b14fc2a..debc957 100644 --- a/internal/service/file_test.go +++ b/internal/service/file_test.go @@ -43,7 +43,7 @@ func newMemStorage() *memStorage { return &memStorage{files: make(map[string][]byte)} } -func (s *memStorage) Save(_ context.Context, path string, reader io.Reader) (int64, error) { +func (s *memStorage) SaveStaged(_ context.Context, path string, reader io.Reader) (int64, error) { data, err := io.ReadAll(reader) if err != nil { return 0, err @@ -54,6 +54,19 @@ func (s *memStorage) Save(_ context.Context, path string, reader io.Reader) (int return int64(len(data)), nil } +func (s *memStorage) PromoteStaged(_ context.Context, stagedPath, finalPath string) error { + s.mu.Lock() + defer s.mu.Unlock() + + data, ok := s.files[stagedPath] + if !ok { + return io.ErrUnexpectedEOF + } + s.files[finalPath] = data + delete(s.files, stagedPath) + return nil +} + func (s *memStorage) Open(_ context.Context, path string) (io.ReadCloser, error) { s.mu.RLock() data, ok := s.files[path] @@ -64,6 +77,10 @@ func (s *memStorage) Open(_ context.Context, path string) (io.ReadCloser, error) return io.NopCloser(bytes.NewReader(data)), nil } +func (s *memStorage) DeleteStaged(ctx context.Context, path string) error { + return s.Delete(ctx, path) +} + func (s *memStorage) Delete(_ context.Context, path string) error { s.mu.Lock() delete(s.files, path) @@ -74,6 +91,10 @@ func (s *memStorage) Delete(_ context.Context, path string) error { var _ storage.StorageBackend = (*memStorage)(nil) func setupFileServiceWithStorage(t *testing.T) (*FileService, *memStorage) { + return setupFileServiceWithStorageAndMaxUploadSize(t, 0) +} + +func setupFileServiceWithStorageAndMaxUploadSize(t *testing.T, maxUploadSize int64) (*FileService, *memStorage) { t.Helper() db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) @@ -87,7 +108,7 @@ func setupFileServiceWithStorage(t *testing.T) (*FileService, *memStorage) { fileRepo := repository.NewFileRepository(db) store := newMemStorage() - return NewFileService(fileRepo, store, 0, nil), store // 0 = unlimited, nil logger defaults to slog.Default() + return NewFileService(fileRepo, store, maxUploadSize, nil), store // nil logger defaults to slog.Default() } func setupFileService(t *testing.T) *FileService { @@ -125,6 +146,34 @@ func TestFileService_Upload(t *testing.T) { } } +func TestFileService_UploadRejectsOversizedFileAndCleansStaging(t *testing.T) { + svc, store := setupFileServiceWithStorageAndMaxUploadSize(t, 5) + ctx := context.Background() + + _, err := svc.Upload(ctx, "user1", nil, "too-large.txt", strings.NewReader("123456")) + assertAppError(t, err, http.StatusRequestEntityTooLarge) + + store.mu.RLock() + defer store.mu.RUnlock() + if len(store.files) != 0 { + t.Fatalf("storage files = %#v, want empty after oversized upload", store.files) + } +} + +func TestFileService_UploadUnlimitedWhenMaxUploadSizeIsZero(t *testing.T) { + svc := setupFileService(t) + ctx := context.Background() + + content := strings.Repeat("a", 128) + info, err := svc.Upload(ctx, "user1", nil, "unlimited.txt", strings.NewReader(content)) + if err != nil { + t.Fatalf("Upload = %v", err) + } + if info.Size != int64(len(content)) { + t.Errorf("Size = %d, want %d", info.Size, len(content)) + } +} + func TestFileService_UploadIntoDirectory(t *testing.T) { svc := setupFileService(t) ctx := context.Background() @@ -498,7 +547,7 @@ func TestFileService_SoftDeleteKeepsStorage(t *testing.T) { t.Fatalf("Delete = %v", err) } - storagePath := "user1/" + info.ID + storagePath := "data/user1/" + info.ID reader, err := store.Open(ctx, storagePath) if err != nil { t.Fatalf("storage should retain content after soft-delete, got: %v", err) diff --git a/internal/storage/local.go b/internal/storage/local.go index 61f0cf5..d8e781c 100644 --- a/internal/storage/local.go +++ b/internal/storage/local.go @@ -9,6 +9,11 @@ import ( "strings" ) +const ( + dataPathPrefix = "data" + stagingPathPrefix = "staging" +) + // LocalStorage implements StorageBackend using the local filesystem. type LocalStorage struct { basePath string @@ -24,12 +29,15 @@ func NewLocalStorage(basePath string) (*LocalStorage, error) { return &LocalStorage{basePath: abs}, nil } -// Save writes the contents of reader to path under the storage root. -func (s *LocalStorage) Save(_ context.Context, path string, reader io.Reader) (int64, error) { +// SaveStaged writes the contents of reader to a staging path under the storage root. +func (s *LocalStorage) SaveStaged(_ context.Context, path string, reader io.Reader) (int64, error) { fullPath := filepath.Join(s.basePath, path) if !isSubPath(s.basePath, fullPath) { return 0, fmt.Errorf("storage: path traversal attempt: %s", path) } + if !hasPathPrefix(path, stagingPathPrefix) { + return 0, fmt.Errorf("storage: staged path must be under %s/: %s", stagingPathPrefix, path) + } if err := os.MkdirAll(filepath.Dir(fullPath), 0750); err != nil { return 0, fmt.Errorf("create parent directory: %w", err) @@ -51,6 +59,41 @@ func (s *LocalStorage) Save(_ context.Context, path string, reader io.Reader) (i return written, nil } +// PromoteStaged moves a staged file to its final path under the storage root. +func (s *LocalStorage) PromoteStaged(_ context.Context, stagedPath, finalPath string) error { + fullStagedPath := filepath.Join(s.basePath, stagedPath) + if !isSubPath(s.basePath, fullStagedPath) { + return fmt.Errorf("storage: path traversal attempt: %s", stagedPath) + } + if !hasPathPrefix(stagedPath, stagingPathPrefix) { + return fmt.Errorf("storage: staged path must be under %s/: %s", stagingPathPrefix, stagedPath) + } + + fullFinalPath := filepath.Join(s.basePath, finalPath) + if !isSubPath(s.basePath, fullFinalPath) { + return fmt.Errorf("storage: path traversal attempt: %s", finalPath) + } + if !hasPathPrefix(finalPath, dataPathPrefix) { + return fmt.Errorf("storage: final path must be under %s/: %s", dataPathPrefix, finalPath) + } + + if _, err := os.Stat(fullFinalPath); err == nil { + return fmt.Errorf("final file already exists: %s", finalPath) + } else if !os.IsNotExist(err) { + return fmt.Errorf("check final file: %w", err) + } + + if err := os.MkdirAll(filepath.Dir(fullFinalPath), 0750); err != nil { + return fmt.Errorf("create parent directory: %w", err) + } + + if err := os.Rename(fullStagedPath, fullFinalPath); err != nil { + return fmt.Errorf("promote staged file: %w", err) + } + + return nil +} + // Open returns a reader for the file at path under the storage root. func (s *LocalStorage) Open(_ context.Context, path string) (io.ReadCloser, error) { fullPath := filepath.Join(s.basePath, path) @@ -68,6 +111,23 @@ func (s *LocalStorage) Open(_ context.Context, path string) (io.ReadCloser, erro return file, nil } +// DeleteStaged removes a staged file under the storage root. +func (s *LocalStorage) DeleteStaged(_ context.Context, path string) error { + fullPath := filepath.Join(s.basePath, path) + if !isSubPath(s.basePath, fullPath) { + return fmt.Errorf("storage: path traversal attempt: %s", path) + } + if !hasPathPrefix(path, stagingPathPrefix) { + return fmt.Errorf("storage: staged path must be under %s/: %s", stagingPathPrefix, path) + } + + err := os.Remove(fullPath) + if os.IsNotExist(err) { + return nil + } + return err +} + // Delete removes the file at path under the storage root. func (s *LocalStorage) Delete(_ context.Context, path string) error { fullPath := filepath.Join(s.basePath, path) @@ -95,3 +155,8 @@ func isSubPath(base, target string) bool { // filepath.Rel returns ".." or starts with "../" for paths outside base. return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) } + +func hasPathPrefix(path, prefix string) bool { + clean := filepath.Clean(path) + return clean == prefix || strings.HasPrefix(clean, prefix+string(filepath.Separator)) +} diff --git a/internal/storage/local_test.go b/internal/storage/local_test.go index a815847..5ee3d77 100644 --- a/internal/storage/local_test.go +++ b/internal/storage/local_test.go @@ -17,15 +17,19 @@ func TestSaveAndOpen(t *testing.T) { ctx := context.Background() content := "hello, mygo storage" - written, err := store.Save(ctx, "test/hello.txt", strings.NewReader(content)) + written, err := store.SaveStaged(ctx, "staging/test/hello.txt", strings.NewReader(content)) if err != nil { - t.Fatalf("Save: %v", err) + t.Fatalf("SaveStaged: %v", err) } if written != int64(len(content)) { t.Errorf("written = %d, want %d", written, len(content)) } - reader, err := store.Open(ctx, "test/hello.txt") + if err := store.PromoteStaged(ctx, "staging/test/hello.txt", "data/test/hello.txt"); err != nil { + t.Fatalf("PromoteStaged: %v", err) + } + + reader, err := store.Open(ctx, "data/test/hello.txt") if err != nil { t.Fatalf("Open: %v", err) } @@ -40,7 +44,7 @@ func TestSaveAndOpen(t *testing.T) { } // Verify the file exists on disk under the base path. - expectedPath := dir + "/test/hello.txt" + expectedPath := dir + "/data/test/hello.txt" if _, err := os.Stat(expectedPath); err != nil { t.Errorf("file not found on disk at %s: %v", expectedPath, err) } @@ -54,16 +58,19 @@ func TestDelete(t *testing.T) { } ctx := context.Background() - _, err = store.Save(ctx, "to_delete.txt", strings.NewReader("delete me")) + _, err = store.SaveStaged(ctx, "staging/to_delete.txt", strings.NewReader("delete me")) if err != nil { - t.Fatalf("Save: %v", err) + t.Fatalf("SaveStaged: %v", err) + } + if err := store.PromoteStaged(ctx, "staging/to_delete.txt", "data/to_delete.txt"); err != nil { + t.Fatalf("PromoteStaged: %v", err) } - if err := store.Delete(ctx, "to_delete.txt"); err != nil { + if err := store.Delete(ctx, "data/to_delete.txt"); err != nil { t.Fatalf("Delete: %v", err) } - _, err = store.Open(ctx, "to_delete.txt") + _, err = store.Open(ctx, "data/to_delete.txt") if err == nil { t.Error("Open should fail after delete") } @@ -91,9 +98,23 @@ func TestPathTraversalRejected(t *testing.T) { } ctx := context.Background() - _, err = store.Save(ctx, "../escape.txt", strings.NewReader("malicious")) + _, err = store.SaveStaged(ctx, "../escape.txt", strings.NewReader("malicious")) if err == nil { - t.Error("Save should reject path traversal") + t.Error("SaveStaged should reject path traversal") + } else if !strings.Contains(err.Error(), "path traversal") { + t.Errorf("expected path traversal error, got: %v", err) + } + + err = store.PromoteStaged(ctx, "../escape.txt", "data/escape.txt") + if err == nil { + t.Error("PromoteStaged should reject staged path traversal") + } else if !strings.Contains(err.Error(), "path traversal") { + t.Errorf("expected path traversal error, got: %v", err) + } + + err = store.PromoteStaged(ctx, "staging/escape.txt", "../escape.txt") + if err == nil { + t.Error("PromoteStaged should reject final path traversal") } else if !strings.Contains(err.Error(), "path traversal") { t.Errorf("expected path traversal error, got: %v", err) } @@ -111,6 +132,51 @@ func TestPathTraversalRejected(t *testing.T) { } else if !strings.Contains(err.Error(), "path traversal") { t.Errorf("expected path traversal error, got: %v", err) } + + err = store.DeleteStaged(ctx, "../escape.txt") + if err == nil { + t.Error("DeleteStaged should reject path traversal") + } else if !strings.Contains(err.Error(), "path traversal") { + t.Errorf("expected path traversal error, got: %v", err) + } +} + +func TestStagedPathsRequireStagingPrefix(t *testing.T) { + dir := t.TempDir() + store, err := NewLocalStorage(dir) + if err != nil { + t.Fatalf("NewLocalStorage: %v", err) + } + ctx := context.Background() + + _, err = store.SaveStaged(ctx, "data/not-staged.txt", strings.NewReader("bad")) + if err == nil { + t.Fatal("SaveStaged should reject non-staging path") + } + + err = store.DeleteStaged(ctx, "data/not-staged.txt") + if err == nil { + t.Fatal("DeleteStaged should reject non-staging path") + } +} + +func TestPromoteRequiresDataFinalPrefix(t *testing.T) { + dir := t.TempDir() + store, err := NewLocalStorage(dir) + if err != nil { + t.Fatalf("NewLocalStorage: %v", err) + } + ctx := context.Background() + + _, err = store.SaveStaged(ctx, "staging/promote.txt", strings.NewReader("content")) + if err != nil { + t.Fatalf("SaveStaged: %v", err) + } + + err = store.PromoteStaged(ctx, "staging/promote.txt", "other/promote.txt") + if err == nil { + t.Fatal("PromoteStaged should reject non-data final path") + } } func TestOpenMissingFile(t *testing.T) { diff --git a/internal/storage/storage.go b/internal/storage/storage.go index 7350cc6..198ac2a 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -6,18 +6,24 @@ import ( "io" ) -// StorageBackend abstracts where file content is written, read, and deleted. -// Paths passed to all methods are relative to the backend's root and must -// not contain path traversal sequences. +// StorageBackend abstracts where file content is staged, promoted, read, and +// deleted. Paths passed to all methods are relative to the backend's root and +// must not contain path traversal sequences. type StorageBackend interface { - // Save writes the contents of reader to path, creating parent directories - // as needed. It returns the number of bytes written. - Save(ctx context.Context, path string, reader io.Reader) (int64, error) + // SaveStaged writes the contents of reader to a staging path, creating + // parent directories as needed. It returns the number of bytes written. + SaveStaged(ctx context.Context, path string, reader io.Reader) (int64, error) + + // PromoteStaged moves a staged object to its final path. + PromoteStaged(ctx context.Context, stagedPath, finalPath string) error // Open returns a reader for the file at path. The caller must close the // returned ReadCloser. Open(ctx context.Context, path string) (io.ReadCloser, error) + // DeleteStaged removes a staged object. It is idempotent. + DeleteStaged(ctx context.Context, path string) error + // Delete removes the file at path. It is idempotent — deleting a // non-existent file is not an error. Delete(ctx context.Context, path string) error