The commit endpoint doesn't properly escape the changelog, which is an issue because the changelog may contain html.
<rdar://problem/54564837>
Created attachment 376974 [details] Patch
Created attachment 376982 [details] Patch
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 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.
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.
Created attachment 377022 [details] Patch
Created attachment 377025 [details] Patch
Created attachment 377028 [details] Patch
(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: '"'. 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 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 ''.
Created attachment 377140 [details] Patch
Comment on attachment 377140 [details] Patch Clearing flags on attachment: 377140 Committed r249061: <https://trac.webkit.org/changeset/249061>
All reviewed patches have been landed. Closing bug.