Bug 201025

Summary:

results.webkit.org: Escape html in changelog

Product:

WebKit

Reporter:

Jonathan Bedard <jbedard>

Component:

Tools / Tests

Assignee:

Jonathan Bedard <jbedard>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aakash_jain, commit-queue, darin, ews-watchlist, glenn, webkit-bug-importer

Priority:

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

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	Details Formatted Diff Diff
Patch (3.57 KB, patch) 2019-08-21 22:00 PDT, Jonathan Bedard	no flags	Details Formatted Diff Diff
Patch (5.69 KB, patch) 2019-08-22 10:11 PDT, Jonathan Bedard	no flags	Details Formatted Diff Diff
Patch (2.82 KB, patch) 2019-08-22 10:39 PDT, Jonathan Bedard	no flags	Details Formatted Diff Diff
Patch (3.05 KB, patch) 2019-08-22 11:17 PDT, Jonathan Bedard	no flags	Details Formatted Diff Diff
Patch (2.91 KB, patch) 2019-08-23 11:37 PDT, Jonathan Bedard	no flags	Details Formatted Diff Diff
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 "<html></html>" 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 "<html></html>" > > 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 "<html></html>" > > > > 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.