Merge pull request #1400 from traPtitech/fix/skip_file_security_validate

ファイルアップロードのときにOpenAPIのsecurity validationをしない

Author: ikura-hamu

src/handler/v2/api.go

@@ -104,6 +104,7 @@ func (api *API) setRoutes(e *echo.Echo) error {
 	// 空のpathのgroupを作成し、oapiMiddleware.OapiRequestValidatorを設定する
 	apiGroup := e.Group("")
 	apiGroup.Use(echomiddleware.OapiRequestValidatorWithOptions(swagger, &echomiddleware.Options{
+		Skipper: fileUploadSkipper,
 		Options: openapi3filter.Options{
 			AuthenticationFunc: api.Checker.check,
 			// validate時にデータがメモリに乗るため、
@@ -113,6 +114,7 @@ func (api *API) setRoutes(e *echo.Echo) error {
 			ExcludeResponseBody: true,
 		},
 	}))
+	apiGroup.Use(api.fileUploadAuthMiddleware)
 	openapi.RegisterHandlersWithBaseURL(apiGroup, api, "/api/v2")
 
 	return nil

src/handler/v2/checker.go

@@ -336,6 +336,8 @@ func (checker *Checker) GameOwnerAuthChecker(ctx context.Context, _ *openapi3fil
 
 // GameMaintainerAuthChecker
 // そのゲームのmaintainer(collaborator)であるかどうかを調べるチェッカー
+//
+// IMPORTANT: [(*Checker).FileUploadAuthMiddleware] は、この関数の第2引数が使われていないことに依存した実装になっている。
 func (checker *Checker) GameMaintainerAuthChecker(ctx context.Context, _ *openapi3filter.AuthenticationInput) error {
 	c := echomiddleware.GetEchoContext(ctx)
 	// GetEchoContextの内部実装をみるとnilがかえりうるので、

src/handler/v2/file_upload.go

@@ -0,0 +1,66 @@
+package v2
+
+import (
+	"context"
+	"net/http"
+	"path"
+	"slices"
+
+	"github.com/labstack/echo/v4"
+	echomiddleware "github.com/oapi-codegen/echo-middleware"
+)
+
+// isFileUploadRequest はファイルをアップロードするエンドポイントならtrueを返す。
+// POST /api/v2/games/:gameID/{files,images,videos} へのリクエストが含まれる。
+func isFileUploadRequest(c echo.Context) bool {
+	if c.Request().Method != http.MethodPost {
+		return false
+	}
+
+	gameID := c.Param("gameID")
+	if gameID == "" {
+		return false
+	}
+
+	targetPathBase := path.Join("/api/v2/games", gameID)
+	targetPaths := []string{
+		path.Join(targetPathBase, "files"),
+		path.Join(targetPathBase, "images"),
+		path.Join(targetPathBase, "videos"),
+	}
+
+	reqPath := path.Clean(c.Request().URL.Path)
+
+	return slices.Contains(targetPaths, reqPath)
+}
+
+// fileUploadSkipper はファイルをアップロードするエンドポイントについてバリデーションをスキップする。
+// OapiRequestValidator は 内部の ValidateSecurityRequirements でリクエストボディを全部読んでいる。
+// そのため、画像・動画・ファイルのアップロード時にメモリ不足になる可能性がある。
+// POST /api/v2/games/:gameID/{files,images,videos} へのリクエストは、バリデーションをスキップする。
+func fileUploadSkipper(c echo.Context) bool {
+	return isFileUploadRequest(c)
+}
+
+// fileUploadAuthMiddleware はファイルをアップロードするエンドポイントに対してのみ認証を行うミドルウェアを返す。
+// POST /api/v2/games/:gameID/{files,images,videos} へのリクエストが含まれる。
+//
+// IMPORTANT: [(*Checker).GameMaintainerAuthChecker] の第2引数が使われていないことに依存した実装になっている。
+func (checker *Checker) fileUploadAuthMiddleware(next echo.HandlerFunc) echo.HandlerFunc {
+	return func(c echo.Context) error {
+		if !isFileUploadRequest(c) {
+			return next(c)
+		}
+
+		// ref: [github.com/oapi-codegen/echo-middleware.GetEchoContext]
+		// ここで echo.Context を context.Context に詰め込んでおかないと、GameMaintainerAuthChecker 内で取得できない。
+		ctx := context.WithValue(c.Request().Context(), echomiddleware.EchoContextKey, c) //nolint:staticcheck
+
+		err := checker.GameMaintainerAuthChecker(ctx, nil)
+		if err != nil {
+			return err
+		}
+
+		return next(c)
+	}
+}

src/handler/v2/file_upload_test.go

@@ -0,0 +1,70 @@
+package v2
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_isFileUploadRequest(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		method string
+		path   string
+		gameID string
+		want   bool
+	}{
+		"POST /api/v2/games/:gameID/files": {
+			method: http.MethodPost,
+			path:   "/api/v2/games/123/files",
+			gameID: "123",
+			want:   true,
+		},
+		"POST /api/v2/games/:gameID/images": {
+			method: http.MethodPost,
+			path:   "/api/v2/games/123/images",
+			gameID: "123",
+			want:   true,
+		},
+		"POST /api/v2/games/:gameID/videos": {
+			method: http.MethodPost,
+			path:   "/api/v2/games/123/videos",
+			gameID: "123",
+			want:   true,
+		},
+		"GET /api/v2/games/:gameID/files": {
+			method: http.MethodGet,
+			path:   "/api/v2/games/123/files",
+			gameID: "123",
+			want:   false,
+		},
+		"POST /api/v2/games/:gameID/others": {
+			method: http.MethodPost,
+			path:   "/api/v2/games/123/others",
+			gameID: "123",
+			want:   false,
+		},
+		"POST /api/v2/games/files": {
+			method: http.MethodPost,
+			path:   "/api/v2/games/files",
+			gameID: "",
+			want:   false,
+		},
+	}
+
+	for name, testCase := range testCases {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			c, _, _ := setupTestRequest(t, testCase.method, testCase.path, nil)
+			c.SetParamNames("gameID")
+			c.SetParamValues(testCase.gameID)
+
+			got := isFileUploadRequest(c)
+
+			assert.Equal(t, testCase.want, got)
+		})
+	}
+}