From 49fd4b4666068da45058707d3da02d4f8220b91c Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Thu, 8 Jul 2021 23:14:00 +0200 Subject: [PATCH 1/3] Prevent double sanitize. --- modules/markup/markdown/markdown.go | 106 ++++++++++++---------------- 1 file changed, 47 insertions(+), 59 deletions(-) diff --git a/modules/markup/markdown/markdown.go b/modules/markup/markdown/markdown.go index cac2a180faeef..c88d15daa166d 100644 --- a/modules/markup/markdown/markdown.go +++ b/modules/markup/markdown/markdown.go @@ -36,13 +36,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 } @@ -56,7 +51,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) @@ -64,16 +58,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())) @@ -159,49 +143,37 @@ 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 := ioutil.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 := ioutil.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 } func render(ctx *markup.RenderContext, input io.Reader, output io.Writer) error { @@ -211,14 +183,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) @@ -261,8 +232,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) } @@ -278,7 +249,24 @@ 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() + }() + + buf := markup.SanitizeReader(rd, "") + _, err := io.Copy(output, buf) + + return err } // RenderRawString renders Markdown to HTML without handling special links and return string From e5c822168cbc29fa0ce5f80a51fd48f3f0de4df2 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Fri, 9 Jul 2021 10:55:13 +0000 Subject: [PATCH 2/3] Lint --- modules/markup/markdown/markdown.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/markup/markdown/markdown.go b/modules/markup/markdown/markdown.go index c88d15daa166d..b62fea257e985 100644 --- a/modules/markup/markdown/markdown.go +++ b/modules/markup/markdown/markdown.go @@ -257,7 +257,7 @@ func RenderRaw(ctx *markup.RenderContext, input io.Reader, output io.Writer) err go func() { if err := render(ctx, input, wr); err != nil { - wr.CloseWithError(err) + _ = wr.CloseWithError(err) return } _ = wr.Close() From 49f199d1faa2dc6ac723877507a95bb64cb177a4 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Tue, 16 Nov 2021 16:42:28 +0000 Subject: [PATCH 3/3] Use SanitizeReaderToWriter. --- modules/markup/markdown/markdown.go | 6 ++---- modules/markup/renderer.go | 3 +-- modules/markup/sanitizer.go | 5 ++--- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/modules/markup/markdown/markdown.go b/modules/markup/markdown/markdown.go index 3f8b4649ab401..92b0144a30ec4 100644 --- a/modules/markup/markdown/markdown.go +++ b/modules/markup/markdown/markdown.go @@ -177,6 +177,7 @@ func actualRender(ctx *markup.RenderContext, input io.Reader, output io.Writer) 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() @@ -264,10 +265,7 @@ func RenderRaw(ctx *markup.RenderContext, input io.Reader, output io.Writer) err _ = wr.Close() }() - buf := markup.SanitizeReader(rd, "") - _, err := io.Copy(output, buf) - - return err + 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 3cd7cea7001f5..0ac0daaea9666 100644 --- 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 c8f9de33b5fb7..a1500d29d3ca4 100644 --- a/modules/markup/sanitizer.go +++ b/modules/markup/sanitizer.go @@ -6,7 +6,6 @@ package markup import ( - "bytes" "io" "regexp" "sync" @@ -146,11 +145,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) }