WordPress redirect to wp-admin/install.php when a user is rate limited in mysql

[anonymooo]

Wordpress redirect to wp-admin/install.php when a user is rate limited in mysql,

instead of catching and displaying a valid error message

Wordpress redirect to wp-admin/install.php when a user is rate limited in mysql,

instead of catching and displaying a valid error message

Trac ticket: 63678

[anonymooo]

WordPress redirect to wp-admin/install.php when a user is rate limited in MySQL.

What are the rate limit parameters in MySQL ?

A MySQL user can be created with a set of parameters to limit resources consumption:

• MAX_QUERIES_PER_HOUR
• MAX_UPDATES_PER_HOUR
• MAX_CONNECTIONS_PER_HOUR
• MAX_USER_CONNECTIONS

​https://dev.mysql.com/doc/refman/8.4/en/user-resources.html

Setting these values may help protect the database server under heavy load, help mitigate deny of service attacks.

How to reproduce ?

1. Set up a WordPress
2. As root in MySQL, run the following query: ALTER USER 'database_user'@'%' WITH MAX_QUERIES_PER_HOUR 1;
3. Refresh page: We are now redirected to the installation page

Once the number of query per hour for the user has been reduced (or MAX_QUERIES_PER_HOUR set to a higher value / disabled), WordPress is reachable again.

Expectations

As this is a documented return code from MySQL, WordPress should display the correct error instead of redirecting to the installation page.

Also, as we are hitting a rate-limiting value, we should not try to retry queries (thus making it harder to go below the limited value) but die on-spot.

[anonymooo]

Screenshot of new error page

WordPress redirect to wp-admin/install.php when a user is rate limited in MySQL.

### What are the rate limit parameters in MySQL ?
A MySQL user can be created with a set of parameters to limit resources consumption:

• MAX_QUERIES_PER_HOUR
• MAX_UPDATES_PER_HOUR
• MAX_CONNECTIONS_PER_HOUR
• MAX_USER_CONNECTIONS

​https://dev.mysql.com/doc/refman/8.4/en/user-resources.html

Setting these values may help protect the database server under heavy load, help mitigate deny of service attacks.

### How to reproduce ?

1. Set up a WordPress
2. As root in MySQL, run the following query: ALTER USER 'database_user'@'%' WITH MAX_QUERIES_PER_HOUR 1;
3. Refresh page: We are now redirected to the installation page

Once the number of query per hour for the user has been reduced (or MAX_QUERIES_PER_HOUR set to a higher value / disabled), WordPress is reachable again.

### Expectations
As this is a documented return code from MySQL, WordPress should display the correct error instead of redirecting to the installation page.

Also, as we are hitting a rate-limiting value, we should not try to retry queries (thus making it harder to go below the limited value) but die on-spot.

[anonymooo]

diff file

[anonymooo]

new diff

[jorbin]

Thanks @anonymooo.

While I fully agree that the experience here shouldn't be to redirect but to error out, I wonder if it should be more a more generic error so that 1) other error codes can also error out. Perhaps all the 1xxx? 2) If it should rely on $this->last_errror rather than creating a new string.

My thinking with using the string from mysql is that since the problem here is on the mysql server side, the error message provides a better starting place for people to search for a solution.

For anyone looking for the list of error codes, ​here are all the server ones and from there you can view the others as well.

[anonymooo]

Replying to jorbin:

Thanks @anonymooo.

While I fully agree that the experience here shouldn't be to redirect but to error out, I wonder if it should be more a more generic error so that 1) other error codes can also error out. Perhaps all the 1xxx? 2) If it should rely on $this->last_errror rather than creating a new string.

My thinking with using the string from mysql is that since the problem here is on the mysql server side, the error message provides a better starting place for people to search for a solution.

For anyone looking for the list of error codes, ​here are all the server ones and from there you can view the others as well.

Hi,

While I agree that errors should be treated in a generic way i.e. catch the error message and display it to help finding the solution (This could even be a setting like "display or not the error message", as some people may prefer hiding the error to visitors and have it in logs or whatever)

Most MySQL errors are properly managed already. This one is a bit specific as it is not really an error, everything works well but the MySQL server refuses to answer anymore, and for a limited duration only.

Moreover, in many cases the retry is useful (A timeout in connection, a database server failover, …). In that case, the retry mechanism makes the server refuse to answer for a longer period of time, that's why this error should be treated differently, and trigger the wp_die as soon as possible.

Also, having the error message rely on $this->last_error gives all the required information to fix the problem (database username, setting to change) without having to parse the error message just to rewrite it.

[desrosj]

After looking through this, I agree with using wp_die() for this specific error provided the $this->last_error is used. My thinking is a wp_die() will guard against the following:

• retrying requests can result in extending the cool down period.
• it's possible that some queries for a request could fail due to rate limiting but others may succeed. The consequence for both would likely be a scenario where only some of the information required for the request is available and the result is a visibly broken page.

[dmsnell]

This is about the installation but we seem to be adding wp_die() in the PR inside of $wpdb->query() — is that right?

Would we want to move the error-handling code into the installation script? Or terminate every single request which hits rate limiting? I am imagining that since many requests involve tens or hundreds of DB calls, we could have partially-successful requests.

Before this change, some of the work in those requests are fulfilled while corruption and inconsistency remains. After this change, we avoid showing the user the corruption, but we might have still left the database in an inconsistent state (because we may have executed some queries before the rate-limiting went into effect).

[wildworks]

This ticket has been inactive for nearly two months, and further discussion seems necessary, so I'd like to punt it to 7.0.

[anonymooo]

Replying to wildworks:

This ticket has been inactive for nearly two months, and further discussion seems necessary, so I'd like to punt it to 7.0.

Hi,

Which further discussion ?

I've been pinging on the slack until this ticket was punted to 6.9. I thought everything was ready to go.

Did I miss something ?

[wildworks]

Replying to anonymooo:

Replying to wildworks:

This ticket has been inactive for nearly two months, and further discussion seems necessary, so I'd like to punt it to 7.0.

Hi,

Which further discussion ?

I've been pinging on the slack until this ticket was punted to 6.9. I thought everything was ready to go.

Did I miss something ?

From what I can see, it seems we need to consider the feedback on this comment, but if the pull request is fully ready, we can proceed. However, since the pull request includes new strings, it needs to be committed before RC1.

Could everyone involved in this ticket confirm again whether we are ready to commit the pull request?

[anonymooo]

Replying to wildworks:

Replying to anonymooo:

Replying to wildworks:

This ticket has been inactive for nearly two months, and further discussion seems necessary, so I'd like to punt it to 7.0.

Hi,

Which further discussion ?

I've been pinging on the slack until this ticket was punted to 6.9. I thought everything was ready to go.

Did I miss something ?

From what I can see, it seems we need to consider the feedback on this comment, but if the pull request is fully ready, we can proceed. However, since the pull request includes new strings, it needs to be committed before RC1.

Could everyone involved in this ticket confirm again whether we are ready to commit the pull request?

For me, PR is ready and tested by other persons, and instead of having an abnormal behaviour (Going back to installation page) we have a proper error message.

At least it won't do more harm than what the behaviour is right now.

About @dmsnell remarks, I may be wrong but I think the best would be to manage this on the database side in a transaction without autocommit. But that is database configuration.