mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2025-08-19 17:01:12 +00:00
fix: prevent user-entered text with | characters from being truncated in activity feed (#8844)
Prevents a variety of user-entered texts that can contain `|` characters from being truncated in the activity feed, affecting: issue & PR titles, comment content, review comments, and review dismissal comments. Where `action.content` was containing a pipe-separated list of UI data fields before, it now uses a JSON-encoded string array. The old format is still supported for reading from the feed. In some places where `action.content` was not using this format, or where user-generated text was not inserted, the old format is retained. Fixes part of the cause behind #8781, allowing small mermaid graphs to be rendered in the feed (for now...) --  ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8844 Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org> Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
Mathieu Fenniak
Earl Warren
parent
f1cfd152e2
commit
1f2bbbd4aa
6 changed files with 140 additions and 14 deletions
|
|
@ -26,6 +26,7 @@ import (
|
|||
"forgejo.org/modules/base"
|
||||
"forgejo.org/modules/container"
|
||||
"forgejo.org/modules/git"
|
||||
"forgejo.org/modules/json"
|
||||
"forgejo.org/modules/log"
|
||||
"forgejo.org/modules/setting"
|
||||
"forgejo.org/modules/structs"
|
||||
|
|
@ -386,8 +387,21 @@ func (a *Action) IsIssueEvent() bool {
|
|||
|
||||
// GetIssueInfos returns a list of associated information with the action.
|
||||
func (a *Action) GetIssueInfos() []string {
|
||||
// Previously multiple pieces of data used to be encoded into a.Content by pipe-separating them, but this doesn't
|
||||
// work well if some of the user-entered pieces of content (issue titles, comments, etc.) contain pipes. The newer
|
||||
// storage format is to json-encode a string array, which we check for and prefer... then fallback to assuming old.
|
||||
var ret []string
|
||||
if strings.HasPrefix(a.Content, "[") && strings.HasSuffix(a.Content, "]") {
|
||||
ret = make([]string, 0, 3)
|
||||
err := json.Unmarshal([]byte(a.Content), &ret)
|
||||
if err != nil {
|
||||
log.Error("GetIssueInfos json decoding error: %v", err)
|
||||
}
|
||||
} else {
|
||||
ret = strings.SplitN(a.Content, "|", 3)
|
||||
}
|
||||
|
||||
// make sure it always returns 3 elements, because there are some access to the a[1] and a[2] without checking the length
|
||||
ret := strings.SplitN(a.Content, "|", 3)
|
||||
for len(ret) < 3 {
|
||||
ret = append(ret, "")
|
||||
}
|
||||
|
|
@ -770,7 +784,9 @@ func DeleteIssueActions(ctx context.Context, repoID, issueID, issueIndex int64)
|
|||
|
||||
_, err := e.Where("repo_id = ?", repoID).
|
||||
In("op_type", ActionCreateIssue, ActionCreatePullRequest).
|
||||
Where("content LIKE ?", strconv.FormatInt(issueIndex, 10)+"|%"). // "IssueIndex|content..."
|
||||
Where(builder.Or(
|
||||
builder.Like{"content", strconv.FormatInt(issueIndex, 10) + "|%"}, // "IssueIndex|content..."
|
||||
builder.Like{"content", "[\"" + strconv.FormatInt(issueIndex, 10) + "\"%"})). // JSON, ["IssueIndex"...
|
||||
Delete(&Action{})
|
||||
return err
|
||||
}
|
||||
|
|
|
|||
|
|
@ -308,14 +308,69 @@ func TestDeleteIssueActions(t *testing.T) {
|
|||
})
|
||||
require.NoError(t, err)
|
||||
err = db.Insert(db.DefaultContext, &activities_model.Action{
|
||||
OpType: activities_model.ActionCreateIssue,
|
||||
RepoID: issue.RepoID,
|
||||
OpType: activities_model.ActionCreateIssue,
|
||||
RepoID: issue.RepoID,
|
||||
// Older Content format...
|
||||
Content: fmt.Sprintf("%d|content...", issue.Index),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
err = db.Insert(db.DefaultContext, &activities_model.Action{
|
||||
OpType: activities_model.ActionCreateIssue,
|
||||
RepoID: issue.RepoID,
|
||||
// JSON-encoded Content format...
|
||||
Content: fmt.Sprintf("[\"%d\",\"content...\"]", issue.Index),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
// assert that the actions exist, then delete them
|
||||
unittest.AssertCount(t, &activities_model.Action{}, 2)
|
||||
unittest.AssertCount(t, &activities_model.Action{}, 3)
|
||||
require.NoError(t, activities_model.DeleteIssueActions(db.DefaultContext, issue.RepoID, issue.ID, issue.Index))
|
||||
unittest.AssertCount(t, &activities_model.Action{}, 0)
|
||||
}
|
||||
|
||||
func TestGetIssueInfos(t *testing.T) {
|
||||
tt := []struct {
|
||||
content string
|
||||
field1 string
|
||||
field2 string
|
||||
field3 string
|
||||
}{
|
||||
{
|
||||
content: "4|",
|
||||
field1: "4",
|
||||
},
|
||||
{
|
||||
content: "2|docs: Add README w/ template sections",
|
||||
field1: "2",
|
||||
field2: "docs: Add README w/ template sections",
|
||||
},
|
||||
{
|
||||
content: "2|docs: Add README w/ template sections|Some comment...",
|
||||
field1: "2",
|
||||
field2: "docs: Add README w/ template sections",
|
||||
field3: "Some comment...",
|
||||
},
|
||||
{
|
||||
content: "[\"4\"]",
|
||||
field1: "4",
|
||||
},
|
||||
{
|
||||
content: "[\"2\", \"docs: Add README w/ | template sections\"]",
|
||||
field1: "2",
|
||||
field2: "docs: Add README w/ | template sections",
|
||||
},
|
||||
{
|
||||
content: "[\"2\", \"docs: Add README w/ | template sections\", \"Some | comment...\"]",
|
||||
field1: "2",
|
||||
field2: "docs: Add README w/ | template sections",
|
||||
field3: "Some | comment...",
|
||||
},
|
||||
}
|
||||
for _, test := range tt {
|
||||
action := &activities_model.Action{Content: test.content}
|
||||
issueInfos := action.GetIssueInfos()
|
||||
assert.Equal(t, test.field1, issueInfos[0])
|
||||
assert.Equal(t, test.field2, issueInfos[1])
|
||||
assert.Equal(t, test.field3, issueInfos[2])
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -71,7 +71,7 @@ func (a *actionNotifier) NewIssue(ctx context.Context, issue *issues_model.Issue
|
|||
ActUserID: issue.Poster.ID,
|
||||
ActUser: issue.Poster,
|
||||
OpType: activities_model.ActionCreateIssue,
|
||||
Content: fmt.Sprintf("%d|%s", issue.Index, issue.Title),
|
||||
Content: encodeContent(fmt.Sprintf("%d", issue.Index), issue.Title),
|
||||
RepoID: repo.ID,
|
||||
Repo: repo,
|
||||
IsPrivate: repo.IsPrivate,
|
||||
|
|
@ -87,7 +87,7 @@ func (a *actionNotifier) IssueChangeStatus(ctx context.Context, doer *user_model
|
|||
act := &activities_model.Action{
|
||||
ActUserID: doer.ID,
|
||||
ActUser: doer,
|
||||
Content: fmt.Sprintf("%d|%s", issue.Index, ""),
|
||||
Content: encodeContent(fmt.Sprintf("%d", issue.Index), ""),
|
||||
RepoID: issue.Repo.ID,
|
||||
Repo: issue.Repo,
|
||||
Comment: actionComment,
|
||||
|
|
@ -135,7 +135,7 @@ func (a *actionNotifier) CreateIssueComment(ctx context.Context, doer *user_mode
|
|||
truncatedContent = truncatedContent[:lastSpaceIdx] + "…"
|
||||
}
|
||||
}
|
||||
act.Content = fmt.Sprintf("%d|%s", issue.Index, truncatedContent)
|
||||
act.Content = encodeContent(fmt.Sprintf("%d", issue.Index), truncatedContent)
|
||||
|
||||
if issue.IsPull {
|
||||
act.OpType = activities_model.ActionCommentPull
|
||||
|
|
@ -167,7 +167,7 @@ func (a *actionNotifier) NewPullRequest(ctx context.Context, pull *issues_model.
|
|||
ActUserID: pull.Issue.Poster.ID,
|
||||
ActUser: pull.Issue.Poster,
|
||||
OpType: activities_model.ActionCreatePullRequest,
|
||||
Content: fmt.Sprintf("%d|%s", pull.Issue.Index, pull.Issue.Title),
|
||||
Content: encodeContent(fmt.Sprintf("%d", pull.Issue.Index), pull.Issue.Title),
|
||||
RepoID: pull.Issue.Repo.ID,
|
||||
Repo: pull.Issue.Repo,
|
||||
IsPrivate: pull.Issue.Repo.IsPrivate,
|
||||
|
|
@ -247,7 +247,7 @@ func (a *actionNotifier) PullRequestReview(ctx context.Context, pr *issues_model
|
|||
actions = append(actions, &activities_model.Action{
|
||||
ActUserID: review.Reviewer.ID,
|
||||
ActUser: review.Reviewer,
|
||||
Content: fmt.Sprintf("%d|%s", review.Issue.Index, strings.Split(comm.Content, "\n")[0]),
|
||||
Content: encodeContent(fmt.Sprintf("%d", review.Issue.Index), strings.Split(comm.Content, "\n")[0]),
|
||||
OpType: activities_model.ActionCommentPull,
|
||||
RepoID: review.Issue.RepoID,
|
||||
Repo: review.Issue.Repo,
|
||||
|
|
@ -263,7 +263,7 @@ func (a *actionNotifier) PullRequestReview(ctx context.Context, pr *issues_model
|
|||
action := &activities_model.Action{
|
||||
ActUserID: review.Reviewer.ID,
|
||||
ActUser: review.Reviewer,
|
||||
Content: fmt.Sprintf("%d|%s", review.Issue.Index, strings.Split(comment.Content, "\n")[0]),
|
||||
Content: encodeContent(fmt.Sprintf("%d", review.Issue.Index), strings.Split(comment.Content, "\n")[0]),
|
||||
RepoID: review.Issue.RepoID,
|
||||
Repo: review.Issue.Repo,
|
||||
IsPrivate: review.Issue.Repo.IsPrivate,
|
||||
|
|
@ -293,7 +293,7 @@ func (*actionNotifier) MergePullRequest(ctx context.Context, doer *user_model.Us
|
|||
ActUserID: doer.ID,
|
||||
ActUser: doer,
|
||||
OpType: activities_model.ActionMergePullRequest,
|
||||
Content: fmt.Sprintf("%d|%s", pr.Issue.Index, pr.Issue.Title),
|
||||
Content: encodeContent(fmt.Sprintf("%d", pr.Issue.Index), pr.Issue.Title),
|
||||
RepoID: pr.Issue.Repo.ID,
|
||||
Repo: pr.Issue.Repo,
|
||||
IsPrivate: pr.Issue.Repo.IsPrivate,
|
||||
|
|
@ -307,7 +307,7 @@ func (*actionNotifier) AutoMergePullRequest(ctx context.Context, doer *user_mode
|
|||
ActUserID: doer.ID,
|
||||
ActUser: doer,
|
||||
OpType: activities_model.ActionAutoMergePullRequest,
|
||||
Content: fmt.Sprintf("%d|%s", pr.Issue.Index, pr.Issue.Title),
|
||||
Content: encodeContent(fmt.Sprintf("%d", pr.Issue.Index), pr.Issue.Title),
|
||||
RepoID: pr.Issue.Repo.ID,
|
||||
Repo: pr.Issue.Repo,
|
||||
IsPrivate: pr.Issue.Repo.IsPrivate,
|
||||
|
|
@ -325,7 +325,7 @@ func (*actionNotifier) NotifyPullRevieweDismiss(ctx context.Context, doer *user_
|
|||
ActUserID: doer.ID,
|
||||
ActUser: doer,
|
||||
OpType: activities_model.ActionPullReviewDismissed,
|
||||
Content: fmt.Sprintf("%d|%s|%s", review.Issue.Index, reviewerName, comment.Content),
|
||||
Content: encodeContent(fmt.Sprintf("%d", review.Issue.Index), reviewerName, comment.Content),
|
||||
RepoID: review.Issue.Repo.ID,
|
||||
Repo: review.Issue.Repo,
|
||||
IsPrivate: review.Issue.Repo.IsPrivate,
|
||||
|
|
@ -482,3 +482,12 @@ func (a *actionNotifier) NewRelease(ctx context.Context, rel *repo_model.Release
|
|||
log.Error("NotifyWatchers: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// ... later decoded in models/activities/action.go:GetIssueInfos
|
||||
func encodeContent(params ...string) string {
|
||||
contentEncoded, err := json.Marshal(params)
|
||||
if err != nil {
|
||||
log.Error("encodeContent: Unexpected json encoding error: %v", err)
|
||||
}
|
||||
return string(contentEncoded)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -76,6 +76,7 @@ func TestFeed(t *testing.T) {
|
|||
data := resp.Body.String()
|
||||
assert.Contains(t, data, `<feed xmlns="http://www.w3.org/2005/Atom"`)
|
||||
assert.Contains(t, data, "This is a very long text, so lets scream together: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
|
||||
assert.Contains(t, data, "Well, this test is short | succient | distinct.")
|
||||
})
|
||||
t.Run("RSS", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
|
|
@ -86,6 +87,7 @@ func TestFeed(t *testing.T) {
|
|||
data := resp.Body.String()
|
||||
assert.Contains(t, data, `<rss version="2.0"`)
|
||||
assert.Contains(t, data, "This is a very long text, so lets scream together: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
|
||||
assert.Contains(t, data, "Well, this test is short | succient | distinct.")
|
||||
})
|
||||
})
|
||||
t.Run("Branch", func(t *testing.T) {
|
||||
|
|
|
|||
|
|
@ -7,3 +7,12 @@
|
|||
is_private: false
|
||||
created_unix: 1680454039
|
||||
content: '1|This is a very long text, so lets scream together: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa…'
|
||||
- id: 1002
|
||||
user_id: 2
|
||||
op_type: 10 # close issue
|
||||
act_user_id: 2
|
||||
comment_id: 1001
|
||||
repo_id: 1 # public
|
||||
is_private: false
|
||||
created_unix: 1680454039
|
||||
content: '["1","Well, this test is short | succient | distinct."]'
|
||||
|
|
|
|||
|
|
@ -87,3 +87,38 @@ func TestDashboardTitleRendering(t *testing.T) {
|
|||
assert.Equal(t, 6, count)
|
||||
})
|
||||
}
|
||||
|
||||
func TestDashboardActionEscaping(t *testing.T) {
|
||||
onGiteaRun(t, func(t *testing.T, u *url.URL) {
|
||||
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
|
||||
sess := loginUser(t, user4.Name)
|
||||
|
||||
repo, _, f := tests.CreateDeclarativeRepo(t, user4, "",
|
||||
[]unit_model.Type{unit_model.TypePullRequests, unit_model.TypeIssues}, nil,
|
||||
[]*files_service.ChangeRepoFile{},
|
||||
)
|
||||
defer f()
|
||||
|
||||
issue := createIssue(t, user4, repo, "Issue with | in title", "Hey here's a | for you")
|
||||
|
||||
_, err := issue_service.CreateIssueComment(db.DefaultContext, user4, repo, issue, "Comment with a | in it", nil)
|
||||
require.NoError(t, err)
|
||||
|
||||
testIssueClose(t, sess, repo.OwnerName, repo.Name, strconv.Itoa(int(issue.Index)), false)
|
||||
|
||||
response := sess.MakeRequest(t, NewRequest(t, "GET", "/"), http.StatusOK)
|
||||
htmlDoc := NewHTMLParser(t, response.Body)
|
||||
|
||||
count := 0
|
||||
htmlDoc.doc.Find("#activity-feed .flex-item-main .title").Each(func(i int, s *goquery.Selection) {
|
||||
count++
|
||||
assert.Equal(t, "Issue with | in title", s.Text())
|
||||
})
|
||||
htmlDoc.doc.Find("#activity-feed .flex-item-main .markup").Each(func(i int, s *goquery.Selection) {
|
||||
count++
|
||||
assert.Equal(t, "Comment with a | in it\n", s.Text())
|
||||
})
|
||||
|
||||
assert.Equal(t, 4, count)
|
||||
})
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue