Merge pull request 'Fix user mention processing' (#3565) from algernon/forgejo:call-me-maybe into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3565
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

go.mod

@@ -7,7 +7,7 @@ require (
 	code.gitea.io/gitea-vet v0.2.3
 	code.gitea.io/sdk/gitea v0.17.1
 	codeberg.org/gusted/mcaptcha v0.0.0-20220723083913-4f3072e1d570
-	connectrpc.com/connect v1.15.0
+	connectrpc.com/connect v1.16.1
 	gitea.com/go-chi/binding v0.0.0-20230415142243-04b515c6d669
 	gitea.com/go-chi/cache v0.2.0
 	gitea.com/go-chi/captcha v0.0.0-20240315150714-fb487f629098

go.sum

@@ -43,8 +43,8 @@ code.gitea.io/sdk/gitea v0.17.1 h1:3jCPOG2ojbl8AcfaUCRYLT5MUcBMFwS0OSK2mA5Zok8=
 code.gitea.io/sdk/gitea v0.17.1/go.mod h1:aCnBqhHpoEWA180gMbaCtdX9Pl6BWBAuuP2miadoTNM=
 codeberg.org/gusted/mcaptcha v0.0.0-20220723083913-4f3072e1d570 h1:TXbikPqa7YRtfU9vS6QJBg77pUvbEb6StRdZO8t1bEY=
 codeberg.org/gusted/mcaptcha v0.0.0-20220723083913-4f3072e1d570/go.mod h1:IIAjsijsd8q1isWX8MACefDEgTQslQ4stk2AeeTt3kM=
-connectrpc.com/connect v1.15.0 h1:lFdeCbZrVVDydAqwr4xGV2y+ULn+0Z73s5JBj2LikWo=
-connectrpc.com/connect v1.15.0/go.mod h1:bQmjpDY8xItMnttnurVgOkHUBMRT9cpsNi2O4AjKhmA=
+connectrpc.com/connect v1.16.1 h1:rOdrK/RTI/7TVnn3JsVxt3n028MlTRwmK5Q4heSpjis=
+connectrpc.com/connect v1.16.1/go.mod h1:XpZAduBQUySsb4/KO5JffORVkDI4B6/EYPi7N8xpNZw=
 dario.cat/mergo v1.0.0 h1:AGCNq9Evsj31mOgNPcLyXc+4PNABt905YmuqPYYpBWk=
 dario.cat/mergo v1.0.0/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk=
 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=

modules/markup/html.go

@@ -623,10 +623,10 @@ func mentionProcessor(ctx *RenderContext, node *html.Node) {
 		if DefaultProcessorHelper.IsUsernameMentionable != nil && DefaultProcessorHelper.IsUsernameMentionable(ctx.Ctx, mentionedUsername) {
 			replaceContent(node, loc.Start, loc.End, createLink(util.URLJoin(ctx.Links.Prefix(), mentionedUsername), mention, "mention"))
 			node = node.NextSibling.NextSibling
+			start = 0
 		} else {
-			node = node.NextSibling
+			start = loc.End
 		}
-		start = 0
 	}
 }
 

modules/templates/util_render_test.go

@@ -50,6 +50,11 @@ func TestApostrophesInMentions(t *testing.T) {
 	assert.EqualValues(t, template.HTML("<p><a href=\"/mention-user\" rel=\"nofollow\">@mention-user</a>&#39;s comment</p>\n"), rendered)
 }
 
+func TestNonExistantUserMention(t *testing.T) {
+	rendered := RenderMarkdownToHtml(context.Background(), "@ThisUserDoesNotExist @mention-user")
+	assert.EqualValues(t, template.HTML("<p>@ThisUserDoesNotExist <a href=\"/mention-user\" rel=\"nofollow\">@mention-user</a></p>\n"), rendered)
+}
+
 func TestRenderCommitBody(t *testing.T) {
 	type args struct {
 		ctx   context.Context

package-lock.json

@@ -68,7 +68,7 @@
         "@playwright/test": "1.43.0",
         "@stoplight/spectral-cli": "6.11.1",
         "@stylistic/eslint-plugin-js": "1.7.2",
-        "@stylistic/stylelint-plugin": "2.1.1",
+        "@stylistic/stylelint-plugin": "2.1.2",
         "@vitejs/plugin-vue": "5.0.4",
         "@vue/test-utils": "2.4.5",
         "eslint": "8.57.0",
@@ -2145,19 +2145,19 @@
       }
     },
     "node_modules/@stylistic/stylelint-plugin": {
-      "version": "2.1.1",
-      "resolved": "https://registry.npmjs.org/@stylistic/stylelint-plugin/-/stylelint-plugin-2.1.1.tgz",
-      "integrity": "sha512-xqHTmQZN7EbnFDW7jw0rAsdFNO4IRqvXhrh3qhUlIwF/x09Zm7kgs/ADktHxsTJYcw346PpGihsB0t4pZhpeHw==",
+      "version": "2.1.2",
+      "resolved": "https://registry.npmjs.org/@stylistic/stylelint-plugin/-/stylelint-plugin-2.1.2.tgz",
+      "integrity": "sha512-JsSqu0Y3vsX+PBl+DwULxC0cIv9C1yIcq1MXkx7pBOGtTqU26a75I8MPYMiEYvrsXgsKLi65xVgy1iLVSZquJA==",
       "dev": true,
       "dependencies": {
-        "@csstools/css-parser-algorithms": "^2.5.0",
-        "@csstools/css-tokenizer": "^2.2.3",
-        "@csstools/media-query-list-parser": "^2.1.7",
+        "@csstools/css-parser-algorithms": "^2.6.1",
+        "@csstools/css-tokenizer": "^2.2.4",
+        "@csstools/media-query-list-parser": "^2.1.9",
         "is-plain-object": "^5.0.0",
-        "postcss-selector-parser": "^6.0.15",
+        "postcss-selector-parser": "^6.0.16",
         "postcss-value-parser": "^4.2.0",
         "style-search": "^0.1.0",
-        "stylelint": "^16.2.1"
+        "stylelint": "^16.4.0"
       },
       "engines": {
         "node": "^18.12 || >=20.9"

package.json

@@ -67,7 +67,7 @@
     "@playwright/test": "1.43.0",
     "@stoplight/spectral-cli": "6.11.1",
     "@stylistic/eslint-plugin-js": "1.7.2",
-    "@stylistic/stylelint-plugin": "2.1.1",
+    "@stylistic/stylelint-plugin": "2.1.2",
     "@vitejs/plugin-vue": "5.0.4",
     "@vue/test-utils": "2.4.5",
     "eslint": "8.57.0",

release-notes/8.0.0/fix/3504.md

@@ -0,0 +1 @@
+Fixed that inline attachments of emails (as they occur for example with Apple Mail) are not attached to comments.

services/mailer/incoming/incoming.go

@@ -219,6 +219,11 @@ loop:
 			}
 
 			err := func() error {
+				if isAlreadyHandled(handledSet, msg) {
+					log.Debug("Skipping already handled message")
+					return nil
+				}
+
 				r := msg.GetBody(section)
 				if r == nil {
 					return fmt.Errorf("could not get body from message: %w", err)
@@ -277,6 +282,11 @@ loop:
 	return nil
 }
 
+// isAlreadyHandled tests if the message was already handled
+func isAlreadyHandled(handledSet *imap.SeqSet, msg *imap.Message) bool {
+	return handledSet.Contains(msg.SeqNum)
+}
+
 // isAutomaticReply tests if the headers indicate an automatic reply
 func isAutomaticReply(env *enmime.Envelope) bool {
 	autoSubmitted := env.GetHeader("Auto-Submitted")
@@ -367,6 +377,14 @@ func getContentFromMailReader(env *enmime.Envelope) *MailContent {
 			Content: attachment.Content,
 		})
 	}
+	for _, inline := range env.Inlines {
+		if inline.FileName != "" {
+			attachments = append(attachments, &Attachment{
+				Name:    inline.FileName,
+				Content: inline.Content,
+			})
+		}
+	}
 
 	return &MailContent{
 		Content:     reply.FromText(env.Text),

services/mailer/incoming/incoming_test.go

@@ -7,10 +7,24 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/emersion/go-imap"
 	"github.com/jhillyerd/enmime"
 	"github.com/stretchr/testify/assert"
 )
 
+func TestNotHandleTwice(t *testing.T) {
+	handledSet := new(imap.SeqSet)
+	msg := imap.NewMessage(90, []imap.FetchItem{imap.FetchBody})
+
+	handled := isAlreadyHandled(handledSet, msg)
+	assert.Equal(t, false, handled)
+
+	handledSet.AddNum(msg.SeqNum)
+
+	handled = isAlreadyHandled(handledSet, msg)
+	assert.Equal(t, true, handled)
+}
+
 func TestIsAutomaticReply(t *testing.T) {
 	cases := []struct {
 		Headers  map[string]string
@@ -95,6 +109,32 @@ func TestGetContentFromMailReader(t *testing.T) {
 	assert.Equal(t, "attachment.txt", content.Attachments[0].Name)
 	assert.Equal(t, []byte("attachment content"), content.Attachments[0].Content)
 
+	mailString = "Content-Type: multipart/mixed; boundary=message-boundary\r\n" +
+		"\r\n" +
+		"--message-boundary\r\n" +
+		"Content-Type: multipart/alternative; boundary=text-boundary\r\n" +
+		"\r\n" +
+		"--text-boundary\r\n" +
+		"Content-Type: text/plain\r\n" +
+		"Content-Disposition: inline\r\n" +
+		"\r\n" +
+		"mail content\r\n" +
+		"--text-boundary--\r\n" +
+		"--message-boundary\r\n" +
+		"Content-Type: text/plain\r\n" +
+		"Content-Disposition: inline; filename=attachment.txt\r\n" +
+		"\r\n" +
+		"attachment content\r\n" +
+		"--message-boundary--\r\n"
+
+	env, err = enmime.ReadEnvelope(strings.NewReader(mailString))
+	assert.NoError(t, err)
+	content = getContentFromMailReader(env)
+	assert.Equal(t, "mail content", content.Content)
+	assert.Len(t, content.Attachments, 1)
+	assert.Equal(t, "attachment.txt", content.Attachments[0].Name)
+	assert.Equal(t, []byte("attachment content"), content.Attachments[0].Content)
+
 	mailString = "Content-Type: multipart/mixed; boundary=message-boundary\r\n" +
 		"\r\n" +
 		"--message-boundary\r\n" +

services/webhook/default.go

@@ -12,6 +12,8 @@ import (
 	"strings"
 
 	webhook_model "code.gitea.io/gitea/models/webhook"
+	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/svg"
 	webhook_module "code.gitea.io/gitea/modules/webhook"
@@ -67,6 +69,18 @@ func (defaultHandler) UnmarshalForm(bind func(any)) forms.WebhookForm {
 }
 
 func (defaultHandler) NewRequest(ctx context.Context, w *webhook_model.Webhook, t *webhook_model.HookTask) (req *http.Request, body []byte, err error) {
+	payloadContent := t.PayloadContent
+	if w.Type == webhook_module.GITEA &&
+		(t.EventType == webhook_module.HookEventCreate || t.EventType == webhook_module.HookEventDelete) {
+		// Woodpecker expects the ref to be short on tag creation only
+		// https://github.com/woodpecker-ci/woodpecker/blob/00ccec078cdced80cf309cd4da460a5041d7991a/server/forge/gitea/helper.go#L134
+		// see https://codeberg.org/codeberg/community/issues/1556
+		payloadContent, err = substituteRefShortName(payloadContent)
+		if err != nil {
+			return nil, nil, fmt.Errorf("could not substiture ref: %w", err)
+		}
+	}
+
 	switch w.HTTPMethod {
 	case "":
 		log.Info("HTTP Method for %s webhook %s [ID: %d] is not set, defaulting to POST", w.Type, w.URL, w.ID)
@@ -74,7 +88,7 @@ func (defaultHandler) NewRequest(ctx context.Context, w *webhook_model.Webhook,
 	case http.MethodPost:
 		switch w.ContentType {
 		case webhook_model.ContentTypeJSON:
-			req, err = http.NewRequest("POST", w.URL, strings.NewReader(t.PayloadContent))
+			req, err = http.NewRequest("POST", w.URL, strings.NewReader(payloadContent))
 			if err != nil {
 				return nil, nil, err
 			}
@@ -82,7 +96,7 @@ func (defaultHandler) NewRequest(ctx context.Context, w *webhook_model.Webhook,
 			req.Header.Set("Content-Type", "application/json")
 		case webhook_model.ContentTypeForm:
 			forms := url.Values{
-				"payload": []string{t.PayloadContent},
+				"payload": []string{payloadContent},
 			}
 
 			req, err = http.NewRequest("POST", w.URL, strings.NewReader(forms.Encode()))
@@ -100,7 +114,7 @@ func (defaultHandler) NewRequest(ctx context.Context, w *webhook_model.Webhook,
 			return nil, nil, fmt.Errorf("invalid URL: %w", err)
 		}
 		vals := u.Query()
-		vals["payload"] = []string{t.PayloadContent}
+		vals["payload"] = []string{payloadContent}
 		u.RawQuery = vals.Encode()
 		req, err = http.NewRequest("GET", u.String(), nil)
 		if err != nil {
@@ -109,12 +123,12 @@ func (defaultHandler) NewRequest(ctx context.Context, w *webhook_model.Webhook,
 	case http.MethodPut:
 		switch w.Type {
 		case webhook_module.MATRIX: // used when t.Version == 1
-			txnID, err := getMatrixTxnID([]byte(t.PayloadContent))
+			txnID, err := getMatrixTxnID([]byte(payloadContent))
 			if err != nil {
 				return nil, nil, err
 			}
 			url := fmt.Sprintf("%s/%s", w.URL, url.PathEscape(txnID))
-			req, err = http.NewRequest("PUT", url, strings.NewReader(t.PayloadContent))
+			req, err = http.NewRequest("PUT", url, strings.NewReader(payloadContent))
 			if err != nil {
 				return nil, nil, err
 			}
@@ -125,6 +139,22 @@ func (defaultHandler) NewRequest(ctx context.Context, w *webhook_model.Webhook,
 		return nil, nil, fmt.Errorf("invalid http method: %v", w.HTTPMethod)
 	}
 
-	body = []byte(t.PayloadContent)
+	body = []byte(payloadContent)
 	return req, body, shared.AddDefaultHeaders(req, []byte(w.Secret), t, body)
 }
+
+func substituteRefShortName(body string) (string, error) {
+	var m map[string]any
+	if err := json.Unmarshal([]byte(body), &m); err != nil {
+		return body, err
+	}
+	ref, ok := m["ref"].(string)
+	if !ok {
+		return body, fmt.Errorf("expected string 'ref', got %T", m["ref"])
+	}
+
+	m["ref"] = git.RefName(ref).ShortName()
+
+	buf, err := json.Marshal(m)
+	return string(buf), err
+}

services/webhook/default_test.go

@@ -0,0 +1,224 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package webhook
+
+import (
+	"context"
+	"testing"
+
+	webhook_model "code.gitea.io/gitea/models/webhook"
+	"code.gitea.io/gitea/modules/json"
+	webhook_module "code.gitea.io/gitea/modules/webhook"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestGiteaPayload(t *testing.T) {
+	dh := defaultHandler{
+		forgejo: false,
+	}
+	hook := &webhook_model.Webhook{
+		RepoID:      3,
+		IsActive:    true,
+		Type:        webhook_module.GITEA,
+		URL:         "https://gitea.example.com/",
+		Meta:        ``,
+		HTTPMethod:  "POST",
+		ContentType: webhook_model.ContentTypeJSON,
+	}
+
+	// Woodpecker expects the ref to be short on tag creation only
+	// https://github.com/woodpecker-ci/woodpecker/blob/00ccec078cdced80cf309cd4da460a5041d7991a/server/forge/gitea/helper.go#L134
+	// see https://codeberg.org/codeberg/community/issues/1556
+	t.Run("Create", func(t *testing.T) {
+		p := createTestPayload()
+		data, err := p.JSONPayload()
+		require.NoError(t, err)
+
+		task := &webhook_model.HookTask{
+			HookID:         hook.ID,
+			EventType:      webhook_module.HookEventCreate,
+			PayloadContent: string(data),
+			PayloadVersion: 2,
+		}
+
+		req, reqBody, err := dh.NewRequest(context.Background(), hook, task)
+		require.NoError(t, err)
+		require.NotNil(t, req)
+		require.NotNil(t, reqBody)
+
+		assert.Equal(t, "POST", req.Method)
+		assert.Equal(t, "https://gitea.example.com/", req.URL.String())
+		assert.Equal(t, "sha256=", req.Header.Get("X-Hub-Signature-256"))
+		assert.Equal(t, "application/json", req.Header.Get("Content-Type"))
+		var body struct {
+			Ref string `json:"ref"`
+		}
+		err = json.NewDecoder(req.Body).Decode(&body)
+		assert.NoError(t, err)
+		assert.Equal(t, "test", body.Ref) // short ref
+	})
+
+	t.Run("Push", func(t *testing.T) {
+		p := pushTestPayload()
+		data, err := p.JSONPayload()
+		require.NoError(t, err)
+
+		task := &webhook_model.HookTask{
+			HookID:         hook.ID,
+			EventType:      webhook_module.HookEventPush,
+			PayloadContent: string(data),
+			PayloadVersion: 2,
+		}
+
+		req, reqBody, err := dh.NewRequest(context.Background(), hook, task)
+		require.NoError(t, err)
+		require.NotNil(t, req)
+		require.NotNil(t, reqBody)
+
+		assert.Equal(t, "POST", req.Method)
+		assert.Equal(t, "https://gitea.example.com/", req.URL.String())
+		assert.Equal(t, "sha256=", req.Header.Get("X-Hub-Signature-256"))
+		assert.Equal(t, "application/json", req.Header.Get("Content-Type"))
+		var body struct {
+			Ref string `json:"ref"`
+		}
+		err = json.NewDecoder(req.Body).Decode(&body)
+		assert.NoError(t, err)
+		assert.Equal(t, "refs/heads/test", body.Ref) // full ref
+	})
+
+	t.Run("Delete", func(t *testing.T) {
+		p := deleteTestPayload()
+		data, err := p.JSONPayload()
+		require.NoError(t, err)
+
+		task := &webhook_model.HookTask{
+			HookID:         hook.ID,
+			EventType:      webhook_module.HookEventDelete,
+			PayloadContent: string(data),
+			PayloadVersion: 2,
+		}
+
+		req, reqBody, err := dh.NewRequest(context.Background(), hook, task)
+		require.NoError(t, err)
+		require.NotNil(t, req)
+		require.NotNil(t, reqBody)
+
+		assert.Equal(t, "POST", req.Method)
+		assert.Equal(t, "https://gitea.example.com/", req.URL.String())
+		assert.Equal(t, "sha256=", req.Header.Get("X-Hub-Signature-256"))
+		assert.Equal(t, "application/json", req.Header.Get("Content-Type"))
+		var body struct {
+			Ref string `json:"ref"`
+		}
+		err = json.NewDecoder(req.Body).Decode(&body)
+		assert.NoError(t, err)
+		assert.Equal(t, "test", body.Ref) // short ref
+	})
+}
+
+func TestForgejoPayload(t *testing.T) {
+	dh := defaultHandler{
+		forgejo: true,
+	}
+	hook := &webhook_model.Webhook{
+		RepoID:      3,
+		IsActive:    true,
+		Type:        webhook_module.FORGEJO,
+		URL:         "https://forgejo.example.com/",
+		Meta:        ``,
+		HTTPMethod:  "POST",
+		ContentType: webhook_model.ContentTypeJSON,
+	}
+
+	// always return the full ref for consistency
+	t.Run("Create", func(t *testing.T) {
+		p := createTestPayload()
+		data, err := p.JSONPayload()
+		require.NoError(t, err)
+
+		task := &webhook_model.HookTask{
+			HookID:         hook.ID,
+			EventType:      webhook_module.HookEventCreate,
+			PayloadContent: string(data),
+			PayloadVersion: 2,
+		}
+
+		req, reqBody, err := dh.NewRequest(context.Background(), hook, task)
+		require.NoError(t, err)
+		require.NotNil(t, req)
+		require.NotNil(t, reqBody)
+
+		assert.Equal(t, "POST", req.Method)
+		assert.Equal(t, "https://forgejo.example.com/", req.URL.String())
+		assert.Equal(t, "sha256=", req.Header.Get("X-Hub-Signature-256"))
+		assert.Equal(t, "application/json", req.Header.Get("Content-Type"))
+		var body struct {
+			Ref string `json:"ref"`
+		}
+		err = json.NewDecoder(req.Body).Decode(&body)
+		assert.NoError(t, err)
+		assert.Equal(t, "refs/heads/test", body.Ref) // full ref
+	})
+
+	t.Run("Push", func(t *testing.T) {
+		p := pushTestPayload()
+		data, err := p.JSONPayload()
+		require.NoError(t, err)
+
+		task := &webhook_model.HookTask{
+			HookID:         hook.ID,
+			EventType:      webhook_module.HookEventPush,
+			PayloadContent: string(data),
+			PayloadVersion: 2,
+		}
+
+		req, reqBody, err := dh.NewRequest(context.Background(), hook, task)
+		require.NoError(t, err)
+		require.NotNil(t, req)
+		require.NotNil(t, reqBody)
+
+		assert.Equal(t, "POST", req.Method)
+		assert.Equal(t, "https://forgejo.example.com/", req.URL.String())
+		assert.Equal(t, "sha256=", req.Header.Get("X-Hub-Signature-256"))
+		assert.Equal(t, "application/json", req.Header.Get("Content-Type"))
+		var body struct {
+			Ref string `json:"ref"`
+		}
+		err = json.NewDecoder(req.Body).Decode(&body)
+		assert.NoError(t, err)
+		assert.Equal(t, "refs/heads/test", body.Ref) // full ref
+	})
+
+	t.Run("Delete", func(t *testing.T) {
+		p := deleteTestPayload()
+		data, err := p.JSONPayload()
+		require.NoError(t, err)
+
+		task := &webhook_model.HookTask{
+			HookID:         hook.ID,
+			EventType:      webhook_module.HookEventDelete,
+			PayloadContent: string(data),
+			PayloadVersion: 2,
+		}
+
+		req, reqBody, err := dh.NewRequest(context.Background(), hook, task)
+		require.NoError(t, err)
+		require.NotNil(t, req)
+		require.NotNil(t, reqBody)
+
+		assert.Equal(t, "POST", req.Method)
+		assert.Equal(t, "https://forgejo.example.com/", req.URL.String())
+		assert.Equal(t, "sha256=", req.Header.Get("X-Hub-Signature-256"))
+		assert.Equal(t, "application/json", req.Header.Get("Content-Type"))
+		var body struct {
+			Ref string `json:"ref"`
+		}
+		err = json.NewDecoder(req.Body).Decode(&body)
+		assert.NoError(t, err)
+		assert.Equal(t, "refs/heads/test", body.Ref) // full ref
+	})
+}

services/webhook/webhook.go

@@ -82,19 +82,17 @@ var hookQueue *queue.WorkerPoolQueue[int64]
 
 // getPayloadBranch returns branch for hook event, if applicable.
 func getPayloadBranch(p api.Payloader) string {
+	var ref string
 	switch pp := p.(type) {
 	case *api.CreatePayload:
-		if pp.RefType == "branch" {
-			return pp.Ref
-		}
+		ref = pp.Ref
 	case *api.DeletePayload:
-		if pp.RefType == "branch" {
-			return pp.Ref
-		}
+		ref = pp.Ref
 	case *api.PushPayload:
-		if strings.HasPrefix(pp.Ref, git.BranchPrefix) {
-			return pp.Ref[len(git.BranchPrefix):]
-		}
+		ref = pp.Ref
+	}
+	if strings.HasPrefix(ref, git.BranchPrefix) {
+		return ref[len(git.BranchPrefix):]
 	}
 	return ""
 }