From 243191475657c2a7c3f9956fcb05c89163e25518 Mon Sep 17 00:00:00 2001 From: Patrick Pacher Date: Wed, 27 Jul 2022 14:57:05 +0200 Subject: [PATCH 1/3] Fix CORS preflight checks requiring authentication --- api/router.go | 68 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/api/router.go b/api/router.go index 71502c3..953768d 100644 --- a/api/router.go +++ b/api/router.go @@ -106,10 +106,9 @@ func (mh *mainHandler) handle(w http.ResponseWriter, r *http.Request) error { }() // Check Cross-Origin Requests. - isCrossSite := false origin := r.Header.Get("Origin") + isPreflighCheck := false if origin != "" { - isCrossSite = true // Parse origin URL. originURL, err := url.Parse(origin) @@ -125,6 +124,9 @@ func (mh *mainHandler) handle(w http.ResponseWriter, r *http.Request) error { // Origin (with port) matches Host. case originURL.Hostname() == r.Host: // Origin (without port) matches Host. + case originURL.Scheme == "chrome-extension": + // Allow access for the browser extension + // TODO(ppacher): can we improve that check here? case devMode() && utils.StringInSlice(allowedDevCORSOrigins, originURL.Hostname()): // We are in dev mode and the request is coming from the allowed @@ -138,6 +140,22 @@ func (mh *mainHandler) handle(w http.ResponseWriter, r *http.Request) error { // If the Host header has a port, and the Origin does not, requests will // also end up here, as we cannot properly check for equality. } + + // Add Cross-Site Headers now as we need them in any case now. + w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Access-Control-Allow-Methods", "*") + w.Header().Set("Access-Control-Allow-Headers", "*") + w.Header().Set("Access-Control-Allow-Credentials", "true") + w.Header().Set("Access-Control-Expose-Headers", "*") + w.Header().Set("Access-Control-Max-Age", "60") + w.Header().Add("Vary", "Origin") + + // if there's a Access-Control-Request-Method header this is a Preflight check. + // In that case, we will just check if the preflighMethod is allowed and then return + // success here + if preflighMethod := r.Header.Get("Access-Control-Request-Method"); r.Method == http.MethodOptions && preflighMethod != "" { + isPreflighCheck = true + } } // Clean URL. @@ -184,21 +202,6 @@ func (mh *mainHandler) handle(w http.ResponseWriter, r *http.Request) error { return nil } - // Check authentication. - apiRequest.AuthToken = authenticateRequest(lrw, r, handler, readMethod) - if apiRequest.AuthToken == nil { - // Authenticator already replied. - return nil - } - - // Wait for the owning module to be ready. - if moduleHandler, ok := handler.(ModuleHandler); ok { - if !moduleIsReady(moduleHandler.BelongsTo()) { - http.Error(lrw, "The API endpoint is not ready yet. Please try again later.", http.StatusServiceUnavailable) - return nil - } - } - // Add security headers. w.Header().Set("Referrer-Policy", "no-referrer") w.Header().Set("X-Content-Type-Options", "nosniff") @@ -217,15 +220,28 @@ func (mh *mainHandler) handle(w http.ResponseWriter, r *http.Request) error { ) } - // Add Cross-Site Headers when handling Cross-Site Requests. - if isCrossSite { - w.Header().Set("Access-Control-Allow-Origin", origin) - w.Header().Set("Access-Control-Allow-Methods", "*") - w.Header().Set("Access-Control-Allow-Headers", "*") - w.Header().Set("Access-Control-Allow-Credentials", "true") - w.Header().Set("Access-Control-Expose-Headers", "*") - w.Header().Set("Access-Control-Max-Age", "60") - w.Header().Add("Vary", "Origin") + // At this point we know the method is allowed and there's a handler for the request. + // If this is just a CORS-Preflight, we'll accept the request with StatusOK now. + // There's no point in trying to authenticate the request because the Browser will + // not send authentication along a preflight check. + if isPreflighCheck && handler != nil { + lrw.WriteHeader(http.StatusOK) + return nil + } + + // Check authentication. + apiRequest.AuthToken = authenticateRequest(lrw, r, handler, readMethod) + if apiRequest.AuthToken == nil { + // Authenticator already replied. + return nil + } + + // Wait for the owning module to be ready. + if moduleHandler, ok := handler.(ModuleHandler); ok { + if !moduleIsReady(moduleHandler.BelongsTo()) { + http.Error(lrw, "The API endpoint is not ready yet. Please try again later.", http.StatusServiceUnavailable) + return nil + } } // Check if we have a handler. From 119dbaef97f2dde97d1329b87cb3724fd88786f9 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 1 Aug 2022 11:03:37 +0200 Subject: [PATCH 2/3] Move security headers to the start of the router --- api/router.go | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/api/router.go b/api/router.go index 953768d..7f9ae3d 100644 --- a/api/router.go +++ b/api/router.go @@ -105,6 +105,24 @@ func (mh *mainHandler) handle(w http.ResponseWriter, r *http.Request) error { tracer.Submit() }() + // Add security headers. + w.Header().Set("Referrer-Policy", "no-referrer") + w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set("X-Frame-Options", "deny") + w.Header().Set("X-XSS-Protection", "1; mode=block") + w.Header().Set("X-DNS-Prefetch-Control", "off") + + // Add CSP Header in production mode. + if !devMode() { + w.Header().Set( + "Content-Security-Policy", + "default-src 'self'; "+ + "connect-src https://*.safing.io 'self'; "+ + "style-src 'self' 'unsafe-inline'; "+ + "img-src 'self' data:", + ) + } + // Check Cross-Origin Requests. origin := r.Header.Get("Origin") isPreflighCheck := false @@ -202,24 +220,6 @@ func (mh *mainHandler) handle(w http.ResponseWriter, r *http.Request) error { return nil } - // Add security headers. - w.Header().Set("Referrer-Policy", "no-referrer") - w.Header().Set("X-Content-Type-Options", "nosniff") - w.Header().Set("X-Frame-Options", "deny") - w.Header().Set("X-XSS-Protection", "1; mode=block") - w.Header().Set("X-DNS-Prefetch-Control", "off") - - // Add CSP Header in production mode. - if !devMode() { - w.Header().Set( - "Content-Security-Policy", - "default-src 'self'; "+ - "connect-src https://*.safing.io 'self'; "+ - "style-src 'self' 'unsafe-inline'; "+ - "img-src 'self' data:", - ) - } - // At this point we know the method is allowed and there's a handler for the request. // If this is just a CORS-Preflight, we'll accept the request with StatusOK now. // There's no point in trying to authenticate the request because the Browser will From 5bf056e584e71b4b99e3ba1c3b786527e9eb30af Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 1 Aug 2022 11:04:12 +0200 Subject: [PATCH 3/3] Elaborate on open questions regarding CORS of browser extensions --- api/router.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/api/router.go b/api/router.go index 7f9ae3d..860767c 100644 --- a/api/router.go +++ b/api/router.go @@ -144,7 +144,10 @@ func (mh *mainHandler) handle(w http.ResponseWriter, r *http.Request) error { // Origin (without port) matches Host. case originURL.Scheme == "chrome-extension": // Allow access for the browser extension - // TODO(ppacher): can we improve that check here? + // TODO(ppacher): + // This currently allows access from any browser extension. + // Can we reduce that to only our browser extension? + // Also, what do we need to support Firefox? case devMode() && utils.StringInSlice(allowedDevCORSOrigins, originURL.Hostname()): // We are in dev mode and the request is coming from the allowed