Prevent double sanitize (#16386)

* Prevent double sanitize. * Use SanitizeReaderToWriter. At the moment `actualRender` uses `SanitizeReader` to sanitize the output. But `SanitizeReader` gets called in `markup.render` too so the output gets sanitized twice. I moved the `SanitizeReader` call into `RenderRaw` because this method does not use `markup.render`. I would like to remove the `RenderRaw`/`RenderRawString` methods too because they are only called from tests, the fuzzer and the `/markup/raw` api endpoint. This endpoint is not in use so I think we could remove them. If we really in the future need a method to render markdown without PostProcessing we could achieve this with a more flexible `renderer.NeedPostProcess` method.
go-gitea · Nov 19, 2021 · a09b40d · a09b40d
1 parent 381e131
commit a09b40d
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 64 deletions.
diff --git a/modules/markup/markdown/markdown.go b/modules/markup/markdown/markdown.go
@@ -35,13 +35,8 @@ var urlPrefixKey = parser.NewContextKey()
 var isWikiKey = parser.NewContextKey()
 var renderMetasKey = parser.NewContextKey()
 
-type closesWithError interface {
-	io.WriteCloser
-	CloseWithError(err error) error
-}
-
 type limitWriter struct {
-	w     closesWithError
+	w     io.Writer
 	sum   int64
 	limit int64
 }
@@ -55,24 +50,13 @@ func (l *limitWriter) Write(data []byte) (int, error) {
 		if err != nil {
 			return n, err
 		}
-		_ = l.w.Close()
 		return n, fmt.Errorf("Rendered content too large - truncating render")
 	}
 	n, err := l.w.Write(data)
 	l.sum += int64(n)
 	return n, err
 }
 
-// Close closes the writer
-func (l *limitWriter) Close() error {
-	return l.w.Close()
-}
-
-// CloseWithError closes the writer
-func (l *limitWriter) CloseWithError(err error) error {
-	return l.w.CloseWithError(err)
-}
-
 // newParserContext creates a parser.Context with the render context set
 func newParserContext(ctx *markup.RenderContext) parser.Context {
 	pc := parser.NewContext(parser.WithIDs(newPrefixedIDs()))
@@ -153,66 +137,54 @@ func actualRender(ctx *markup.RenderContext, input io.Reader, output io.Writer)
 
 	})
 
-	rd, wr := io.Pipe()
-	defer func() {
-		_ = rd.Close()
-		_ = wr.Close()
-	}()
-
 	lw := &limitWriter{
-		w:     wr,
+		w:     output,
 		limit: setting.UI.MaxDisplayFileSize * 3,
 	}
 
-	// FIXME: should we include a timeout that closes the pipe to abort the renderer and sanitizer if it takes too long?
-	go func() {
-		defer func() {
-			err := recover()
-			if err == nil {
-				return
-			}
-
-			log.Warn("Unable to render markdown due to panic in goldmark: %v", err)
-			if log.IsDebug() {
-				log.Debug("Panic in markdown: %v\n%s", err, string(log.Stack(2)))
-			}
-			_ = lw.CloseWithError(fmt.Errorf("%v", err))
-		}()
-
-		// FIXME: Don't read all to memory, but goldmark doesn't support
-		pc := newParserContext(ctx)
-		buf, err := io.ReadAll(input)
-		if err != nil {
-			log.Error("Unable to ReadAll: %v", err)
+	// FIXME: should we include a timeout to abort the renderer if it takes too long?
+	defer func() {
+		err := recover()
+		if err == nil {
 			return
 		}
-		if err := converter.Convert(giteautil.NormalizeEOL(buf), lw, parser.WithContext(pc)); err != nil {
-			log.Error("Unable to render: %v", err)
-			_ = lw.CloseWithError(err)
-			return
+
+		log.Warn("Unable to render markdown due to panic in goldmark: %v", err)
+		if log.IsDebug() {
+			log.Debug("Panic in markdown: %v\n%s", err, string(log.Stack(2)))
 		}
-		_ = lw.Close()
 	}()
-	buf := markup.SanitizeReader(rd, "")
-	_, err := io.Copy(output, buf)
-	return err
+
+	// FIXME: Don't read all to memory, but goldmark doesn't support
+	pc := newParserContext(ctx)
+	buf, err := io.ReadAll(input)
+	if err != nil {
+		log.Error("Unable to ReadAll: %v", err)
+		return err
+	}
+	if err := converter.Convert(giteautil.NormalizeEOL(buf), lw, parser.WithContext(pc)); err != nil {
+		log.Error("Unable to render: %v", err)
+		return err
+	}
+
+	return nil
 }
 
+// Note: The output of this method must get sanitized.
 func render(ctx *markup.RenderContext, input io.Reader, output io.Writer) error {
 	defer func() {
 		err := recover()
 		if err == nil {
 			return
 		}
 
-		log.Warn("Unable to render markdown due to panic in goldmark - will return sanitized raw bytes")
+		log.Warn("Unable to render markdown due to panic in goldmark - will return raw bytes")
 		if log.IsDebug() {
 			log.Debug("Panic in markdown: %v\n%s", err, string(log.Stack(2)))
 		}
-		ret := markup.SanitizeReader(input, "")
-		_, err = io.Copy(output, ret)
+		_, err = io.Copy(output, input)
 		if err != nil {
-			log.Error("SanitizeReader failed: %v", err)
+			log.Error("io.Copy failed: %v", err)
 		}
 	}()
 	return actualRender(ctx, input, output)
@@ -255,8 +227,8 @@ func (Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io.Wri
 
 // Render renders Markdown to HTML with all specific handling stuff.
 func Render(ctx *markup.RenderContext, input io.Reader, output io.Writer) error {
-	if ctx.Filename == "" {
-		ctx.Filename = "a.md"
+	if ctx.Type == "" {
+		ctx.Type = MarkupName
 	}
 	return markup.Render(ctx, input, output)
 }
@@ -272,7 +244,21 @@ func RenderString(ctx *markup.RenderContext, content string) (string, error) {
 
 // RenderRaw renders Markdown to HTML without handling special links.
 func RenderRaw(ctx *markup.RenderContext, input io.Reader, output io.Writer) error {
-	return render(ctx, input, output)
+	rd, wr := io.Pipe()
+	defer func() {
+		_ = rd.Close()
+		_ = wr.Close()
+	}()
+
+	go func() {
+		if err := render(ctx, input, wr); err != nil {
+			_ = wr.CloseWithError(err)
+			return
+		}
+		_ = wr.Close()
+	}()
+
+	return markup.SanitizeReader(rd, "", output)
 }
 
 // RenderRawString renders Markdown to HTML without handling special links and return string

diff --git a/modules/markup/renderer.go b/modules/markup/renderer.go
@@ -144,8 +144,7 @@ func render(ctx *RenderContext, renderer Renderer, input io.Reader, output io.Wr
 
 	wg.Add(1)
 	go func() {
-		buf := SanitizeReader(pr2, renderer.Name())
-		_, err = io.Copy(output, buf)
+		err = SanitizeReader(pr2, renderer.Name(), output)
 		_ = pr2.Close()
 		wg.Done()
 	}()

diff --git a/modules/markup/sanitizer.go b/modules/markup/sanitizer.go
@@ -6,7 +6,6 @@
 package markup
 
 import (
-	"bytes"
 	"io"
 	"regexp"
 	"sync"
@@ -149,11 +148,11 @@ func Sanitize(s string) string {
 }
 
 // SanitizeReader sanitizes a Reader
-func SanitizeReader(r io.Reader, renderer string) *bytes.Buffer {
+func SanitizeReader(r io.Reader, renderer string, w io.Writer) error {
 	NewSanitizer()
 	policy, exist := sanitizer.rendererPolicies[renderer]
 	if !exist {
 		policy = sanitizer.defaultPolicy
 	}
-	return policy.SanitizeReader(r)
+	return policy.SanitizeReaderToWriter(r, w)
 }