I think this request is reasonable.

We've considered Markdown. Although it is more secure, the problem is it is not compatible with Buildbot, which may complicate the transition.  

I think we could find a Go lib that sanitizes HTML.

The great thing about Markdown is that it is human readable, hence it would still look okay if not rendered on BuildBot. It would also be suitable for including in plain text emails and other places.

If we go with HTML, please use one of the SecOps approved libraries.

I like the markdown idea. I'd like us to have a command line client for accessing build results and this would be compatible with that.

Perhaps existing usage of step text is compatible with md because perhaps we use only <br>s and they are compatible with it

I didn't find a GoogleSecurity-approved Go library for Markdown processing. In particular, https://github.com/russross/blackfriday is disapproved for untrusted input

Could I get a status update on this?

this is p3 bug, we didn't do anything about this bug since #7

if you know a secure Markdown or HTML sanitization library in Go, please let us know. Absence of a good library is the primary blocker of this bug.

We could use https://godoc.org/golang.org/x/net/html

I am okay with rendering markdown (provided we find a approved library), but I object at supporting rendering arbitrary HTML because:
* The amount of rendering "looks" we support becomes unbounded.  What if someone throws in a style, expecting it to show up on the buildbot theme, which brings me to my next point
* The buildbot theme is not meant to last forever.  We do want to allow for future teams to create a completely different theme that isn't tied to how buildbot looks today.  Allow arbitrary HTML will make that job significantly harder, if there are dependencies on how buildbot looks today
* Markdown is constrained enough that this becomes less of an issue, since markdown could potentially work in any theme.
* Allowing <img> tags is still probably a dumb idea, I hope no one actually does that.

I don't think anyone suggested to support arbitrary HTML
#11 implied a limited (very limited) HTML as the bug description specifies. In particular, I think the only HTML tags we may support is:

ul, ol, li: no attributes
strong, em: no attributes
a: attributes "href" and "alt"
table, tr, td: attributes "rowspan" and "colspan"

Users input should not be able to control style. We will control the style of user-supplied rich text.

<p> should be fine too

Regardless of this discussion, we should limit width of step text.

here, I wrote HTML sanitization package https://codereview.chromium.org/2849353002/

We probably want some type of red (failure)/green (success)/yellow (warning)/purple (infra failure) coloring?

Or maybe <span style="failure"> / <span style="success"> / etc....

Predefined CSS classes sgtm

I want to put in my vote for markdown since it will look good both rendered and as plain text. I hope that eventually we will have a milo command line client for accessing failing step data associated with a user's build and raw markdown will be readable but html won't.

yeah, markdown would be great. It is just we don't have a secure lib to render it. Waiting for it and not implementing a limited set of HTML is an option too

if we prefer markdown, we can essentially mitigate the problem by printing nicely looking markdown everywhere. In this case, Milo build pages immediately look OKish even without a markdown renderer (because markdown looks nice as is). And when we find a secure markdown lib in Go, Milo build pages will just become a bit nicer.

I don't think we should use markdown for this. 

Markdown is a good language for humans to author text, but it's at best kinda awkward to generate programmatically, and it's fairly silly to have a machine generate markdown just to then convert it back into HTML.

I also don't think this is an area where we should spend a lot of time customizing the step output; we should really be shooting for a world where things just all pass or don't (i.e., the webkit_tests complexity is something we should move away from, not embrace).

BTW, it shouldn't be hard to render HTML tables/lists/paragraphs as text in terminal, given we already have an HTML parser

The following revision refers to this bug:
  https://chromium.googlesource.com/external/github.com/luci/luci-go.git/+/e3b4c50fa889a3825db10a0eb87adf7bc0807cb3

commit e3b4c50fa889a3825db10a0eb87adf7bc0807cb3
Author: nodir <nodir@chromium.org>
Date: Wed May 10 17:19:11 2017

sanitizehtml: add a package to sanitize HTML

It is based on an existing HTML parser in golang.org/x/net/html.

The motivation is to support user-supplied HTML in step_text in Milo.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=704387

Review-Url: https://codereview.chromium.org/2849353002

[add] https://crrev.com/e3b4c50fa889a3825db10a0eb87adf7bc0807cb3/common/data/text/sanitizehtml/sanitize.go
[add] https://crrev.com/e3b4c50fa889a3825db10a0eb87adf7bc0807cb3/common/data/text/sanitizehtml/sanitize_test.go

The following revision refers to this bug:
  https://chromium.googlesource.com/external/github.com/luci/luci-go.git/+/67031d2b5cda2e5a7342fd218917d6d5c61ce389

commit 67031d2b5cda2e5a7342fd218917d6d5c61ce389
Author: nodir <nodir@chromium.org>
Date: Wed May 10 19:51:25 2017

sanitizehtml: disallow tables

It is not trivial to control presentation of tables. In particular, tables do
not honor ancestor node boundaries.

Disallow them for now.

R=hinoka@chromium.org
BUG=704387

Review-Url: https://codereview.chromium.org/2873983003

[modify] https://crrev.com/67031d2b5cda2e5a7342fd218917d6d5c61ce389/common/data/text/sanitizehtml/sanitize.go
[modify] https://crrev.com/67031d2b5cda2e5a7342fd218917d6d5c61ce389/common/data/text/sanitizehtml/sanitize_test.go

Hi nodir@ and hinoka@,

What is the status of this bug? It looks like there is some progress with the sanitizehtml stuff?

It also looks like you have disabled tables, which is one feature that would have been super useful in my examples above...

I also believe phosek@ is after some similar features for some of the work he is doing.

Tim 'mithro' Ansell

the status is there is not enough consensus on our team about this bug. I am fine with rendering sanitized HTML in step text. This will at least solve the problem that currently LUCI builds with webkit failures look terrible: https://ci.chromium.org/swarming/task/37ec3b2785898d10, look at the width there.
I think hinoka@ and estaab@ are on the fence.

The tables were disabled because it is hard to control their presentation via CSS. E.g. we cannot limit their bounds.

I think phosek@ needs to render large amount of HTML which we cannot allow on a build page, so fixing this bug probably won't help them.

nodir@, we only need hyperlinks actually.

recipe engine already has API for links: https://cs.chromium.org/search/?q=presentation.links&sq=package:chromium&type=cs

I just realized this bug is only for Milo output, what we really need is support for HTML in LogDog's web viewer. I'll file a separate bug for that.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

the new protocol supports Markdown
https://chromium.googlesource.com/infra/luci/luci-go/+/b4019a5efec0c7b2890bb5a7c5c8b2036ccdd9d5/buildbucket/proto/step.proto#91

Milo doesn't use it yet, that's bug 850113.