Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2849353002:
sanitizehtml: add a package to sanitize HTML (Closed)
Descriptionsanitizehtml: 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
Committed: https://github.com/luci/luci-go/commit/e3b4c50fa889a3825db10a0eb87adf7bc0807cb3
Patch Set 1 #Patch Set 2 : nit #
Total comments: 22
Patch Set 3 : rewrite #Patch Set 4 : nit #Patch Set 5 : add test #
Total comments: 14
Patch Set 6 : address nigeltao's comments #Patch Set 7 : re xtof #
Total comments: 4
Patch Set 8 : re nigeltao #Patch Set 9 : malformed #
Total comments: 6
Patch Set 10 : final comments #Patch Set 11 : fix comments #
Messages
Total messages: 37 (11 generated)
PTAL
Package golang.org/x/net/html says "It is the caller's responsibility to ensure that the Reader provides UTF-8 encoded HTML.". I should probably ensure that.
On 2017/05/01 23:43:20, nodir wrote: > Package golang.org/x/net/html says "It is the caller's responsibility to ensure > that the Reader provides UTF-8 encoded HTML.". I should probably ensure that. Ignore this.
tansell@chromium.org changed reviewers: + tansell@chromium.org
You need someone from security to approve this. As mentioned SecOps doesn't let us write things like this without review from them due to the likelihood of screwing it up.
https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:5: // Package sanitizehtml implements a sanitizer of a very limited HTML. it would be great to elaborate what exact subset of HTML is supported https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:52: "a": { may be a good idea to mandatory add rel="noopener" target="_blank" to all links https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize_test.go (right): https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize_test.go:51: // Links add a test for a relative URL https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize_test.go:65: `<a href="about:blank">link</a>`, duplication https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize_test.go:110: for _, c := range cases { add a test for various malformed HTML, e.g. <p><a>blah</p></a>
hinoka@chromium.org changed reviewers: + hinoka@chromium.org
I'm gonna think about if I can break this, since an escape could mean a job has the potential to steal a user's milo credentials. I general i'd feel more comfortable about constructing templates using the html/template library, and then injecting data by executing the templates. https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:43: return s return u.EscapedPath() seems safer. It'll force-escape URL params https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_S... https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:135: if sanitizer, ok := allowedAttrs[key]; a.Namespace == "" && ok { Using template/html execute might be better here because it knows the context. https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize_test.go (right): https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize_test.go:18: cases := []struct{ in, out string }{ Some error test cases would be nice. eg invalid html "<div><p>foo</p>" "<<a href=abc>" "<p></a alt="blah"></p>" etc..
xtof@google.com changed reviewers: + xtof@google.com
https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:43: return s On 2017/05/02 00:36:15, hinoka wrote: > return u.EscapedPath() seems safer. It'll force-escape URL params Not-properly escaped query parameters are generally harmless wrt XSS; the main important thing is that the URL will be parsed by the browser as one with a safe scheme. However, one benefit of letting url re-serialize the URL is that it we'll have higher confidence that what url emitted corresponds to what it thought it parsed. > > > https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_S... https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:135: if sanitizer, ok := allowedAttrs[key]; a.Namespace == "" && ok { On 2017/05/02 00:36:15, hinoka wrote: > Using template/html execute might be better here because it knows the context. I think what's here is fine, and (much) simpler. Using html/template would be somewhat tricky, because it makes the fundamental assumption that the template text is trustworthy. Contextual escaping of a trustworthy template has a different threat model vs sanitizing untrustworthy HTML markup. https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:149: write(n.Data) write(tag) would be slightly more "obviously safe" since that's the string that was validated above. https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:153: // ignore the tag, but visit children. I *think* this will cause the content of <script> and <style> tags and the like to be rendered as text (b/c in recursion, that content will be treated as a TextNode. This is purely cosmetic (not a security issue), but might result in somewhat ugly output if it happens. One way to handle this could be to add an enum to each entry of your allowedElements table that indicates how to handle the element (traverse children, discard content). Side note: you can then also use this to label and handle "void elements" (https://www.w3.org/TR/html5/syntax.html#void-elements), which would get rid of the special case for <br>, <hr> and the like.
Generally this looks good. The approach you're taking is exactly the right one: parse the HTML; filter by a strict whitelist of elements/attributes, and then re-serialize HTML from scratch. This ensures that the emitted HTML markup is fully under your control. It wouldn't even matter if there's a bug in the HTML parser (outside remote code execution; but this is Go, and the HTML parser should be robust wrt malicious input) that causes it to somehow mis-parse input, The way this is structured it's easy to see that the emitted HTML is well-formed, and within the small subset of HTML permitted by the policy specified through allowedElems.
I've rewrote this thing. The map of elements was nice, but it leaked and we had to have some special cases in Sanitize() so it did not really represent the logic entirely, i.e. the logic was implemented in two places. So, I've replaced it with a switch statement. PTAL https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:5: // Package sanitizehtml implements a sanitizer of a very limited HTML. On 2017/05/02 00:16:24, Vadim Sh. wrote: > it would be great to elaborate what exact subset of HTML is supported done, in Sanitize comment https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:43: return s On 2017/05/04 18:26:18, xtof wrote: > On 2017/05/02 00:36:15, hinoka wrote: > > return u.EscapedPath() seems safer. It'll force-escape URL params > Not-properly escaped query parameters are generally harmless wrt XSS; the main > important thing is that the URL will be parsed by the browser as one with a safe > scheme. However, one benefit of letting url re-serialize the URL is that it > we'll have higher confidence that what url emitted corresponds to what it > thought it parsed. > > > > > > > > https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_S... > Done. https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:52: "a": { On 2017/05/02 00:16:24, Vadim Sh. wrote: > may be a good idea to mandatory add rel="noopener" target="_blank" to all links Done. https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:135: if sanitizer, ok := allowedAttrs[key]; a.Namespace == "" && ok { On 2017/05/04 18:26:18, xtof wrote: > On 2017/05/02 00:36:15, hinoka wrote: > > Using template/html execute might be better here because it knows the context. > > I think what's here is fine, and (much) simpler. Using html/template would be > somewhat tricky, because it makes the fundamental assumption that the template > text is trustworthy. Contextual escaping of a trustworthy template has a > different threat model vs sanitizing untrustworthy HTML markup. Acknowledged. https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:149: write(n.Data) On 2017/05/04 18:26:18, xtof wrote: > write(tag) would be slightly more "obviously safe" since that's the string that > was validated above. I've changed it a bit. I hope it is still OK because the parameter is explicitly called safeElement https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:153: // ignore the tag, but visit children. On 2017/05/04 18:26:18, xtof wrote: > I *think* this will cause the content of <script> and <style> tags and the like > to be rendered as text (b/c in recursion, that content will be treated as a > TextNode. > > This is purely cosmetic (not a security issue), but might result in somewhat > ugly output if it happens. > > One way to handle this could be to add an enum to each entry of your > allowedElements table that indicates how to handle the element (traverse > children, discard content). > > Side note: you can then also use this to label and handle "void elements" > (https://www.w3.org/TR/html5/syntax.html#void-elements), which would get rid of > the special case for <br>, <hr> and the like. indeed, i should not print contents of <script> and <style> There was too many special cases and the allowedElems map abstraction leaked, so I've replaced it with a switch statement. Please take a look again https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize_test.go (right): https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize_test.go:18: cases := []struct{ in, out string }{ On 2017/05/02 00:36:16, hinoka wrote: > Some error test cases would be nice. eg invalid html > > "<div><p>foo</p>" > "<<a href=abc>" > "<p></a alt="blah"></p>" > etc.. Done. https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize_test.go:51: // Links On 2017/05/02 00:16:24, Vadim Sh. wrote: > add a test for a relative URL Done. https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize_test.go:65: `<a href="about:blank">link</a>`, On 2017/05/02 00:16:24, Vadim Sh. wrote: > duplication Done. https://codereview.chromium.org/2849353002/diff/20001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize_test.go:110: for _, c := range cases { On 2017/05/02 00:16:24, Vadim Sh. wrote: > add a test for various malformed HTML, e.g. > > <p><a>blah</p></a> Done.
nigeltao@chromium.org changed reviewers: + nigeltao@chromium.org
Some drive-by comments by the author of the "golang.org/x/net/html" package. I hope it's not too rude. :-) https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:29: if !unicode.IsDigit(r) { I'd just look for ASCII digits. It's not like you're expecting anything else in the http://www.fileformat.info/info/unicode/category/Nd/list.htm list. if r < '0' || '9' < r { etc } https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:72: w io.Writer If you care enough about efficiency (both in terms of string to []byte conversions, and doing one big vs many small writes to disk or network), it might be worth changing this type to: sw stringWriter and outside: type stringWriter interface { WriteString(string) (int, error) } and in func Sanitize: sw, ok, := w.(stringWriter) if !ok { bw := bufio.NewWriter(w) defer func(){ ferr := bw.Flush() if err == nil { err = ferr } }() sw = bw } s := sanitizer{sw: sw} etc and change the io.WriteString call in sanitizer.p. https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:103: // If allowedAttrs is nil, all attributes are ommitted. Typo in "omitted". https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:132: switch strings.ToLower(n.Data) { The ToLower'ing is unnecessary if you compare atoms (uint32 values) instead of strings. uint32 comparison is also faster. ``` switch n.Atom { case atom.Br: etc case atom.Script: etc etc } ``` You will have to import "golang.org/x/net/html/atom".
https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:29: if !unicode.IsDigit(r) { On 2017/05/04 23:44:23, nigeltao1 wrote: > I'd just look for ASCII digits. It's not like you're expecting anything else in > the http://www.fileformat.info/info/unicode/category/Nd/list.htm list. > > if r < '0' || '9' < r { etc } Done. https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:72: w io.Writer On 2017/05/04 23:44:23, nigeltao1 wrote: > If you care enough about efficiency (both in terms of string to []byte > conversions, and doing one big vs many small writes to disk or network), it > might be worth changing this type to: > > sw stringWriter > > and outside: > > type stringWriter interface { > WriteString(string) (int, error) > } > > and in func Sanitize: > > sw, ok, := w.(stringWriter) > if !ok { > bw := bufio.NewWriter(w) > defer func(){ > ferr := bw.Flush() > if err == nil { > err = ferr > } > }() > sw = bw > } > s := sanitizer{sw: sw} > etc > > and change the io.WriteString call in sanitizer.p. Done. https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:103: // If allowedAttrs is nil, all attributes are ommitted. On 2017/05/04 23:44:23, nigeltao1 wrote: > Typo in "omitted". Done. https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:132: switch strings.ToLower(n.Data) { On 2017/05/04 23:44:23, nigeltao1 wrote: > The ToLower'ing is unnecessary if you compare atoms (uint32 values) instead of > strings. uint32 comparison is also faster. > > ``` > switch n.Atom { > case atom.Br: > etc > > case atom.Script: > etc > > etc > } > ``` > > > You will have to import "golang.org/x/net/html/atom". nice, thanks, done
On 2017/05/04 22:11:01, nodir wrote: > I've rewrote this thing. The map of elements was nice, but it leaked and we had > to have some special cases in Sanitize() so it did not really represent the > logic entirely, i.e. the logic was implemented in two places. So, I've replaced > it with a switch statement. PTAL I have to say I found the table-driven approach easier to review, even with a few special cases. Since the subset of HTML being accepted is so small, it probably doesn't make that much of a difference. But if you need to add additional elements/attributes in the future, the switch-statement approach might get kind of unwieldy. (not suggesting to change it back now; it's fine -- just something to keep in mind).
https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:40: return false, "#invalid-url-stripped" It would be preferable to use about:invalid#reason, so that it's an absolute URL. As is, it's a URL for an anchor in the current document. https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:62: "rowspan": sanitizeNum, If you want to remove a little code, feel free to change rowspan/colspan to alwaysSafe. Their values are supposed to be numbers, but there are no known nor imaginable security issues if they're not (as long as the value is correctly quoted and escaped, which will be the case here). The browser will just itself safely ignore them. https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:199: // harmful and advised to be logged. There are many other ways HTML can execute script which you don't flag (<object> elements that instantiate flash, onerror/onclick/etc handlers, etc), so this is of limited usefulness. If you want to indeed log potentially dodgy HTML, it might be easier to have the caller match against a regex of common exploit patterns. But I'm not sure this is even necessary.
I agree that table was easier to review/comprehend. However I felt that, although individually it is simple, overall the sanitization logic was in two places, which is complex. https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:40: return false, "#invalid-url-stripped" On 2017/05/05 15:29:16, xtof wrote: > It would be preferable to use about:invalid#reason, so that it's an absolute > URL. As is, it's a URL for an anchor in the current document. Done. I was doing what Gitiles is doing: https://gerrit.googlesource.com/gitiles/+/c4cfe00b525228f6ddcd18eb8db76ef5b4a... https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:62: "rowspan": sanitizeNum, On 2017/05/05 15:29:16, xtof wrote: > If you want to remove a little code, feel free to change rowspan/colspan to > alwaysSafe. Their values are supposed to be numbers, but there are no known nor > imaginable security issues if they're not (as long as the value is correctly > quoted and escaped, which will be the case here). The browser will just itself > safely ignore them. Done. I am always for deleting code. https://codereview.chromium.org/2849353002/diff/80001/common/data/text/saniti... common/data/text/sanitizehtml/sanitize.go:199: // harmful and advised to be logged. On 2017/05/05 15:29:16, xtof wrote: > There are many other ways HTML can execute script which you don't flag (<object> > elements that instantiate flash, onerror/onclick/etc handlers, etc), so this is > of limited usefulness. If you want to indeed log potentially dodgy HTML, it > might be easier to have the caller match against a regex of common exploit > patterns. But I'm not sure this is even necessary. Removed
https://codereview.chromium.org/2849353002/diff/120001/common/data/text/sanit... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/120001/common/data/text/sanit... common/data/text/sanitizehtml/sanitize.go:31: invalidityReason = err.Error() err.Error() can contain arbitrary unsanitized content, possibly containing snippets of the attacker-supplied s string. I'm not sure if this is safe. xtof? https://codereview.chromium.org/2849353002/diff/120001/common/data/text/sanit... common/data/text/sanitizehtml/sanitize.go:100: tag := atom.Atom(safeElement.DataAtom).String() The atom.Atom is unnecessary. tag := safeElement.DataAtom.String()
https://codereview.chromium.org/2849353002/diff/120001/common/data/text/sanit... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/120001/common/data/text/sanit... common/data/text/sanitizehtml/sanitize.go:31: invalidityReason = err.Error() On 2017/05/06 01:03:31, nigeltao1 wrote: > err.Error() can contain arbitrary unsanitized content, possibly containing > snippets of the attacker-supplied s string. I'm not sure if this is safe. xtof? i see now that xtof did not necessarily imply that "#reason" must be the reason from err. I've replaced it with a const https://codereview.chromium.org/2849353002/diff/120001/common/data/text/sanit... common/data/text/sanitizehtml/sanitize.go:100: tag := atom.Atom(safeElement.DataAtom).String() On 2017/05/06 01:03:31, nigeltao1 wrote: > The atom.Atom is unnecessary. > > tag := safeElement.DataAtom.String() oops, for some reason i thought DataAtom is int
On 2017/05/06 01:40:47, nodir wrote: > https://codereview.chromium.org/2849353002/diff/120001/common/data/text/sanit... > File common/data/text/sanitizehtml/sanitize.go (right): > > https://codereview.chromium.org/2849353002/diff/120001/common/data/text/sanit... > common/data/text/sanitizehtml/sanitize.go:31: invalidityReason = err.Error() > On 2017/05/06 01:03:31, nigeltao1 wrote: > > err.Error() can contain arbitrary unsanitized content, possibly containing > > snippets of the attacker-supplied s string. I'm not sure if this is safe. > xtof? It should have been safe - once a browser has parsed the about:invalid# prefix, there really is no conceivable way anything in the fragment could cause trouble (as long as it's HTML escaped when embedded in the attribute). If you wanted to be double sure, you could %-encode the string. But there's probably no need to include that much detail; a constant message should be sufficient. > > i see now that xtof did not necessarily imply that "#reason" must be the reason > from err. I've replaced it with a const > > https://codereview.chromium.org/2849353002/diff/120001/common/data/text/sanit... > common/data/text/sanitizehtml/sanitize.go:100: tag := > atom.Atom(safeElement.DataAtom).String() > On 2017/05/06 01:03:31, nigeltao1 wrote: > > The atom.Atom is unnecessary. > > > > tag := safeElement.DataAtom.String() > > oops, for some reason i thought DataAtom is int
lgtm https://codereview.chromium.org/2849353002/diff/160001/common/data/text/sanit... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/160001/common/data/text/sanit... common/data/text/sanitizehtml/sanitize.go:31: invalidityReason = "url-is-malformed" Nit: After the transformation, the URL is no longer malformed. Maybe just remove the "is"? about:invalid#malformed-url about:invalid#disallowed-scheme Or you could be more verbose about:invalid#sanitized&reason=malformed-url https://codereview.chromium.org/2849353002/diff/160001/common/data/text/sanit... common/data/text/sanitizehtml/sanitize.go:34: invalidityReason = "url-is-not-http-or-https" Instead of the indirection via invalidityReason, it might be easier to read to just return directly (perhaps with a const to avoid typos) return aboutInvalid + "disallowed-scheme"
lgtm https://codereview.chromium.org/2849353002/diff/160001/common/data/text/sanit... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/160001/common/data/text/sanit... common/data/text/sanitizehtml/sanitize.go:185: func Sanitize(r io.Reader, w io.Writer) (err error) { The general Go style is destination args before source args. For example, the io.Copy function is Copy(Writer, Reader), not Copy(Reader, Writer).
Patchset #11 (id:200001) has been deleted
The CQ bit was checked by nodir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nigeltao@chromium.org, xtof@google.com Link to the patchset: https://codereview.chromium.org/2849353002/#ps220001 (title: "fix comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
nigeltao, xtof, thank you so much for review! https://codereview.chromium.org/2849353002/diff/160001/common/data/text/sanit... File common/data/text/sanitizehtml/sanitize.go (right): https://codereview.chromium.org/2849353002/diff/160001/common/data/text/sanit... common/data/text/sanitizehtml/sanitize.go:31: invalidityReason = "url-is-malformed" On 2017/05/09 16:06:21, xtof wrote: > Nit: After the transformation, the URL is no longer malformed. Maybe just > remove the "is"? > about:invalid#malformed-url > about:invalid#disallowed-scheme > > Or you could be more verbose > about:invalid#sanitized&reason=malformed-url Done. https://codereview.chromium.org/2849353002/diff/160001/common/data/text/sanit... common/data/text/sanitizehtml/sanitize.go:34: invalidityReason = "url-is-not-http-or-https" On 2017/05/09 16:06:21, xtof wrote: > Instead of the indirection via invalidityReason, it might be easier to read to > just return directly (perhaps with a const to avoid typos) > return aboutInvalid + "disallowed-scheme" yeah, i like this more. done https://codereview.chromium.org/2849353002/diff/160001/common/data/text/sanit... common/data/text/sanitizehtml/sanitize.go:185: func Sanitize(r io.Reader, w io.Writer) (err error) { On 2017/05/10 02:27:20, nigeltao1 wrote: > The general Go style is destination args before source args. For example, the > io.Copy function is Copy(Writer, Reader), not Copy(Reader, Writer). Done.
On 2017/05/10 06:48:19, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started once the patch has received an L-G-T-M from a full > committer. > Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a > full super star committer. > Committers are members of the group "project-infra-committers". > Note that this has nothing to do with OWNERS files. Vadim, I still need your approval
lgtm
The CQ bit was checked by nodir@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494436308935930,
"parent_rev": "172994b061ceb23138f51c0a4f937e979b51a895", "commit_rev":
"e3b4c50fa889a3825db10a0eb87adf7bc0807cb3"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://github.com/luci/luci-go/commit/e3b4c50fa889a3825db10a0eb87adf7bc0807cb3 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://github.com/luci/luci-go/commit/e3b4c50fa889a3825db10a0eb87adf7bc0807cb3 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
