Skip to content

Commit 786cffa

Browse files
committed
fix(csrf): Replace gorilla/csrf with net/http.CrossOriginProtection
govulncheck reported Harbor's own code as affected by CVE-2025-24358, with a call trace through server/middleware/csrf/csrf.go. The fix is gorilla/csrf v1.7.3, and we were pinned below it on purpose: v1.7.3 broke HTTP-only logins (goharbor/harbor#22010), upstream reverted, and .github/renovate.json was taught to block the bump. There was no way out of that while the dependency stayed. Lifting the pin was tried first and does not work. v1.7.3 reconstructs the origin it expects from a presumed scheme plus the Host header, and neither survives a proxy. Against a real HTTP deployment behind Harbor's own nginx: - nginx sends 'proxy_set_header Host $host', which drops the port, so the library compared 192.168.0.5 against the browser's 192.168.0.5:8099 and answered 403 even with the plaintext flag set. - The plaintext flag is one global boolean off EXT_ENDPOINT's scheme. On a deployment answering as us./eu./harbor.dev over https with EXT_ENDPOINT on http, every ingress returned 403. One flag cannot describe N ingresses. CrossOriginProtection judges each request on the origin the browser actually used. Sec-Fetch-Site decides it, and no configured endpoint, Host or scheme enters the decision, so several ingresses on different schemes behind Host-rewriting proxies all work with nothing to configure. All four hostnames answer 200 in that same deployment. It also closes the attack the token scheme let through. A sibling subdomain is same-site, so SameSite=Strict still sent _gorilla_csrf and the subdomain could write cookies on the parent domain, making both halves of the double submit attacker-controlled; only the Origin check stood in the way, and CVE-2025-24358 is that the check never ran. That request is now refused. One gap is closed beyond what the library does. CrossOriginProtection admits a request carrying neither Sec-Fetch-Site nor Origin, reading it as non-browser traffic. csrfSkipper has already excused the registry, API and service routes that non-browser clients use, so an unsafe request arriving without either header is refused instead, matching the token scheme which rejected any write presenting no token. gorilla/csrf and gorilla/securecookie leave go.mod and the Renovate pin goes with them. govulncheck drops from one affected vulnerability to none, and no longer reports CVE-2025-47909 either, which has no fix in any release. csrfSkipper is untouched. CSRF_KEY is now inert and the portal's token interceptor is dead code; both degrade quietly and are removed separately rather than folded into a security fix. Signed-off-by: Prasanth Baskar <prasanth@8gears.com>
1 parent fdf2dbd commit 786cffa

5 files changed

Lines changed: 201 additions & 144 deletions

File tree

.github/renovate.json

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -134,13 +134,6 @@
134134
"docker"
135135
]
136136
},
137-
{
138-
"description": "gorilla/csrf is deliberately pinned to v1.7.2 (goharbor/harbor#22010) — block the v1.7.3+ updates Renovate keeps proposing, including via vulnerability alerts",
139-
"matchPackageNames": [
140-
"github.com/gorilla/csrf"
141-
],
142-
"allowedVersions": "<= 1.7.2"
143-
},
144137
{
145138
"description": "Bare deps: commit type everywhere — overrides :semanticPrefixFixDepsChoreOthers from config:recommended, which would otherwise emit fix(deps)/chore(deps)",
146139
"matchPackageNames": [

src/go.mod

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,6 @@ require (
3636
github.com/gomodule/redigo v2.0.0+incompatible
3737
github.com/google/go-containerregistry v0.22.0
3838
github.com/google/uuid v1.6.0
39-
// pin the github.com/gorilla/csrf to v1.7.2 because of issue https://github.com/goharbor/harbor/issues/22010
40-
github.com/gorilla/csrf v1.7.2
4139
github.com/gorilla/mux v1.8.1
4240
github.com/graph-gophers/dataloader v5.0.0+incompatible
4341
github.com/jackc/pgx/v5 v5.10.0
@@ -154,7 +152,6 @@ require (
154152
github.com/google/s2a-go v0.1.9 // indirect
155153
github.com/googleapis/enterprise-certificate-proxy v0.3.20 // indirect
156154
github.com/googleapis/gax-go/v2 v2.24.0 // indirect
157-
github.com/gorilla/securecookie v1.1.2 // indirect
158155
github.com/grpc-ecosystem/grpc-gateway/v2 v2.30.0 // indirect
159156
github.com/h2non/parth v0.0.0-20190131123155-b4df798d6542 // indirect
160157
github.com/hashicorp/golang-lru v1.0.2 // indirect

src/go.sum

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3767,8 +3767,6 @@ github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17
37673767
github.com/google/go-querystring v1.2.0 h1:yhqkPbu2/OH+V9BfpCVPZkNmUXhb2gBxJArfhIxNtP0=
37683768
github.com/google/go-querystring v1.2.0/go.mod h1:8IFJqpSRITyJ8QhQ13bmbeMBDfmeEJZD5A0egEOmkqU=
37693769
github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
3770-
github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0=
3771-
github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
37723770
github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXiIaQGbYVAs8BPL6v8lEs=
37733771
github.com/google/martian/v3 v3.0.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0=
37743772
github.com/google/martian/v3 v3.1.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0=
@@ -3860,16 +3858,12 @@ github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORR
38603858
github.com/gopherjs/gopherjs v1.17.2 h1:fQnZVsXk8uxXIStYb0N4bGk7jeyTalG/wsZjQ25dO0g=
38613859
github.com/gopherjs/gopherjs v1.17.2/go.mod h1:pRRIvn/QzFLrKfvEz3qUuEhtE/zLCWfreZ6J5gM2i+k=
38623860
github.com/gorilla/context v1.1.1/go.mod h1:kBGZzfjB9CEq2AlWe17Uuf7NDRt0dE0s8S51q0aT7Yg=
3863-
github.com/gorilla/csrf v1.7.2 h1:oTUjx0vyf2T+wkrx09Trsev1TE+/EbDAeHtSTbtC2eI=
3864-
github.com/gorilla/csrf v1.7.2/go.mod h1:F1Fj3KG23WYHE6gozCmBAezKookxbIvUJT+121wTuLk=
38653861
github.com/gorilla/mux v1.6.2/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs=
38663862
github.com/gorilla/mux v1.7.3/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs=
38673863
github.com/gorilla/mux v1.8.0/go.mod h1:DVbg23sWSpFRCP0SfiEN6jmj59UnW/n46BH5rLB71So=
38683864
github.com/gorilla/mux v1.8.1 h1:TuBL49tXwgrFYWhqrNgrUNEY92u81SPhu7sTdzQEiWY=
38693865
github.com/gorilla/mux v1.8.1/go.mod h1:AKf9I4AEqPTmMytcMc0KkNouC66V3BtZ4qD5fmWSiMQ=
38703866
github.com/gorilla/securecookie v1.1.1/go.mod h1:ra0sb63/xPlUeL+yeDciTfxMRAA+MP+HVt/4epWDjd4=
3871-
github.com/gorilla/securecookie v1.1.2 h1:YCIWL56dvtr73r6715mJs5ZvhtnY73hBvEF8kXD8ePA=
3872-
github.com/gorilla/securecookie v1.1.2/go.mod h1:NfCASbcHqRSY+3a8tlWJwsQap2VX5pwzwo4h3eOamfo=
38733867
github.com/gorilla/sessions v1.2.1/go.mod h1:dk2InVEVJ0sfLlnXv9EAgkf6ecYs/i80K/zI+bUmuGM=
38743868
github.com/gorilla/websocket v0.0.0-20170926233335-4201258b820c/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoAMk2YaljkQ=
38753869
github.com/graph-gophers/dataloader v5.0.0+incompatible h1:R+yjsbrNq1Mo3aPG+Z/EKYrXrXXUNJHOgbRt+U6jOug=

src/server/middleware/csrf/csrf.go

Lines changed: 43 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -16,77 +16,68 @@ package csrf
1616

1717
import (
1818
"net/http"
19-
"os"
19+
"slices"
2020
"strings"
21-
"sync"
2221

23-
"github.com/gorilla/csrf"
24-
25-
"github.com/goharbor/harbor/src/common/utils"
2622
"github.com/goharbor/harbor/src/lib"
27-
"github.com/goharbor/harbor/src/lib/config"
2823
"github.com/goharbor/harbor/src/lib/errors"
2924
lib_http "github.com/goharbor/harbor/src/lib/http"
3025
"github.com/goharbor/harbor/src/lib/log"
3126
"github.com/goharbor/harbor/src/server/middleware"
3227
)
3328

34-
const (
35-
csrfKeyEnv = "CSRF_KEY"
36-
tokenHeader = "X-Harbor-CSRF-Token"
37-
)
29+
// protect judges each request on the origin that browser actually used, so it
30+
// needs no configuration and is safe for concurrent use.
31+
var protect = http.NewCrossOriginProtection()
3832

39-
var (
40-
once sync.Once
41-
secureFlag = true
42-
protect func(handler http.Handler) http.Handler
43-
)
33+
// safeMethods are the methods RFC 7231 defines as safe, which carry no state
34+
// change and so need no cross-origin check.
35+
var safeMethods = []string{http.MethodGet, http.MethodHead, http.MethodOptions}
4436

45-
// attachToken makes sure if csrf generate a new token it will be included in the response header
46-
func attachToken(w http.ResponseWriter, r *http.Request) {
47-
if t := csrf.Token(r); len(t) > 0 {
48-
w.Header().Set(tokenHeader, t)
49-
} else {
50-
log.Warningf("token not found in context, skip attaching")
37+
// check applies the cross-origin protection, then closes the one gap it leaves
38+
// open by design.
39+
//
40+
// CrossOriginProtection admits a request carrying neither Sec-Fetch-Site nor
41+
// Origin, reading it as non-browser traffic. That is the right default for a
42+
// library, but not here: csrfSkipper has already excused the registry, API and
43+
// service routes that non-browser clients use, so an unsafe request arriving
44+
// without either header is one we cannot vouch for. Refusing it matches what the
45+
// previous token scheme did, which rejected any write that failed to present a
46+
// token.
47+
func check(req *http.Request) error {
48+
if err := protect.Check(req); err != nil {
49+
return err
5150
}
52-
}
5351

54-
func handleError(w http.ResponseWriter, r *http.Request) {
55-
attachToken(w, r)
56-
lib_http.SendError(w, errors.New(csrf.FailureReason(r)).WithCode(errors.ForbiddenCode))
57-
}
52+
if slices.Contains(safeMethods, req.Method) {
53+
return nil
54+
}
55+
56+
if req.Header.Get("Sec-Fetch-Site") == "" && req.Header.Get("Origin") == "" {
57+
return errors.New("request carries neither Sec-Fetch-Site nor Origin, cannot confirm its origin")
58+
}
5859

59-
func attach(handler http.Handler) http.Handler {
60-
return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
61-
attachToken(rw, req)
62-
handler.ServeHTTP(rw, req)
63-
})
60+
return nil
6461
}
6562

66-
// Middleware initialize the middleware to apply csrf selectively
63+
// Middleware rejects cross-origin requests that carry a session. It reads the
64+
// Fetch metadata headers browsers have sent since 2023, falling back to
65+
// comparing Origin against the Host the client addressed.
66+
//
67+
// Deliberately no configured endpoint takes part in the decision. Harbor is
68+
// commonly reachable on several ingresses, over different schemes, behind
69+
// proxies that rewrite Host; a single configured origin describes none of that,
70+
// and every attempt to reconstruct "the" expected origin gets one of those
71+
// deployments wrong.
6772
func Middleware() func(handler http.Handler) http.Handler {
68-
once.Do(func() {
69-
key := os.Getenv(csrfKeyEnv)
70-
if len(key) == 0 {
71-
key = utils.GenerateRandomString()
72-
} else if len(key) != 32 {
73-
log.Errorf("Invalid CSRF key length from the environment: %s. Please ensure the key length is 32 characters.", key)
74-
protect = func(_ http.Handler) http.Handler {
75-
return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
76-
lib_http.SendError(w, errors.New("invalid CSRF key length from the environment. Please ensure the key length is 32 characters"))
77-
})
78-
}
73+
return middleware.New(func(rw http.ResponseWriter, req *http.Request, next http.Handler) {
74+
if err := check(req); err != nil {
75+
log.Debugf("Rejected cross-origin request for %s: %v", lib.TrimLineBreaks(req.URL.Path), err)
76+
lib_http.SendError(rw, errors.New(err).WithCode(errors.ForbiddenCode))
7977
return
8078
}
81-
secureFlag = secureCookie()
82-
protect = csrf.Protect([]byte(key), csrf.RequestHeader(tokenHeader),
83-
csrf.Secure(secureFlag),
84-
csrf.ErrorHandler(http.HandlerFunc(handleError)),
85-
csrf.SameSite(csrf.SameSiteStrictMode),
86-
csrf.Path("/"))
87-
})
88-
return middleware.New(func(rw http.ResponseWriter, req *http.Request, next http.Handler) {
89-
protect(attach(next)).ServeHTTP(rw, req)
79+
80+
next.ServeHTTP(rw, req)
9081
}, csrfSkipper)
9182
}
9283

@@ -100,12 +91,3 @@ func csrfSkipper(req *http.Request) bool {
10091
}
10192
return false
10293
}
103-
104-
func secureCookie() bool {
105-
ep, err := config.ExtEndpoint()
106-
if err != nil {
107-
log.Warningf("Failed to get external endpoint: %v, set cookie secure flag to true", err)
108-
return true
109-
}
110-
return !strings.HasPrefix(strings.ToLower(ep), "http://")
111-
}

0 commit comments

Comments
 (0)