RESOLVED FIXED201025
results.webkit.org: Escape html in changelog 
https://bugs.webkit.org/show_bug.cgi?id=201025
Summary results.webkit.org: Escape html in changelog
Jonathan Bedard
Reported 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.
Attachments
Patch (3.23 KB, patch)
2019-08-21 20:52 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (3.57 KB, patch)
2019-08-21 22:00 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (5.69 KB, patch)
2019-08-22 10:11 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (2.82 KB, patch)
2019-08-22 10:39 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (3.05 KB, patch)
2019-08-22 11:17 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (2.91 KB, patch)
2019-08-23 11:37 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Jonathan Bedard
Comment 1 2019-08-21 20:45:39 PDT
<rdar://problem/54564837>
Jonathan Bedard
Comment 2 2019-08-21 20:52:00 PDT
Created attachment 376974 [details] Patch
Jonathan Bedard
Comment 3 2019-08-21 22:00:15 PDT
Created attachment 376982 [details] Patch
Darin Adler
Comment 4 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?
Jonathan Bedard
Comment 5 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.
Darin Adler
Comment 6 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.
Jonathan Bedard
Comment 7 2019-08-22 10:11:36 PDT
Created attachment 377022 [details] Patch
Jonathan Bedard
Comment 8 2019-08-22 10:39:00 PDT
Created attachment 377025 [details] Patch
Jonathan Bedard
Comment 9 2019-08-22 11:17:09 PDT
Created attachment 377028 [details] Patch
Jonathan Bedard
Comment 10 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.
Darin Adler
Comment 11 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 ''.
Jonathan Bedard
Comment 12 2019-08-23 11:37:39 PDT
Created attachment 377140 [details] Patch
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2019-08-23 12:25:39 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.