fix: 아키텍처 리뷰 HIGH/MEDIUM 이슈 10건 수정

HIGH (3건): - 런처 파일 업로드 시 PE 헤더 검증 + 500MB 크기 제한 추가 - 체인 노드 URL 파싱 시 scheme/host 유효성 검증 - Dockerfile 비루트 사용자(app:1000) 실행 MEDIUM (7건): - SSAFY username 충돌 시 랜덤 suffix로 최대 3회 재시도 - 내부 API username 검증 validID(256자) → validUsername(3~50자) 분리 - 동시 업로드 경합 방지 sync.Mutex 추가 - 프로덕션 환경변수 검증 강화 (DB_PASSWORD, OPERATOR_KEY_HEX, INTERNAL_API_KEY) - Redis 에러 시 멱등성 요청 통과 → 503 거부로 변경 - CORS AllowOrigins 환경변수화 (CORS_ALLOW_ORIGINS) - Refresh 엔드포인트 rate limiting 추가 (IP당 5 req/min) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-20 15:56:58 +09:00
parent e16e1b5e0a
commit cc9884bdfe
9 changed files with 140 additions and 47 deletions
--- a/6
+++ b/6
@@ -10,9 +10,11 @@ RUN CGO_ENABLED=0 GOOS=linux go build -o server .
 # Stage 2: Run
 FROM alpine:latest
 RUN apk --no-cache add tzdata ca-certificates curl
-RUN mkdir -p /data/game
+RUN addgroup -g 1000 app && adduser -D -u 1000 -G app app
+RUN mkdir -p /data/game && chown app:app /data/game
 WORKDIR /app
-COPY --from=builder /build/a301_server/server .
+COPY --from=builder --chown=app:app /build/a301_server/server .
+USER app
 EXPOSE 8080
 HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=3 \
  CMD curl -f http://localhost:8080/health || exit 1
--- a/internal/auth/service.go
+++ b/internal/auth/service.go
@@ -426,9 +426,6 @@ func (s *Service) SSAFYLogin(code, state string) (accessToken, refreshToken stri

 		ssafyID := userInfo.UserID
 		// SSAFY ID에서 영문 소문자+숫자만 추출하여 안전한 username 생성
-		// NOTE: Username collision is handled by the DB unique constraint.
-		// If collision occurs, the transaction will rollback and return a generic error.
-		// A retry with random suffix could improve UX but is not critical.
 		safeID := sanitizeForUsername(ssafyID)
 		if safeID == "" {
 			safeID = hex.EncodeToString(randomBytes[:8])
@@ -437,32 +434,47 @@ func (s *Service) SSAFYLogin(code, state string) (accessToken, refreshToken stri
 		if len(username) > 50 {
 			username = username[:50]
 		}
+		// DB unique constraint 충돌 시 랜덤 suffix로 최대 3회 재시도
+		maxRetries := 3
+		baseUsername := username

-		err = s.repo.Transaction(func(txRepo *Repository) error {
-			user = &User{
-				Username:     username,
-				PasswordHash: string(hash),
-				Role:         RoleUser,
-				SsafyID:      &ssafyID,
-			}
-			if err := txRepo.Create(user); err != nil {
-				return err
-			}
-
-			if s.walletCreator != nil {
-				if err := s.walletCreator(user.ID); err != nil {
-					return fmt.Errorf("wallet creation failed: %w", err)
+		for attempt := 0; attempt < maxRetries; attempt++ {
+			if attempt > 0 {
+				suffix := hex.EncodeToString(randomBytes[attempt*2 : attempt*2+4])
+				username = baseUsername + "_" + suffix
+				if len(username) > 50 {
+					username = username[:50]
 				}
 			}
-			if s.profileCreator != nil {
-				if err := s.profileCreator(user.ID); err != nil {
-					return fmt.Errorf("profile creation failed: %w", err)
+			err = s.repo.Transaction(func(txRepo *Repository) error {
+				user = &User{
+					Username:     username,
+					PasswordHash: string(hash),
+					Role:         RoleUser,
+					SsafyID:      &ssafyID,
 				}
+				if err := txRepo.Create(user); err != nil {
+					return err
+				}
+
+				if s.walletCreator != nil {
+					if err := s.walletCreator(user.ID); err != nil {
+						return fmt.Errorf("wallet creation failed: %w", err)
+					}
+				}
+				if s.profileCreator != nil {
+					if err := s.profileCreator(user.ID); err != nil {
+						return fmt.Errorf("profile creation failed: %w", err)
+					}
+				}
+				return nil
+			})
+			if err == nil {
+				break
 			}
-			return nil
-		})
+			log.Printf("SSAFY user creation attempt %d failed: %v", attempt+1, err)
+		}
 		if err != nil {
-			log.Printf("SSAFY user creation transaction failed: %v", err)
 			return "", "", nil, fmt.Errorf("계정 생성 실패: %v", err)
 		}
 	}
--- a/internal/chain/handler.go
+++ b/internal/chain/handler.go
@@ -49,6 +49,10 @@ func validID(s string) bool {
 	return s != "" && len(s) <= maxIDLength
 }

+func validUsername(s string) bool {
+	return len(s) >= 3 && len(s) <= 50
+}
+
 // chainError classifies chain errors into appropriate HTTP responses.
 // TxError (on-chain execution failure) maps to 422 with the chain's error detail.
 // Other errors (network, timeout, build failures) remain 500.
@@ -640,8 +644,8 @@ func (h *Handler) InternalGrantReward(c *fiber.Ctx) error {
 	if err := c.BodyParser(&req); err != nil {
 		return apperror.ErrBadRequest
 	}
-	if !validID(req.Username) {
-		return apperror.BadRequest("username은 필수입니다")
+	if !validUsername(req.Username) {
+		return apperror.BadRequest("username은 3~50자여야 합니다")
 	}
 	result, err := h.svc.GrantRewardByUsername(req.Username, req.TokenAmount, req.Assets)
 	if err != nil {
@@ -672,8 +676,8 @@ func (h *Handler) InternalMintAsset(c *fiber.Ctx) error {
 	if err := c.BodyParser(&req); err != nil {
 		return apperror.ErrBadRequest
 	}
-	if !validID(req.TemplateID) || !validID(req.Username) {
-		return apperror.BadRequest("templateId와 username은 필수입니다")
+	if !validID(req.TemplateID) || !validUsername(req.Username) {
+		return apperror.BadRequest("templateId와 username은 필수입니다 (username: 3~50자)")
 	}
 	result, err := h.svc.MintAssetByUsername(req.TemplateID, req.Username, req.Properties)
 	if err != nil {
@@ -695,8 +699,8 @@ func (h *Handler) InternalMintAsset(c *fiber.Ctx) error {
 // @Router       /api/internal/chain/balance [get]
 func (h *Handler) InternalGetBalance(c *fiber.Ctx) error {
 	username := c.Query("username")
-	if !validID(username) {
-		return apperror.BadRequest("username은 필수입니다")
+	if !validUsername(username) {
+		return apperror.BadRequest("username은 3~50자여야 합니다")
 	}
 	result, err := h.svc.GetBalanceByUsername(username)
 	if err != nil {
@@ -720,8 +724,8 @@ func (h *Handler) InternalGetBalance(c *fiber.Ctx) error {
 // @Router       /api/internal/chain/assets [get]
 func (h *Handler) InternalGetAssets(c *fiber.Ctx) error {
 	username := c.Query("username")
-	if !validID(username) {
-		return apperror.BadRequest("username은 필수입니다")
+	if !validUsername(username) {
+		return apperror.BadRequest("username은 3~50자여야 합니다")
 	}
 	offset, limit := parsePagination(c)
 	result, err := h.svc.GetAssetsByUsername(username, offset, limit)
@@ -745,8 +749,8 @@ func (h *Handler) InternalGetAssets(c *fiber.Ctx) error {
 // @Router       /api/internal/chain/inventory [get]
 func (h *Handler) InternalGetInventory(c *fiber.Ctx) error {
 	username := c.Query("username")
-	if !validID(username) {
-		return apperror.BadRequest("username은 필수입니다")
+	if !validUsername(username) {
+		return apperror.BadRequest("username은 3~50자여야 합니다")
 	}
 	result, err := h.svc.GetInventoryByUsername(username)
 	if err != nil {
--- a/internal/download/service.go
+++ b/internal/download/service.go
@@ -11,13 +11,17 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
+	"sync"
 )

+const maxLauncherSize = 500 * 1024 * 1024 // 500MB
+
 var versionRe = regexp.MustCompile(`v\d+\.\d+(\.\d+)?`)

 type Service struct {
-	repo    *Repository
-	gameDir string
+	repo     *Repository
+	gameDir  string
+	uploadMu sync.Mutex
 }

 func NewService(repo *Repository, gameDir string) *Service {
@@ -37,6 +41,9 @@ func (s *Service) LauncherFilePath() string {
 }

 func (s *Service) UploadLauncher(body io.Reader, baseURL string) (*Info, error) {
+	s.uploadMu.Lock()
+	defer s.uploadMu.Unlock()
+
 	if err := os.MkdirAll(s.gameDir, 0755); err != nil {
 		return nil, fmt.Errorf("디렉토리 생성 실패: %w", err)
 	}
@@ -49,9 +56,7 @@ func (s *Service) UploadLauncher(body io.Reader, baseURL string) (*Info, error)
 		return nil, fmt.Errorf("파일 생성 실패: %w", err)
 	}

-	// NOTE: Partial uploads (client closes cleanly mid-transfer) are saved.
-	// The hashGameExeFromZip check mitigates this for game uploads but not for launcher uploads.
-	n, err := io.Copy(f, body)
+	n, err := io.Copy(f, io.LimitReader(body, maxLauncherSize+1))
 	if closeErr := f.Close(); closeErr != nil && err == nil {
 		err = closeErr
 	}
@@ -61,6 +66,16 @@ func (s *Service) UploadLauncher(body io.Reader, baseURL string) (*Info, error)
 		}
 		return nil, fmt.Errorf("파일 저장 실패: %w", err)
 	}
+	if n > maxLauncherSize {
+		os.Remove(tmpPath)
+		return nil, fmt.Errorf("런처 파일이 너무 큽니다 (최대 %dMB)", maxLauncherSize/1024/1024)
+	}
+
+	// PE 헤더 검증 (MZ magic bytes)
+	if err := validatePEHeader(tmpPath); err != nil {
+		os.Remove(tmpPath)
+		return nil, err
+	}

 	if err := os.Rename(tmpPath, finalPath); err != nil {
 		if removeErr := os.Remove(tmpPath); removeErr != nil {
@@ -88,6 +103,9 @@ func (s *Service) UploadLauncher(body io.Reader, baseURL string) (*Info, error)

 // Upload streams the body directly to disk, then extracts metadata from the zip.
 func (s *Service) Upload(filename string, body io.Reader, baseURL string) (*Info, error) {
+	s.uploadMu.Lock()
+	defer s.uploadMu.Unlock()
+
 	if err := os.MkdirAll(s.gameDir, 0755); err != nil {
 		return nil, fmt.Errorf("디렉토리 생성 실패: %w", err)
 	}
@@ -153,6 +171,22 @@ func (s *Service) Upload(filename string, body io.Reader, baseURL string) (*Info
 	return info, s.repo.Save(info)
 }

+func validatePEHeader(path string) error {
+	f, err := os.Open(path)
+	if err != nil {
+		return fmt.Errorf("파일 검증 실패: %w", err)
+	}
+	defer f.Close()
+	header := make([]byte, 2)
+	if _, err := io.ReadFull(f, header); err != nil {
+		return fmt.Errorf("유효하지 않은 실행 파일입니다")
+	}
+	if header[0] != 'M' || header[1] != 'Z' {
+		return fmt.Errorf("유효하지 않은 실행 파일입니다")
+	}
+	return nil
+}
+
 func hashFileToHex(path string) string {
 	f, err := os.Open(path)
 	if err != nil {
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -5,6 +5,7 @@ import (
 	"time"

 	"a301_server/pkg/apperror"
+	"a301_server/pkg/config"
 	"a301_server/pkg/metrics"
 	"a301_server/pkg/middleware"

@@ -36,7 +37,7 @@ func New() *fiber.App {
 	}))
 	app.Use(middleware.SecurityHeaders)
 	app.Use(cors.New(cors.Config{
-		AllowOrigins:     "https://a301.tolelom.xyz",
+		AllowOrigins:     config.C.CORSAllowOrigins,
 		AllowHeaders:     "Origin, Content-Type, Authorization, Idempotency-Key, X-API-Key, X-Requested-With",
 		AllowMethods:     "GET, POST, PUT, PATCH, DELETE",
 		AllowCredentials: true,
@@ -72,6 +73,21 @@ func APILimiter() fiber.Handler {
 	})
 }

+// RefreshLimiter returns a rate limiter for refresh token endpoint (5 req/min per IP).
+// Separate from AuthLimiter to avoid NAT collisions while still preventing abuse.
+func RefreshLimiter() fiber.Handler {
+	return limiter.New(limiter.Config{
+		Max:        5,
+		Expiration: 1 * time.Minute,
+		KeyGenerator: func(c *fiber.Ctx) string {
+			return "refresh:" + c.IP()
+		},
+		LimitReached: func(c *fiber.Ctx) error {
+			return apperror.ErrRateLimited
+		},
+	})
+}
+
 // ChainUserLimiter returns a rate limiter for chain transactions (20 req/min per user).
 func ChainUserLimiter() fiber.Handler {
 	return limiter.New(limiter.Config{
--- a/main.go
+++ b/main.go
@@ -140,7 +140,7 @@ func main() {

 	routes.Register(app, authHandler, annHandler, dlHandler, chainHandler, brHandler, playerHandler,
 		server.AuthLimiter(), server.APILimiter(), server.HealthCheck(), server.ReadyCheck(db, rdb),
-		server.ChainUserLimiter(), authMw, serverAuthMw, idempotencyReqMw)
+		server.ChainUserLimiter(), authMw, serverAuthMw, idempotencyReqMw, server.RefreshLimiter())

 	// ── 백그라운드 워커 ──────────────────────────────────────────────

--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -2,6 +2,7 @@ package config

 import (
 	"log"
+	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -35,6 +36,9 @@ type Config struct {
 	OperatorKeyHex      string
 	WalletEncryptionKey string

+	// CORS
+	CORSAllowOrigins string
+
 	// Server-to-server auth
 	InternalAPIKey string

@@ -72,6 +76,8 @@ func Load() {
 		OperatorKeyHex:      getEnv("OPERATOR_KEY_HEX", ""),
 		WalletEncryptionKey: getEnv("WALLET_ENCRYPTION_KEY", ""),

+		CORSAllowOrigins: getEnv("CORS_ALLOW_ORIGINS", "https://a301.tolelom.xyz"),
+
 		InternalAPIKey: getEnv("INTERNAL_API_KEY", ""),

 		SSAFYClientID:     getEnv("SSAFY_CLIENT_ID", ""),
@@ -83,6 +89,9 @@ func Load() {
 	if raw := getEnv("CHAIN_NODE_URLS", ""); raw != "" {
 		for _, u := range strings.Split(raw, ",") {
 			if u = strings.TrimSpace(u); u != "" {
+				if parsed, err := url.Parse(u); err != nil || parsed.Scheme == "" || parsed.Host == "" {
+					log.Fatalf("FATAL: invalid CHAIN_NODE_URL: %q (must be http:// or https://)", u)
+				}
 				C.ChainNodeURLs = append(C.ChainNodeURLs, u)
 			}
 		}
@@ -114,8 +123,23 @@ func WarnInsecureDefaults() {
 		log.Println("WARNING: WALLET_ENCRYPTION_KEY is empty — blockchain wallet features will fail")
 	}

+	if isProd {
+		if C.DBPassword == "" {
+			log.Println("FATAL: DB_PASSWORD must be set in production")
+			insecure = true
+		}
+		if C.OperatorKeyHex == "" {
+			log.Println("FATAL: OPERATOR_KEY_HEX must be set in production")
+			insecure = true
+		}
+		if C.InternalAPIKey == "" {
+			log.Println("FATAL: INTERNAL_API_KEY must be set in production")
+			insecure = true
+		}
+	}
+
 	if isProd && insecure {
-		log.Fatal("FATAL: insecure default secrets detected in production — set JWT_SECRET, REFRESH_SECRET, and ADMIN_PASSWORD")
+		log.Fatal("FATAL: insecure defaults detected in production — check warnings above")
 	}
 }

--- a/pkg/middleware/idempotency.go
+++ b/pkg/middleware/idempotency.go
@@ -57,9 +57,9 @@ func Idempotency(rdb *redis.Client) fiber.Handler {
 		// Atomically claim the key using SET NX (only succeeds if key doesn't exist)
 		set, err := rdb.SetNX(ctx, redisKey, "processing", idempotencyTTL).Result()
 		if err != nil {
-			// Redis error — let the request through rather than blocking
-			log.Printf("WARNING: idempotency SetNX failed (key=%s): %v", key, err)
-			return c.Next()
+			// Redis error — reject to prevent duplicate transactions
+			log.Printf("ERROR: idempotency SetNX failed (key=%s): %v", key, err)
+			return apperror.New("internal_error", "서버 오류가 발생했습니다. 잠시 후 다시 시도해주세요", 503)
 		}

 		if !set {
--- a/routes/routes.go
+++ b/routes/routes.go
@@ -28,6 +28,7 @@ func Register(
 	authMw fiber.Handler,
 	serverAuthMw fiber.Handler,
 	idempotencyReqMw fiber.Handler,
+	refreshLimiter fiber.Handler,
 ) {
 	// Swagger UI
 	app.Get("/swagger/*", swagger.HandlerDefault)
@@ -80,7 +81,7 @@ func Register(
 	a := api.Group("/auth")
 	a.Post("/register", authLimiter, authH.Register)
 	a.Post("/login", authLimiter, authH.Login)
-	a.Post("/refresh", authH.Refresh) // refresh는 유효한 쿠키 필요 — authLimiter 제외 (NAT 환경 한도 초과 방지)
+	a.Post("/refresh", refreshLimiter, authH.Refresh)
 	a.Post("/logout", authMw, authH.Logout)
 	// /verify moved to internal API (ServerAuth) — see internal section below
 	a.Get("/ssafy/login", authH.SSAFYLoginURL)