[webkit-reviews] review denied: [Bug 176911] Add an URL method to remove both query string and fragment identifier : [Attachment 320831] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied youenn fablet
<youennf at gmail.com>'s request for review:
Bug 176911: Add an URL method to remove both query string and fragment
identifier
https://bugs.webkit.org/show_bug.cgi?id=176911

Attachment 320831: Patch

https://bugs.webkit.org/attachment.cgi?id=320831&action=review




--- Comment #5 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 320831
  --> https://bugs.webkit.org/attachment.cgi?id=320831
Patch

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

> Source/WebCore/platform/URL.cpp:861
> -    *this = URLParser { makeString(StringView { m_string }.substring(0,
m_queryEnd), '#', identifier) }.result();
> +    m_string = makeString(StringView { m_string }.substring(0, m_queryEnd),
'#', identifier);

This is not safe.  We could put non-ASCII characters or control characters like
the bell character in the new identifier, and they would need to be
percent-encoded.  Just don't touch this function.
As long as you're adding tests, please add a test for this case.

> Source/WebCore/platform/URL.cpp:882
> +    if (!hasQuery()) {
> +	   removeFragmentIdentifier();
> +	   return;
> +    }

This is unnecessary.

> Source/WebCore/platform/URL.cpp:899
> +	   m_string = makeString(view.substring(0, m_pathEnd), "?", query,
view.substring(m_queryEnd));

Same thing here.  query might have some characters that need encoding.	Query
encoding is more complicated, and it includes more characters and some are
dependent on the scheme.
We do actually need to slightly change this function to be more spec-compliant,
but that should be a separate patch.  I think the problem was when we set the
query to the empty string, just a '?' or two, etc.