Bug 192102

Created attachment 355918 [details] Patch

Created attachment 355939 [details] Patch

Created attachment 355944 [details] Patch

Created attachment 355957 [details] Patch

<rdar://problem/46423563>

Comment on attachment 355957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355957&action=review r- just so that we can better test the quoting function in AuditTestCase.js > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:117 > + for (let i = 0; i < this._test.length; ++i) { > + let c = this._test[i]; > + > + let backslashes = 0; > + while (this._test[i - 1 - backslashes] === "\\") > + ++backslashes; 1. Its unclear to me exactly what this is trying to do and that is not covered in the ChangeLog or with comments. It should probably be in a well named function that can (and should) be tested. 2. This seems to affect any test, and just becomes apparent with explanation text. So it sounds separate from adding explanation text an could probably be broken out into its own patch, which would be great cause it could be testable separately. 3. This backtracking is potentially slow. If there are 10 in a row for example this code would cause ( N + ((N*(N+1)/2) ) 65 reads. Even though in practice this shouldn't be a problem this would be ingesting user data so we try to protect against a malicious user that just provides a lot of backslashes in a row. Also, the backtracking seems unnecessary. Is this substantially different then something like: let escaped = JSON.stringify(str); let test = escaped.substr(1, escaped.length - 2); I suspect it is (especially around template literals) but I just want to make sure. > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:130 > + const linkRegExp = /\[(.+?)\]\((.+?)\)/g; This is a poor-mans Markdown, right? We should have a comment and maybe consider using a markdown library eventually. > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:145 > + if (backslashes % 2 === 0) { Again with the backslashes, without a high level comment I don't know if this is actually doing what it wants to do, because this is complex.

Comment on attachment 355957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355957&action=review Considering the number of edge cases I can imagine would arise from escaping things, I think I might want to table this for a bit to rethink how it should be done. >> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:117 >> + ++backslashes; > > 1. Its unclear to me exactly what this is trying to do and that is not covered in the ChangeLog or with comments. It should probably be in a well named function that can (and should) be tested. > 2. This seems to affect any test, and just becomes apparent with explanation text. So it sounds separate from adding explanation text an could probably be broken out into its own patch, which would be great cause it could be testable separately. > 3. This backtracking is potentially slow. If there are 10 in a row for example this code would cause ( N + ((N*(N+1)/2) ) 65 reads. Even though in practice this shouldn't be a problem this would be ingesting user data so we try to protect against a malicious user that just provides a lot of backslashes in a row. Also, the backtracking seems unnecessary. > > Is this substantially different then something like: > > let escaped = JSON.stringify(str); > let test = escaped.substr(1, escaped.length - 2); > > I suspect it is (especially around template literals) but I just want to make sure. Due to the fact that we insert `_test` inside an `eval(`(${this._test})`)` when calling `Runtime.evaluate`, I wanted to make sure that all escaped characters remained in their escaped state after being eval'd (e.g. `eval('("foo\"bar")')` vs `eval('("foo\\\"bar")')`). As a side note, the `eval(${test})` should be rewritten as `eval(\`(${test})\`)` so that the test code is treated as a string, not actual code (e.g. someone could have their test code be `false); (function() { window.location = "bad";}` as a form of exploit). This was also addressed in <https://webkit.org/b/193149>. >> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:130 >> + const linkRegExp = /\[(.+?)\]\((.+?)\)/g; > > This is a poor-mans Markdown, right? We should have a comment and maybe consider using a markdown library eventually. Yes, very much so. It's just the link syntax from Markdown. >> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:145 >> + if (backslashes % 2 === 0) { > > Again with the backslashes, without a high level comment I don't know if this is actually doing what it wants to do, because this is complex. It's trying to determine whether the character is escaped or not. "\[" is escaped (odd number of '\'), while "\\[" is not (even number of '\').

Comment on attachment 355957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355957&action=review >>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:117 >>> + ++backslashes; >> >> 1. Its unclear to me exactly what this is trying to do and that is not covered in the ChangeLog or with comments. It should probably be in a well named function that can (and should) be tested. >> 2. This seems to affect any test, and just becomes apparent with explanation text. So it sounds separate from adding explanation text an could probably be broken out into its own patch, which would be great cause it could be testable separately. >> 3. This backtracking is potentially slow. If there are 10 in a row for example this code would cause ( N + ((N*(N+1)/2) ) 65 reads. Even though in practice this shouldn't be a problem this would be ingesting user data so we try to protect against a malicious user that just provides a lot of backslashes in a row. Also, the backtracking seems unnecessary. >> >> Is this substantially different then something like: >> >> let escaped = JSON.stringify(str); >> let test = escaped.substr(1, escaped.length - 2); >> >> I suspect it is (especially around template literals) but I just want to make sure. > > Due to the fact that we insert `_test` inside an `eval(`(${this._test})`)` when calling `Runtime.evaluate`, I wanted to make sure that all escaped characters remained in their escaped state after being eval'd (e.g. `eval('("foo\"bar")')` vs `eval('("foo\\\"bar")')`). > > As a side note, the `eval(${test})` should be rewritten as `eval(\`(${test})\`)` so that the test code is treated as a string, not actual code (e.g. someone could have their test code be `false); (function() { window.location = "bad";}` as a form of exploit). This was also addressed in <https://webkit.org/b/193149>. Something I forgot to mention: the code below only tries to add extra escapes for characters that would require an escape due to the characters around them (e.g. "a\"b" should add an extra escape to the middle \" since it's within a "-type group, whereas "a'b" wouldn't need to worry about that). This logic somewhat fails for ` however, in that ${...} code blocks can add other characters that would also need extra escaping (e.g. `a${"b\"c"}d`).