Bug 201025

Summary: results.webkit.org: Escape html in changelog
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: Tools / TestsAssignee: Jonathan Bedard <jbedard>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, commit-queue, darin, ews-watchlist, glenn, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jonathan Bedard 2019-08-21 20:45:21 PDT 
The commit endpoint doesn't properly escape the changelog, which is an issue because the changelog may contain html.
Comment 1 Jonathan Bedard 2019-08-21 20:45:39 PDT 
<rdar://problem/54564837>
Comment 2 Jonathan Bedard 2019-08-21 20:52:00 PDT 
Created attachment 376974 [details]
Patch
Comment 3 Jonathan Bedard 2019-08-21 22:00:15 PDT 
Created attachment 376982 [details]
Patch
Comment 4 Darin Adler 2019-08-22 09:19:36 PDT 
Comment on attachment 376982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376982&action=review

> Tools/resultsdbpy/resultsdbpy/view/static/js/common.js:150
> +            return String.fromCharCode(encoded);

Doesn’t look to me like this line of code will work. What kinds of escape sequences do we expect this to work for?
Comment 5 Jonathan Bedard 2019-08-22 10:02:17 PDT 
Comment on attachment 376982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376982&action=review

>> Tools/resultsdbpy/resultsdbpy/view/static/js/common.js:150
>> +            return String.fromCharCode(encoded);
> 
> Doesn’t look to me like this line of code will work. What kinds of escape sequences do we expect this to work for?

I think maybe this function should be moved into commit.html or renamed 'unescapeJinjaString'

When we don't send string through the 'safe' filter in jinja (what we're using in commit.html), we get something like this:
"<html></html>" becomes "&lt;html&gt;&lt;/html&gt;"

We basically want to decode that.
Comment 6 Darin Adler 2019-08-22 10:04:22 PDT 
Comment on attachment 376982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376982&action=review

>>> Tools/resultsdbpy/resultsdbpy/view/static/js/common.js:150
>>> +            return String.fromCharCode(encoded);
>> 
>> Doesn’t look to me like this line of code will work. What kinds of escape sequences do we expect this to work for?
> 
> I think maybe this function should be moved into commit.html or renamed 'unescapeJinjaString'
> 
> When we don't send string through the 'safe' filter in jinja (what we're using in commit.html), we get something like this:
> "<html></html>" becomes "&lt;html&gt;&lt;/html&gt;"
> 
> We basically want to decode that.

Right, but I’m talking about the String.fromCharCode part of this function. Not the gt/lt part.
Comment 7 Jonathan Bedard 2019-08-22 10:11:36 PDT 
Created attachment 377022 [details]
Patch
Comment 8 Jonathan Bedard 2019-08-22 10:39:00 PDT 
Created attachment 377025 [details]
Patch
Comment 9 Jonathan Bedard 2019-08-22 11:17:09 PDT 
Created attachment 377028 [details]
Patch
Comment 10 Jonathan Bedard 2019-08-22 11:21:34 PDT 
(In reply to Darin Adler from comment #6)
> Comment on attachment 376982 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376982&action=review
> 
> >>> Tools/resultsdbpy/resultsdbpy/view/static/js/common.js:150
> >>> +            return String.fromCharCode(encoded);
> >> 
> >> Doesn’t look to me like this line of code will work. What kinds of escape sequences do we expect this to work for?
> > 
> > I think maybe this function should be moved into commit.html or renamed 'unescapeJinjaString'
> > 
> > When we don't send string through the 'safe' filter in jinja (what we're using in commit.html), we get something like this:
> > "<html></html>" becomes "&lt;html&gt;&lt;/html&gt;"
> > 
> > We basically want to decode that.
> 
> Right, but I’m talking about the String.fromCharCode part of this function.
> Not the gt/lt part.

You were correct, =that didn't work because Jinja was encoding those string like this: '&#34'.

We also had another problem where a ` in the changelog would have created problems. I tested with a string like this: '\n\'`"&<>', which basically contains all the characters that would be problems.
Comment 11 Darin Adler 2019-08-22 16:57:27 PDT 
Comment on attachment 377028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377028&action=review

While I’m not an expert at this, I’ll review since I spotted a bug earlier.

> Tools/resultsdbpy/resultsdbpy/view/templates/commit.html:64
> +                return '';

Maybe this should return match instead of ''.
Comment 12 Jonathan Bedard 2019-08-23 11:37:39 PDT 
Created attachment 377140 [details]
Patch
Comment 13 WebKit Commit Bot 2019-08-23 12:25:38 PDT 
Comment on attachment 377140 [details]
Patch

Clearing flags on attachment: 377140

Committed r249061: <https://trac.webkit.org/changeset/249061>
Comment 14 WebKit Commit Bot 2019-08-23 12:25:39 PDT 
All reviewed patches have been landed.  Closing bug.