HH-438: close upload and remote URL attack surfaces (#89)

* HH-438: close upload and remote URL attack surfaces

* HH-438: use valid PNG in direct upload test

* HH-438: align PostgreSQL upload staging schema

* HH-438: run upload migrations before PostgreSQL E2E

---------

Co-authored-by: Rogee <rogee@ipao.vip>
This commit is contained in:
Rogee
2026-08-21 20:36:52 +08:00
committed by GitHub
co-authored by rogee
parent 7a9fec33c5
commit 77c91662d2
18 changed files with 1311 additions and 259 deletions
@@ -1103,7 +1103,7 @@ func (s *ProfileHandlerTestSuite) TestUpdate_MultipartFormProfileParity() {
s.Require().NoError(writer.WriteField("profile[ui_settings][editor_message_key]", "enter"))
fileWriter, err := writer.CreateFormFile("profile[avatar]", "avatar.png")
s.Require().NoError(err)
_, err = fileWriter.Write([]byte("fake image bytes"))
_, err = fileWriter.Write(handlerTestPNG)
s.Require().NoError(err)
s.Require().NoError(writer.Close())
@@ -1,6 +1,7 @@
package v1
import (
"encoding/json"
"errors"
"net/http"
"strings"
@@ -8,13 +9,21 @@ import (
"github.com/gin-gonic/gin"
"gorm.io/gorm"
"github.com/gochat/gochat/internal/auth"
"github.com/gochat/gochat/internal/service"
"github.com/gochat/gochat/pkg/response"
)
// UploadHandler handles file upload endpoints (account-level + widget direct + account direct).
type UploadHandler struct {
svc *service.UploadService
svc *service.UploadService
jwtSvc *auth.JWTService
}
// WithAccessAuth enables authenticated serving of local private uploads.
func (h *UploadHandler) WithAccessAuth(jwtSvc *auth.JWTService) *UploadHandler {
h.jwtSvc = jwtSvc
return h
}
// NewUploadHandler creates a new UploadHandler.
@@ -25,6 +34,7 @@ func NewUploadHandler(svc *service.UploadService) *UploadHandler {
// Upload handles POST /api/v1/accounts/:id/upload — account-level file upload.
// Reference: Chatwoot api/v1/accounts/:account_id/upload
func (h *UploadHandler) Upload(c *gin.Context) {
h.limitBody(c)
accountID := getAccountID(c)
if accountID == 0 {
response.AbortWithStatusError(c, http.StatusBadRequest, response.ErrValidation, "account_id is required")
@@ -48,7 +58,11 @@ func (h *UploadHandler) Upload(c *gin.Context) {
fileHeader, err = c.FormFile("file")
}
if err != nil {
c.JSON(http.StatusUnprocessableEntity, gin.H{"error": "missing input"})
status := http.StatusUnprocessableEntity
if isUploadBodyTooLarge(err) {
status = http.StatusRequestEntityTooLarge
}
c.JSON(status, gin.H{"error": "missing input"})
return
}
result, svcErr = h.svc.AccountUpload(c.Request.Context(), accountID, service.AccountUploadRequest{
@@ -74,6 +88,7 @@ func (h *UploadHandler) Upload(c *gin.Context) {
// DirectUpload handles POST /api/v1/widget/direct_uploads — widget direct file upload.
// Reference: Chatwoot POST /widget/direct_uploads
func (h *UploadHandler) DirectUpload(c *gin.Context) {
h.limitBody(c)
if strings.Contains(c.GetHeader("Content-Type"), "application/json") {
var req service.ActiveStorageDirectUploadRequest
if err := c.ShouldBindJSON(&req); err != nil {
@@ -93,12 +108,18 @@ func (h *UploadHandler) DirectUpload(c *gin.Context) {
fileHeader, err := c.FormFile("file")
if err != nil {
response.AbortWithStatusError(c, http.StatusBadRequest, response.ErrValidation, "file is required")
if isUploadBodyTooLarge(err) {
c.AbortWithStatusJSON(http.StatusRequestEntityTooLarge, gin.H{"error": "upload body is too large"})
} else {
response.AbortWithStatusError(c, http.StatusBadRequest, response.ErrValidation, "file is required")
}
return
}
result, svcErr := h.svc.WidgetDirectUpload(c.Request.Context(), service.WidgetDirectUploadRequest{
FileHeader: fileHeader,
WebsiteToken: c.Query("website_token"),
AuthToken: c.GetHeader("X-Auth-Token"),
FileHeader: fileHeader,
})
if svcErr != nil {
handleServiceError(c, svcErr)
@@ -109,7 +130,8 @@ func (h *UploadHandler) DirectUpload(c *gin.Context) {
}
func (h *UploadHandler) CompleteWidgetDirectUpload(c *gin.Context) {
result, svcErr := h.svc.CompleteWidgetDirectUpload(c.Request.Context(), c.Param("upload_uuid"), c.Request.Body)
h.limitBody(c)
result, svcErr := h.svc.CompleteWidgetDirectUpload(c.Request.Context(), c.Param("upload_uuid"), c.Request.Body, c.Query("token"))
if svcErr != nil {
handleServiceError(c, svcErr)
return
@@ -121,6 +143,7 @@ func (h *UploadHandler) CompleteWidgetDirectUpload(c *gin.Context) {
// Returns a blob/UUID for later attachment to messages.
// Reference: Chatwoot POST /api/v1/accounts/:account_id/direct_uploads
func (h *UploadHandler) AccountDirectUpload(c *gin.Context) {
h.limitBody(c)
accountID := getAccountID(c)
if accountID == 0 {
response.AbortWithStatusError(c, http.StatusBadRequest, response.ErrValidation, "account_id is required")
@@ -129,7 +152,11 @@ func (h *UploadHandler) AccountDirectUpload(c *gin.Context) {
fileHeader, err := c.FormFile("file")
if err != nil {
response.AbortWithStatusError(c, http.StatusBadRequest, response.ErrValidation, "file is required")
if isUploadBodyTooLarge(err) {
c.AbortWithStatusJSON(http.StatusRequestEntityTooLarge, gin.H{"error": "upload body is too large"})
} else {
response.AbortWithStatusError(c, http.StatusBadRequest, response.ErrValidation, "file is required")
}
return
}
@@ -148,6 +175,7 @@ func (h *UploadHandler) AccountDirectUpload(c *gin.Context) {
// endpoint used by the reused dashboard message composer.
// Reference: POST /api/v1/accounts/:account_id/conversations/:conversation_id/direct_uploads
func (h *UploadHandler) ConversationDirectUpload(c *gin.Context) {
h.limitBody(c)
accountID := getAccountID(c)
if accountID == 0 {
response.AbortWithStatusError(c, http.StatusBadRequest, response.ErrValidation, "account_id is required")
@@ -178,6 +206,7 @@ func (h *UploadHandler) ConversationDirectUpload(c *gin.Context) {
}
func (h *UploadHandler) CompleteConversationDirectUpload(c *gin.Context) {
h.limitBody(c)
accountID := getAccountID(c)
if accountID == 0 {
response.AbortWithStatusError(c, http.StatusBadRequest, response.ErrValidation, "account_id is required")
@@ -196,3 +225,67 @@ func (h *UploadHandler) CompleteConversationDirectUpload(c *gin.Context) {
}
response.OK(c, result)
}
// ServeUpload replaces the public StaticFS route with account/widget-session
// authorization. Deliberately return 404 for every denied lookup to avoid an
// attachment existence oracle across tenants.
func (h *UploadHandler) ServeUpload(c *gin.Context) {
fileURL := "/uploads/" + strings.TrimPrefix(c.Param("filepath"), "/")
fullPath, ok := h.svc.ResolveAuthorizedUpload(c.Request.Context(), fileURL, h.dashboardAccountID(c), widgetAccessToken(c))
if !ok {
c.Status(http.StatusNotFound)
return
}
c.Header("Cache-Control", "private, no-store")
c.Header("Content-Security-Policy", "sandbox")
c.Header("X-Content-Type-Options", "nosniff")
c.File(fullPath)
}
func (h *UploadHandler) limitBody(c *gin.Context) {
if c.Request.Body != nil {
limit := int64(21 << 20)
if h.svc != nil {
limit = h.svc.MaxRequestBodySize()
}
c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, limit)
}
}
func (h *UploadHandler) dashboardAccountID(c *gin.Context) uint {
if h.jwtSvc == nil {
return 0
}
token := strings.TrimSpace(c.GetHeader("access-token"))
if authorization := c.GetHeader("Authorization"); strings.HasPrefix(authorization, "Bearer ") {
token = strings.TrimPrefix(authorization, "Bearer ")
}
if token == "" {
if cookie, err := c.Cookie("cw_d_session_info"); err == nil {
var session map[string]any
if json.Unmarshal([]byte(cookie), &session) == nil {
token, _ = session["access-token"].(string)
}
}
}
claims, err := h.jwtSvc.ValidateAccessToken(token)
if err != nil {
return 0
}
return claims.AccountID
}
func widgetAccessToken(c *gin.Context) string {
for _, token := range []string{c.GetHeader("X-Widget-Token"), c.GetHeader("X-Auth-Token")} {
if token != "" {
return token
}
}
token, _ := c.Cookie("cw_conversation")
return token
}
func isUploadBodyTooLarge(err error) bool {
var maxBytesError *http.MaxBytesError
return errors.As(err, &maxBytesError) || strings.Contains(strings.ToLower(err.Error()), "request body too large")
}
@@ -11,6 +11,7 @@ import (
"strconv"
"strings"
"testing"
"time"
"github.com/gin-gonic/gin"
"github.com/stretchr/testify/assert"
@@ -19,12 +20,15 @@ import (
"gorm.io/gorm"
"gorm.io/gorm/logger"
"github.com/gochat/gochat/internal/auth"
"github.com/gochat/gochat/internal/config"
"github.com/gochat/gochat/internal/model"
"github.com/gochat/gochat/internal/repository"
"github.com/gochat/gochat/internal/service"
)
var handlerTestPNG = []byte{0x89, 'P', 'N', 'G', '\r', '\n', 0x1a, '\n', 0, 0, 0, 0}
// mockUploadService implements a mock for UploadService handler tests.
// We can't easily mock UploadService because it's a concrete type, not an interface.
// Instead, we test the handler layer by checking HTTP status codes and response shapes.
@@ -99,7 +103,7 @@ func TestUploadHandler_Upload_ChatwootAttachmentFieldRawPayload(t *testing.T) {
uploadSvc := service.NewUploadService(repository.NewDirectUploadRepo(db), &config.Config{Storage: config.StorageConfig{LocalPath: tmpDir, MaxFileSize: 50 << 20}})
router := setupUploadHandlerRouter(NewUploadHandler(uploadSvc))
body, contentType, err := makeMultipartUploadBodyWithField("attachment", "macro.png", []byte("fake png"))
body, contentType, err := makeMultipartUploadBodyWithField("attachment", "macro.png", handlerTestPNG)
require.NoError(t, err)
req, _ := http.NewRequest("POST", "/api/v1/accounts/1/upload", body)
req.Header.Set("Content-Type", contentType)
@@ -116,7 +120,7 @@ func TestUploadHandler_Upload_ChatwootAttachmentFieldRawPayload(t *testing.T) {
assert.Equal(t, payload["blob_id"], payload["blob_key"])
}
func TestUploadHandler_Upload_ChatwootExternalURLRawPayload(t *testing.T) {
func TestUploadHandler_Upload_RejectsLoopbackExternalURL(t *testing.T) {
tmpDir := t.TempDir()
db, err := gorm.Open(sqlite.Open("file::memory:"), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)})
require.NoError(t, err)
@@ -136,12 +140,10 @@ func TestUploadHandler_Upload_ChatwootExternalURLRawPayload(t *testing.T) {
router.ServeHTTP(w, req)
require.Equal(t, http.StatusOK, w.Code)
require.Equal(t, http.StatusUnprocessableEntity, w.Code)
var payload map[string]any
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &payload))
assert.NotContains(t, payload, "success")
assert.NotEmpty(t, payload["file_url"])
assert.NotEmpty(t, payload["blob_id"])
assert.Contains(t, payload["error"], "SSRF")
}
func TestUploadHandler_DirectUpload_NoFile(t *testing.T) {
@@ -158,6 +160,21 @@ func TestUploadHandler_DirectUpload_NoFile(t *testing.T) {
assert.Equal(t, http.StatusBadRequest, w.Code)
}
func TestUploadHandler_DirectUploadRejectsOversizedBody(t *testing.T) {
db, err := gorm.Open(sqlite.Open("file::memory:"), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)})
require.NoError(t, err)
svc := service.NewUploadService(repository.NewDirectUploadRepo(db), &config.Config{Storage: config.StorageConfig{MaxFileSize: 1}})
router := setupUploadHandlerRouter(NewUploadHandler(svc))
body, contentType, err := makeMultipartUploadBody("large.png", bytes.Repeat([]byte{'x'}, 2<<20))
require.NoError(t, err)
req := httptest.NewRequest(http.MethodPost, "/api/v1/widget/direct_uploads?website_token=test", body)
req.Header.Set("Content-Type", contentType)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
assert.Equal(t, http.StatusRequestEntityTooLarge, w.Code)
}
func TestUploadHandler_WidgetActiveStorageDirectUploadFlow(t *testing.T) {
gin.SetMode(gin.TestMode)
tmpDir := t.TempDir()
@@ -187,10 +204,11 @@ func TestUploadHandler_WidgetActiveStorageDirectUploadFlow(t *testing.T) {
}).WithWidgetAuth(repository.NewInboxRepo(db), repository.NewContactInboxRepo(db))
router := setupUploadHandlerRouter(NewUploadHandler(uploadSvc))
uploadContent := handlerTestPNG
metadataBody, err := json.Marshal(map[string]any{
"blob": map[string]any{
"filename": "visitor.png",
"byte_size": 11,
"byte_size": len(uploadContent),
"checksum": "checksum-token",
"content_type": "image/png",
"metadata": map[string]any{"identified": true},
@@ -212,16 +230,16 @@ func TestUploadHandler_WidgetActiveStorageDirectUploadFlow(t *testing.T) {
require.NotEmpty(t, signedID)
assert.Equal(t, "visitor.png", createResp["filename"])
directUpload := createResp["direct_upload"].(map[string]any)
assert.Equal(t, "/api/v1/widget/direct_uploads/"+signedID, directUpload["url"])
assert.Contains(t, directUpload["url"], "/api/v1/widget/direct_uploads/"+signedID+"?token=")
assert.Equal(t, map[string]any{"Content-Type": "image/png"}, directUpload["headers"])
assert.Equal(t, "gochat_local", createResp["service_name"])
assert.Equal(t, float64(11), createResp["byte_size"])
assert.Equal(t, float64(len(uploadContent)), createResp["byte_size"])
assert.Equal(t, "checksum-token", createResp["checksum"])
assert.Equal(t, true, createResp["metadata"].(map[string]any)["identified"])
assert.NotEmpty(t, createResp["key"])
wPut := httptest.NewRecorder()
reqPut, _ := http.NewRequest("PUT", directUpload["url"].(string), bytes.NewReader([]byte("hello image")))
reqPut, _ := http.NewRequest("PUT", directUpload["url"].(string), bytes.NewReader(uploadContent))
reqPut.Header.Set("Content-Type", "image/png")
router.ServeHTTP(wPut, reqPut)
require.Equal(t, http.StatusOK, wPut.Code)
@@ -233,14 +251,14 @@ func TestUploadHandler_WidgetActiveStorageDirectUploadFlow(t *testing.T) {
assert.Equal(t, "visitor.png", completeData["original_name"])
assert.Equal(t, "image", completeData["file_type"])
assert.Equal(t, "image/png", completeData["mime_type"])
assert.Equal(t, float64(11), completeData["file_size"])
assert.Equal(t, float64(len(uploadContent)), completeData["file_size"])
var upload model.DirectUpload
require.NoError(t, db.Where("upload_uuid = ?", signedID).First(&upload).Error)
assert.Equal(t, account.ID, upload.AccountID)
storedBytes, err := os.ReadFile(filepath.Join(tmpDir, "widget_direct", signedID+".png"))
require.NoError(t, err)
assert.Equal(t, []byte("hello image"), storedBytes)
assert.Equal(t, uploadContent, storedBytes)
}
func TestUploadHandler_ConversationActiveStorageDirectUploadFlow(t *testing.T) {
@@ -279,10 +297,11 @@ func TestUploadHandler_ConversationActiveStorageDirectUploadFlow(t *testing.T) {
}).WithConversationRepo(repository.NewConversationRepo(db))
router := setupUploadHandlerRouter(NewUploadHandler(uploadSvc))
uploadContent := []byte("%PDF-1.4\nhello report")
metadataBody, err := json.Marshal(map[string]any{
"blob": map[string]any{
"filename": "agent-note.pdf",
"byte_size": 12,
"byte_size": len(uploadContent),
"checksum": "pdf-checksum",
"content_type": "application/pdf",
"metadata": map[string]any{"identified": true},
@@ -304,15 +323,15 @@ func TestUploadHandler_ConversationActiveStorageDirectUploadFlow(t *testing.T) {
require.NotEmpty(t, signedID)
assert.Equal(t, "agent-note.pdf", createResp["filename"])
directUpload := createResp["direct_upload"].(map[string]any)
assert.Equal(t, createPath+"/"+signedID, directUpload["url"])
assert.Contains(t, directUpload["url"], createPath+"/"+signedID+"?token=")
assert.Equal(t, map[string]any{"Content-Type": "application/pdf"}, directUpload["headers"])
assert.Equal(t, "gochat_local", createResp["service_name"])
assert.Equal(t, float64(12), createResp["byte_size"])
assert.Equal(t, float64(len(uploadContent)), createResp["byte_size"])
assert.Equal(t, "pdf-checksum", createResp["checksum"])
assert.NotEmpty(t, createResp["key"])
wPut := httptest.NewRecorder()
reqPut, _ := http.NewRequest("PUT", directUpload["url"].(string), bytes.NewReader([]byte("hello report")))
reqPut, _ := http.NewRequest("PUT", directUpload["url"].(string), bytes.NewReader(uploadContent))
reqPut.Header.Set("Content-Type", "application/pdf")
router.ServeHTTP(wPut, reqPut)
require.Equal(t, http.StatusOK, wPut.Code)
@@ -324,7 +343,7 @@ func TestUploadHandler_ConversationActiveStorageDirectUploadFlow(t *testing.T) {
assert.Equal(t, "agent-note.pdf", completeData["original_name"])
assert.Equal(t, "file", completeData["file_type"])
assert.Equal(t, "application/pdf", completeData["mime_type"])
assert.Equal(t, float64(12), completeData["file_size"])
assert.Equal(t, float64(len(uploadContent)), completeData["file_size"])
var upload model.DirectUpload
require.NoError(t, db.Where("upload_uuid = ?", signedID).First(&upload).Error)
@@ -332,7 +351,7 @@ func TestUploadHandler_ConversationActiveStorageDirectUploadFlow(t *testing.T) {
assert.Equal(t, model.DirectUploadSourceAccount, upload.Source)
storedBytes, err := os.ReadFile(filepath.Join(tmpDir, "account", strconv.FormatUint(uint64(account.ID), 10), signedID+".pdf"))
require.NoError(t, err)
assert.Equal(t, []byte("hello report"), storedBytes)
assert.Equal(t, uploadContent, storedBytes)
}
func TestUploadHandler_ConversationActiveStorageDirectUploadRejectsUnsupportedMIME(t *testing.T) {
@@ -421,7 +440,7 @@ func TestUploadHandler_AccountDirectUpload_WithFileButNilService(t *testing.T) {
h := &UploadHandler{svc: nil}
r := setupUploadHandlerRouter(h)
body, contentType, err := makeMultipartUploadBody("test.png", []byte("fake png"))
body, contentType, err := makeMultipartUploadBody("test.png", handlerTestPNG)
assert.NoError(t, err)
req, _ := http.NewRequest("POST", "/api/v1/accounts/1/direct_uploads", body)
@@ -439,6 +458,54 @@ func TestUploadHandler_AccountDirectUpload_WithFileButNilService(t *testing.T) {
r.ServeHTTP(w, req)
}
func TestUploadHandler_PrivateUploadReturns404AcrossTenants(t *testing.T) {
gin.SetMode(gin.TestMode)
tmpDir := t.TempDir()
db, err := gorm.Open(sqlite.Open("file::memory:"), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)})
require.NoError(t, err)
require.NoError(t, db.AutoMigrate(&model.DirectUpload{}))
fileURL := "/uploads/account/1/private.png"
require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "account", "1"), 0o755))
require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "account", "1", "private.png"), handlerTestPNG, 0o600))
require.NoError(t, db.Create(&model.DirectUpload{
UploadUUID: "private-upload", AccountID: 1, Status: model.DirectUploadStatusCompleted,
Source: model.DirectUploadSourceAccount, OriginalName: "private.png", FileType: "image",
MimeType: "image/png", FileSize: int64(len(handlerTestPNG)), FileURL: fileURL, ExpiresAt: time.Now().Add(time.Hour),
}).Error)
jwtCfg := &config.JWTConfig{Secret: "upload-access-test-secret", ExpiryHours: 1, RefreshExpiryHours: 1}
jwtSvc := auth.NewJWTService(jwtCfg)
uploadSvc := service.NewUploadService(repository.NewDirectUploadRepo(db), &config.Config{Storage: config.StorageConfig{LocalPath: tmpDir}}).WithAccessDB(db)
handler := NewUploadHandler(uploadSvc).WithAccessAuth(jwtSvc)
router := gin.New()
router.GET("/uploads/*filepath", handler.ServeUpload)
tokenFor := func(accountID uint) string {
pair, tokenErr := jwtSvc.GenerateTokenPair(&model.User{Base: model.Base{ID: 1}, Provider: "email"}, accountID, "agent")
require.NoError(t, tokenErr)
return pair.AccessToken
}
for _, test := range []struct {
name string
token string
want int
}{
{name: "anonymous", want: http.StatusNotFound},
{name: "other tenant", token: tokenFor(2), want: http.StatusNotFound},
{name: "own tenant", token: tokenFor(1), want: http.StatusOK},
} {
t.Run(test.name, func(t *testing.T) {
req := httptest.NewRequest(http.MethodGet, fileURL, nil)
if test.token != "" {
req.Header.Set("access-token", test.token)
}
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
assert.Equal(t, test.want, w.Code)
})
}
}
func TestUploadHandler_ResponseStructure(t *testing.T) {
// Verify UploadResponse DTO structure matches expected JSON keys
resp := service.UploadResponse{