Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent double sanitize #16386

Merged
merged 8 commits into from
Nov 19, 2021
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 47 additions & 59 deletions modules/markup/markdown/markdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -56,24 +51,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()))
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cliff hanger here, reading it for the first time doesn't immediately hint me what 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 {
KN4CK3R marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
KN4CK3R marked this conversation as resolved.
Show resolved Hide resolved

return err
}

// RenderRawString renders Markdown to HTML without handling special links and return string
Expand Down