Bug 192102 – Web Inspector: Audit: add "explanation" result value support

• Status: NEW
• Component: Web Inspector

[Devin Rousso]

This string can be used by developers to provide a summary (*cough* explanation *cough*) as to why the result level was chosen.

[Devin Rousso]

Created attachment 355918 [details]
Patch

[EWS Watchlist]

Comment on attachment 355918 [details]
Patch

Attachment 355918 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10187711

New failing tests:
inspector/audit/data-domNodes.html
inspector/audit/basic.html

[EWS Watchlist]

Created attachment 355933 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 355918 [details]
Patch

Attachment 355918 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10187735

New failing tests:
inspector/audit/data-domNodes.html
inspector/audit/basic.html

[EWS Watchlist]

Created attachment 355938 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 355939 [details]
Patch

[Devin Rousso]

Created attachment 355944 [details]
Patch

[EWS Watchlist]

Comment on attachment 355944 [details]
Patch

Attachment 355944 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10189804

New failing tests:
inspector/audit/data-explanation.html

[EWS Watchlist]

Created attachment 355952 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 355957 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/46423563>

[Joseph Pecoraro]

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.

[Devin Rousso]

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 '\').

[Devin Rousso]

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`).