From cb9e1a3ff66f24f89d99f839376e304161c12962 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 May 2024 22:09:38 +0800 Subject: [PATCH 1/6] Upgrade chi-binding (#30826) Front port #30742 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 2c1fc5d6f2..bab5a64069 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( code.gitea.io/sdk/gitea v0.17.1 codeberg.org/gusted/mcaptcha v0.0.0-20220723083913-4f3072e1d570 connectrpc.com/connect v1.15.0 - gitea.com/go-chi/binding v0.0.0-20240316035258-17450c5f3028 + gitea.com/go-chi/binding v0.0.0-20240430071103-39a851e106ed gitea.com/go-chi/cache v0.2.0 gitea.com/go-chi/captcha v0.0.0-20240315150714-fb487f629098 gitea.com/go-chi/session v0.0.0-20240316035857-16768d98ec96 diff --git a/go.sum b/go.sum index 8c26b4a7a6..3bb4cbaa42 100644 --- a/go.sum +++ b/go.sum @@ -20,8 +20,8 @@ git.sr.ht/~mariusor/go-xsd-duration v0.0.0-20220703122237-02e73435a078 h1:cliQ4H git.sr.ht/~mariusor/go-xsd-duration v0.0.0-20220703122237-02e73435a078/go.mod h1:g/V2Hjas6Z1UHUp4yIx6bATpNzJ7DYtD0FG3+xARWxs= gitea.com/gitea/act v0.259.1 h1:8GG1o/xtUHl3qjn5f0h/2FXrT5ubBn05TJOM5ry+FBw= gitea.com/gitea/act v0.259.1/go.mod h1:UxZWRYqQG2Yj4+4OqfGWW5a3HELwejyWFQyU7F1jUD8= -gitea.com/go-chi/binding v0.0.0-20240316035258-17450c5f3028 h1:6/QAx4+s0dyRwdaTFPTnhGppuiuu0OqxIH9szyTpvKw= -gitea.com/go-chi/binding v0.0.0-20240316035258-17450c5f3028/go.mod h1:E3i3cgB04dDx0v3CytCgRTTn9Z/9x891aet3r456RVw= +gitea.com/go-chi/binding v0.0.0-20240430071103-39a851e106ed h1:EZZBtilMLSZNWtHHcgq2mt6NSGhJSZBuduAlinMEmso= +gitea.com/go-chi/binding v0.0.0-20240430071103-39a851e106ed/go.mod h1:E3i3cgB04dDx0v3CytCgRTTn9Z/9x891aet3r456RVw= gitea.com/go-chi/cache v0.2.0 h1:E0npuTfDW6CT1yD8NMDVc1SK6IeRjfmRL2zlEsCEd7w= gitea.com/go-chi/cache v0.2.0/go.mod h1:iQlVK2aKTZ/rE9UcHyz9pQWGvdP9i1eI2spOpzgCrtE= gitea.com/go-chi/captcha v0.0.0-20240315150714-fb487f629098 h1:p2ki+WK0cIeNQuqjR98IP2KZQKRzJJiV7aTeMAFwaWo= From 9235442ba58524c8d12ae54865d583acfa1f439d Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 2 May 2024 16:43:23 +0200 Subject: [PATCH 2/6] Remove external API calls in `TestPassword` (#30716) The test had a dependency on `https://api.pwnedpasswords.com` which caused many failures on CI recently: ``` --- FAIL: TestPassword (2.37s) pwn_test.go:41: Get "https://api.pwnedpasswords.com/range/e6b6a": context deadline exceeded (Client.Timeout exceeded while awaiting headers) FAIL coverage: 82.9% of statements ``` --- go.mod | 2 + go.sum | 6 ++ modules/auth/password/pwn/pwn_test.go | 101 ++++++-------------------- 3 files changed, 32 insertions(+), 77 deletions(-) diff --git a/go.mod b/go.mod index bab5a64069..8afefc6367 100644 --- a/go.mod +++ b/go.mod @@ -59,6 +59,7 @@ require ( github.com/google/uuid v1.6.0 github.com/gorilla/feeds v1.1.2 github.com/gorilla/sessions v1.2.2 + github.com/h2non/gock v1.2.0 github.com/hashicorp/go-version v1.6.0 github.com/hashicorp/golang-lru/v2 v2.0.7 github.com/huandu/xstrings v1.4.0 @@ -209,6 +210,7 @@ require ( github.com/gorilla/handlers v1.5.2 // indirect github.com/gorilla/mux v1.8.1 // indirect github.com/gorilla/securecookie v1.1.2 // indirect + github.com/h2non/parth v0.0.0-20190131123155-b4df798d6542 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect github.com/hashicorp/go-retryablehttp v0.7.5 // indirect github.com/hashicorp/hcl v1.0.0 // indirect diff --git a/go.sum b/go.sum index 3bb4cbaa42..1d493f4ca4 100644 --- a/go.sum +++ b/go.sum @@ -430,6 +430,10 @@ github.com/gorilla/securecookie v1.1.2/go.mod h1:NfCASbcHqRSY+3a8tlWJwsQap2VX5pw github.com/gorilla/sessions v1.2.0/go.mod h1:dk2InVEVJ0sfLlnXv9EAgkf6ecYs/i80K/zI+bUmuGM= github.com/gorilla/sessions v1.2.2 h1:lqzMYz6bOfvn2WriPUjNByzeXIlVzURcPmgMczkmTjY= github.com/gorilla/sessions v1.2.2/go.mod h1:ePLdVu+jbEgHH+KWw8I1z2wqd0BAdAQh/8LRvBeoNcQ= +github.com/h2non/gock v1.2.0 h1:K6ol8rfrRkUOefooBC8elXoaNGYkpp7y2qcxGG6BzUE= +github.com/h2non/gock v1.2.0/go.mod h1:tNhoxHYW2W42cYkYb1WqzdbYIieALC99kpYr7rH/BQk= +github.com/h2non/parth v0.0.0-20190131123155-b4df798d6542 h1:2VTzZjLZBgl62/EtslCrtky5vbi9dd7HrQPQIx6wqiw= +github.com/h2non/parth v0.0.0-20190131123155-b4df798d6542/go.mod h1:Ow0tF8D4Kplbc8s8sSb3V2oUCygFHVp8gC3Dn6U4MNI= github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ= github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48= github.com/hashicorp/go-hclog v0.9.2/go.mod h1:5CU+agLiy3J7N7QjHK5d05KxGsuXiQLrjA0H7acj2lQ= @@ -591,6 +595,8 @@ github.com/mschoch/smat v0.2.0 h1:8imxQsjDm8yFEAVBe7azKmKSgzSkZXDuKkSq9374khM= github.com/mschoch/smat v0.2.0/go.mod h1:kc9mz7DoBKqDyiRL7VZN8KvXQMWeTaVnttLRXOlotKw= github.com/msteinert/pam v1.2.0 h1:mYfjlvN2KYs2Pb9G6nb/1f/nPfAttT/Jee5Sq9r3bGE= github.com/msteinert/pam v1.2.0/go.mod h1:d2n0DCUK8rGecChV3JzvmsDjOY4R7AYbsNxAT+ftQl0= +github.com/nbio/st v0.0.0-20140626010706-e9e8d9816f32 h1:W6apQkHrMkS0Muv8G/TipAy/FJl/rCYT0+EuS8+Z0z4= +github.com/nbio/st v0.0.0-20140626010706-e9e8d9816f32/go.mod h1:9wM+0iRr9ahx58uYLpLIr5fm8diHn0JbqRycJi6w0Ms= github.com/niklasfasching/go-org v1.7.0 h1:vyMdcMWWTe/XmANk19F4k8XGBYg0GQ/gJGMimOjGMek= github.com/niklasfasching/go-org v1.7.0/go.mod h1:WuVm4d45oePiE0eX25GqTDQIt/qPW1T9DGkRscqLW5o= github.com/nwaples/rardecode v1.1.0/go.mod h1:5DzqNKiOdpKKBH87u8VlvAnPZMXcGRhxWkRpHbbfGS0= diff --git a/modules/auth/password/pwn/pwn_test.go b/modules/auth/password/pwn/pwn_test.go index a2a6b3a174..b3e7734c3f 100644 --- a/modules/auth/password/pwn/pwn_test.go +++ b/modules/auth/password/pwn/pwn_test.go @@ -4,12 +4,11 @@ package pwn import ( - "math/rand/v2" "net/http" - "strings" "testing" "time" + "github.com/h2non/gock" "github.com/stretchr/testify/assert" ) @@ -18,86 +17,34 @@ var client = New(WithHTTP(&http.Client{ })) func TestPassword(t *testing.T) { - // Check input error - _, err := client.CheckPassword("", false) + defer gock.Off() + + count, err := client.CheckPassword("", false) assert.ErrorIs(t, err, ErrEmptyPassword, "blank input should return ErrEmptyPassword") + assert.Equal(t, -1, count) - // Should fail - fail := "password1234" - count, err := client.CheckPassword(fail, false) - assert.NotEmpty(t, count, "%s should fail as a password", fail) + gock.New("https://api.pwnedpasswords.com").Get("/range/5c1d8").Times(1).Reply(200).BodyString("EAF2F254732680E8AC339B84F3266ECCBB5:1\r\nFC446EB88938834178CB9322C1EE273C2A7:2") + count, err = client.CheckPassword("pwned", false) assert.NoError(t, err) + assert.Equal(t, 1, count) - // Should fail (with padding) - failPad := "administrator" - count, err = client.CheckPassword(failPad, true) - assert.NotEmpty(t, count, "%s should fail as a password", failPad) + gock.New("https://api.pwnedpasswords.com").Get("/range/ba189").Times(1).Reply(200).BodyString("FD4CB34F0378BCB15D23F6FFD28F0775C9E:3\r\nFDF342FCD8C3611DAE4D76E8A992A3E4169:4") + count, err = client.CheckPassword("notpwned", false) assert.NoError(t, err) + assert.Equal(t, 0, count) - // Checking for a "good" password isn't going to be perfect, but we can give it a good try - // with hopefully minimal error. Try five times? - assert.Condition(t, func() bool { - for i := 0; i <= 5; i++ { - count, err = client.CheckPassword(testPassword(), false) - assert.NoError(t, err) - if count == 0 { - return true - } - } - return false - }, "no generated passwords passed. there is a chance this is a fluke") + gock.New("https://api.pwnedpasswords.com").Get("/range/a1733").Times(1).Reply(200).BodyString("C4CE0F1F0062B27B9E2F41AF0C08218017C:1\r\nFC446EB88938834178CB9322C1EE273C2A7:2\r\nFE81480327C992FE62065A827429DD1318B:0") + count, err = client.CheckPassword("paddedpwned", true) + assert.NoError(t, err) + assert.Equal(t, 1, count) - // Again, but with padded responses - assert.Condition(t, func() bool { - for i := 0; i <= 5; i++ { - count, err = client.CheckPassword(testPassword(), true) - assert.NoError(t, err) - if count == 0 { - return true - } - } - return false - }, "no generated passwords passed. there is a chance this is a fluke") -} - -// Credit to https://golangbyexample.com/generate-random-password-golang/ -// DO NOT USE THIS FOR AN ACTUAL PASSWORD GENERATOR -var ( - lowerCharSet = "abcdedfghijklmnopqrst" - upperCharSet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" - specialCharSet = "!@#$%&*" - numberSet = "0123456789" - allCharSet = lowerCharSet + upperCharSet + specialCharSet + numberSet -) - -func testPassword() string { - var password strings.Builder - - // Set special character - for i := 0; i < 5; i++ { - random := rand.IntN(len(specialCharSet)) - password.WriteString(string(specialCharSet[random])) - } - - // Set numeric - for i := 0; i < 5; i++ { - random := rand.IntN(len(numberSet)) - password.WriteString(string(numberSet[random])) - } - - // Set uppercase - for i := 0; i < 5; i++ { - random := rand.IntN(len(upperCharSet)) - password.WriteString(string(upperCharSet[random])) - } - - for i := 0; i < 5; i++ { - random := rand.IntN(len(allCharSet)) - password.WriteString(string(allCharSet[random])) - } - inRune := []rune(password.String()) - rand.Shuffle(len(inRune), func(i, j int) { - inRune[i], inRune[j] = inRune[j], inRune[i] - }) - return string(inRune) + gock.New("https://api.pwnedpasswords.com").Get("/range/5617b").Times(1).Reply(200).BodyString("FD4CB34F0378BCB15D23F6FFD28F0775C9E:3\r\nFDF342FCD8C3611DAE4D76E8A992A3E4169:4\r\nFE81480327C992FE62065A827429DD1318B:0") + count, err = client.CheckPassword("paddednotpwned", true) + assert.NoError(t, err) + assert.Equal(t, 0, count) + + gock.New("https://api.pwnedpasswords.com").Get("/range/79082").Times(1).Reply(200).BodyString("FDF342FCD8C3611DAE4D76E8A992A3E4169:4\r\nFE81480327C992FE62065A827429DD1318B:0\r\nAFEF386F56EB0B4BE314E07696E5E6E6536:0") + count, err = client.CheckPassword("paddednotpwnedzero", true) + assert.NoError(t, err) + assert.Equal(t, 0, count) } From 6f89d5e3a0886d02ead732005f593ae003f78f78 Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 2 May 2024 16:56:17 +0200 Subject: [PATCH 3/6] Add hover outline to heatmap squares (#30828) Makes it easier to use because you see which square is currently hovered: Screenshot 2024-05-02 at 15 38 20 I did try a `scoped` style for this, but that did not work for some reason. --- web_src/css/features/heatmap.css | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/web_src/css/features/heatmap.css b/web_src/css/features/heatmap.css index 364754751a..c064590c46 100644 --- a/web_src/css/features/heatmap.css +++ b/web_src/css/features/heatmap.css @@ -31,6 +31,10 @@ padding: 0 5px; } +#user-heatmap .vch__day__square:hover { + outline: 1.5px solid var(--color-text); +} + /* move the "? contributions in the last ? months" text from top to bottom */ #user-heatmap .total-contributions { font-size: 11px; From 677032d36af9a4052b838e011142d9e0bc706ef5 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 2 May 2024 23:24:21 +0800 Subject: [PATCH 4/6] Fix incorrect message id for releaes email (#30825) Make generateMessageIDForRelease outputs the same format as generateMessageIDForIssue (old `createReference`) --- services/mailer/mail.go | 10 +++++++--- services/mailer/mail_release.go | 4 ++-- services/mailer/mail_test.go | 14 +++++++++++--- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/services/mailer/mail.go b/services/mailer/mail.go index a63ba7a52a..04194dcf26 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -289,8 +289,8 @@ func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipient } // Make sure to compose independent messages to avoid leaking user emails - msgID := createReference(ctx.Issue, ctx.Comment, ctx.ActionType) - reference := createReference(ctx.Issue, nil, activities_model.ActionType(0)) + msgID := generateMessageIDForIssue(ctx.Issue, ctx.Comment, ctx.ActionType) + reference := generateMessageIDForIssue(ctx.Issue, nil, activities_model.ActionType(0)) var replyPayload []byte if ctx.Comment != nil { @@ -362,7 +362,7 @@ func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipient return msgs, nil } -func createReference(issue *issues_model.Issue, comment *issues_model.Comment, actionType activities_model.ActionType) string { +func generateMessageIDForIssue(issue *issues_model.Issue, comment *issues_model.Comment, actionType activities_model.ActionType) string { var path string if issue.IsPull { path = "pulls" @@ -389,6 +389,10 @@ func createReference(issue *issues_model.Issue, comment *issues_model.Comment, a return fmt.Sprintf("<%s/%s/%d%s@%s>", issue.Repo.FullName(), path, issue.Index, extra, setting.Domain) } +func generateMessageIDForRelease(release *repo_model.Release) string { + return fmt.Sprintf("<%s/releases/%d@%s>", release.Repo.FullName(), release.ID, setting.Domain) +} + func generateAdditionalHeaders(ctx *mailCommentContext, reason string, recipient *user_model.User) map[string]string { repo := ctx.Issue.Repo diff --git a/services/mailer/mail_release.go b/services/mailer/mail_release.go index 6682774a04..2aac21e552 100644 --- a/services/mailer/mail_release.go +++ b/services/mailer/mail_release.go @@ -86,11 +86,11 @@ func mailNewRelease(ctx context.Context, lang string, tos []string, rel *repo_mo msgs := make([]*Message, 0, len(tos)) publisherName := rel.Publisher.DisplayName() - relURL := "<" + rel.HTMLURL() + ">" + msgID := generateMessageIDForRelease(rel) for _, to := range tos { msg := NewMessageFrom(to, publisherName, setting.MailService.FromEmail, subject, mailBody.String()) msg.Info = subject - msg.SetHeader("Message-ID", relURL) + msg.SetHeader("Message-ID", msgID) msgs = append(msgs, msg) } diff --git a/services/mailer/mail_test.go b/services/mailer/mail_test.go index d87c57ffe7..0739f4233f 100644 --- a/services/mailer/mail_test.go +++ b/services/mailer/mail_test.go @@ -288,7 +288,7 @@ func TestGenerateAdditionalHeaders(t *testing.T) { } } -func Test_createReference(t *testing.T) { +func TestGenerateMessageIDForIssue(t *testing.T) { _, _, issue, comment := prepareMailerTest(t) _, _, pullIssue, _ := prepareMailerTest(t) pullIssue.IsPull = true @@ -388,10 +388,18 @@ func Test_createReference(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := createReference(tt.args.issue, tt.args.comment, tt.args.actionType) + got := generateMessageIDForIssue(tt.args.issue, tt.args.comment, tt.args.actionType) if !strings.HasPrefix(got, tt.prefix) { - t.Errorf("createReference() = %v, want %v", got, tt.prefix) + t.Errorf("generateMessageIDForIssue() = %v, want %v", got, tt.prefix) } }) } } + +func TestGenerateMessageIDForRelease(t *testing.T) { + msgID := generateMessageIDForRelease(&repo_model.Release{ + ID: 1, + Repo: &repo_model.Repository{OwnerName: "owner", Name: "repo"}, + }) + assert.Equal(t, "", msgID) +} From 872caa17c0a30d95f85ab75c068d606e07bd10b3 Mon Sep 17 00:00:00 2001 From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com> Date: Thu, 2 May 2024 09:33:31 -0700 Subject: [PATCH 5/6] Catch and handle unallowed file type errors in issue attachment API (#30791) Before, we would just throw 500 if a user passes an attachment that is not an allowed type. This commit catches this error and throws a 422 instead since this should be considered a validation error. --- routers/api/v1/repo/issue_attachment.go | 9 +++++- .../api/v1/repo/issue_comment_attachment.go | 10 ++++++- templates/swagger/v1_json.tmpl | 6 ++++ .../api_comment_attachment_test.go | 28 +++++++++++++++++++ .../integration/api_issue_attachment_test.go | 27 ++++++++++++++++++ 5 files changed, 78 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/repo/issue_attachment.go b/routers/api/v1/repo/issue_attachment.go index 7a5c6d554d..f5a28e6fa6 100644 --- a/routers/api/v1/repo/issue_attachment.go +++ b/routers/api/v1/repo/issue_attachment.go @@ -14,6 +14,7 @@ import ( "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/services/attachment" "code.gitea.io/gitea/services/context" + "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/convert" issue_service "code.gitea.io/gitea/services/issue" ) @@ -153,6 +154,8 @@ func CreateIssueAttachment(ctx *context.APIContext) { // "$ref": "#/responses/error" // "404": // "$ref": "#/responses/error" + // "422": + // "$ref": "#/responses/validationError" // "423": // "$ref": "#/responses/repoArchivedError" @@ -185,7 +188,11 @@ func CreateIssueAttachment(ctx *context.APIContext) { IssueID: issue.ID, }) if err != nil { - ctx.Error(http.StatusInternalServerError, "UploadAttachment", err) + if upload.IsErrFileTypeForbidden(err) { + ctx.Error(http.StatusUnprocessableEntity, "", err) + } else { + ctx.Error(http.StatusInternalServerError, "UploadAttachment", err) + } return } diff --git a/routers/api/v1/repo/issue_comment_attachment.go b/routers/api/v1/repo/issue_comment_attachment.go index 4096cbf07b..77aa7f0400 100644 --- a/routers/api/v1/repo/issue_comment_attachment.go +++ b/routers/api/v1/repo/issue_comment_attachment.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/services/attachment" "code.gitea.io/gitea/services/context" + "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/convert" issue_service "code.gitea.io/gitea/services/issue" ) @@ -160,6 +161,8 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) { // "$ref": "#/responses/forbidden" // "404": // "$ref": "#/responses/error" + // "422": + // "$ref": "#/responses/validationError" // "423": // "$ref": "#/responses/repoArchivedError" @@ -194,9 +197,14 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) { CommentID: comment.ID, }) if err != nil { - ctx.Error(http.StatusInternalServerError, "UploadAttachment", err) + if upload.IsErrFileTypeForbidden(err) { + ctx.Error(http.StatusUnprocessableEntity, "", err) + } else { + ctx.Error(http.StatusInternalServerError, "UploadAttachment", err) + } return } + if err := comment.LoadAttachments(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "LoadAttachments", err) return diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 0c5e5c974d..5ca499e708 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -7478,6 +7478,9 @@ "404": { "$ref": "#/responses/error" }, + "422": { + "$ref": "#/responses/validationError" + }, "423": { "$ref": "#/responses/repoArchivedError" } @@ -8097,6 +8100,9 @@ "404": { "$ref": "#/responses/error" }, + "422": { + "$ref": "#/responses/validationError" + }, "423": { "$ref": "#/responses/repoArchivedError" } diff --git a/tests/integration/api_comment_attachment_test.go b/tests/integration/api_comment_attachment_test.go index 2d7587bbde..0ec950d4c2 100644 --- a/tests/integration/api_comment_attachment_test.go +++ b/tests/integration/api_comment_attachment_test.go @@ -120,6 +120,34 @@ func TestAPICreateCommentAttachment(t *testing.T) { unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: apiAttachment.ID, CommentID: comment.ID}) } +func TestAPICreateCommentAttachmentWithUnallowedFile(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 2}) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID}) + repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + + session := loginUser(t, repoOwner.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue) + + filename := "file.bad" + body := &bytes.Buffer{} + + // Setup multi-part. + writer := multipart.NewWriter(body) + _, err := writer.CreateFormFile("attachment", filename) + assert.NoError(t, err) + err = writer.Close() + assert.NoError(t, err) + + req := NewRequestWithBody(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/issues/comments/%d/assets", repoOwner.Name, repo.Name, comment.ID), body). + AddTokenAuth(token). + SetHeader("Content-Type", writer.FormDataContentType()) + + session.MakeRequest(t, req, http.StatusUnprocessableEntity) +} + func TestAPIEditCommentAttachment(t *testing.T) { defer tests.PrepareTestEnv(t)() diff --git a/tests/integration/api_issue_attachment_test.go b/tests/integration/api_issue_attachment_test.go index 497dd0155e..b4196ec6db 100644 --- a/tests/integration/api_issue_attachment_test.go +++ b/tests/integration/api_issue_attachment_test.go @@ -96,6 +96,33 @@ func TestAPICreateIssueAttachment(t *testing.T) { unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: apiAttachment.ID, IssueID: issue.ID}) } +func TestAPICreateIssueAttachmentWithUnallowedFile(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: repo.ID}) + repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + + session := loginUser(t, repoOwner.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue) + + filename := "file.bad" + body := &bytes.Buffer{} + + // Setup multi-part. + writer := multipart.NewWriter(body) + _, err := writer.CreateFormFile("attachment", filename) + assert.NoError(t, err) + err = writer.Close() + assert.NoError(t, err) + + req := NewRequestWithBody(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/assets", repoOwner.Name, repo.Name, issue.Index), body). + AddTokenAuth(token) + req.Header.Add("Content-Type", writer.FormDataContentType()) + + session.MakeRequest(t, req, http.StatusUnprocessableEntity) +} + func TestAPIEditIssueAttachment(t *testing.T) { defer tests.PrepareTestEnv(t)() From 5c542ca94caa3587329167cfe9e949357ca15cf1 Mon Sep 17 00:00:00 2001 From: Archer Date: Thu, 2 May 2024 19:05:59 +0200 Subject: [PATCH 6/6] Prevent automatic OAuth grants for public clients (#30790) This commit forces the resource owner (user) to always approve OAuth 2.0 authorization requests if the client is public (e.g. native applications). As detailed in [RFC 6749 Section 10.2](https://www.rfc-editor.org/rfc/rfc6749.html#section-10.2), > The authorization server SHOULD NOT process repeated authorization requests automatically (without active resource owner interaction) without authenticating the client or relying on other measures to ensure that the repeated request comes from the original client and not an impersonator. With the implementation prior to this patch, attackers with access to the redirect URI (e.g., the loopback interface for `git-credential-oauth`) can get access to the user account without any user interaction if they can redirect the user to the `/login/oauth/authorize` endpoint somehow (e.g., with `xdg-open` on Linux). Fixes #25061. Co-authored-by: wxiaoguang --- routers/web/auth/oauth.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index c9cb7859cd..354e70bcbf 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -470,8 +470,9 @@ func AuthorizeOAuth(ctx *context.Context) { return } - // Redirect if user already granted access - if grant != nil { + // Redirect if user already granted access and the application is confidential. + // I.e. always require authorization for public clients as recommended by RFC 6749 Section 10.2 + if app.ConfidentialClient && grant != nil { code, err := grant.GenerateNewAuthorizationCode(ctx, form.RedirectURI, form.CodeChallenge, form.CodeChallengeMethod) if err != nil { handleServerError(ctx, form.State, form.RedirectURI)