MyGO/mygo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor: add AppError helpers and harden upload/download error handling

Browse Source 
- refactor: replace inline AppError literals with model.New*Error
  constructors in service and handler layers
- feat: PromoteStaged falls back to copy/delete when os.Rename returns
  EXDEV (cross-device link)
- fix: upload error messages no longer leak internal multipart field
  names or parser implementation details
- fix: upload size violations are converted to domain error
  ErrUploadTooLarge at the handler boundary
- fix: download logs error and size mismatch when io.Copy fails or
  writes fewer bytes than expected
- test: add tests for field name leak prevention, parser error
  sanitization, read errors after response start, and EXDEV fallback
This commit is contained in:
Sunday
2026-07-05 17:35:34 +08:00
parent fc2b9312fa
commit 28e17a5b08
7 changed files with 305 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/handler/file.go
+56 -7
View File
@@ -15,6 +15,7 @@ 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"
)
@@ -24,6 +25,13 @@ const (
multipartFileField = "file"
)
var (
errMissingFileField = errors.New("missing file field")
errMissingFileName = errors.New("missing file name")
errUnexpectedMultipartField = errors.New("unexpected multipart field")
errInvalidMultipartForm = errors.New("invalid multipart form")
)
// FileHandler handles file management endpoints.
type FileHandler struct {
fileService *service.FileService
@@ -111,12 +119,12 @@ func (h *FileHandler) Upload(c *gin.Context) {
api.Error(c, http.StatusRequestEntityTooLarge, "request body is too large")
return
}
api.Error(c, http.StatusBadRequest, err.Error())
api.Error(c, http.StatusBadRequest, uploadPartErrorMessage(err))
return
}
defer part.Close()
info, err := h.fileService.Upload(c.Request.Context(), userID, parentID, fileName, part)
info, err := h.fileService.Upload(c.Request.Context(), userID, parentID, fileName, uploadBodyReader{reader: part})
if err != nil {
api.RespondError(c, err)
return
@@ -129,30 +137,55 @@ func nextUploadFilePart(reader *multipart.Reader) (*multipart.Part, string, erro
part, err := reader.NextPart()
if err != nil {
if err == io.EOF {
return nil, "", errors.New("missing file field")
return nil, "", errMissingFileField
}
return nil, "", err
return nil, "", fmt.Errorf("%w: %v", errInvalidMultipartForm, err)
}
if part.FormName() != multipartFileField {
part.Close()
return nil, "", fmt.Errorf("unexpected multipart field %q", part.FormName())
return nil, "", fmt.Errorf("%w: %q", errUnexpectedMultipartField, part.FormName())
}
fileName := part.FileName()
if fileName == "" {
part.Close()
return nil, "", errors.New("missing file name")
return nil, "", errMissingFileName
}
return part, fileName, nil
}
func uploadPartErrorMessage(err error) string {
switch {
case errors.Is(err, errMissingFileField):
return "missing file field"
case errors.Is(err, errMissingFileName):
return "missing file name"
case errors.Is(err, errUnexpectedMultipartField):
return "unexpected multipart field"
default:
return "invalid multipart form"
}
}
func isRequestBodyTooLarge(err error) bool {
var maxBytesErr *http.MaxBytesError
return errors.As(err, &maxBytesErr)
}
type uploadBodyReader struct {
reader io.Reader
}
func (r uploadBodyReader) Read(p []byte) (int, error) {
n, err := r.reader.Read(p)
if isRequestBodyTooLarge(err) {
return n, model.ErrUploadTooLarge
}
return n, err
}
// List handles GET /api/v1/files.
func (h *FileHandler) List(c *gin.Context) {
userID := middleware.MustGetUserID(c)
@@ -223,7 +256,23 @@ func (h *FileHandler) Download(c *gin.Context) {
c.Header("Content-Length", strconv.FormatInt(info.Size, 10))
c.Status(http.StatusOK)
io.Copy(c.Writer, reader)
written, err := io.Copy(c.Writer, reader)
if err != nil {
slog.WarnContext(c.Request.Context(), "download copy failed",
"user_id", userID,
"file_id", fileID,
"written", written,
"expected_size", info.Size,
"error", err)
return
}
if written != info.Size {
slog.WarnContext(c.Request.Context(), "download size mismatch",
"user_id", userID,
"file_id", fileID,
"written", written,
"expected_size", info.Size)
}
}
// Update handles PUT /api/v1/files/:id.
internal/handler/file_test.go
+112 -6
View File
@@ -4,6 +4,7 @@ import (
"bytes"
"context"
"encoding/json"
"errors"
"io"
"mime/multipart"
"net/http"
@@ -23,7 +24,8 @@ import (
// inMemStore implements storage.StorageBackend with an in-memory map.
type inMemStore struct {
files map[string][]byte
files map[string][]byte
failReads bool
}
func newInMemStore() *inMemStore {
@@ -54,6 +56,9 @@ func (s *inMemStore) Open(_ context.Context, path string) (io.ReadCloser, error)
if !ok {
return nil, &fsNotFoundError{path}
}
if s.failReads {
return io.NopCloser(&errorAfterReader{data: data, err: errors.New("download read failed")}), nil
}
return io.NopCloser(bytes.NewReader(data)), nil
}
@@ -70,13 +75,27 @@ type fsNotFoundError struct{ path string }
func (e *fsNotFoundError) Error() string { return "file not found on disk: " + e.path }
type errorAfterReader struct {
data []byte
err error
}
func (r *errorAfterReader) Read(p []byte) (int, error) {
if len(r.data) == 0 {
return 0, r.err
}
n := copy(p, r.data)
r.data = r.data[n:]
return n, nil
}
var _ storage.StorageBackend = (*inMemStore)(nil)
func setupFileHandler(t *testing.T) *FileHandler {
func setupFileHandler(t *testing.T) (*FileHandler, *inMemStore) {
return setupFileHandlerWithMaxUploadSize(t, 0)
}
func setupFileHandlerWithMaxUploadSize(t *testing.T, maxUploadSize int64) *FileHandler {
func setupFileHandlerWithMaxUploadSize(t *testing.T, maxUploadSize int64) (*FileHandler, *inMemStore) {
t.Helper()
db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{})
@@ -91,7 +110,7 @@ func setupFileHandlerWithMaxUploadSize(t *testing.T, maxUploadSize int64) *FileH
store := newInMemStore()
fileService := service.NewFileService(fileRepo, store, maxUploadSize, nil)
return NewFileHandler(fileService)
return NewFileHandler(fileService), store
}
func setupFileRouter(t *testing.T) *gin.Engine {
@@ -99,9 +118,14 @@ func setupFileRouter(t *testing.T) *gin.Engine {
}
func setupFileRouterWithMaxUploadSize(t *testing.T, maxUploadSize int64) *gin.Engine {
r, _ := setupFileRouterWithMaxUploadSizeAndStore(t, maxUploadSize)
return r
}
func setupFileRouterWithMaxUploadSizeAndStore(t *testing.T, maxUploadSize int64) (*gin.Engine, *inMemStore) {
t.Helper()
handler := setupFileHandlerWithMaxUploadSize(t, maxUploadSize)
handler, store := setupFileHandlerWithMaxUploadSize(t, maxUploadSize)
gin.SetMode(gin.TestMode)
r := gin.New()
@@ -125,7 +149,7 @@ func setupFileRouterWithMaxUploadSize(t *testing.T, maxUploadSize int64) *gin.En
files.DELETE("/:id", handler.Delete)
}
return r
return r, store
}
func TestFileHandler_Upload(t *testing.T) {
@@ -234,6 +258,52 @@ func TestFileHandler_UploadNoFile(t *testing.T) {
}
}
func TestFileHandler_UploadUnexpectedFieldDoesNotEchoFieldName(t *testing.T) {
r := setupFileRouter(t)
body := &bytes.Buffer{}
writer := multipart.NewWriter(body)
part, _ := writer.CreateFormField("secret_token")
part.Write([]byte("secret"))
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.StatusBadRequest {
t.Fatalf("status = %d, want %d; body = %s", rec.Code, http.StatusBadRequest, rec.Body.String())
}
if !strings.Contains(rec.Body.String(), "unexpected multipart field") {
t.Errorf("body = %s, want safe unexpected field message", rec.Body.String())
}
if strings.Contains(rec.Body.String(), "secret_token") {
t.Errorf("body leaks multipart field name: %s", rec.Body.String())
}
}
func TestFileHandler_UploadMalformedMultipartDoesNotLeakParserError(t *testing.T) {
r := setupFileRouter(t)
req := httptest.NewRequest(http.MethodPost, "/api/v1/files", strings.NewReader("not a valid multipart body"))
req.Header.Set("Content-Type", "multipart/form-data; boundary=mygo-boundary")
req.Header.Set("X-User-ID", "user1")
rec := httptest.NewRecorder()
r.ServeHTTP(rec, req)
if rec.Code != http.StatusBadRequest {
t.Fatalf("status = %d, want %d; body = %s", rec.Code, http.StatusBadRequest, rec.Body.String())
}
if !strings.Contains(rec.Body.String(), "invalid multipart form") {
t.Errorf("body = %s, want safe invalid multipart message", rec.Body.String())
}
if strings.Contains(rec.Body.String(), "NextPart") || strings.Contains(rec.Body.String(), "EOF") {
t.Errorf("body leaks parser internals: %s", rec.Body.String())
}
}
func TestFileHandler_CreateDir(t *testing.T) {
r := setupFileRouter(t)
@@ -380,6 +450,42 @@ func TestFileHandler_Download(t *testing.T) {
}
}
func TestFileHandler_DownloadReadErrorAfterResponseStarted(t *testing.T) {
r, store := setupFileRouterWithMaxUploadSizeAndStore(t, 0)
body := &bytes.Buffer{}
writer := multipart.NewWriter(body)
part, _ := writer.CreateFormFile("file", "download-error.txt")
content := []byte("partial response")
part.Write(content)
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)
var uploaded service.FileInfo
if err := json.Unmarshal(rec.Body.Bytes(), &uploaded); err != nil {
t.Fatalf("unmarshal upload: %v", err)
}
store.failReads = true
req = httptest.NewRequest(http.MethodGet, "/api/v1/files/"+uploaded.ID+"/content", nil)
req.Header.Set("X-User-ID", "user1")
rec = httptest.NewRecorder()
r.ServeHTTP(rec, req)
if rec.Code != http.StatusOK {
t.Errorf("status = %d, want %d", rec.Code, http.StatusOK)
}
if !bytes.Equal(rec.Body.Bytes(), content) {
t.Errorf("content = %q, want %q", rec.Body.String(), content)
}
}
func TestFileHandler_Update(t *testing.T) {
r := setupFileRouter(t)