fix sandbox

Author: wxiaoguang

custom/conf/app.example.ini

@@ -2540,13 +2540,17 @@ LEVEL = Info
 ;; * sanitized: Sanitize the content and render it inside current page, default to only allow a few HTML tags and attributes. Customized sanitizer rules can be defined in [markup.sanitizer.*] .
 ;; * no-sanitizer: Disable the sanitizer and render the content inside current page. It's **insecure** and may lead to XSS attack if the content contains malicious code.
 ;; * iframe: Render the content in a separate standalone page and embed it into current page by iframe. The iframe is in sandbox mode with same-origin disabled, and the JS code are safely isolated from parent page.
-;RENDER_CONTENT_MODE=sanitized
-;;
+;RENDER_CONTENT_MODE = sanitized
+;; The sandbox applied to the iframe and Content-Security-Policy header when RENDER_CONTENT_MODE is `iframe`.
+;; It defaults to a safe set of "allow-*" restrictions (space separated).
+;; You can also set it to "disabled" to disable the sandbox completely (for example: if the content is only PDF)
+;; Don't set it unless you know what you are doing and you are sure there is no security risk.
+;RENDER_CONTENT_SANDBOX =
 ;; Whether post-process the rendered HTML content, including:
 ;; resolve relative links and image sources, recognizing issue/commit references, escaping invisible characters,
 ;; mentioning users, rendering permlink code blocks, replacing emoji shorthands, etc.
 ;; By default, this is true when RENDER_CONTENT_MODE is `sanitized`, otherwise false.
-;NEED_POST_PROCESS=false
+;NEED_POST_PROCESS = false
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

modules/httplib/serve.go

@@ -126,6 +126,7 @@ func setServeHeadersByFile(r *http.Request, w http.ResponseWriter, mineBuf []byt
 		// no sandbox attribute for pdf as it breaks rendering in at least safari. this
 		// should generally be safe as scripts inside PDF can not escape the PDF document
 		// see https://bugs.chromium.org/p/chromium/issues/detail?id=413851 for more discussion
+		// HINT: PDF-RENDER-SANDBOX: PDF won't render in sandboxed context
 		w.Header().Set("Content-Security-Policy", "default-src 'none'; style-src 'unsafe-inline'")
 	}
 

modules/markup/external/external.go

@@ -58,14 +58,11 @@ func (p *Renderer) SanitizerRules() []setting.MarkupSanitizerRule {
 	return p.MarkupSanitizerRules
 }
 
-// SanitizerDisabled disabled sanitize if return true
-func (p *Renderer) SanitizerDisabled() bool {
-	return p.RenderContentMode == setting.RenderContentModeNoSanitizer || p.RenderContentMode == setting.RenderContentModeIframe
-}
-
-// DisplayInIFrame represents whether render the content with an iframe
-func (p *Renderer) DisplayInIFrame() bool {
-	return p.RenderContentMode == setting.RenderContentModeIframe
+func (p *Renderer) GetExternalRendererOptions() (ret markup.ExternalRendererOptions) {
+	ret.SanitizerDisabled = p.RenderContentMode == setting.RenderContentModeNoSanitizer || p.RenderContentMode == setting.RenderContentModeIframe
+	ret.DisplayInIframe = p.RenderContentMode == setting.RenderContentModeIframe
+	ret.ContentSandbox = p.RenderContentSandbox
+	return ret
 }
 
 func envMark(envName string) string {

modules/markup/render.go

@@ -165,28 +165,19 @@ func RenderString(ctx *RenderContext, content string) (string, error) {
 	return buf.String(), nil
 }
 
-func renderIFrame(ctx *RenderContext, output io.Writer) error {
+func renderIFrame(ctx *RenderContext, sandbox string, output io.Writer) error {
 	src := fmt.Sprintf("%s/%s/%s/render/%s/%s", setting.AppSubURL,
 		url.PathEscape(ctx.RenderOptions.Metas["user"]),
 		url.PathEscape(ctx.RenderOptions.Metas["repo"]),
 		util.PathEscapeSegments(ctx.RenderOptions.Metas["RefTypeNameSubURL"]),
 		util.PathEscapeSegments(ctx.RenderOptions.RelativePath),
 	)
 
-	defaultWidth := "100%"
-	defaultHeight := "300"
-
-	// ATTENTION! at the moment, only "allow-scripts" is allowed for sandbox mode.
-	// "allow-same-origin" should never be used, it leads to XSS attack, and it makes the JS in iframe can access parent window's config and CSRF token
-	iframe := htmlutil.HTMLFormat(`
-<iframe data-src="%s"
-	class="external-render-iframe"
-	sandbox="allow-scripts allow-popups"
-	width="%s" height="%s"
-></iframe>
-`,
-		src, defaultWidth, defaultHeight)
-
+	var sandboxAttrValue template.HTML
+	if sandbox != "" {
+		sandboxAttrValue = htmlutil.HTMLFormat(`sandbox="%s"`, sandbox)
+	}
+	iframe := htmlutil.HTMLFormat(`<iframe data-src="%s" class="external-render-iframe" %s></iframe>`, src, sandboxAttrValue)
 	_, err := io.WriteString(output, string(iframe))
 	return err
 }
@@ -199,13 +190,20 @@ func pipes() (io.ReadCloser, io.WriteCloser, func()) {
 	}
 }
 
+func getExternalRendererOptions(renderer Renderer) (ret ExternalRendererOptions, _ bool) {
+	if externalRender, ok := renderer.(ExternalRenderer); ok {
+		return externalRender.GetExternalRendererOptions(), true
+	}
+	return ret, false
+}
+
 func RenderWithRenderer(ctx *RenderContext, renderer Renderer, input io.Reader, output io.Writer) error {
 	var extraHeadHTML template.HTML
-	if externalRender, ok := renderer.(ExternalRenderer); ok && externalRender.DisplayInIFrame() {
+	if extOpts, ok := getExternalRendererOptions(renderer); ok && extOpts.DisplayInIframe {
 		if !ctx.RenderOptions.InStandalonePage {
 			// for an external "DisplayInIFrame" render, it could only output its content in a standalone page
 			// otherwise, a <iframe> should be outputted to embed the external rendered page
-			return renderIFrame(ctx, output)
+			return renderIFrame(ctx, extOpts.ContentSandbox, output)
 		}
 		// else: this is a standalone page, fallthrough to the real rendering, and add extra JS/CSS
 		extraStyleHref := setting.AppSubURL + "/assets/css/external-render-iframe.css"
@@ -230,7 +228,7 @@ func RenderWithRenderer(ctx *RenderContext, renderer Renderer, input io.Reader,
 	eg, _ := errgroup.WithContext(ctx)
 	var pw2 io.WriteCloser = util.NopCloser{Writer: finalProcessor}
 
-	if r, ok := renderer.(ExternalRenderer); !ok || !r.SanitizerDisabled() {
+	if r, ok := renderer.(ExternalRenderer); !ok || !r.GetExternalRendererOptions().SanitizerDisabled {
 		var pr2 io.ReadCloser
 		var close2 func()
 		pr2, pw2, close2 = pipes()

modules/markup/renderer.go

@@ -25,13 +25,15 @@ type PostProcessRenderer interface {
 	NeedPostProcess() bool
 }
 
+type ExternalRendererOptions struct {
+	SanitizerDisabled bool
+	DisplayInIframe   bool
+	ContentSandbox    string
+}
+
 // ExternalRenderer defines an interface for external renderers
 type ExternalRenderer interface {
-	// SanitizerDisabled disabled sanitize if return true
-	SanitizerDisabled() bool
-
-	// DisplayInIFrame represents whether render the content with an iframe
-	DisplayInIFrame() bool
+	GetExternalRendererOptions() ExternalRendererOptions
 }
 
 // RendererContentDetector detects if the content can be rendered

modules/setting/markup.go

@@ -63,6 +63,7 @@ type MarkupRenderer struct {
 	NeedPostProcess      bool
 	MarkupSanitizerRules []MarkupSanitizerRule
 	RenderContentMode    string
+	RenderContentSandbox string
 }
 
 // MarkupSanitizerRule defines the policy for whitelisting attributes on
@@ -253,13 +254,22 @@ func newMarkupRenderer(name string, sec ConfigSection) {
 		renderContentMode = RenderContentModeSanitized
 	}
 
+	// ATTENTION! at the moment, only a safe set like "allow-scripts" are allowed for sandbox mode.
+	// "allow-same-origin" should never be used, it leads to XSS attack, and it makes the JS in iframe can access parent window's config and CSRF token
+	renderContentSandbox := sec.Key("RENDER_CONTENT_SANDBOX").MustString("allow-scripts allow-popups")
+	if renderContentSandbox == "disabled" {
+		renderContentSandbox = ""
+	}
+
 	ExternalMarkupRenderers = append(ExternalMarkupRenderers, &MarkupRenderer{
-		Enabled:           sec.Key("ENABLED").MustBool(false),
-		MarkupName:        name,
-		FileExtensions:    exts,
-		Command:           command,
-		IsInputFile:       sec.Key("IS_INPUT_FILE").MustBool(false),
-		RenderContentMode: renderContentMode,
+		Enabled:        sec.Key("ENABLED").MustBool(false),
+		MarkupName:     name,
+		FileExtensions: exts,
+		Command:        command,
+		IsInputFile:    sec.Key("IS_INPUT_FILE").MustBool(false),
+
+		RenderContentMode:    renderContentMode,
+		RenderContentSandbox: renderContentSandbox,
 
 		// if no sanitizer is needed, no post process is needed
 		NeedPostProcess: sec.Key("NEED_POST_PROCESS").MustBool(renderContentMode == RenderContentModeSanitized),

routers/web/repo/render.go

@@ -4,18 +4,13 @@
 package repo
 
 import (
-	"bytes"
-	"io"
 	"net/http"
 	"path"
 
 	"code.gitea.io/gitea/models/renderhelper"
-	"code.gitea.io/gitea/modules/charset"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/markup"
-	"code.gitea.io/gitea/modules/typesniffer"
-	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/services/context"
 )
 
@@ -44,22 +39,8 @@ func RenderFile(ctx *context.Context) {
 	}
 	defer dataRc.Close()
 
-	buf := make([]byte, 1024)
-	n, _ := util.ReadAtMost(dataRc, buf)
-	buf = buf[:n]
-
-	st := typesniffer.DetectContentType(buf)
-	isTextFile := st.IsText()
-
-	rd := charset.ToUTF8WithFallbackReader(io.MultiReader(bytes.NewReader(buf), dataRc), charset.ConvertOpts{})
-	ctx.Resp.Header().Add("Content-Security-Policy", "frame-src 'self'; sandbox allow-scripts allow-popups")
-
 	if markupType := markup.DetectMarkupTypeByFileName(blob.Name()); markupType == "" {
-		if isTextFile {
-			_, _ = io.Copy(ctx.Resp, rd)
-		} else {
-			http.Error(ctx.Resp, "Unsupported file type render", http.StatusInternalServerError)
-		}
+		http.Error(ctx.Resp, "Unsupported file type render", http.StatusBadRequest)
 		return
 	}
 
@@ -68,7 +49,29 @@ func RenderFile(ctx *context.Context) {
 		CurrentTreePath: path.Dir(ctx.Repo.TreePath),
 	}).WithRelativePath(ctx.Repo.TreePath).WithInStandalonePage(true)
 
-	err = markup.Render(rctx, rd, ctx.Resp)
+	renderer, err := markup.FindRendererByContext(rctx)
+	if err != nil {
+		http.Error(ctx.Resp, "Unable to find renderer", http.StatusBadRequest)
+		return
+	}
+
+	extRenderer, ok := renderer.(markup.ExternalRenderer)
+	if !ok {
+		http.Error(ctx.Resp, "Unable to get external renderer", http.StatusBadRequest)
+		return
+	}
+
+	// To render PDF in iframe, the sandbox must NOT be used (iframe & CSP header).
+	// Chrome blocks the PDF rendering when sandboxed, even if all "allow-*" are set.
+	// HINT: PDF-RENDER-SANDBOX: PDF won't render in sandboxed context
+	extRendererOpts := extRenderer.GetExternalRendererOptions()
+	if extRendererOpts.ContentSandbox != "" {
+		ctx.Resp.Header().Add("Content-Security-Policy", "frame-src 'self'; sandbox "+extRendererOpts.ContentSandbox)
+	} else {
+		ctx.Resp.Header().Add("Content-Security-Policy", "frame-src 'self'")
+	}
+
+	err = markup.RenderWithRenderer(rctx, renderer, dataRc, ctx.Resp)
 	if err != nil {
 		log.Error("Failed to render file %q: %v", ctx.Repo.TreePath, err)
 		http.Error(ctx.Resp, "Failed to render file", http.StatusInternalServerError)

tests/integration/markup_external_test.go

@@ -4,7 +4,6 @@
 package integration
 
 import (
-	"io"
 	"net/http"
 	"net/url"
 	"strings"
@@ -31,12 +30,12 @@ func TestExternalMarkupRenderer(t *testing.T) {
 	}
 
 	onGiteaRun(t, func(t *testing.T, _ *url.URL) {
-		t.Run("RenderNoSanitizer", func(t *testing.T) {
-			user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
-			repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
-			_, err := createFile(user2, repo1, "file.no-sanitizer", "master", `any content`)
-			require.NoError(t, err)
+		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+		repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+		_, err := createFile(user2, repo1, "file.no-sanitizer", "master", `any content`)
+		require.NoError(t, err)
 
+		t.Run("RenderNoSanitizer", func(t *testing.T) {
 			req := NewRequest(t, "GET", "/user2/repo1/src/branch/master/file.no-sanitizer")
 			resp := MakeRequest(t, req, http.StatusOK)
 			doc := NewHTMLParser(t, resp.Body)
@@ -63,20 +62,40 @@ func TestExternalMarkupRenderer(t *testing.T) {
 	defer test.MockVariableValue(&r.RenderContentMode, setting.RenderContentModeIframe)()
 
 	t.Run("RenderContentInIFrame", func(t *testing.T) {
-		req := NewRequest(t, "GET", "/user30/renderer/src/branch/master/README.html")
-		resp := MakeRequest(t, req, http.StatusOK)
-		assert.Equal(t, "text/html; charset=utf-8", resp.Header().Get("Content-Type"))
-		doc := NewHTMLParser(t, resp.Body)
-		iframe := doc.Find("iframe")
-		assert.Empty(t, iframe.AttrOr("src", "")) // src should be empty, "data-src" is used instead
-		assert.Equal(t, "/user30/renderer/render/branch/master/README.html", iframe.AttrOr("data-src", ""))
+		t.Run("DefaultSandbox", func(t *testing.T) {
+			req := NewRequest(t, "GET", "/user30/renderer/src/branch/master/README.html")
+			var iframeDataSrc, iframeSandbox string
+			t.Run("ParentPage", func(t *testing.T) {
+				respParent := MakeRequest(t, req, http.StatusOK)
+				assert.Equal(t, "text/html; charset=utf-8", respParent.Header().Get("Content-Type"))
+				iframe := NewHTMLParser(t, respParent.Body).Find("iframe")
+				iframeDataSrc = iframe.AttrOr("data-src", "")
+				iframeSandbox = iframe.AttrOr("sandbox", "")
+				assert.Empty(t, iframe.AttrOr("src", "")) // src should be empty, "data-src" is used instead
+				assert.Equal(t, "/user30/renderer/render/branch/master/README.html", iframeDataSrc)
+			})
+			t.Run("SubPage", func(t *testing.T) {
+				req = NewRequest(t, "GET", iframeDataSrc)
+				respSub := MakeRequest(t, req, http.StatusOK)
+				assert.Equal(t, "text/html; charset=utf-8", respSub.Header().Get("Content-Type"))
 
-		req = NewRequest(t, "GET", "/user30/renderer/render/branch/master/README.html")
-		resp = MakeRequest(t, req, http.StatusOK)
-		assert.Equal(t, "text/html; charset=utf-8", resp.Header().Get("Content-Type"))
-		bs, err := io.ReadAll(resp.Body)
-		assert.NoError(t, err)
-		assert.Equal(t, "frame-src 'self'; sandbox allow-scripts allow-popups", resp.Header().Get("Content-Security-Policy"))
-		assert.Equal(t, "<script src=\"/assets/js/external-render-iframe.js\"></script><link rel=\"stylesheet\" href=\"/assets/css/external-render-iframe.css\"><div>\n\ttest external renderer\n</div>\n", string(bs))
+				// default sandbox on both parent and sub pages
+				assert.Equal(t, "allow-scripts allow-popups", iframeSandbox)
+				assert.Equal(t, "frame-src 'self'; sandbox allow-scripts allow-popups", respSub.Header().Get("Content-Security-Policy"))
+				assert.Equal(t, "<script src=\"/assets/js/external-render-iframe.js\"></script><link rel=\"stylesheet\" href=\"/assets/css/external-render-iframe.css\"><div>\n\ttest external renderer\n</div>\n", respSub.Body.String())
+			})
+		})
+
+		t.Run("NoSanitizerNoSandbox", func(t *testing.T) {
+			req := NewRequest(t, "GET", "/user2/repo1/src/branch/master/file.no-sanitizer")
+			respParent := MakeRequest(t, req, http.StatusOK)
+			iframe := NewHTMLParser(t, respParent.Body).Find("iframe")
+			req = NewRequest(t, "GET", "/user2/repo1/render/branch/master/file.no-sanitizer")
+			respSub := MakeRequest(t, req, http.StatusOK)
+
+			// no sandbox (disabled by RENDER_CONTENT_SANDBOX)
+			assert.Empty(t, iframe.AttrOr("sandbox", ""))
+			assert.Equal(t, "frame-src 'self'", respSub.Header().Get("Content-Security-Policy"))
+		})
 	})
 }

tests/sqlite.ini.tmpl

@@ -124,6 +124,7 @@ ENABLED = true
 FILE_EXTENSIONS = .no-sanitizer
 RENDER_COMMAND = echo '<script>window.alert("hi")</script>'
 RENDER_CONTENT_MODE = no-sanitizer
+RENDER_CONTENT_SANDBOX = disabled
 
 [actions]
 ENABLED = true

web_src/css/markup/content.css

@@ -545,6 +545,11 @@ In markup content, we always use bottom margin for all elements */
   margin: 0 0.25em;
 }
 
+.external-render-iframe {
+  width: 100%;
+  height: max(300px, 80vh);
+}
+
 .markup-content-iframe {
   display: block;
   border: none;