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.
This commit is contained in:
KN4CK3R 2021-11-19 11:46:47 +01:00 committed by GitHub
parent 381e131fc8
commit a09b40de8d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 48 additions and 64 deletions

View File

@ -35,13 +35,8 @@ var urlPrefixKey = parser.NewContextKey()
var isWikiKey = parser.NewContextKey() var isWikiKey = parser.NewContextKey()
var renderMetasKey = parser.NewContextKey() var renderMetasKey = parser.NewContextKey()
type closesWithError interface {
io.WriteCloser
CloseWithError(err error) error
}
type limitWriter struct { type limitWriter struct {
w closesWithError w io.Writer
sum int64 sum int64
limit int64 limit int64
} }
@ -55,7 +50,6 @@ func (l *limitWriter) Write(data []byte) (int, error) {
if err != nil { if err != nil {
return n, err return n, err
} }
_ = l.w.Close()
return n, fmt.Errorf("Rendered content too large - truncating render") return n, fmt.Errorf("Rendered content too large - truncating render")
} }
n, err := l.w.Write(data) n, err := l.w.Write(data)
@ -63,16 +57,6 @@ func (l *limitWriter) Write(data []byte) (int, error) {
return n, err 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 // newParserContext creates a parser.Context with the render context set
func newParserContext(ctx *markup.RenderContext) parser.Context { func newParserContext(ctx *markup.RenderContext) parser.Context {
pc := parser.NewContext(parser.WithIDs(newPrefixedIDs())) pc := parser.NewContext(parser.WithIDs(newPrefixedIDs()))
@ -153,19 +137,12 @@ func actualRender(ctx *markup.RenderContext, input io.Reader, output io.Writer)
}) })
rd, wr := io.Pipe()
defer func() {
_ = rd.Close()
_ = wr.Close()
}()
lw := &limitWriter{ lw := &limitWriter{
w: wr, w: output,
limit: setting.UI.MaxDisplayFileSize * 3, 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? // FIXME: should we include a timeout to abort the renderer if it takes too long?
go func() {
defer func() { defer func() {
err := recover() err := recover()
if err == nil { if err == nil {
@ -176,7 +153,6 @@ func actualRender(ctx *markup.RenderContext, input io.Reader, output io.Writer)
if log.IsDebug() { if log.IsDebug() {
log.Debug("Panic in markdown: %v\n%s", err, string(log.Stack(2))) 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 // FIXME: Don't read all to memory, but goldmark doesn't support
@ -184,20 +160,17 @@ func actualRender(ctx *markup.RenderContext, input io.Reader, output io.Writer)
buf, err := io.ReadAll(input) buf, err := io.ReadAll(input)
if err != nil { if err != nil {
log.Error("Unable to ReadAll: %v", err) log.Error("Unable to ReadAll: %v", err)
return return err
} }
if err := converter.Convert(giteautil.NormalizeEOL(buf), lw, parser.WithContext(pc)); err != nil { if err := converter.Convert(giteautil.NormalizeEOL(buf), lw, parser.WithContext(pc)); err != nil {
log.Error("Unable to render: %v", err) log.Error("Unable to render: %v", err)
_ = lw.CloseWithError(err)
return
}
_ = lw.Close()
}()
buf := markup.SanitizeReader(rd, "")
_, err := io.Copy(output, buf)
return 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 { func render(ctx *markup.RenderContext, input io.Reader, output io.Writer) error {
defer func() { defer func() {
err := recover() err := recover()
@ -205,14 +178,13 @@ func render(ctx *markup.RenderContext, input io.Reader, output io.Writer) error
return 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() { if log.IsDebug() {
log.Debug("Panic in markdown: %v\n%s", err, string(log.Stack(2))) log.Debug("Panic in markdown: %v\n%s", err, string(log.Stack(2)))
} }
ret := markup.SanitizeReader(input, "") _, err = io.Copy(output, input)
_, err = io.Copy(output, ret)
if err != nil { if err != nil {
log.Error("SanitizeReader failed: %v", err) log.Error("io.Copy failed: %v", err)
} }
}() }()
return actualRender(ctx, input, output) 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. // Render renders Markdown to HTML with all specific handling stuff.
func Render(ctx *markup.RenderContext, input io.Reader, output io.Writer) error { func Render(ctx *markup.RenderContext, input io.Reader, output io.Writer) error {
if ctx.Filename == "" { if ctx.Type == "" {
ctx.Filename = "a.md" ctx.Type = MarkupName
} }
return markup.Render(ctx, input, output) 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. // RenderRaw renders Markdown to HTML without handling special links.
func RenderRaw(ctx *markup.RenderContext, input io.Reader, output io.Writer) error { 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 // RenderRawString renders Markdown to HTML without handling special links and return string

View File

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

View File

@ -6,7 +6,6 @@
package markup package markup
import ( import (
"bytes"
"io" "io"
"regexp" "regexp"
"sync" "sync"
@ -149,11 +148,11 @@ func Sanitize(s string) string {
} }
// SanitizeReader sanitizes a Reader // 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() NewSanitizer()
policy, exist := sanitizer.rendererPolicies[renderer] policy, exist := sanitizer.rendererPolicies[renderer]
if !exist { if !exist {
policy = sanitizer.defaultPolicy policy = sanitizer.defaultPolicy
} }
return policy.SanitizeReader(r) return policy.SanitizeReaderToWriter(r, w)
} }