Skip to content

Fix registration token API #36793

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lunny wants to merge 2 commits into from
Closed

Fix registration token API #36793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
298f86a
Fix bug
lunny Mar 1, 2026
771c686
Fix fmt
lunny Mar 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 46 additions & 36 deletions routers/api/v1/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,48 +301,56 @@ func checkTokenPublicOnly() func(ctx *context.APIContext) {
// if a token is not being used, reqToken will enforce other sign in methods
func tokenRequiresScopes(requiredScopeCategories ...auth_model.AccessTokenScopeCategory) func(ctx *context.APIContext) {
return func(ctx *context.APIContext) {
// no scope required
if len(requiredScopeCategories) == 0 {
return
}

// Need OAuth2 token to be present.
scope, scopeExists := ctx.Data["ApiTokenScope"].(auth_model.AccessTokenScope)
if ctx.Data["IsApiToken"] != true || !scopeExists {
return
}

// use the http method to determine the access level
requiredScopeLevel := auth_model.Read
if ctx.Req.Method == http.MethodPost || ctx.Req.Method == http.MethodPut || ctx.Req.Method == http.MethodPatch || ctx.Req.Method == http.MethodDelete {
requiredScopeLevel = auth_model.Write
}
checkTokenScopes(ctx, requiredScopeLevel, requiredScopeCategories...)
}
}

// get the required scope for the given access level and category
requiredScopes := auth_model.GetRequiredScopes(requiredScopeLevel, requiredScopeCategories...)
allow, err := scope.HasScope(requiredScopes...)
if err != nil {
ctx.APIError(http.StatusForbidden, "checking scope failed: "+err.Error())
return
}
func tokenRequiresScopesLevel(requiredScopeLevel auth_model.AccessTokenScopeLevel, requiredScopeCategories ...auth_model.AccessTokenScopeCategory) func(ctx *context.APIContext) {
return func(ctx *context.APIContext) {
checkTokenScopes(ctx, requiredScopeLevel, requiredScopeCategories...)
}
}

if !allow {
ctx.APIError(http.StatusForbidden, fmt.Sprintf("token does not have at least one of required scope(s), required=%v, token scope=%v", requiredScopes, scope))
return
}
func checkTokenScopes(ctx *context.APIContext, requiredScopeLevel auth_model.AccessTokenScopeLevel, requiredScopeCategories ...auth_model.AccessTokenScopeCategory) {
// no scope required
if len(requiredScopeCategories) == 0 {
return
}

ctx.Data["requiredScopeCategories"] = requiredScopeCategories
// Need OAuth2 token to be present.
scope, scopeExists := ctx.Data["ApiTokenScope"].(auth_model.AccessTokenScope)
if ctx.Data["IsApiToken"] != true || !scopeExists {
return
}

// check if scope only applies to public resources
publicOnly, err := scope.PublicOnly()
if err != nil {
ctx.APIError(http.StatusForbidden, "parsing public resource scope failed: "+err.Error())
return
}
// get the required scope for the given access level and category
requiredScopes := auth_model.GetRequiredScopes(requiredScopeLevel, requiredScopeCategories...)
allow, err := scope.HasScope(requiredScopes...)
if err != nil {
ctx.APIError(http.StatusForbidden, "checking scope failed: "+err.Error())
return
}

if !allow {
ctx.APIError(http.StatusForbidden, fmt.Sprintf("token does not have at least one of required scope(s), required=%v, token scope=%v", requiredScopes, scope))
return
}

// assign to true so that those searching should only filter public repositories/users/organizations
ctx.PublicOnly = publicOnly
ctx.Data["requiredScopeCategories"] = requiredScopeCategories

// check if scope only applies to public resources
publicOnly, err := scope.PublicOnly()
if err != nil {
ctx.APIError(http.StatusForbidden, "parsing public resource scope failed: "+err.Error())
return
}

// assign to true so that those searching should only filter public repositories/users/organizations
ctx.PublicOnly = publicOnly
}

// Contexter middleware already checks token for user sign in process.
Expand Down Expand Up @@ -897,6 +905,7 @@ func Routes() *web.Router {
m *web.Router,
reqChecker func(ctx *context.APIContext),
act actions.API,
requiredScopeCategories ...auth_model.AccessTokenScopeCategory,
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addActionsRoutes now takes requiredScopeCategories as a variadic argument, but checkTokenScopes becomes a no-op when the slice is empty. That makes it easy for a future call site to omit the category and accidentally reintroduce the original scope-bypass on GET .../registration-token. Consider making this a required (non-variadic) parameter for addActionsRoutes, or defensively failing fast when it’s empty for endpoints that must enforce scopes.

Suggested change
requiredScopeCategories ...auth_model.AccessTokenScopeCategory,
requiredScopeCategories []auth_model.AccessTokenScopeCategory,

Copilot uses AI. Check for mistakes.
) {
m.Group("/actions", func() {
m.Group("/secrets", func() {
Expand All @@ -917,7 +926,7 @@ func Routes() *web.Router {

m.Group("/runners", func() {
m.Get("", reqToken(), reqChecker, act.ListRunners)
m.Get("/registration-token", reqToken(), reqChecker, act.GetRegistrationToken)
m.Get("/registration-token", reqToken(), reqChecker, tokenRequiresScopesLevel(auth_model.Write, requiredScopeCategories...), act.GetRegistrationToken)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET .../registration-token still maps to shared.GetRegistrationToken, which can create a new runner token when none exists or the latest is inactive. Even with the write-scope requirement, using GET for a potentially state-changing operation is problematic (cache/prefetch/logging semantics) and differs from GitHub’s POST-only behavior. Consider making GET strictly read-only (never creating) or returning 405 and requiring POST to create tokens.

Suggested change
m.Get("/registration-token", reqToken(), reqChecker, tokenRequiresScopesLevel(auth_model.Write, requiredScopeCategories...), act.GetRegistrationToken)
m.Get("/registration-token", reqToken(), reqChecker, tokenRequiresScopesLevel(auth_model.Write, requiredScopeCategories...), func(ctx *context.APIContext) {
// For security and HTTP semantics reasons, registration token creation
// must use POST. GET is not allowed to perform this state-changing action.
ctx.Status(http.StatusMethodNotAllowed)
})

Copilot uses AI. Check for mistakes.
m.Post("/registration-token", reqToken(), reqChecker, act.CreateRegistrationToken)
m.Get("/{runner_id}", reqToken(), reqChecker, act.GetRunner)
Comment on lines +929 to 931
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description mentions removing the GET method as a resolution, but the route is still present (now with stricter scope enforcement). If the intended fix is to eliminate GET entirely, this should be removed/disabled (and related swagger/docs/tests updated) rather than kept.

Copilot uses AI. Check for mistakes.
m.Delete("/{runner_id}", reqToken(), reqChecker, act.DeleteRunner)
Expand Down Expand Up @@ -1045,7 +1054,7 @@ func Routes() *web.Router {

m.Group("/runners", func() {
m.Get("", reqToken(), user.ListRunners)
m.Get("/registration-token", reqToken(), user.GetRegistrationToken)
m.Get("/registration-token", reqToken(), tokenRequiresScopesLevel(auth_model.Write, auth_model.AccessTokenScopeCategoryUser), user.GetRegistrationToken)
m.Post("/registration-token", reqToken(), user.CreateRegistrationToken)
m.Get("/{runner_id}", reqToken(), user.GetRunner)
m.Delete("/{runner_id}", reqToken(), user.DeleteRunner)
Expand Down Expand Up @@ -1166,7 +1175,7 @@ func Routes() *web.Router {
m.Post("/reject", repo.RejectTransfer)
}, reqToken())

addActionsRoutes(m, reqOwner(), repo.NewAction()) // it adds the routes for secrets/variables and runner management
addActionsRoutes(m, reqOwner(), repo.NewAction(), auth_model.AccessTokenScopeCategoryRepository) // it adds the routes for secrets/variables and runner management

m.Group("/actions/workflows", func() {
m.Get("", repo.ActionsListRepositoryWorkflows)
Expand Down Expand Up @@ -1621,6 +1630,7 @@ func Routes() *web.Router {
m,
reqOrgOwnership(),
org.NewAction(),
auth_model.AccessTokenScopeCategoryOrganization,
)
m.Group("/public_members", func() {
m.Get("", org.ListPublicMembers)
Expand Down Expand Up @@ -1735,7 +1745,7 @@ func Routes() *web.Router {
m.Get("/jobs", admin.ListWorkflowJobs)
})
m.Group("/runners", func() {
m.Get("/registration-token", admin.GetRegistrationToken)
m.Get("/registration-token", tokenRequiresScopesLevel(auth_model.Write, auth_model.AccessTokenScopeCategoryAdmin), admin.GetRegistrationToken)
})
}, tokenRequiresScopes(auth_model.AccessTokenScopeCategoryAdmin), reqToken(), reqSiteAdmin())

Expand Down
22 changes: 22 additions & 0 deletions tests/integration/api_actions_runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ func TestAPIActionsRunner(t *testing.T) {
func testActionsRunnerAdmin(t *testing.T) {
defer tests.PrepareTestEnv(t)()
adminUsername := "user1"
readToken := getUserToken(t, adminUsername, auth_model.AccessTokenScopeReadAdmin)
readReq := NewRequest(t, "GET", "/api/v1/admin/runners/registration-token").AddTokenAuth(readToken)
MakeRequest(t, readReq, http.StatusForbidden)

Comment on lines +30 to +33
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new assertions cover that read-scoped tokens are forbidden for the GET registration-token endpoint, but there’s no corresponding positive test that a write-scoped token can still successfully GET a registration token. Adding a success-path assertion here (or in one of the other contexts) would help catch route/middleware ordering mistakes that could inadvertently block the endpoint entirely.

Copilot uses AI. Check for mistakes.
token := getUserToken(t, adminUsername, auth_model.AccessTokenScopeWriteAdmin)
req := NewRequest(t, "POST", "/api/v1/admin/actions/runners/registration-token").AddTokenAuth(token)
tokenResp := MakeRequest(t, req, http.StatusOK)
Expand Down Expand Up @@ -76,6 +80,10 @@ func testActionsRunnerAdmin(t *testing.T) {
func testActionsRunnerUser(t *testing.T) {
defer tests.PrepareTestEnv(t)()
userUsername := "user1"
readToken := getUserToken(t, userUsername, auth_model.AccessTokenScopeReadUser)
readReq := NewRequest(t, "GET", "/api/v1/user/actions/runners/registration-token").AddTokenAuth(readToken)
MakeRequest(t, readReq, http.StatusForbidden)

token := getUserToken(t, userUsername, auth_model.AccessTokenScopeWriteUser)
req := NewRequest(t, "POST", "/api/v1/user/actions/runners/registration-token").AddTokenAuth(token)
tokenResp := MakeRequest(t, req, http.StatusOK)
Expand Down Expand Up @@ -202,6 +210,13 @@ func testActionsRunnerOwner(t *testing.T) {
MakeRequest(t, req, http.StatusForbidden)
})

t.Run("GetRegistrationTokenReadScopeForbidden", func(t *testing.T) {
userUsername := "user2"
token := getUserToken(t, userUsername, auth_model.AccessTokenScopeReadOrganization)
req := NewRequest(t, "GET", "/api/v1/orgs/org3/actions/runners/registration-token").AddTokenAuth(token)
MakeRequest(t, req, http.StatusForbidden)
})

t.Run("GetRepoScopeForbidden", func(t *testing.T) {
userUsername := "user2"
token := getUserToken(t, userUsername, auth_model.AccessTokenScopeReadRepository)
Expand Down Expand Up @@ -306,6 +321,13 @@ func testActionsRunnerRepo(t *testing.T) {
MakeRequest(t, req, http.StatusForbidden)
})

t.Run("GetRegistrationTokenReadScopeForbidden", func(t *testing.T) {
userUsername := "user2"
token := getUserToken(t, userUsername, auth_model.AccessTokenScopeReadRepository)
req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/actions/runners/registration-token").AddTokenAuth(token)
MakeRequest(t, req, http.StatusForbidden)
})

t.Run("GetOrganizationScopeForbidden", func(t *testing.T) {
userUsername := "user2"
token := getUserToken(t, userUsername, auth_model.AccessTokenScopeReadOrganization)
Expand Down