$_GET['q'] not in UTF-8 leading to warning on 404 - htmlspecialchars(): Invalid multibyte sequence in argument in check_plain()

the same problem here..

I can confirm that there are still issues related to this on a fresh Drupal 7.16 installation.

When I try to access /id-governor%C2

I get the following warning

Warning: htmlspecialchars(): Invalid multibyte sequence in argument in check_plain() (line 1572 of /home/user/public_html/drupal-7.16/includes/bootstrap.inc).

I have more information about this.

When a 404 error occurs $_GET['q'] is still in iso8859-1 and when check_plain is called with $_GET['q'] as a parameter the htmlspecialchars($text, ENT_QUOTES, 'UTF-8') function inside check_plain raises this warning because the passed string is not UTF-8.

Passing $_GET['q'] via drupal_convert_to_utf8 before the call to check_plain fixes the problem but I am not sure if that is the right solution.

For regular page requests that does not result in 404 errors what encoding is $_GET['q'] in D7? UTF-8? If so shouldn't $_GET['q'] be in UTF-8 as well when there is a 404? If not, then the above fix to pass $_GET['q'] via drupal_convert_to_utf8 before calling check_plain should fix the issue.

This is a classic IIS bug. By default, IIS converts the encoding of the headers before passing them to the FastCGI handler. You need some trickery to make it behave appropriately.

Not sure if this is an IIS issue. I had tested this issue on a fresh D7.16 install on Apache2 on Ubuntu 11.04. The issue was tested in Firefox 14.0.1

I have more information on this. I added the following before the call to the check_plain for the $_GET['q'] in MENU_NOT_FOUND case in drupal_deliver_html_page in common.in

$var = $_GET['q'];
for($i = 0; $i < strlen($var); $i++) {
   echo ord($var[$i]) . "-" . $var[$i] . "<br />";
}

$var = $_SERVER['REQUEST_URI'];
for($i = 0; $i < strlen($var); $i++) {
   echo ord($var[$i]) . "-" . $var[$i] . "<br />";
}

echo ":". htmlspecialchars($_GET['q']) . ":<br />";
echo ":". htmlspecialchars($_SERVER['REQUEST_URI']) . ":<br />";
exit;

This is what I get

105-i
100-d
45--
103-g
111-o
118-v
101-e
114-r
110-n
111-o
114-r
194-�

...
47-/
105-i
100-d
45--
103-g
111-o
118-v
101-e
114-r
110-n
111-o
114-r
37-%
67-C
50-2

:id-governor�:
:/~user/drupal7/id-governor%C2:

So in $_GET['q'] the url_unencoded character (chr(194)) is present whereas in $_SERVER['REQUEST_URI'] the original string typed into the browser address bar is present.

%C2 isn't valid UTF-8 as far as I know. This patch attempts to identify invalid unicode characters and fix them where it can. This propogates to fix the database errors (since it will then be valid).

This second patch is applied against D8-dev. The previous patch is the D7-dev backport.

Regarding the CP1252 conversion, the following URLs provide some details.
http://en.wikipedia.org/wiki/UTF-8
http://www.php.net/manual/en/function.iconv.php
http://www.linuxquestions.org/questions/linux-software-2/iconv-from-utf-...
They notes a similar encoding so UTF-8 cannot read it without a conversion. CP1252 is used in some Windows file encodings.

This patch is based on GIGO, so only if the URL is otherwise unreadable does it attempt to convert it from this.

This latest patch only contains a spelling correct, no code change.

To add a note for later:

<Fabianx> superspring: I'd link to these sites from the issue summary and also give a little more explanation of the why there and then mark it RTBC.

I don't think we need this comment, the code is self-documenting enough. Quick re-roll to remove?

+    // Oh noes! Invalid UTF-8. Now we fix it.

Same as above but with the #16 comment change.

" [...] immediately die if the incoming path is not utf-8. Do not match it, do not do anything, just [400] out."

The patch has been modifed to, instead of repairing the string, return a 404 response.

The only possible concern I have with this is performance. I think drupal_normal_path is called quite a lot during a normal drupal requeust, but this should really be only done once for the request path from symfony - if I see this correctly.

It is great work, thanks for the patch, just thinking about this ...

Same patch as before but caching has been added to the function.

Same patch as above but using caching more efficiently.

Hey @sah62,

Are you asking for a Drupal 7 version of this patch? If so I can write one after/if it gets into Drupal 8.

Hey @sah62,

I think the patch I first wrote for you is more what you're looking for.
The later patch just gives a 404 error instead of a 500 error.

sah62: It will be added to D7 only after it's added to D8 ... we always patch the newest version first, and then backport to previous major releases. This workflow makes sure we don't introduce regressions by patching D7 and then forgetting to patch D8.

+++ b/core/includes/path.inc
@@ -264,16 +266,31 @@ function drupal_get_path_alias($path = NULL, $langcode = NULL) {
+  if (isset($cache[$path]) && isset($cache[$path][$langcode])) {

The second isset() covers the first one.

This needs tests to be considered for inclusion in core.

(I came here from the nasty [#1906626] which should be fixed by this.)

This looks like the wrong place to do this. We need this check up the stack, where we build the current path.

Drupal 7.27 + PostgreSQL 8.4.21

When I try to access /id-governor%C2

I get the following

Warning: htmlspecialchars() [function.htmlspecialchars]: Invalid multibyte sequence in argument in check_plain() (line 1566 of /home/hosting/oooo/htdocs/includes/bootstrap.inc).
PDOException: in drupal_lookup_path() (line 176 of /home/hosting/oooo/htdocs/includes/path.inc).

PostgreSQL log:
postgres[9839]: [9-1] ERROR: invalid byte sequence for encoding "UTF8": 0xc220
postgres[9839]: [9-2] HINT: This error can also happen if the byte sequence does not match the encoding expected by the server, which is controlled by "client_encoding".

The patch eliminates the error on my site

Marking this for review so that the testbot can check the patch.

I tried to reproduce this bug in D8. This exact same error will no longer happen because the access log was removed from the statistics module in #1446956: Remove the accesslog from statistics. However I'm seeing errors in the logs:

Drupal\Core\Database\DatabaseExceptionWrapper:  in Drupal\Core\Path\AliasManager->writeCache() (line 150 of core/lib/Drupal/Core/Path/AliasManager. 

which is this line:

$this->cache->set($this->cacheKey, $path_lookups, REQUEST_TIME + $twenty_four_hours);

With #32 D7 on PHP 5.3.10 gives me

even though I've turned messages off (related info).

The bug is not present in D8.

I'm sorry, I lost track of this...

What you're saying is "if it's not valid UTF-8 then assume it's CP1252 (Windows Latin 1 / Western European) and force it to UTF-8". I suspect that this assumption is not correct.

Moreover, this page explains that not all code points in CP1252 can be converted to UTF-8.

I read RFC 3986 as saying roughly that anything that is not printable 7-bit ASCII must be percent-encoded UTF-8. If drupal_validate_utf8() returns FALSE, all bets are off and the best thing we can do is to generate a 404 error.

I may be wrong though...

I think anything that is not UTF-8 (which is a superset of US-ASCII) is more likely to be malicious than wrongly encoded and we should protect ourselves by aborting with a 404 error.

A browser that uses a non-standard encoding probably causes all sorts of grief to the user and is not likely to stay around for long.

IMHO this issue is just about avoiding the PHP warning/error, not about avoiding the 404.

(See the first note on https://secure.php.net/manual/en/function.mb-detect-encoding.php)

I think patch from #21 is more appropriate for this, but it is for D8.

I'm also getting bot traffic with these odd URLs causing PDOException errors. I would love to get #51 integrated into D7 core - is there anything I could do to help with the process?

Bot traffic is causing these constantly on my site as well. I'm going to apply the patch, but would love to see it incorporated into D7 core.