Skip to content

Commit f9b7b65

Browse files
lunnywxiaoguang
andauthored
fix(security): enforce wiki git writes and LFS token access at request time (#37695)
This PR fixes two permission-checking gaps in Git and LFS request handling. ## What it changes - keep wiki Git HTTP pushes on the normal write-permission path, even when proc-receive support is enabled - revalidate LFS bearer token requests against the current user state and current repository permissions before allowing access - add regression coverage for unauthorized wiki HTTP pushes - add LFS tests for blocked users, revoked repository access, read-only upload attempts, and valid write access ## Why - wiki repositories should not inherit the relaxed refs/for handling used for normal code repositories - LFS authorization tokens should not remain usable after a user is disabled or loses repository access Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
1 parent 5b3575a commit f9b7b65

4 files changed

Lines changed: 143 additions & 85 deletions

File tree

routers/web/repo/githttp.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,8 +180,8 @@ func httpBase(ctx *context.Context, optGitService ...string) *serviceHandler {
180180
}
181181

182182
if repoExist {
183-
// Because of special ref "refs/for" (agit) , need delay write permission check
184-
if git.DefaultFeatures().SupportProcReceive {
183+
// Only the main code repo accepts refs/for pushes, so wiki pushes must keep write checks.
184+
if git.DefaultFeatures().SupportProcReceive && !isWiki {
185185
accessMode = perm.AccessModeRead
186186
}
187187

services/lfs/server.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import (
3232
"code.gitea.io/gitea/modules/log"
3333
"code.gitea.io/gitea/modules/setting"
3434
"code.gitea.io/gitea/modules/storage"
35+
"code.gitea.io/gitea/modules/util"
3536
"code.gitea.io/gitea/services/context"
3637

3738
"github.com/golang-jwt/jwt/v5"
@@ -605,6 +606,18 @@ func handleLFSToken(ctx stdCtx.Context, tokenSHA string, target *repo_model.Repo
605606
log.Error("Unable to GetUserById[%d]: Error: %v", claims.UserID, err)
606607
return nil, err
607608
}
609+
if !u.IsActive || u.ProhibitLogin {
610+
return nil, util.NewPermissionDeniedErrorf("not allowed to access any repository")
611+
}
612+
613+
perm, err := access_model.GetDoerRepoPermission(ctx, target, u)
614+
if err != nil {
615+
log.Error("Unable to GetDoerRepoPermission for user[%d] repo[%d]: %v", claims.UserID, target.ID, err)
616+
return nil, err
617+
}
618+
if !perm.CanAccess(mode, unit.TypeCode) {
619+
return nil, util.NewPermissionDeniedErrorf("no permission to access the repository")
620+
}
608621
return u, nil
609622
}
610623

services/lfs/server_test.go

Lines changed: 58 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,11 @@ import (
77
"strings"
88
"testing"
99

10+
"code.gitea.io/gitea/models/db"
1011
perm_model "code.gitea.io/gitea/models/perm"
1112
repo_model "code.gitea.io/gitea/models/repo"
1213
"code.gitea.io/gitea/models/unittest"
14+
user_model "code.gitea.io/gitea/models/user"
1315
"code.gitea.io/gitea/services/contexttest"
1416

1517
"github.com/stretchr/testify/assert"
@@ -22,11 +24,15 @@ func TestMain(m *testing.M) {
2224

2325
func TestAuthenticate(t *testing.T) {
2426
require.NoError(t, unittest.PrepareTestDatabase())
27+
ctx, _ := contexttest.MockContext(t, "/")
28+
2529
repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
2630

27-
token2, _ := GetLFSAuthTokenWithBearer(AuthTokenOptions{Op: "download", UserID: 2, RepoID: 1})
28-
_, token2, _ = strings.Cut(token2, " ")
29-
ctx, _ := contexttest.MockContext(t, "/")
31+
getUserToken := func(op string, userID int64, repo *repo_model.Repository) string {
32+
s, _ := GetLFSAuthTokenWithBearer(AuthTokenOptions{Op: op, UserID: userID, RepoID: repo.ID})
33+
_, token, _ := strings.Cut(s, " ")
34+
return token
35+
}
3036

3137
t.Run("handleLFSToken", func(t *testing.T) {
3238
u, err := handleLFSToken(ctx, "", repo1, perm_model.AccessModeRead)
@@ -37,15 +43,62 @@ func TestAuthenticate(t *testing.T) {
3743
require.Error(t, err)
3844
assert.Nil(t, u)
3945

40-
u, err = handleLFSToken(ctx, token2, repo1, perm_model.AccessModeRead)
46+
u, err = handleLFSToken(ctx, getUserToken("download", 2, repo1), repo1, perm_model.AccessModeRead)
4147
require.NoError(t, err)
4248
assert.EqualValues(t, 2, u.ID)
4349
})
4450

4551
t.Run("authenticate", func(t *testing.T) {
4652
const prefixBearer = "Bearer "
53+
token := getUserToken("download", 2, repo1)
4754
assert.False(t, authenticate(ctx, repo1, "", true, false))
4855
assert.False(t, authenticate(ctx, repo1, prefixBearer+"invalid", true, false))
49-
assert.True(t, authenticate(ctx, repo1, prefixBearer+token2, true, false))
56+
assert.True(t, authenticate(ctx, repo1, prefixBearer+token, true, false))
57+
})
58+
59+
handleLFSTokenTestPerm := func(op string, userID int64, repo *repo_model.Repository, accessMode perm_model.AccessMode) error {
60+
token := getUserToken(op, userID, repo)
61+
u, err := handleLFSToken(ctx, token, repo, accessMode)
62+
if err == nil {
63+
assert.Equal(t, userID, u.ID)
64+
}
65+
return err
66+
}
67+
68+
t.Run("handleLFSToken blocks prohibited users", func(t *testing.T) {
69+
user37 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 37})
70+
71+
// prohibited user
72+
assert.True(t, user37.ProhibitLogin)
73+
err := handleLFSTokenTestPerm("download", 37, repo1, perm_model.AccessModeRead)
74+
assert.ErrorContains(t, err, "not allowed to access any repository")
75+
76+
// normal user
77+
_, _ = db.GetEngine(t.Context()).ID(37).Cols("prohibit_login").Update(&user_model.User{ProhibitLogin: false})
78+
err = handleLFSTokenTestPerm("download", 37, repo1, perm_model.AccessModeRead)
79+
assert.NoError(t, err)
80+
81+
// inactive user
82+
_, _ = db.GetEngine(t.Context()).ID(37).Cols("is_active").Update(&user_model.User{IsActive: false})
83+
err = handleLFSTokenTestPerm("download", 37, repo1, perm_model.AccessModeRead)
84+
assert.ErrorContains(t, err, "not allowed to access any repository")
85+
})
86+
87+
t.Run("handleLFSToken blocks users without repo access", func(t *testing.T) {
88+
repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2})
89+
err := handleLFSTokenTestPerm("download", 10, repo2, perm_model.AccessModeRead)
90+
assert.ErrorContains(t, err, "no permission to access the repository")
91+
})
92+
93+
t.Run("handleLFSToken requires write access for uploads", func(t *testing.T) {
94+
err := handleLFSTokenTestPerm("download", 10, repo1, perm_model.AccessModeRead)
95+
assert.NoError(t, err)
96+
err = handleLFSTokenTestPerm("upload", 10, repo1, perm_model.AccessModeWrite)
97+
assert.ErrorContains(t, err, "no permission to access the repository")
98+
})
99+
100+
t.Run("handleLFSToken allows writes for authorized users", func(t *testing.T) {
101+
err := handleLFSTokenTestPerm("upload", 2, repo1, perm_model.AccessModeWrite)
102+
assert.NoError(t, err)
50103
})
51104
}

tests/integration/wiki_test.go

Lines changed: 70 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -13,52 +13,17 @@ import (
1313

1414
auth_model "code.gitea.io/gitea/models/auth"
1515
"code.gitea.io/gitea/modules/git"
16-
api "code.gitea.io/gitea/modules/structs"
17-
"code.gitea.io/gitea/modules/util"
16+
"code.gitea.io/gitea/modules/git/gitcmd"
1817
"code.gitea.io/gitea/tests"
1918

2019
"github.com/PuerkitoBio/goquery"
2120
"github.com/stretchr/testify/assert"
21+
"github.com/stretchr/testify/require"
2222
)
2323

24-
func assertFileExist(t *testing.T, p string) {
25-
exist, err := util.IsExist(p)
26-
assert.NoError(t, err)
27-
assert.True(t, exist)
28-
}
29-
30-
func assertFileEqual(t *testing.T, p string, content []byte) {
31-
bs, err := os.ReadFile(p)
32-
assert.NoError(t, err)
33-
assert.Equal(t, content, bs)
34-
}
35-
36-
func TestRepoCloneWiki(t *testing.T) {
37-
onGiteaRun(t, func(t *testing.T, u *url.URL) {
38-
defer tests.PrepareTestEnv(t)()
39-
40-
dstPath := t.TempDir()
41-
42-
r := u.String() + "user2/repo1.wiki.git"
43-
u, _ = url.Parse(r)
44-
u.User = url.UserPassword("user2", userPassword)
45-
t.Run("Clone", func(t *testing.T) {
46-
assert.NoError(t, git.Clone(t.Context(), u.String(), dstPath, git.CloneRepoOptions{}))
47-
assertFileEqual(t, filepath.Join(dstPath, "Home.md"), []byte("# Home page\n\nThis is the home page!\n"))
48-
assertFileExist(t, filepath.Join(dstPath, "Page-With-Image.md"))
49-
assertFileExist(t, filepath.Join(dstPath, "Page-With-Spaced-Name.md"))
50-
assertFileExist(t, filepath.Join(dstPath, "images"))
51-
assertFileExist(t, filepath.Join(dstPath, "files/Non-Renderable-File.zip"))
52-
assertFileExist(t, filepath.Join(dstPath, "jpeg.jpg"))
53-
})
54-
})
55-
}
56-
57-
func Test_RepoWikiPages(t *testing.T) {
24+
func TestRepoWikiPages(t *testing.T) {
5825
defer tests.PrepareTestEnv(t)()
59-
60-
url := "/user2/repo1/wiki/?action=_pages"
61-
req := NewRequest(t, "GET", url)
26+
req := NewRequest(t, "GET", "/user2/repo1/wiki/?action=_pages")
6227
resp := MakeRequest(t, req, http.StatusOK)
6328

6429
doc := NewHTMLParser(t, resp.Body)
@@ -74,45 +39,72 @@ func Test_RepoWikiPages(t *testing.T) {
7439
})
7540
}
7641

77-
func Test_WikiClone(t *testing.T) {
42+
func testRepoWikiCloneHTTP(t *testing.T, u *url.URL) {
43+
// When proc-receive support is enabled globally, the HTTP receive-pack pre-check
44+
// must still require write access for wiki repositories. Exercise this with a
45+
// normal wiki push because the regression is about the pre-check, not agit refs.
46+
require.True(t, git.DefaultFeatures().SupportProcReceive) // modern git should all support proc-receive
47+
48+
wikiURL := *u
49+
wikiURL.Path = "/user2/repo1.wiki.git"
50+
51+
dstLocalPath := t.TempDir()
52+
53+
// reader can clone
54+
wikiURL.User = url.UserPassword("user20", userPassword)
55+
require.NoError(t, git.Clone(t.Context(), wikiURL.String(), dstLocalPath, git.CloneRepoOptions{}))
56+
_, _, runErr := gitcmd.NewCommand("fast-import").WithDir(dstLocalPath).WithStdinBytes([]byte(`commit refs/heads/master
57+
committer unauthorized-user <user20@example.com> 1714310400 +0000
58+
data <<EOM
59+
dummy-message
60+
EOM
61+
from refs/heads/master^0
62+
M 100644 inline Home.md
63+
data <<EOF
64+
changed-content
65+
EOF
66+
`)).RunStdString(t.Context())
67+
require.NoError(t, runErr)
68+
69+
content, err := os.ReadFile(filepath.Join(dstLocalPath, "Home.md"))
70+
assert.NoError(t, err)
71+
assert.Equal(t, "# Home page\n\nThis is the home page!\n", string(content))
72+
73+
// reader can't push
74+
_, _, runErr = gitcmd.NewCommand("push", "origin", "refs/heads/master").WithDir(dstLocalPath).RunStdString(t.Context())
75+
assert.True(t, gitcmd.StderrContains(runErr, "remote: Repository not found\n"))
76+
req := NewRequest(t, "GET", "/user2/repo1/wiki/raw/Home.md")
77+
resp := MakeRequest(t, req, http.StatusOK)
78+
assert.Contains(t, resp.Body.String(), "This is the home page!")
79+
80+
// owner can push
81+
wikiURL.User = url.UserPassword("user2", userPassword)
82+
_, _, runErr = gitcmd.NewCommand("remote", "add", "origin-owner").AddDynamicArguments(wikiURL.String()).WithDir(dstLocalPath).RunStdString(t.Context())
83+
require.NoError(t, runErr)
84+
_, _, runErr = gitcmd.NewCommand("push", "origin-owner", "refs/heads/master").WithDir(dstLocalPath).RunStdString(t.Context())
85+
assert.NoError(t, runErr)
86+
req = NewRequest(t, "GET", "/user2/repo1/wiki/raw/Home.md")
87+
resp = MakeRequest(t, req, http.StatusOK)
88+
assert.Equal(t, "changed-content", strings.TrimSpace(resp.Body.String()))
89+
}
90+
91+
func testRepoWikiCloneSSH(t *testing.T, u *url.URL) {
92+
dstLocalPath := t.TempDir()
93+
baseAPITestContext := NewAPITestContext(t, "user2", "repo1", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
94+
sshURL := createSSHUrl("/user2/repo1.wiki.git", u)
95+
96+
withKeyFile(t, "my-testing-key", func(keyFile string) {
97+
t.Run("CreateUserKey", doAPICreateUserKey(baseAPITestContext, "test-key", keyFile))
98+
assert.NoError(t, git.Clone(t.Context(), sshURL.String(), dstLocalPath, git.CloneRepoOptions{}))
99+
content, err := os.ReadFile(filepath.Join(dstLocalPath, "Home.md"))
100+
assert.NoError(t, err)
101+
assert.Equal(t, "# Home page\n\nThis is the home page!\n", string(content))
102+
})
103+
}
104+
105+
func TestRepoWikiClonePush(t *testing.T) {
78106
onGiteaRun(t, func(t *testing.T, u *url.URL) {
79-
username := "user2"
80-
reponame := "repo1"
81-
wikiPath := username + "/" + reponame + ".wiki.git"
82-
keyname := "my-testing-key"
83-
baseAPITestContext := NewAPITestContext(t, username, "repo1", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
84-
85-
u.Path = wikiPath
86-
87-
t.Run("Clone HTTP", func(t *testing.T) {
88-
defer tests.PrintCurrentTest(t)()
89-
90-
dstLocalPath := t.TempDir()
91-
assert.NoError(t, git.Clone(t.Context(), u.String(), dstLocalPath, git.CloneRepoOptions{}))
92-
content, err := os.ReadFile(filepath.Join(dstLocalPath, "Home.md"))
93-
assert.NoError(t, err)
94-
assert.Equal(t, "# Home page\n\nThis is the home page!\n", string(content))
95-
})
96-
97-
t.Run("Clone SSH", func(t *testing.T) {
98-
defer tests.PrintCurrentTest(t)()
99-
100-
dstLocalPath := t.TempDir()
101-
sshURL := createSSHUrl(wikiPath, u)
102-
103-
withKeyFile(t, keyname, func(keyFile string) {
104-
var keyID int64
105-
t.Run("CreateUserKey", doAPICreateUserKey(baseAPITestContext, "test-key", keyFile, func(t *testing.T, key api.PublicKey) {
106-
keyID = key.ID
107-
}))
108-
assert.NotZero(t, keyID)
109-
110-
// Setup clone folder
111-
assert.NoError(t, git.Clone(t.Context(), sshURL.String(), dstLocalPath, git.CloneRepoOptions{}))
112-
content, err := os.ReadFile(filepath.Join(dstLocalPath, "Home.md"))
113-
assert.NoError(t, err)
114-
assert.Equal(t, "# Home page\n\nThis is the home page!\n", string(content))
115-
})
116-
})
107+
t.Run("SSH", func(t *testing.T) { testRepoWikiCloneSSH(t, u) })
108+
t.Run("HTTP", func(t *testing.T) { testRepoWikiCloneHTTP(t, u) })
117109
})
118110
}

0 commit comments

Comments
 (0)