diff --git a/routers/api/packages/api.go b/routers/api/packages/api.go index 47e582eb92..3b51ddc406 100644 --- a/routers/api/packages/api.go +++ b/routers/api/packages/api.go @@ -118,6 +118,7 @@ func verifyAuth(r *web.Route, authMethods []auth.Method) { ctx.Doer, err = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session) if err != nil { log.Info("Failed to verify user: %v", err) + container.APIUnauthorizedError(ctx) ctx.Error(http.StatusUnauthorized, "authGroup.Verify") return } diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index 873ce5c23a..373847b3ab 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -125,15 +125,16 @@ func apiErrorDefined(ctx *context.Context, err *namedError) { }) } -func apiUnauthorizedError(ctx *context.Context) { - ctx.Resp.Header().Add("WWW-Authenticate", `Bearer realm="`+setting.AppURL+`v2/token",service="container_registry",scope="*"`) +func APIUnauthorizedError(ctx *context.Context) { + ctx.Resp.Header().Set("WWW-Authenticate", `Bearer realm="`+setting.AppURL+`v2/token",service="container_registry",scope="*",`+ + `Basic realm="`+setting.AppURL+`v2",service="container_registry",scope="*"`) apiErrorDefined(ctx, errUnauthorized) } // ReqContainerAccess is a middleware which checks the current user valid (real user or ghost if anonymous access is enabled) func ReqContainerAccess(ctx *context.Context) { if ctx.Doer == nil || (setting.Service.RequireSignInView && ctx.Doer.IsGhost()) { - apiUnauthorizedError(ctx) + APIUnauthorizedError(ctx) } } @@ -158,7 +159,7 @@ func Authenticate(ctx *context.Context) { u := ctx.Doer if u == nil { if setting.Service.RequireSignInView { - apiUnauthorizedError(ctx) + APIUnauthorizedError(ctx) return } diff --git a/services/auth/auth.go b/services/auth/auth.go index 52f1f65cce..14d78cdbc0 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -33,7 +33,9 @@ func isAttachmentDownload(req *http.Request) bool { // isContainerPath checks if the request targets the container endpoint func isContainerPath(req *http.Request) bool { - return strings.HasPrefix(req.URL.Path, "/v2/") + // Go's URL omits trailing slashes from `Path`. That means that `/v2/`, the top-level endpoint, appears as `/v2`. + // strings.HasPrefix(req.URL.Path, "/v2") would be inappropriate because it would match paths like `/v2-abcd`, too. + return req.URL.Path == "/v2" || strings.HasPrefix(req.URL.Path, "/v2/") } var ( diff --git a/services/auth/auth_test.go b/services/auth/auth_test.go index a6c6c74022..82308402d2 100644 --- a/services/auth/auth_test.go +++ b/services/auth/auth_test.go @@ -6,9 +6,13 @@ package auth import ( "net/http" + "net/url" "testing" "forgejo.org/modules/setting" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_isGitRawOrLFSPath(t *testing.T) { @@ -132,3 +136,63 @@ func Test_isGitRawOrLFSPath(t *testing.T) { } setting.LFS.StartServer = origLFSStartServer } + +func TestAuth_isContainerPath(t *testing.T) { + testCases := []struct { + name string + input string + isContainerPath bool + }{ + { + name: "without trailing slash", + input: "https://example.com/v2", + isContainerPath: true, + }, + { + name: "with trailing slash", + input: "https://example.com/v2/", + isContainerPath: true, + }, + { + name: "with additional path components", + input: "https://example.com/v2/example/blobs/uploads/", + isContainerPath: true, + }, + { + name: "without v2", + input: "https://example.com/", + isContainerPath: false, + }, + { + name: "v2 not at the beginning", + input: "https://example.com/something/v2/", + isContainerPath: false, + }, + { + name: "v2 with prefix", + input: "https://example.com/abcd-v2/", + isContainerPath: false, + }, + { + name: "v2 with suffix", + input: "https://example.com/v2-abcd/", + isContainerPath: false, + }, + { + name: "v1", + input: "https://example.com/v1/", + isContainerPath: false, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + inputURL, err := url.Parse(testCase.input) + require.NoError(t, err) + + request := http.Request{URL: inputURL} + + assert.Equal(t, testCase.isContainerPath, isContainerPath(&request)) + }) + } +} diff --git a/tests/integration/api_packages_container_cleanup_sha256_test.go b/tests/integration/api_packages_container_cleanup_sha256_test.go index 19b73f7698..d672504509 100644 --- a/tests/integration/api_packages_container_cleanup_sha256_test.go +++ b/tests/integration/api_packages_container_cleanup_sha256_test.go @@ -63,7 +63,8 @@ func TestPackageContainerCleanupSHA256(t *testing.T) { Token string `json:"token"` } - authenticate := []string{`Bearer realm="` + setting.AppURL + `v2/token",service="container_registry",scope="*"`} + authenticate := []string{`Bearer realm="` + setting.AppURL + `v2/token",service="container_registry",scope="*",` + + `Basic realm="` + setting.AppURL + `v2",service="container_registry",scope="*"`} t.Run("User", func(t *testing.T) { req := NewRequest(t, "GET", fmt.Sprintf("%sv2", setting.AppURL)) diff --git a/tests/integration/api_packages_container_test.go b/tests/integration/api_packages_container_test.go index b961415feb..9143ff973b 100644 --- a/tests/integration/api_packages_container_test.go +++ b/tests/integration/api_packages_container_test.go @@ -86,7 +86,8 @@ func TestPackageContainer(t *testing.T) { Token string `json:"token"` } - authenticate := []string{`Bearer realm="` + setting.AppURL + `v2/token",service="container_registry",scope="*"`} + authenticate := []string{`Bearer realm="` + setting.AppURL + `v2/token",service="container_registry",scope="*",` + + `Basic realm="` + setting.AppURL + `v2",service="container_registry",scope="*"`} t.Run("Anonymous", func(t *testing.T) { defer tests.PrintCurrentTest(t)() @@ -166,6 +167,33 @@ func TestPackageContainer(t *testing.T) { MakeRequest(t, req, http.StatusOK) }) }) + + t.Run("No token issued if credentials are invalid", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", fmt.Sprintf("%sv2/token", setting.AppURL)) + // Setting the header explicitly instead of using AddBasicAuth to supply an invalid password. + req.Request.Header.Set("Authorization", "Basic "+base64.StdEncoding.EncodeToString([]byte("user2:very-invalid"))) + resp := MakeRequest(t, req, http.StatusUnauthorized) + + assert.Equal(t, authenticate, resp.Header().Values("WWW-Authenticate")) + }) + + t.Run("Basic authentication", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", fmt.Sprintf("%sv2", setting.AppURL)) + req.AddBasicAuth("does-not-exist") + resp := MakeRequest(t, req, http.StatusUnauthorized) + + assert.Equal(t, authenticate, resp.Header().Values("WWW-Authenticate")) + + req = NewRequest(t, "GET", fmt.Sprintf("%sv2", setting.AppURL)) + req.AddBasicAuth(user.Name) + resp = MakeRequest(t, req, http.StatusOK) + + assert.Empty(t, resp.Header().Get("WWW-Authenticate")) + }) }) t.Run("DetermineSupport", func(t *testing.T) {