Add a warning when Cloudflare blocks REST API requests #8640

pento · 2018-08-07T03:43:07Z

Description

When Cloudflare blocks REST API requests, there's nothing in the UI to suggest what the cause is, which is a confusing and frustrating experience.

Adding extra information when we detect the error helps folks determine their best course of action.

This is a temporary measure, to help with issues like #2704, until Cloudflare roll out their fix. As such, the message box is a fairly lazy copy/paste of the existing block warning in the Classic Editor.

How has this been tested?

With Cloudflare:

Enable the WAF on your site. You may need to disable rules WP0003 and WP0004, to ensure you're not being whitelisted.
Try to save a post.
After the save fails, disable rules WP0025A and WP0025B.
Try to save it again: the save should succeed.

On your local site, you can simulate the REST API being blocked by Cloudflare with this snippet:

add_action( 'rest_api_init', function() {
	header( 'Content-Type: text/html' );
	status_header( 403 );
	echo '<span>Cloudflare Ray ID: foo</span>';
	die();
} );

Screenshots

Clicking Learn More will redirect you to the Classic Editor, showing this message:

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

…oudflare block.

gziolo · 2018-08-07T04:38:56Z

lib/compat.php

+					<p><?php _e( 'Your site uses Cloudflare, which provides a Web Application Firewall (WAF) to secure your site against attacks. Unfortunately, some of these WAF rules are incorrectly blocking legitimate access to your site, preventing Gutenberg from functioning correctly.', 'gutenberg' ); ?></p>
+					<p><?php _e( "We're working closely with Cloudflare to fix this issue, but in the mean time, you can work around it in one of two ways:", 'gutenberg' ); ?></p>
+					<ul>
+						<li><?php _e( 'If you have a Cloudflare Pro account, login to Cloudflare, visit the Firewall settings page, open the "Cloudflare Rule Set" details, open the "Cloudflare WordPress" ruleset, then set the rules "WP0025A" and "WP0025A" to "Disable".', 'gutenberg' ); ?></li>


"WP0025A" and "WP0025A" - the same id is listed twice.

If I follow the description of the PR right, it should be "WP0025A" and "WP0025B"

gziolo · 2018-08-07T04:45:42Z

packages/api-fetch/src/index.js

@@ -68,16 +76,24 @@ function apiFetch( options ) {
 					throw invalidJsonError;
 				}

+				const responseClone = response.clone();


Unit test fails with the following message:

[TypeError: r[1] esponse.clone is not a function]

I bet you need to mock it because it is a part of the response.

…d to it.

gziolo · 2018-08-07T07:02:21Z

packages/api-fetch/src/index.js

@@ -19,7 +19,7 @@ function registerMiddleware( middleware ) {
 }

 function checkCloudflareError( error ) {
-	if ( error.indexOf( 'Cloudflare Ray ID' ) >= 0 ) {
+	if ( error.indexOf && error.indexOf( 'Cloudflare Ray ID' ) >= 0 ) {


Or maybe:

typeof error === 'string'

gziolo · 2018-08-07T07:06:37Z

It works as advertised. I would personally consider wrapping those setting names and errors codes like WP0025A with strong/em to make them easier to distinguish from the big blob of text.

When I edit post in Gutenberg and try to save it as draft I can see the notice, but when I click link to learn more I see need to accept dialog informing that my changes weren't saved. It would be great to tweak it to ensure that everyone easily navigates to the Classic Editor.

gziolo

I noticed that current implementation only show noticed when you try to update the status of the post. However there are many more cases when it can fail like:

fetching taxonomies
fetching authors
dynamic blocks (latest posts, categories, ...)

And probably many more.

pento · 2018-08-07T07:27:40Z

I noticed that current implementation only show noticed when you try to update the status of the post.

Yeah, I was originally looking at implementing it for all requests (1bd7f2a). There were a couple of problems with this approach:

api-fetch didn't seem like the right place to create UI for showing a message.
Making api-fetch rely on the data package (so that it could wipe the post dirty state, preventing the "save your changes" warning you ran into) seemed like a worse idea. 🙂

So, I went with the post save action as being a reasonable tradeoff, where the user should notice the message when the first autosave happens, before they've really got into writing anything.

gziolo · 2018-08-07T07:36:02Z

@youknowriad implemented plugins API for api-fetch, I’m wondering if we could use it to scan all requests and show the notice only once when it happens for the first time.

When you visit Gutenberg for the first time, Document sidebar is expanded. This means it will trigger network requests for taxonomies and authors. You should be able to display a notice for Cloudflare users almost immediately. This would mitigate another concern I shared in the PR that you need to accept a dialog before you navigate away.

aduth · 2018-08-07T12:43:55Z

lib/compat.php

+					<p><?php _e( 'Your site uses Cloudflare, which provides a Web Application Firewall (WAF) to secure your site against attacks. Unfortunately, some of these WAF rules are incorrectly blocking legitimate access to your site, preventing Gutenberg from functioning correctly.', 'gutenberg' ); ?></p>
+					<p><?php _e( "We're working closely with Cloudflare to fix this issue, but in the mean time, you can work around it in one of two ways:", 'gutenberg' ); ?></p>
+					<ul>
+						<li><?php _e( 'If you have a Cloudflare Pro account, login to Cloudflare, visit the Firewall settings page, open the "Cloudflare Rule Set" details, open the "Cloudflare WordPress" ruleset, then set the rules "WP0025A" and "WP0025B" to "Disable".', 'gutenberg' ); ?></li>


Grammar: As a verb, the correct usage is "log in" as two words.

http://grammarist.com/spelling/log-in-login/

Core handbook also says to use "log in": https://make.wordpress.org/core/handbook/best-practices/spelling/

aduth · 2018-08-07T12:47:48Z

packages/api-fetch/src/index.js

@@ -68,16 +76,24 @@ function apiFetch( options ) {
 					throw invalidJsonError;
 				}

+				const responseClone = response.clone();


Why do we need to clone the response? (Add as an inline code comment)

aduth · 2018-08-07T12:49:08Z

packages/editor/src/store/effects/posts.js

+			'cloudflare-error': '',
+		} );
+
+	const cloudflaredMessage = error && 'cloudflare_error' === error.code ?


Typo: cloudflared => cloudflare

Edit: Or... intentional? I guess?

Intentional, the reason for this even needing to exist is silly, and given that it'll (hopefully) be reverted next week, we can be a little silly with the variable names, too.

aduth · 2018-08-07T12:50:09Z

packages/editor/src/store/effects/posts.js

+		</p> :
+		noticeMessage;
+
+	dispatch( createErrorNotice( cloudflaredMessage, { id: SAVE_POST_NOTICE_ID } ) );


Not loving how non-isolated the Cloudflare behaviors are.

Same, but as I mentioned above, this is super temporary, it'll disappear before too long.

aduth · 2018-08-07T12:51:22Z

packages/api-fetch/src/index.js

 				return response.json()
 					.catch( () => {
-						throw invalidJsonError;
+						return responseClone.text()
+							.then( ( text ) => {


Aside: Async/Await could make this nicer to read:

.catch( async () => { const text = await responseClone.text(); checkCloudflareError( text ); throw invalidJsonError; } );

swissspidy · 2018-08-08T06:55:12Z

lib/compat.php

+						<li>
+						<?php
+							printf(
+								/* translators: link to a comment in the Gutenberg repo */


This should be /* translators: %s: link to a comment in the Gutenberg GitHub repository */

swissspidy · 2018-08-08T06:55:49Z

lib/compat.php

+					<p>
+					<?php
+						printf(
+							/* translators: link to an issue in the Gutenberg repo */


Again missing the %s in the comment + I wouldn't shorten repository here.

/* translators: %s: link to an issue in the Gutenberg GitHub repository */

…t it's a string.

pento · 2018-08-08T07:39:47Z

I've fixed all the little things that needed to be fixed, but I haven't tried to re-architect it to catch all API requests.

I also haven't fixed the "unsaved changes" dialog appearing, I guess we could do something to UnsavedChangesWarning to not show it under these circumstances, or to fake the result of isEditedPostDirty().

It's kind of hacky, and not ideal, but hopefully it'll be reverted by this time next week, so I'm not too concerned. 🙂

gziolo

Let's get it in. It is much better then it was before. Given that we hope to remove it before 3.6 gets out, I'm fine with the current proposal. Tested again, still works as intended.

aduth · 2018-10-31T21:15:16Z

This is a temporary measure, to help with issues like #2704, until Cloudflare roll out their fix.

Is it still needed? Where can I follow progress on the removal of this code?

pento added 6 commits August 7, 2018 10:32

Catch a Cloudflare response, and redirect to the Classic editor..

1bd7f2a

Ensure the url package is loaded when api-fetch needs it.

6148a05

Show a message in the Classic Editor when we redirect there from a Cl…

b8af9ea

…oudflare block.

Don't auto-redirect to Classic, give a link, instead.

598a73a

Linting.

d41f658

Pinking shears

8c384cc

pento added the [Type] Enhancement A suggestion for improvement. label Aug 7, 2018

pento added this to the 3.5 milestone Aug 7, 2018

pento requested a review from a team August 7, 2018 03:43

gziolo reviewed Aug 7, 2018

View reviewed changes

pento added 3 commits August 7, 2018 14:51

Fix the incorrect Cloudflare rule ID being listed.

4ad8095

Mock Resonse.clone() in the tests.

671208e

Make checkCloudflareError() more resilient to non-strings being passe…

9191016

…d to it.

gziolo reviewed Aug 7, 2018

View reviewed changes

gziolo requested changes Aug 7, 2018

View reviewed changes

gziolo added the [Priority] High Used to indicate top priority items that need quick attention label Aug 7, 2018

aduth reviewed Aug 7, 2018

View reviewed changes

swissspidy reviewed Aug 8, 2018

View reviewed changes

pento added 5 commits August 8, 2018 17:18

Use typeof instead of checking for .indexOf on the error to check tha…

2219590

…t it's a string.

s/login/log in/

fbe7d6f

Improve translator comments.

483beab

Improve readability of the JSON parsing error handling

9c70019

Add a comment explaining why the response is being cloned.

468d024

gziolo approved these changes Aug 8, 2018

View reviewed changes

gziolo merged commit ac2fa9a into master Aug 8, 2018

youknowriad deleted the add/cloudflare-warning branch August 8, 2018 09:49

pento mentioned this pull request Nov 1, 2018

Remove Cloudflare warning #11350

Merged

4 tasks

dependabot-preview bot mentioned this pull request Dec 31, 2019

Bump @wordpress/i18n from 2.0.0 to 3.7.0 wearerequired/rest-likes#97

Closed

dependabot-preview bot mentioned this pull request Feb 4, 2020

Bump @wordpress/jest-console from 3.4.0 to 3.5.0 tdreyno/druyan#215

Merged

dependabot-preview bot mentioned this pull request Apr 15, 2020

Bump @wordpress/babel-plugin-makepot from 3.4.0 to 3.5.0 swissspidy/gutenberg-i18n-block#93

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a warning when Cloudflare blocks REST API requests #8640

Add a warning when Cloudflare blocks REST API requests #8640

pento commented Aug 7, 2018

gziolo Aug 7, 2018 •

edited

Loading

pento Aug 7, 2018

gziolo Aug 7, 2018

pento Aug 7, 2018

gziolo Aug 7, 2018 •

edited

Loading

pento Aug 8, 2018

gziolo commented Aug 7, 2018

gziolo left a comment

pento commented Aug 7, 2018

gziolo commented Aug 7, 2018

aduth Aug 7, 2018

swissspidy Aug 8, 2018

pento Aug 8, 2018

aduth Aug 7, 2018

pento Aug 8, 2018

aduth Aug 7, 2018

pento Aug 8, 2018

aduth Aug 7, 2018

pento Aug 8, 2018

aduth Aug 7, 2018

pento Aug 8, 2018

swissspidy Aug 8, 2018

pento Aug 8, 2018

swissspidy Aug 8, 2018

pento Aug 8, 2018

pento commented Aug 8, 2018

gziolo left a comment

aduth commented Oct 31, 2018

Add a warning when Cloudflare blocks REST API requests #8640

Add a warning when Cloudflare blocks REST API requests #8640

Conversation

pento commented Aug 7, 2018

Description

How has this been tested?

Screenshots

Checklist:

gziolo Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Aug 7, 2018

gziolo left a comment

Choose a reason for hiding this comment

pento commented Aug 7, 2018

gziolo commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pento commented Aug 8, 2018

gziolo left a comment

Choose a reason for hiding this comment

aduth commented Oct 31, 2018

gziolo Aug 7, 2018 •

edited

Loading

gziolo Aug 7, 2018 •

edited

Loading