From f9d48375676f374f828c7a8e3e3b658928f3827c Mon Sep 17 00:00:00 2001
From: Dmitry Verkhoturov <paskal.07@gmail.com>
Date: Sun, 4 Aug 2024 23:00:16 +0100
Subject: [PATCH] Add Content-Security-Policy and Permissions-Policy headers

`Content-Security-Policy` now restricts resource loading and execution
to enhance security:
  - `default-src 'none'`: Disallow all resource loading by default.
  - `base-uri 'none'`: Prevents the use of `<base>` tag to change the
  base URL for relative URLs.
  - `form-action 'none'`: Disallows form submissions.
  - `connect-src 'self'`: Restricts the origins that can be connected to
   (via XHR, WebSockets, etc.) to the same origin.
  - `frame-src 'self'`: Restricts the origins that can be embedded using
   `<frame>` and `<iframe>` to the same origin (for `/web/` demo
    endpoint).
  - `frame-ancestors %s;`: Specifies the origins that are allowed to
  embed this content in a frame. If no specific origins are allowed, it
  defaults to `*` (any origin). This enhances security by controlling
  which sites can embed your content.
  - `img-src 'self'`: Allows images to be loaded only from the same
  origin. If `imageProxyEnabled` is true, allows images from any origin
  (`*`).
  - `script-src 'self' 'unsafe-inline'`: Allows scripts to be loaded and
   executed only from the same origin and allows inline scripts.
  - `style-src 'self' 'unsafe-inline'`: Allows styles to be loaded and
  applied only from the same origin and allows inline styles.
  - `font-src data:`: Allows fonts to be loaded from data URIs.
  - `object-src 'none'`: Disallows the use of `<object>`, `<embed>`, and
   `<applet>` tags.

`Permissions-Policy` now restricts the use of certain browser features
which we don't use to enhance user privacy and security:
  - `accelerometer=()`: Disables the use of the accelerometer sensor.
  - `autoplay=()`: Disables automatic playback of media.
  - `camera=()`: Disables the use of the camera.
  - `cross-origin-isolated=()`: Disallows the page from being treated as
   cross-origin isolated.
  - `display-capture=()`: Disables the ability to capture the display.
  - `encrypted-media=()`: Disables the use of Encrypted Media Extensions
  .
  - `fullscreen=()`: Disables the ability to use fullscreen mode.
  - `geolocation=()`: Disables the use of geolocation.
  - `gyroscope=()`: Disables the use of the gyroscope sensor.
  - `keyboard-map=()`: Disables the use of the keyboard map.
  - `magnetometer=()`: Disables the use of the magnetometer sensor.
  - `microphone=()`: Disables the use of the microphone.
  - `midi=()`: Disables the use of the MIDI API.
  - `payment=()`: Disables the Payment Request API.
  - `picture-in-picture=()`: Disables the use of Picture-in-Picture mode
  .
  - `publickey-credentials-get=()`: Disables the use of the Web
  Authentication API.
  - `screen-wake-lock=()`: Disables the ability to prevent the screen
  from dimming.
  - `sync-xhr=()`: Disables synchronous XMLHttpRequest.
  - `usb=()`: Disables the use of the USB API.
  - `xr-spatial-tracking=()`: Disables the use of spatial tracking in
  WebXR.
  - `clipboard-read=()`: Disables the ability to read from the clipboard
  .
  - `clipboard-write=()`: Disables the ability to write to the clipboard
  .
  - `gamepad=()`: Disables the use of the Gamepad API.
  - `hid=()`: Disables the use of the Human Interface Device API.
  - `idle-detection=()`: Disables the ability to detect idle state.
  - `interest-cohort=()`: Disables the use of interest cohort tracking.
  - `serial=()`: Disables the use of the Serial API.
  - `unload=()`: Disables the ability to use the `beforeunload` and
  `unload` events.
  - `window-management=()`: Disables the ability to use window
  management APIs.
---
 backend/app/cmd/server.go         |  1 +
 backend/app/rest/api/rest.go      | 35 +++++++++++++------------
 backend/app/rest/api/rest_test.go | 43 +++++++++++++++----------------
 3 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/backend/app/cmd/server.go b/backend/app/cmd/server.go
index 121ed85f..c2bbfd4b 100644
--- a/backend/app/cmd/server.go
+++ b/backend/app/cmd/server.go
@@ -608,6 +608,7 @@ func (s *ServerCommand) newServerApp(ctx context.Context) (*serverApp, error) {
 		SubscribersOnly:            s.SubscribersOnly,
 		DisableSignature:           s.DisableSignature,
 		DisableFancyTextFormatting: s.DisableFancyTextFormatting,
+		ExternalImageProxy:         s.ImageProxy.CacheExternal,
 	}
 
 	srv.ScoreThresholds.Low, srv.ScoreThresholds.Critical = s.LowScore, s.CriticalScore
diff --git a/backend/app/rest/api/rest.go b/backend/app/rest/api/rest.go
index 8c875008..9f38e45b 100644
--- a/backend/app/rest/api/rest.go
+++ b/backend/app/rest/api/rest.go
@@ -70,6 +70,7 @@ type Rest struct {
 	SubscribersOnly            bool
 	DisableSignature           bool // prevent signature from being added to headers
 	DisableFancyTextFormatting bool // disables SmartyPants in the comment text rendering of the posted comments
+	ExternalImageProxy         bool
 
 	SSLConfig   SSLConfig
 	httpsServer *http.Server
@@ -205,6 +206,7 @@ func (s *Rest) routes() chi.Router {
 	}
 	router := chi.NewRouter()
 	router.Use(middleware.Throttle(1000), middleware.RealIP, R.Recoverer(log.Default()))
+	router.Use(securityHeadersMiddleware(s.ExternalImageProxy, s.AllowedAncestors))
 	if !s.DisableSignature {
 		router.Use(R.AppInfo("remark42", "umputun", s.Version))
 	}
@@ -226,11 +228,6 @@ func (s *Rest) routes() chi.Router {
 		router.Use(corsMiddleware.Handler)
 	}
 
-	if len(s.AllowedAncestors) > 0 {
-		log.Printf("[INFO] allowed from %+v only", s.AllowedAncestors)
-		router.Use(frameAncestors(s.AllowedAncestors))
-	}
-
 	ipFn := func(ip string) string { return store.HashValue(ip, s.SharedSecret)[:12] } // logger uses it for anonymization
 	logInfoWithBody := logger.New(logger.Log(log.Default()), logger.WithBody, logger.IPfn(ipFn), logger.Prefix("[INFO]")).Handler
 
@@ -623,19 +620,23 @@ func cacheControl(expiration time.Duration, version string) func(http.Handler) h
 	}
 }
 
-// frameAncestors is a middleware setting Content-Security-Policy "frame-ancestors host1 host2 ..."
-// prevents loading of comments widgets from any other origins. In case if the list of allowed empty, ignored.
-func frameAncestors(hosts []string) func(http.Handler) http.Handler {
-	return func(h http.Handler) http.Handler {
-		fn := func(w http.ResponseWriter, r *http.Request) {
-			if len(hosts) == 0 {
-				h.ServeHTTP(w, r)
-				return
+// securityHeadersMiddleware sets security-related headers: Content-Security-Policy and Permissions-Policy
+func securityHeadersMiddleware(imageProxyEnabled bool, allowedAncestors []string) func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			imgSrc := "'self'"
+			if imageProxyEnabled {
+				imgSrc = "*"
 			}
-			w.Header().Set("Content-Security-Policy", "frame-ancestors "+strings.Join(hosts, " ")+";")
-			h.ServeHTTP(w, r)
-		}
-		return http.HandlerFunc(fn)
+			frameAncestors := "*"
+			if len(allowedAncestors) > 0 {
+				log.Printf("[INFO] frame embedding allowed from %+v only", allowedAncestors)
+				frameAncestors = strings.Join(allowedAncestors, " ")
+			}
+			w.Header().Set("Content-Security-Policy", fmt.Sprintf("default-src 'none'; base-uri 'none'; form-action 'none'; connect-src 'self'; frame-src 'self'; img-src %s; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; font-src data:; object-src 'none'; frame-ancestors %s;", imgSrc, frameAncestors))
+			w.Header().Set("Permissions-Policy", "accelerometer=(), autoplay=(), camera=(), cross-origin-isolated=(), display-capture=(), encrypted-media=(), fullscreen=(), geolocation=(), gyroscope=(), keyboard-map=(), magnetometer=(), microphone=(), midi=(), payment=(), picture-in-picture=(), publickey-credentials-get=(), screen-wake-lock=(), sync-xhr=(), usb=(), xr-spatial-tracking=(), clipboard-read=(), clipboard-write=(), gamepad=(), hid=(), idle-detection=(), interest-cohort=(), serial=(), unload=(), window-management=()")
+			next.ServeHTTP(w, r)
+		})
 	}
 }
 
diff --git a/backend/app/rest/api/rest_test.go b/backend/app/rest/api/rest_test.go
index 22e8126e..63f19e34 100644
--- a/backend/app/rest/api/rest_test.go
+++ b/backend/app/rest/api/rest_test.go
@@ -320,30 +320,29 @@ func TestRest_cacheControl(t *testing.T) {
 }
 
 func TestRest_frameAncestors(t *testing.T) {
-	tbl := []struct {
-		hosts  []string
-		header string
-	}{
-		{[]string{"http://example.com"}, "frame-ancestors http://example.com;"},
-		{[]string{}, ""},
-		{[]string{"http://example.com", "http://example2.com"}, "frame-ancestors http://example.com http://example2.com;"},
-	}
+	ts, _, teardown := startupT(t, func(o *Rest) {
+		o.AllowedAncestors = []string{"'self'", "https://example.com"}
+	})
 
-	for i, tt := range tbl {
-		tt := tt
-		t.Run(strconv.Itoa(i), func(t *testing.T) {
-			req := httptest.NewRequest("GET", "http://example.com", http.NoBody)
-			w := httptest.NewRecorder()
+	// Test case with frame-ancestors
+	client := http.Client{}
+	resp, err := client.Get(ts.URL + "/web/index.html")
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+	assert.Contains(t, resp.Header.Get("Content-Security-Policy"), "frame-ancestors 'self' https://example.com;")
+	teardown()
 
-			h := frameAncestors(tt.hosts)(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}))
-			h.ServeHTTP(w, req)
-			resp := w.Result()
-			assert.Equal(t, http.StatusOK, resp.StatusCode)
-			assert.NoError(t, resp.Body.Close())
-			t.Logf("%+v", resp.Header)
-			assert.Equal(t, tt.header, resp.Header.Get("Content-Security-Policy"))
-		})
-	}
+	// Test case without frame-ancestors
+	ts, _, teardown = startupT(t, func(srv *Rest) {
+		srv.AllowedAncestors = []string{}
+	})
+	defer teardown()
+	resp, err = client.Get(ts.URL + "/web/index.html")
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+	assert.Contains(t, resp.Header.Get("Content-Security-Policy"), "frame-ancestors *;")
 }
 
 func TestRest_subscribersOnly(t *testing.T) {