Bug 204954 – Web Inspector: non-regex Local Overrides and Script Blackboxing shouldn't apply to scripts that just contain the URL

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

`Inspector::ContentSearchUtilities::createSearchRegex` doesn't add `^` and `$` to the beginning and end of the search string, meaning that the `InspectorNetworkAgent` would think that it should intercept <https://webkit.org/example> if it was given a non-regex Local Override for <https://webkit.org/>, since <https://webkit.org/example> _contains_ the string "https://webkit.org/".

[Devin Rousso]

Created attachment 385048 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 385048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385048&action=review

> Source/WebCore/ChangeLog:10
> +        If `isRegex` is false, add `^` and `$` to the beginning and end of the search string to
> +        ensure that the search string is exactly matched, not just contained within the potentially
> +        intercepted URL.

Isn't this incorrect if there are any special characters?

For example if my URL was:

    "http://example.com/(test"
    "http://example.com/$/test"

I'd expect each of these to match potential URLs not treated like regexes. However it seems (1) would be an invalid regex and (2) would behave different if treated like a regex.

[Devin Rousso]

Comment on attachment 385048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385048&action=review

>> Source/WebCore/ChangeLog:10
>> +        intercepted URL.
> 
> Isn't this incorrect if there are any special characters?
> 
> For example if my URL was:
> 
>     "http://example.com/(test"
>     "http://example.com/$/test"
> 
> I'd expect each of these to match potential URLs not treated like regexes. However it seems (1) would be an invalid regex and (2) would behave different if treated like a regex.

Both `Inspector::ContentSearchUtilities::createSearchRegex` and `Inspector::ContentSearchUtilities::createMatchRegex` escape the query string if the `isRegex` parameter is false (they add "\\" before any of "[](){}+-*.,?\\^$|").  In the case that it is true, we don't add the `^` and `$` to the query string, and let it wholly be treated as a regex.

See how `createSearchRegexSource` is used in 'Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp'.

[Joseph Pecoraro]

Comment on attachment 385048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385048&action=review

> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:139
> +    return RegularExpression { isRegex ? query : makeString("^"_s, createSearchRegexSource(query), "$"_s), caseSensitive ? TextCaseSensitive : TextCaseInsensitive };

A `makeString` shouldn't require WTFStrings for single characters. It likely could just be:

    makeString('^', ..., '$')

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:858
>      for (auto& intercept : m_intercepts) {
> -        auto regex = ContentSearchUtilities::createSearchRegex(intercept.url, intercept.caseSensitive, intercept.isRegex);
> +        auto regex = ContentSearchUtilities::createMatchRegex(intercept.url, intercept.caseSensitive, intercept.isRegex);
>          if (regex.match(urlString) != -1)
>              return true;
>      }

I'd expect to see:

    if (intercept.isRegex) {
        ... regex ...
    } else {
        ... string compare ...
    }

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 385048 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385048&action=review
> 
> >> Source/WebCore/ChangeLog:10
> >> +        intercepted URL.
> > 
> > Isn't this incorrect if there are any special characters?
> > 
> > For example if my URL was:
> > 
> >     "http://example.com/(test"
> >     "http://example.com/$/test"
> > 
> > I'd expect each of these to match potential URLs not treated like regexes. However it seems (1) would be an invalid regex and (2) would behave different if treated like a regex.
> 
> Both `Inspector::ContentSearchUtilities::createSearchRegex` and
> `Inspector::ContentSearchUtilities::createMatchRegex` escape the query
> string if the `isRegex` parameter is false (they add "\\" before any of
> "[](){}+-*.,?\\^$|").  In the case that it is true, we don't add the `^` and
> `$` to the query string, and let it wholly be treated as a regex.
> 
> See how `createSearchRegexSource` is used in
> 'Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp'.

Ugh, gross, and very non-obvious.

[Joseph Pecoraro]

Comment on attachment 385048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385048&action=review

> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:137
> +RegularExpression createMatchRegex(const String& query, bool caseSensitive, bool isRegex)

Maybe name this `createExactMatchRegex`?

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:858
>>      }
> 
> I'd expect to see:
> 
>     if (intercept.isRegex) {
>         ... regex ...
>     } else {
>         ... string compare ...
>     }

I see you answer this because `ContentSearchUtilities` is clever.

[Devin Rousso]

Comment on attachment 385048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385048&action=review

>> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:137
>> +RegularExpression createMatchRegex(const String& query, bool caseSensitive, bool isRegex)
> 
> Maybe name this `createExactMatchRegex`?

In my mind, "exact match" implies that the resulting regex would match against an _entire_ string, which isn't really accurate for when `isRegex` is true (e.g. `https\:\/\/webkit\.org\/` would match every response to any URL that contained "https://webkit.org/", even if the URL was something like "https://example.org/?redirect=https://webkit.org/?params=foobarbaz").

>> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:139
>> +    return RegularExpression { isRegex ? query : makeString("^"_s, createSearchRegexSource(query), "$"_s), caseSensitive ? TextCaseSensitive : TextCaseInsensitive };
> 
> A `makeString` shouldn't require WTFStrings for single characters. It likely could just be:
> 
>     makeString('^', ..., '$')

What is the usual style for this.  I'd though to use `ASCIILiteral` for all ASCII strings, regardless of the number of characters, but perhaps that is unnecessary.  I'm fine either way.  I'll change this.

[Devin Rousso]

Created attachment 385050 [details]
Patch

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 385048 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385048&action=review
> 
> >> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:139
> >> +    return RegularExpression { isRegex ? query : makeString("^"_s, createSearchRegexSource(query), "$"_s), caseSensitive ? TextCaseSensitive : TextCaseInsensitive };
> > 
> > A `makeString` shouldn't require WTFStrings for single characters. It likely could just be:
> > 
> >     makeString('^', ..., '$')
> 
> What is the usual style for this.  I'd though to use `ASCIILiteral` for all
> ASCII strings, regardless of the number of characters, but perhaps that is
> unnecessary.  I'm fine either way.  I'll change this.

`makeString` is constructing a single WTFString by concatenating components. See StringConcatenate.h. Therefore the simplest components that you can concatenate together should cause the most efficient construction.

Lets compare:

    1. makeString(char, WTFString, char)
    2. makeString(ASCIILiteral, WTFString, ASCIILiteral)

(1) is likely to be simpler then (2) because its components are simpler.

Note that in StringConcatenate.h that an ASCIILiteral has a special case. This means an ASCIILiteral like "foo"_s should be treated exactly the same as char* "foo". As it should be, because why convert to a WTFString and back to characters to concatenate! But a char literal ('f') is simpler than a char* literal ("f"), it avoids a potential strlen(...) and StringImpl::copyCharacters call, both of which are unnecessary complexity for a single character.

In general, operations (searching, comparing, concatenating) are almost always simpler / more efficient to use a character instead of a char*/ASCIILiteral/WTFString.

[Joseph Pecoraro]

Comment on attachment 385050 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385050&action=review

> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:139
> +    return RegularExpression { isRegex ? query : makeString('^', createSearchRegexSource(query), '$'), caseSensitive ? TextCaseSensitive : TextCaseInsensitive };

A better name for `createSearchRegexSource` seems like `escapeRegex` or something.

I find these functions confusing. "createFooRegex" which takes an `isRegex` flag seems counter-intuitive.

Perhaps this could be better: `createRegularExpressionForSearchString(const String& searchString, bool caseSensitive, SearchStringType type)` where `enum SearchStringType { Regex, ExactString, ContainsString }`.

Maybe I'm over thinking it but it seems very error prone to read this function.

[Devin Rousso]

Comment on attachment 385050 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385050&action=review

>> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:139
>> +    return RegularExpression { isRegex ? query : makeString('^', createSearchRegexSource(query), '$'), caseSensitive ? TextCaseSensitive : TextCaseInsensitive };
> 
> A better name for `createSearchRegexSource` seems like `escapeRegex` or something.
> 
> I find these functions confusing. "createFooRegex" which takes an `isRegex` flag seems counter-intuitive.
> 
> Perhaps this could be better: `createRegularExpressionForSearchString(const String& searchString, bool caseSensitive, SearchStringType type)` where `enum SearchStringType { Regex, ExactString, ContainsString }`.
> 
> Maybe I'm over thinking it but it seems very error prone to read this function.

I like this!  I'll take a stab at it :)

[Devin Rousso]

Created attachment 385080 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 385080 [details]:

webgl/1.0.3/conformance/ogles/GL/mat/mat_025_to_032.html bug 204981 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 385080 [details]
Patch

Clearing flags on attachment: 385080

Committed r253246: <https://trac.webkit.org/changeset/253246>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57725755>