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.

diff --git a/modules/markup/markdown/markdown.go b/modules/markup/markdown/markdown.go
index 2574585573f49557abeac23593d1ecb7c2875428..5ce36a1d162c33ceef079f785c22d2a2d7217b07 100644 (file)
--- 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,7 +50,6 @@ 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)
@@ -63,16 +57,6 @@ func (l *limitWriter) Write(data []byte) (int, error) {
        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,51 +137,40 @@ 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()
@@ -205,14 +178,13 @@ func render(ctx *markup.RenderContext, input io.Reader, output io.Writer) error
                        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
index 3cd7cea7001f51406321eb33c1ebcf1e9d90af76..0ac0daaea9666cb1fb4d16926db0371e8403d619 100644 (file)
--- 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
index 5ff26a3109425029e6bc2ffc123615106f8b3c53..92dd19f0a1e5b399a48a8dc26a1f5ba55cc90108 100644 (file)
--- 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)
 }