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
This commit is contained in:
2026-07-16 00:06:31 +08:00
parent bb86950632
commit 6604ecb026
8 changed files with 292 additions and 42 deletions
+32 -3
View File
@@ -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())
}
}
+7 -1
View File
@@ -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
}
+7 -3
View File
@@ -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)
}
}
+136 -13
View File
@@ -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")
}
+5 -6
View File
@@ -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 {
+87 -15
View File
@@ -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")
}