From 7f4f3434ec156c29ebff393dfaddabdb4fd537ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Ri=C3=9Fe?= Date: Fri, 7 Feb 2025 07:39:00 +0000 Subject: [PATCH] fix: consider HEAD requests to be pulls (#6750) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously an anonymous GET request to e.g. https://codeberg.org/forgejo/forgejo/HEAD was allowed, as GET requests are considered pulls and those don't need authentication for a public repository, but a HEAD request to the same URL was rejected with a 401. Since the result of a HEAD request is a subset of the result of a GET request it is safe to allow HEAD as well. This isn't really a practical issue for Forgejo itself, but I have encountered this in https://codeberg.org/forgejo-aneksajo/forgejo-aneksajo/issues/40. Since the fix isn't git-annex specific I am proposing it here. ## 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... - [ ] 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 - [x] I do not want this change to show in the release notes. - [ ] 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/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6750 Reviewed-by: Michael Kriese Reviewed-by: Earl Warren Co-authored-by: Matthias Riße Co-committed-by: Matthias Riße --- routers/web/repo/githttp.go | 2 +- tests/integration/git_smart_http_test.go | 135 +++++++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) diff --git a/routers/web/repo/githttp.go b/routers/web/repo/githttp.go index c1adca174f..bced8e61b1 100644 --- a/routers/web/repo/githttp.go +++ b/routers/web/repo/githttp.go @@ -78,7 +78,7 @@ func httpBase(ctx *context.Context) *serviceHandler { strings.HasSuffix(ctx.Req.URL.Path, "git-upload-archive") { isPull = true } else { - isPull = ctx.Req.Method == "GET" + isPull = ctx.Req.Method == "GET" || ctx.Req.Method == "HEAD" } var accessMode perm.AccessMode diff --git a/tests/integration/git_smart_http_test.go b/tests/integration/git_smart_http_test.go index 2b904ed99f..5d65cd9bd8 100644 --- a/tests/integration/git_smart_http_test.go +++ b/tests/integration/git_smart_http_test.go @@ -1,14 +1,24 @@ // Copyright 2021 The Gitea Authors. All rights reserved. +// Copyright 2025 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package integration import ( + "fmt" "io" "net/http" "net/url" "testing" + auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/perm" + unit_model "code.gitea.io/gitea/models/unit" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/tests" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -67,3 +77,128 @@ func testGitSmartHTTP(t *testing.T, u *url.URL) { }) } } + +// Test that the git http endpoints have the same authentication behavior irrespective of if it is a GET or a HEAD request. +func TestGitHTTPSameStatusCodeForGetAndHeadRequests(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + type caseType struct { + User *user_model.User + IsCollaborator bool + RepoIsPrivate bool + Endpoint string + ExpectedStatusCode int + } + cases := []caseType{ + {owner, false, false, "HEAD", 200}, + {owner, false, false, "git-receive-pack", 405}, + {owner, false, false, "git-upload-pack", 405}, + {owner, false, false, "info/refs", 200}, + {owner, false, false, "objects/info/alternates", 404}, + {owner, false, false, "objects/info/http-alternates", 404}, + {owner, false, false, "objects/info/packs", 200}, + {owner, false, true, "HEAD", 200}, + {owner, false, true, "git-receive-pack", 405}, + {owner, false, true, "git-upload-pack", 405}, + {owner, false, true, "info/refs", 200}, + {owner, false, true, "objects/info/alternates", 404}, + {owner, false, true, "objects/info/http-alternates", 404}, + {owner, false, true, "objects/info/packs", 200}, + {user2, false, false, "HEAD", 200}, + {user2, false, false, "git-receive-pack", 405}, + {user2, false, false, "git-upload-pack", 405}, + {user2, false, false, "info/refs", 200}, + {user2, false, false, "objects/info/alternates", 404}, + {user2, false, false, "objects/info/http-alternates", 404}, + {user2, false, false, "objects/info/packs", 200}, + {user2, false, true, "HEAD", 404}, + {user2, false, true, "git-receive-pack", 405}, + {user2, false, true, "git-upload-pack", 405}, + {user2, false, true, "info/refs", 404}, + {user2, false, true, "objects/info/alternates", 404}, + {user2, false, true, "objects/info/http-alternates", 404}, + {user2, false, true, "objects/info/packs", 404}, + // user2 with IsCollaborator=true must come after IsCollaborator=false, because + // the addition as a collaborator is not reset + {user2, true, false, "HEAD", 200}, + {user2, true, false, "git-receive-pack", 405}, + {user2, true, false, "git-upload-pack", 405}, + {user2, true, false, "info/refs", 200}, + {user2, true, false, "objects/info/alternates", 404}, + {user2, true, false, "objects/info/http-alternates", 404}, + {user2, true, false, "objects/info/packs", 200}, + {user2, true, true, "HEAD", 200}, + {user2, true, true, "git-receive-pack", 405}, + {user2, true, true, "git-upload-pack", 405}, + {user2, true, true, "info/refs", 200}, + {user2, true, true, "objects/info/alternates", 404}, + {user2, true, true, "objects/info/http-alternates", 404}, + {user2, true, true, "objects/info/packs", 200}, + {nil, false, false, "HEAD", 200}, + {nil, false, false, "git-receive-pack", 405}, + {nil, false, false, "git-upload-pack", 405}, + {nil, false, false, "info/refs", 200}, + {nil, false, false, "objects/info/alternates", 404}, + {nil, false, false, "objects/info/http-alternates", 404}, + {nil, false, false, "objects/info/packs", 200}, + {nil, false, true, "HEAD", 401}, + {nil, false, true, "git-receive-pack", 405}, + {nil, false, true, "git-upload-pack", 405}, + {nil, false, true, "info/refs", 401}, + {nil, false, true, "objects/info/alternates", 401}, + {nil, false, true, "objects/info/http-alternates", 401}, + {nil, false, true, "objects/info/packs", 401}, + } + + caseToTestName := func(c caseType) string { + var user string + if c.User == nil { + user = "nil" + } else if c.User == owner { + user = "owner" + } else { + user = c.User.Name + } + return fmt.Sprintf( + "User=%s,IsCollaborator=%t,RepoIsPrivate=%t,Endpoint=%s,ExpectedStatusCode=%d", + user, + c.IsCollaborator, + c.RepoIsPrivate, + c.Endpoint, + c.ExpectedStatusCode, + ) + } + + repo, _, f := tests.CreateDeclarativeRepo(t, owner, "get-and-head-requests", []unit_model.Type{unit_model.TypeCode}, nil, nil) + defer f() + + for _, c := range cases { + t.Run(caseToTestName(c), func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + session := emptyTestSession(t) + if c.User != nil { + session = loginUser(t, c.User.Name) + } + if c.IsCollaborator { + testCtx := NewAPITestContext(t, owner.Name, repo.Name, auth_model.AccessTokenScopeWriteRepository) + doAPIAddCollaborator(testCtx, c.User.Name, perm.AccessModeRead)(t) + } + repo.IsPrivate = c.RepoIsPrivate + _, err := db.GetEngine(db.DefaultContext).Cols("is_private").Update(repo) + require.NoError(t, err) + + // Given the test parameters check that the endpoint returns the same status + // code for both GET and HEAD, which needs to equal the test cases expected + // status code + getReq := NewRequestf(t, "GET", "%s/%s", repo.Link(), c.Endpoint) + getResp := session.MakeRequest(t, getReq, NoExpectedStatus) + headReq := NewRequestf(t, "HEAD", "%s/%s", repo.Link(), c.Endpoint) + headResp := session.MakeRequest(t, headReq, NoExpectedStatus) + require.Equal(t, getResp.Result().StatusCode, headResp.Result().StatusCode) + require.Equal(t, c.ExpectedStatusCode, headResp.Result().StatusCode) + }) + } +}