statistics.php can exit early and prevent output errors

So I'm wondering whether we should use some form of tokens to validate that the input is valid in any way?

Does anyone has an idea here how to tackle this.
My first thought would be an extra check to determine range of IDs. However this add's an extra query.

Let's try this. This is probably simplest approach to do, not sure it's the best.
Added two tests to demonstrate the problem.

Back to needs review.

+++ b/core/modules/statistics/statistics.php
@@ -22,7 +22,9 @@
+    "options" => array("min_range" => 0, "max_range" => 9999999999),

That's not the maximum size of int(10)... see https://dev.mysql.com/doc/refman/5.5/en/integer-types.html

The largest size is 4294967295.

Thanks!
Adjusted the patch.
I'm just not sure if it wouldn't be better to somehow define the value somewhere as a constant.

This should probably better start against 8.3.x. Sorry for the noise

+++ b/core/modules/statistics/src/Tests/StatisticsInvalidPostTest.php
@@ -0,0 +1,60 @@
+    $this->client = \Drupal::service('http_client_factory')
+      ->fromOptions(['config/curl' => [CURLOPT_TIMEOUT => 10]]);

Is there a reason we cannot just use the default guzzle client but rather have to setup custom options? I'm curious here.
Note: you can also use ['timeout' => ], guzzle maps that internally.

Nope no reason, but i'll try to refactor it. (won't be today however)
only reason now is copy paste from StatisticsAdminTest

Note to self, if refactoring this works, file followup for StatisticsAdminTest

Adjusted the client declaration in the new test and created #2785997: Clean up guzzle declaration in core tests to refactor the current tests.

1. +++ b/core/modules/statistics/src/Tests/StatisticsInvalidPostTest.php
   @@ -0,0 +1,60 @@
   +    $this->client = new Client(['timeout' => 10]);
   

   To be honest I don't get in the first place, why we need a timeout :)

2. +++ b/core/modules/statistics/src/Tests/StatisticsInvalidPostTest.php
   @@ -0,0 +1,60 @@
   +   */
   +  public function testInvalidPost() {
   +    global $base_url;
   +
   +    // Manually calling statistics.php.
   +    $nid = 'a string instead of an integer';
   +    $post = array('nid' => $nid);
   +    $stats_path = $base_url . '/' . drupal_get_path('module', 'statistics') . '/statistics.php';
   +    $this->client->post($stats_path, array('form_params' => $post));
   +
   +    // An id greater than int(10), the maximum nid database limit.
   +    $nid = 123456789012;
   +    $post = array('nid' => $nid);
   +    $stats_path = $base_url . '/' . drupal_get_path('module', 'statistics') . '/statistics.php';
   +    $this->client->post($stats_path, array('form_params' => $post));
   +  }
   +
   

   IMHO we need some kind of assertions ...

@dawehner thanks!

I thought about the assertions as well. Problem is i have no idea what to assert.
We're testing we're not getting a PDO exception.

i'll update the timeout, it's also in tehe other tests, thats why i added it here.

Well, we could check whether we have the expected node views recorded.

I'm gonna retest this one, strangely this works locally.

And in the meantime added the remarks from dawehner.

Seems the guzzle adjustments still fail here but work locally. The other two fails seems a problem in head.

Still no idea why the default guzzle instantiation fails, and the 'drupal' one works on the testbot. Both work locally.
Let's not hold this patch up on that? And fix that in #2785997: Clean up guzzle declaration in core tests ?
Adjusted back to drupal initialization.

Ok that was a wrong patch ;-)

Yeah, that's indeed the right solution. You can shortcut that by using \Drupal::httpClient();

So only thing left here, is adjust declaration in test from
$this->client = \Drupal::service('http_client_factory')->fromOptions();

to

$this->client = \Drupal::httpClient();

And adjusted this, i think this is ready now.

Forgot an unused use statement

+++ b/core/modules/statistics/src/Tests/StatisticsInvalidPostTest.php
@@ -0,0 +1,73 @@
+    global $base_url;
...
+    $stats_path = $base_url . '/' . drupal_get_path('module', 'statistics') . '/statistics.php';
+    $this->client->post($stats_path, array('form_params' => $post));
...
+    $stats_path = $base_url . '/' . drupal_get_path('module', 'statistics') . '/statistics.php';
+    $this->client->post($stats_path, array('form_params' => $post));

Can't we use $this->buildUrl() instead?

based on #38

Here is a patch against 8.4.x-dev branch with some coding standard changes and changes based on #38.

Thanks @stefank, great to see this coming back after 3 months, sorry to have to push it to needs work.

I wonder if we could do this with a kernel test, if not it should be a browser test and not a simpletest based test.

Sure, here is a browser test, not sure if its right though.

+++ b/core/modules/statistics/statistics.php
@@ -22,7 +22,12 @@
+  $nid = filter_input(INPUT_POST, 'nid', FILTER_VALIDATE_INT, [
+    'options' => [
+      'min_range' => 0,
+      'max_range' => 4294967295,
+    ],
+  ]);
   if ($nid) {

It would be nice to document why this validation is applied here. (Otherwise we get a DB exception)

Would something like that be sufficient?

 + // Checks if Posted nid is in range of valid integers and prevent PDOException.
  $nid = filter_input(INPUT_POST, 'nid', FILTER_VALIDATE_INT, [
    'options' => [
      'min_range' => 0,
      'max_range' => 4294967295,
    ],
  ]);

Nit: Checks if posted nid is in range of valid integers and prevent a PDOException.

Lowercase P, and an 'a' before PDOException. But look corrects according to me.

Cool, @mallezie thanks for that.

Sorry, somehow managed to upload the patch twice.

+++ b/core/modules/statistics/statistics.php
@@ -22,7 +22,13 @@
+  //Checks if posted nid is in range of valid integers and prevent a PDOException.

Need a space after //, and no more than 80 characters per line. So just drop PDOException. down to the next line.

Thanks @timmillwood. That is updated.

Looks good

Nice!

mysql> create table test (nid int(10) unsigned NOT NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;
Query OK, 0 rows affected (0.03 sec)

mysql> insert into test values (4294967296);
ERROR 1264 (22003): Out of range value for column 'nid' at row 1
mysql> insert into test values (4294967295);
Query OK, 1 row affected (0.00 sec)

mysql>

So the number chosen looks great...

But I have a couple of concerns. One this value is different on postgres :( Here's a similar test on a postrges db.

drupal8alt=> \d+ node
nid	integer	not null default nextval('node_nid_seq'::regclass)	plain
vid	bigint		plain
type	character varying(32)	not null	extended		The ID of the target entity.
uuid	character varying(128)	not null	extended
langcode	character varying(12)	not null	extended
drupal8alt=> CREATE TABLE test (nid integer NOT NULL);
drupal8alt=> insert into test values (4294967296);
ERROR:  integer out of range
drupal8alt=> insert into test values (4294967295);
ERROR:  integer out of range

And the other concern is that the test added should be fail if we update the node table to hold larger integers so we know we need to update statistics.php if we do that.

So thinking about this some more

This causes a PDOException and could be used as an attack vector if repeatedly called to try and take down a site.

How can this take down a site? I've looked at the original issue on security.drupal.org and there is no supporting evidence of this. Yes a user can generate an error by manipulating a URL - is that a problem? If it is then perhaps we need to catch exceptions here. However, I've confirmed that if you've got error logging set to not display error messages all you see a 500 with a message...:

The website encountered an unexpected error. Please try again later.
<br />

I'm not sure that we even need to fix this.

There is this usual argument that once you have a 500 your reverse proxy believes your site is down. I'm not sure at the moment whether this is per path or just for the entire domain. On the other hand this would probably just make the statistics to no longer appear.

In general I believe a 500 would cause less resources than a database insert.

There is this usual argument that once you have a 500 your reverse proxy believes your site is down.

If this were the case then a single 500 response on d.o would bring all of d.o down. I'm 99.9999% certain that the reverse proxy will not conclude that the entire site is down. (If it happens for GET requests on the frontpage, that's a different story.) I'm 80% certain that most reverse proxies only consider a site to be down when their requests to origin time out.

In general I believe a 500 would cause less resources than a database insert.

True. That would make this a scalability rather than a security improvement issue.

OTOH, what do we really gain from letting the DB to attempt this query? The whole point of this controller is to receive requests from anonymous users to track page views. It's totally possible to DDoS Drupal by sending millions of requests to this. Similarly, you can bypass Drupal's page cache by providing a random URL query string. These are not things Drupal can protect against, that's up to the layer above it.

So, I agree with @alexpott:

I'm not sure that we even need to fix this.

Oh, looks like @catch has a different experience — quoting him from #2:

I don't agree this is a DOS as such, but I have seen load balancers configured to take web servers out of rotation when they return enough 5** errors, and this would allow you to trigger that condition.

Let's get his thoughts on this, also because he created this issue.

If this were the case then a single 500 response on d.o would bring all of d.o down. I'm 99.9999% certain that the reverse proxy will not conclude that the entire site is down. (If it happens for GET requests on the frontpage, that's a different story.) I'm 80% certain that most reverse proxies only consider a site to be down when their requests to origin time out.

It is not an issue when you customize your settings. A better alternative for the uptime of your site is a dedicated script. Individual 500s then don't kill your site.

If we want to stop this page throwing 500s that might be reasonable. Whilst I'm not sure that the site being taken down is a good argument I think saying that a script like this should not present an error ever might be. Therefore catching exceptions is what we should do rather than trying to filter because we can't get that right for the different databases we support.

#61 sounds great!

First, rebasing.

This implements #61.

Do you mind adding back in the test?

Rerolled the patch against 8.6.x branch because its failed to apply.

added comments with changes

@vaplas Re: #67 - This used to be a server side request back in early releases of Drupal 7 and before, but it meant you couldn't use it with reverse proxy caching like Varnish, because only the first view after a cache clear would get counted. The AJAX request was put in to bypass the caching and count all views.

Re #68 @Yogesh Pawar

+++ b/core/modules/statistics/statistics.php
@@ -8,23 +8,28 @@
+  // Never pres

Incomplete comment

Re #69 @shardulagg

+++ b/core/modules/statistics/statistics.php
@@ -8,23 +8,36 @@
+try { // added try/catch block to handle PDOException ¶
...
+    ¶
+	  $nid = filter_input(INPUT_POST, 'nid', FILTER_VALIDATE_INT, [
+    	// range set so that value of nid does not goes out of bound and PDOException is not triggered
+		'options' => [
+						'min_range' => 0,
+      					'max_range' => 4294967295,
+    				 ],
+  	]);  ¶
+	  ¶
...
+  // do nothing ¶

Lots of odd white space added.
Missing capital letters and full stops / periods.
Comments longer than 80 characters.

Updated patch & interdiff as per comment #71

As suggested in #66, can we have the test back?

1. +++ b/core/modules/statistics/statistics.php
   @@ -8,23 +8,31 @@
   +  // Added try/catch block to handle PDOException.
   

   I think we can remove this comment.

2. +++ b/core/modules/statistics/statistics.php
   @@ -8,23 +8,31 @@
   +    $nid = filter_input(INPUT_POST, 'nid', FILTER_VALIDATE_INT, ['options' => ['min_range' => 0, 'max_range' => 4294967295]]);
   

   The options array looks like it can be removed?

This is bug and why test is lost? #66 said it as well

+++ b/core/modules/statistics/tests/src/Functional/StaitsticsInvalidPostTest.php
@@ -0,0 +1,40 @@
+class StatisticsInvalidPostTest extends StatisticsTestBase {

this one from #62

1. +++ b/core/modules/statistics/statistics.php
   @@ -8,23 +8,31 @@
   +    $nid = filter_input(INPUT_POST, 'nid', FILTER_VALIDATE_INT, ['options' => ['min_range' => 0, 'max_range' => 4294967295]]);
   

   better to expand options array to next lines

2. +++ b/core/modules/statistics/statistics.php
   @@ -8,23 +8,31 @@
   +  // Do nothing.
   

   better to tell why... do nothing

Adding removed test cases again. see #66
Fixed issues mentioned by @andypost
Need some more help in refining catch block description further.

Starting at #76 not sure why the test was added to the file module? The namespace even says statistics so I went ahead and moved that.

Updated phpcs errors and deprecated calls.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed failed commands on #86
Attached patch against Drupal 10.1.x

+++ b/core/modules/statistics/statistics.php
@@ -8,23 +8,38 @@
+    // Range set so that value of nid does not goes out of bound
+    // and PDOException is not triggered.
+    $nid = filter_input(INPUT_POST, 'nid', FILTER_VALIDATE_INT, [
+      'options' => [
+        'min_range' => 0,
+        'max_range' => 4294967295,
+      ],
+    ]);

In #61 we agreed to not validate the range. It's not possible to set a value for all DB types supported by core. The correct patch to be re-rolling is #68 - unfortunately #69 added this back in without explanation and after it was decided that filtering by number is incorrect.

Address #92 by removing extra range filtering which is database dependent and will need real controller to check further access (currently it using config setting) and re-titled

Interdiff vs #89 - we should not add theme to base test as child ones must care

Hope this kind of optimization fits

+++ b/core/modules/statistics/statistics.php
@@ -8,23 +8,34 @@
+  $request = Request::createFromGlobals();
+  $kernel = DrupalKernel::createFromRequest($request, $autoloader, 'prod');
...
-    $container->get('request_stack')->push(Request::createFromGlobals());
...
+    $container->get('request_stack')->push($request);

I see no reason to have 2 requests because of bug #2613044: Requests are pushed onto the request stack twice, popped once

Feel bad I was part of that carrying the bad approach forward.

Can we update the issue summary for the proposed solution and remaining tasks?

Updated to the last patch and #61

Thanks for the quick turnaround.

So reading the proposed solution compared to the patch I see it now.

Ran the test locally just to make sure it failed but it passes without the fix. So think they need to be tweaked.

Statistics is approved for removal. See #3266457: [Policy] Deprecate Statistics module in D10 and move to contrib in D11

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

It will be moved to a contributed Statistics project once the project is created and the Drupal 11 branch is open.

Reworded IS to account for changes since D8.

Pinging @catch since you were the original issue poster. @mallezie @stefank since you did the most patches before the split.

Daniel's suggestion in #5 is relevant in the "transitions tracking" part of the plan #3436823: Plan for Statistics 1.x.

Regarding the nid validation : the maximum check was removed for DB independence, but can we agree we should really keep the min_range => 1 check ? I do not think there is any case in which a nid can be zero or negative, is there ?

Although it will not be inserted because the column is marked UNSIGNED, applying the check in the filter will save a roundtrip to the DB in that case.

Merged 9 years later. Thanks everyone.