From 6604ecb026c5cecfc3e55ff0be4fa8b6f0ebc1b4 Mon Sep 17 00:00:00 2001 From: Huxley Date: Thu, 16 Jul 2026 00:06:31 +0800 Subject: [PATCH] feat(file): conceal file resource existence from other users - feat: cross-user file access returns not-found instead of permission-denied - fix: repository delete now only affects active rows and returns ErrNotFound when no row changes; repeated deletes yield not-found - feat: parent directory operations (list, upload, create-dir, move) conceal ownership of other user's directories - test: update handler, service, repository, and integration tests to assert not-found behavior for cross-user and missing file operations --- docs/server/decisions.md | 17 +++ docs/server/roadmap.md | 2 +- internal/handler/file_test.go | 35 +++++- internal/repository/file.go | 8 +- internal/repository/file_test.go | 10 +- internal/server/file_integration_test.go | 149 +++++++++++++++++++++-- internal/service/file.go | 11 +- internal/service/file_test.go | 102 +++++++++++++--- 8 files changed, 292 insertions(+), 42 deletions(-) diff --git a/docs/server/decisions.md b/docs/server/decisions.md index 99ea246..c3f756c 100644 --- a/docs/server/decisions.md +++ b/docs/server/decisions.md @@ -106,3 +106,20 @@ - REST remains a handler/API concern, and future protocols can reuse services without HTTP leakage. - Error responses keep the same top-level shape, with optional `log_id` instead of embedding log references in the message. - Admin and auth middleware behavior is testable through service contracts rather than database access. + +## 2026-07-15: Conceal File Resource Existence + +**Context**: Returning permission-denied errors for cross-user file access allowed authenticated callers to distinguish another user's resource from a missing resource. Idempotent success for deleting missing files also conflicted with a consistent not-found concealment policy. + +**Decisions**: + +| Decision | Guidance | +|----------|----------| +| Conceal ownership | File and parent-directory ownership failures return the same not-found kind and safe message as missing or deleted resources. | +| Strict delete result | Deleting an active owned file returns success once; missing, deleted, and cross-user targets return not found. | +| Atomic soft delete | Repository deletes update only active rows and return `model.ErrNotFound` when no row changes. | + +**Consequences**: +- Authenticated file operations cannot use status codes or messages to determine whether another user's resource exists. +- A successful first delete returns `204 No Content`; repeated deletes return `404 Not Found` while remaining idempotent in effect. +- Unauthenticated requests remain `401 Unauthorized`, non-empty directory deletion remains `409 Conflict`, and permission-denied behavior outside the file boundary remains unchanged. diff --git a/docs/server/roadmap.md b/docs/server/roadmap.md index 9264a9e..91fa845 100644 --- a/docs/server/roadmap.md +++ b/docs/server/roadmap.md @@ -27,7 +27,7 @@ Package-level implementation order (each task includes unit tests): 10. `internal/handler` — auth, account, file, admin handlers 🛠 (HTTP DTO mapping in place) 11. `internal/server` — Gin router, route registration, graceful shutdown ✅ 12. `cmd/serve.go`, `cmd/config.go`, `cmd/status.go` ✅ (serve done) -13. Integration tests 🛠 (authenticated register → login → upload → list → download route flow covered with a small file; file API authentication bypass, invalid token, and cross-user isolation boundaries covered) +13. Integration tests 🛠 (authenticated register → login → upload → list → download route flow covered with a small file; authentication bypass, invalid token, concealed cross-user resources, and strict delete boundaries covered) 14. Architecture boundary tests ✅ ## Future diff --git a/internal/handler/file_test.go b/internal/handler/file_test.go index 8b1bddb..e1133e1 100644 --- a/internal/handler/file_test.go +++ b/internal/handler/file_test.go @@ -588,6 +588,16 @@ func TestFileHandler_Delete(t *testing.T) { t.Errorf("status = %d, want %d; body = %s", rec.Code, http.StatusNoContent, rec.Body.String()) } + // Repeated deletes return not found. + req = httptest.NewRequest(http.MethodDelete, "/api/v1/files/"+uploaded.ID, nil) + req.Header.Set(testUserIDHeader, "user1") + rec = httptest.NewRecorder() + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusNotFound { + t.Errorf("repeated delete status = %d, want %d; body = %s", rec.Code, http.StatusNotFound, rec.Body.String()) + } + // Verify gone. req = httptest.NewRequest(http.MethodGet, "/api/v1/files/"+uploaded.ID, nil) req.Header.Set(testUserIDHeader, "user1") @@ -599,7 +609,20 @@ func TestFileHandler_Delete(t *testing.T) { } } -func TestFileHandler_ForbiddenAccess(t *testing.T) { +func TestFileHandler_DeleteMissingReturnsNotFound(t *testing.T) { + r := setupFileRouter(t) + + req := httptest.NewRequest(http.MethodDelete, "/api/v1/files/missing-file", nil) + req.Header.Set(testUserIDHeader, "user1") + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusNotFound { + t.Errorf("status = %d, want %d; body = %s", rec.Code, http.StatusNotFound, rec.Body.String()) + } +} + +func TestFileHandler_CrossUserAccessReturnsNotFound(t *testing.T) { r := setupFileRouter(t) // Upload as user1. @@ -624,7 +647,13 @@ func TestFileHandler_ForbiddenAccess(t *testing.T) { rec = httptest.NewRecorder() r.ServeHTTP(rec, req) - if rec.Code != http.StatusForbidden { - t.Errorf("status = %d, want %d", rec.Code, http.StatusForbidden) + if rec.Code != http.StatusNotFound { + t.Errorf("status = %d, want %d", rec.Code, http.StatusNotFound) + } + if !strings.Contains(rec.Body.String(), "file not found") { + t.Errorf("body = %s, want file not found message", rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "access denied") { + t.Errorf("body leaks authorization result: %s", rec.Body.String()) } } diff --git a/internal/repository/file.go b/internal/repository/file.go index 8f6a370..fb916d2 100644 --- a/internal/repository/file.go +++ b/internal/repository/file.go @@ -115,9 +115,15 @@ func (r *fileRepository) Update(ctx context.Context, file *model.File) error { } func (r *fileRepository) Delete(ctx context.Context, id string) error { - result := r.db.WithContext(ctx).Model(&model.File{}).Where("id = ?", id).Update("status", model.StatusUserDeleted) + result := r.db.WithContext(ctx). + Model(&model.File{}). + Where("id = ? AND status = ?", id, model.StatusActive). + Update("status", model.StatusUserDeleted) if result.Error != nil { return result.Error } + if result.RowsAffected == 0 { + return model.ErrNotFound + } return nil } diff --git a/internal/repository/file_test.go b/internal/repository/file_test.go index cc140c5..581245e 100644 --- a/internal/repository/file_test.go +++ b/internal/repository/file_test.go @@ -2,6 +2,7 @@ package repository import ( "context" + "errors" "testing" "gorm.io/driver/sqlite" @@ -259,7 +260,7 @@ func TestFileRepository_StatusFilter(t *testing.T) { } } -func TestFileRepository_DeleteIdempotent(t *testing.T) { +func TestFileRepository_DeleteReturnsNotFoundAfterDeletion(t *testing.T) { repo := setupFileRepo(t) ctx := context.Background() @@ -276,8 +277,11 @@ func TestFileRepository_DeleteIdempotent(t *testing.T) { if err := repo.Delete(ctx, "file-1"); err != nil { t.Fatalf("first Delete = %v", err) } - if err := repo.Delete(ctx, "file-1"); err != nil { - t.Fatalf("second Delete = %v", err) + if err := repo.Delete(ctx, "file-1"); !errors.Is(err, model.ErrNotFound) { + t.Fatalf("second Delete = %v, want ErrNotFound", err) + } + if err := repo.Delete(ctx, "missing-file"); !errors.Is(err, model.ErrNotFound) { + t.Fatalf("missing Delete = %v, want ErrNotFound", err) } } diff --git a/internal/server/file_integration_test.go b/internal/server/file_integration_test.go index dee55e0..bb97e0b 100644 --- a/internal/server/file_integration_test.go +++ b/internal/server/file_integration_test.go @@ -7,6 +7,7 @@ import ( "mime/multipart" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -44,6 +45,32 @@ type fileIntegrationList struct { Total int64 `json:"total"` } +func assertFileIntegrationNotFound( + t *testing.T, + rec *httptest.ResponseRecorder, + wantMessage string, +) { + t.Helper() + if rec.Code != http.StatusNotFound { + t.Fatalf("status=%d, want %d; body=%s", rec.Code, http.StatusNotFound, rec.Body.String()) + } + + var response struct { + Error struct { + Message string `json:"message"` + } `json:"error"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &response); err != nil { + t.Fatalf("decode error response: %v", err) + } + if response.Error.Message != wantMessage { + t.Fatalf("message=%q, want %q", response.Error.Message, wantMessage) + } + if strings.Contains(rec.Body.String(), "access denied") { + t.Fatalf("response leaks authorization result: %s", rec.Body.String()) + } +} + func newFileIntegrationFixture(t *testing.T) *fileIntegrationFixture { t.Helper() @@ -402,6 +429,7 @@ func TestFileRoutesEnforceAuthenticatedOwnership(t *testing.T) { if otherList.Total != 0 || len(otherList.Files) != 0 { t.Fatalf("other user's root list=%+v, want empty list", otherList) } + otherFile := fixture.uploadFile(t, otherTokens.AccessToken, "other.txt", []byte("other content")) createBody, err := json.Marshal(map[string]any{ "name": "intrusion", @@ -410,18 +438,81 @@ func TestFileRoutesEnforceAuthenticatedOwnership(t *testing.T) { if err != nil { t.Fatalf("marshal cross-owner create body: %v", err) } - createRequest := newAuthenticatedFileRequest( - http.MethodPost, - "/api/v1/files", - bytes.NewReader(createBody), + moveBody, err := json.Marshal(map[string]any{"parent_id": privateDir.ID}) + if err != nil { + t.Fatalf("marshal cross-owner move body: %v", err) + } + parentRequests := []struct { + name string + newRequest func(*testing.T) *http.Request + }{ + { + name: "list", + newRequest: func(*testing.T) *http.Request { + return newAuthenticatedFileRequest( + http.MethodGet, + "/api/v1/files?parent_id="+privateDir.ID, + nil, + otherTokens.AccessToken, + ) + }, + }, + { + name: "create directory", + newRequest: func(*testing.T) *http.Request { + req := newAuthenticatedFileRequest( + http.MethodPost, + "/api/v1/files", + bytes.NewReader(createBody), + otherTokens.AccessToken, + ) + req.Header.Set("Content-Type", "application/json") + return req + }, + }, + { + name: "upload", + newRequest: func(t *testing.T) *http.Request { + return newFileUploadRequest( + t, + "/api/v1/files?parent_id="+privateDir.ID, + otherTokens.AccessToken, + "intrusion.txt", + []byte("must not persist"), + ) + }, + }, + { + name: "move", + newRequest: func(*testing.T) *http.Request { + req := newAuthenticatedFileRequest( + http.MethodPut, + "/api/v1/files/"+otherFile.ID, + bytes.NewReader(moveBody), + otherTokens.AccessToken, + ) + req.Header.Set("Content-Type", "application/json") + return req + }, + }, + } + + for _, request := range parentRequests { + t.Run("parent/"+request.name, func(t *testing.T) { + rec := httptest.NewRecorder() + fixture.router.ServeHTTP(rec, request.newRequest(t)) + assertFileIntegrationNotFound(t, rec, "parent directory not found") + }) + } + missingParentRequest := newAuthenticatedFileRequest( + http.MethodGet, + "/api/v1/files?parent_id=missing-directory", + nil, otherTokens.AccessToken, ) - createRequest.Header.Set("Content-Type", "application/json") - createResponse := httptest.NewRecorder() - fixture.router.ServeHTTP(createResponse, createRequest) - if createResponse.Code != http.StatusForbidden { - t.Fatalf("cross-owner create: status=%d, want %d; body=%s", createResponse.Code, http.StatusForbidden, createResponse.Body.String()) - } + missingParentResponse := httptest.NewRecorder() + fixture.router.ServeHTTP(missingParentResponse, missingParentRequest) + assertFileIntegrationNotFound(t, missingParentResponse, "parent directory not found") updateBody := []byte(`{"name":"stolen.txt"}`) requests := []struct { @@ -451,9 +542,7 @@ func TestFileRoutesEnforceAuthenticatedOwnership(t *testing.T) { rec := httptest.NewRecorder() fixture.router.ServeHTTP(rec, req) - if rec.Code != http.StatusForbidden { - t.Fatalf("status=%d, want %d; body=%s", rec.Code, http.StatusForbidden, rec.Body.String()) - } + assertFileIntegrationNotFound(t, rec, "file not found") }) } @@ -499,4 +588,38 @@ func TestFileRoutesEnforceAuthenticatedOwnership(t *testing.T) { if dirList.Total != 0 || len(dirList.Files) != 0 { t.Fatalf("private directory after rejected create=%+v, want empty list", dirList) } + + otherRootList := fixture.listFiles(t, otherTokens.AccessToken, "/api/v1/files") + if otherRootList.Total != 1 || len(otherRootList.Files) != 1 || otherRootList.Files[0].ID != otherFile.ID { + t.Fatalf("other user's root list after rejected move=%+v, want original file", otherRootList) + } +} + +func TestFileDeleteReturnsNotFoundForMissingAndDeletedFiles(t *testing.T) { + fixture := newFileIntegrationFixture(t) + _, tokens := fixture.registerAndLogin(t, "delete-user") + + missingRequest := newAuthenticatedFileRequest( + http.MethodDelete, + "/api/v1/files/missing-file", + nil, + tokens.AccessToken, + ) + missingResponse := httptest.NewRecorder() + fixture.router.ServeHTTP(missingResponse, missingRequest) + assertFileIntegrationNotFound(t, missingResponse, "file not found") + + uploaded := fixture.uploadFile(t, tokens.AccessToken, "delete-me.txt", []byte("delete content")) + deleteTarget := "/api/v1/files/" + uploaded.ID + firstRequest := newAuthenticatedFileRequest(http.MethodDelete, deleteTarget, nil, tokens.AccessToken) + firstResponse := httptest.NewRecorder() + fixture.router.ServeHTTP(firstResponse, firstRequest) + if firstResponse.Code != http.StatusNoContent { + t.Fatalf("first delete: status=%d, want %d; body=%s", firstResponse.Code, http.StatusNoContent, firstResponse.Body.String()) + } + + secondRequest := newAuthenticatedFileRequest(http.MethodDelete, deleteTarget, nil, tokens.AccessToken) + secondResponse := httptest.NewRecorder() + fixture.router.ServeHTTP(secondResponse, secondRequest) + assertFileIntegrationNotFound(t, secondResponse, "file not found") } diff --git a/internal/service/file.go b/internal/service/file.go index 25adc1c..3f65ee3 100644 --- a/internal/service/file.go +++ b/internal/service/file.go @@ -304,10 +304,6 @@ func (s *FileService) Update(ctx context.Context, userID, fileID string, newName func (s *FileService) Delete(ctx context.Context, userID, fileID string) error { file, err := s.getOwnedFile(ctx, userID, fileID) if err != nil { - var ae *model.AppError - if errors.As(err, &ae) && errors.Is(ae.Err, model.ErrNotFound) { - return nil // idempotent: already deleted - } return err } @@ -323,6 +319,9 @@ func (s *FileService) Delete(ctx context.Context, userID, fileID string) error { } if err := s.fileRepo.Delete(ctx, fileID); err != nil { + if errors.Is(err, model.ErrNotFound) { + return model.NewNotFoundError("file not found", model.ErrNotFound) + } return model.NewInternalError("delete file record", err) } @@ -378,7 +377,7 @@ func (s *FileService) getOwnedFile(ctx context.Context, userID, fileID string) ( } if file.UserID != userID { - return nil, model.NewForbiddenError("access denied", model.ErrForbidden) + return nil, model.NewNotFoundError("file not found", model.ErrNotFound) } return file, nil @@ -395,7 +394,7 @@ func (s *FileService) verifyParent(ctx context.Context, userID, parentID string) } if parent.UserID != userID { - return model.NewForbiddenError("access denied", model.ErrForbidden) + return model.NewNotFoundError("parent directory not found", model.ErrNotFound) } if !parent.IsDir { diff --git a/internal/service/file_test.go b/internal/service/file_test.go index 655dabb..d14ba07 100644 --- a/internal/service/file_test.go +++ b/internal/service/file_test.go @@ -32,6 +32,18 @@ func assertAppError(t *testing.T, err error, wantKind model.ErrorKind) bool { return true } +func assertAppErrorMessage(t *testing.T, err error, wantKind model.ErrorKind, wantMessage string) { + t.Helper() + if !assertAppError(t, err, wantKind) { + return + } + var ae *model.AppError + errors.As(err, &ae) + if ae.Message != wantMessage { + t.Errorf("message = %q, want %q", ae.Message, wantMessage) + } +} + // memStorage is an in-memory implementation of storage.StorageBackend for tests. type memStorage struct { mu sync.RWMutex @@ -274,7 +286,7 @@ func TestFileService_Download(t *testing.T) { } } -func TestFileService_DownloadForbidden(t *testing.T) { +func TestFileService_DownloadReturnsNotFoundForOtherUser(t *testing.T) { svc := setupFileService(t) ctx := context.Background() @@ -284,7 +296,7 @@ func TestFileService_DownloadForbidden(t *testing.T) { } _, _, err = svc.Download(ctx, "user2", info.ID) - assertAppError(t, err, model.KindPermissionDenied) + assertAppErrorMessage(t, err, model.KindNotFound, "file not found") } func TestFileService_DownloadDirectory(t *testing.T) { @@ -328,7 +340,7 @@ func TestFileService_GetNotFound(t *testing.T) { assertAppError(t, err, model.KindNotFound) } -func TestFileService_GetForbidden(t *testing.T) { +func TestFileService_GetReturnsNotFoundForOtherUser(t *testing.T) { svc := setupFileService(t) ctx := context.Background() @@ -338,7 +350,7 @@ func TestFileService_GetForbidden(t *testing.T) { } _, err = svc.Get(ctx, "user2", info.ID) - assertAppError(t, err, model.KindPermissionDenied) + assertAppErrorMessage(t, err, model.KindNotFound, "file not found") } func TestFileService_List(t *testing.T) { @@ -395,6 +407,19 @@ func TestFileService_Update(t *testing.T) { } } +func TestFileService_UpdateReturnsNotFoundForOtherUser(t *testing.T) { + svc := setupFileService(t) + ctx := context.Background() + + info, err := svc.Upload(ctx, "user1", nil, "private.txt", strings.NewReader("data")) + if err != nil { + t.Fatalf("Upload = %v", err) + } + + _, err = svc.Update(ctx, "user2", info.ID, "stolen.txt", nil) + assertAppErrorMessage(t, err, model.KindNotFound, "file not found") +} + func TestFileService_UpdateMove(t *testing.T) { svc := setupFileService(t) ctx := context.Background() @@ -458,7 +483,7 @@ func TestFileService_DeleteNonEmptyDirectory(t *testing.T) { } } -func TestFileService_DeleteForbidden(t *testing.T) { +func TestFileService_DeleteReturnsNotFoundForOtherUser(t *testing.T) { svc := setupFileService(t) ctx := context.Background() @@ -468,7 +493,14 @@ func TestFileService_DeleteForbidden(t *testing.T) { } err = svc.Delete(ctx, "user2", info.ID) - assertAppError(t, err, model.KindPermissionDenied) + assertAppErrorMessage(t, err, model.KindNotFound, "file not found") +} + +func TestFileService_DeleteReturnsNotFoundForMissingFile(t *testing.T) { + svc := setupFileService(t) + + err := svc.Delete(context.Background(), "user1", "missing-file") + assertAppErrorMessage(t, err, model.KindNotFound, "file not found") } func TestFileService_CreateDir(t *testing.T) { @@ -502,7 +534,7 @@ func TestFileService_CreateDirDuplicateName(t *testing.T) { } } -func TestFileService_VerifyParentForbidden(t *testing.T) { +func TestFileService_ParentOperationsReturnNotFoundForOtherUser(t *testing.T) { svc := setupFileService(t) ctx := context.Background() @@ -510,9 +542,50 @@ func TestFileService_VerifyParentForbidden(t *testing.T) { if err != nil { t.Fatalf("CreateDir = %v", err) } + otherFile, err := svc.Upload(ctx, "user2", nil, "other.txt", strings.NewReader("data")) + if err != nil { + t.Fatalf("Upload other file = %v", err) + } - _, err = svc.Upload(ctx, "user2", &dir.ID, "stolen.txt", strings.NewReader("data")) - assertAppError(t, err, model.KindPermissionDenied) + tests := []struct { + name string + run func() error + }{ + { + name: "list", + run: func() error { + _, err := svc.List(ctx, "user2", &dir.ID, 0, 50) + return err + }, + }, + { + name: "upload", + run: func() error { + _, err := svc.Upload(ctx, "user2", &dir.ID, "stolen.txt", strings.NewReader("data")) + return err + }, + }, + { + name: "create directory", + run: func() error { + _, err := svc.CreateDir(ctx, "user2", &dir.ID, "stolen-dir") + return err + }, + }, + { + name: "move", + run: func() error { + _, err := svc.Update(ctx, "user2", otherFile.ID, "", &dir.ID) + return err + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assertAppErrorMessage(t, tt.run(), model.KindNotFound, "parent directory not found") + }) + } } func TestFileService_SoftDelete(t *testing.T) { @@ -562,7 +635,7 @@ func TestFileService_SoftDeleteKeepsStorage(t *testing.T) { } } -func TestFileService_SoftDeleteIdempotent(t *testing.T) { +func TestFileService_SoftDeleteRepeatedReturnsNotFound(t *testing.T) { svc := setupFileService(t) ctx := context.Background() @@ -574,12 +647,11 @@ func TestFileService_SoftDeleteIdempotent(t *testing.T) { if err := svc.Delete(ctx, "user1", info.ID); err != nil { t.Fatalf("first Delete = %v", err) } - if err := svc.Delete(ctx, "user1", info.ID); err != nil { - t.Fatalf("second Delete should be idempotent, got: %v", err) - } + err = svc.Delete(ctx, "user1", info.ID) + assertAppErrorMessage(t, err, model.KindNotFound, "file not found") } -func TestFileService_SoftDeleteForbidden(t *testing.T) { +func TestFileService_SoftDeleteReturnsNotFoundForOtherUser(t *testing.T) { svc := setupFileService(t) ctx := context.Background() @@ -589,5 +661,5 @@ func TestFileService_SoftDeleteForbidden(t *testing.T) { } err = svc.Delete(ctx, "user2", info.ID) - assertAppError(t, err, model.KindPermissionDenied) + assertAppErrorMessage(t, err, model.KindNotFound, "file not found") }