Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a warning when Cloudflare blocks REST API requests #8640

Merged
merged 14 commits into from
Aug 8, 2018
Merged

Add a warning when Cloudflare blocks REST API requests #8640

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1bd7f2a
Catch a Cloudflare response, and redirect to the Classic editor..
pento Aug 7, 2018
6148a05
Ensure the url package is loaded when api-fetch needs it.
pento Aug 7, 2018
b8af9ea
Show a message in the Classic Editor when we redirect there from a Cl…
pento Aug 7, 2018
598a73a
Don't auto-redirect to Classic, give a link, instead.
pento Aug 7, 2018
d41f658
Linting.
pento Aug 7, 2018
8c384cc
Pinking shears
pento Aug 7, 2018
4ad8095
Fix the incorrect Cloudflare rule ID being listed.
pento Aug 7, 2018
671208e
Mock Resonse.clone() in the tests.
pento Aug 7, 2018
9191016
Make checkCloudflareError() more resilient to non-strings being passe…
pento Aug 7, 2018
2219590
Use typeof instead of checking for .indexOf on the error to check tha…
pento Aug 8, 2018
fbe7d6f
s/login/log in/
pento Aug 8, 2018
483beab
Improve translator comments.
pento Aug 8, 2018
9c70019
Improve readability of the JSON parsing error handling
pento Aug 8, 2018
468d024
Add a comment explaining why the response is being cloned.
pento Aug 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 141 additions & 2 deletions lib/compat.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,15 +167,19 @@ function gutenberg_check_if_classic_needs_warning_about_blocks() {
return;
}

if ( ! gutenberg_post_has_blocks( $post ) ) {
if ( ! gutenberg_post_has_blocks( $post ) && ! isset( $_REQUEST['cloudflare-error'] ) ) {
return;
}

// Enqueue the JS we're going to need in the dialog.
wp_enqueue_script( 'wp-a11y' );
wp_enqueue_script( 'wp-sanitize' );

add_action( 'admin_footer', 'gutenberg_warn_classic_about_blocks' );
if ( isset( $_REQUEST['cloudflare-error'] ) ) {
add_action( 'admin_footer', 'gutenberg_warn_classic_about_cloudflare' );
} else {
add_action( 'admin_footer', 'gutenberg_warn_classic_about_blocks' );
}
}
add_action( 'admin_enqueue_scripts', 'gutenberg_check_if_classic_needs_warning_about_blocks' );

Expand Down Expand Up @@ -322,3 +326,138 @@ function gutenberg_warn_classic_about_blocks() {
</script>
<?php
}

/**
* Adds a warning to the Classic Editor when CloudFlare is blocking REST API requests.
*
* @since 3.4.0
*/
function gutenberg_warn_classic_about_cloudflare() {
?>
<style type="text/css">
#cloudflare-block-dialog .notification-dialog {
padding: 25px;
}

#cloudflare-block-dialog ul {
list-style: initial;
padding-left: 20px;
}

@media only screen and (max-height: 480px), screen and (max-width: 450px) {
#cloudflare-block-dialog .notification-dialog {
width: 100%;
height: 100%;
max-height: 100%;
position: fixed;
top: 0;
margin: 0;
left: 0;
}
}
</style>

<div id="cloudflare-block-dialog" class="notification-dialog-wrap">
<div class="notification-dialog-background"></div>
<div class="notification-dialog">
<div class="cloudflare-block-message">
<h2><?php _e( 'Cloudflare is blocking REST API requests.', 'gutenberg' ); ?></h2>
<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>
Copy link
Member

@gziolo gziolo Aug 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

__( 'For free Cloudflare accounts, you can <a href="%s">change the REST API URL</a>, to avoid triggering the WAF rules. Please be aware that this may cause issues with other plugins that use the REST API, and removes any other protection Cloudflare may be offering for the REST API.', 'gutenberg' ),
'https://github.com/WordPress/gutenberg/issues/2704#issuecomment-410582252'
);
?>
</li>
</ul>
<p>
<?php
printf(
/* translators: link to an issue in the Gutenberg repo */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

__( 'If neither of these options are possible for you, please <a href="%s">follow this issue for updates</a>. We hope to have this issue rectifed soon!', 'gutenberg' ),
'https://github.com/WordPress/gutenberg/issues/2704'
);
?>
</p>
</div>
<p>
<button type="button" class="button button-primary cloudflare-block-classic-button"><?php _e( 'Continue to Classic Editor', 'gutenberg' ); ?></button>
</p>
</div>
</div>

<script type="text/javascript">
/* <![CDATA[ */
( function( $ ) {
var dialog = {};

dialog.init = function() {
// The modal
dialog.warning = $( '#cloudflare-block-dialog' );
// Get the links and buttons within the modal.
dialog.warningTabbables = dialog.warning.find( 'a, button' );

// Get the text within the modal.
dialog.rawMessage = dialog.warning.find( '.cloudflare-block-message' ).text();

// Hide all the #wpwrap content from assistive technologies.
$( '#wpwrap' ).attr( 'aria-hidden', 'true' );

// Detach the warning modal from its position and append it to the body.
$( document.body )
.addClass( 'modal-open' )
.append( dialog.warning.detach() );

// Reveal the modal and set focus on the Gutenberg button.
dialog.warning
.removeClass( 'hidden' )
.find( '.cloudflare-block-gutenberg-button' ).focus();

// Attach event handlers.
dialog.warningTabbables.on( 'keydown', dialog.constrainTabbing );
dialog.warning.on( 'click', '.cloudflare-block-classic-button', dialog.dismissWarning );

// Make screen readers announce the warning message after a short delay (necessary for some screen readers).
setTimeout( function() {
wp.a11y.speak( wp.sanitize.stripTags( dialog.rawMessage.replace( /\s+/g, ' ' ) ), 'assertive' );
}, 1000 );
};

dialog.constrainTabbing = function( event ) {
var firstTabbable, lastTabbable;

if ( 9 !== event.which ) {
return;
}

firstTabbable = dialog.warningTabbables.first()[0];
lastTabbable = dialog.warningTabbables.last()[0];

if ( lastTabbable === event.target && ! event.shiftKey ) {
firstTabbable.focus();
event.preventDefault();
} else if ( firstTabbable === event.target && event.shiftKey ) {
lastTabbable.focus();
event.preventDefault();
}
};

dialog.dismissWarning = function() {
// Hide modal.
dialog.warning.remove();
$( '#wpwrap' ).removeAttr( 'aria-hidden' );
$( 'body' ).removeClass( 'modal-open' );
};

$( document ).ready( dialog.init );
} )( jQuery );
/* ]]> */
</script>
<?php
}
18 changes: 17 additions & 1 deletion packages/api-fetch/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ function registerMiddleware( middleware ) {
middlewares.push( middleware );
}

function checkCloudflareError( error ) {
if ( error.indexOf( 'Cloudflare Ray ID' ) >= 0 ) {
throw {
code: 'cloudflare_error',
};
}
}

function apiFetch( options ) {
const raw = ( nextOptions ) => {
const { url, path, body, data, parse = true, ...remainingOptions } = nextOptions;
Expand Down Expand Up @@ -68,16 +76,24 @@ function apiFetch( options ) {
throw invalidJsonError;
}

const responseClone = response.clone();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻


return response.json()
.catch( () => {
throw invalidJsonError;
return responseClone.text()
.then( ( text ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

checkCloudflareError( text );
throw invalidJsonError;
} );
} )
.then( ( error ) => {
const unknownError = {
code: 'unknown_error',
message: __( 'An unknown error occurred.' ),
};

checkCloudflareError( error );

throw error || unknownError;
} );
} );
Expand Down
23 changes: 22 additions & 1 deletion packages/editor/src/store/effects/posts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { pick, includes } from 'lodash';
*/
import apiFetch from '@wordpress/api-fetch';
import { __ } from '@wordpress/i18n';
import { addQueryArgs } from '@wordpress/url';

/**
* Internal dependencies
Expand Down Expand Up @@ -256,7 +257,27 @@ export const requestPostUpdateFailure = ( action, store ) => {
const noticeMessage = ! isPublished && publishStatus.indexOf( edits.status ) !== -1 ?
messages[ edits.status ] :
__( 'Updating failed' );
dispatch( createErrorNotice( noticeMessage, { id: SAVE_POST_NOTICE_ID } ) );

const cloudflareDetailsLink = addQueryArgs(
'post.php',
{
post: post.id,
action: 'edit',
'classic-editor': '',
'cloudflare-error': '',
} );

const cloudflaredMessage = error && 'cloudflare_error' === error.code ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: cloudflared => cloudflare

Edit: Or... intentional? I guess?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<p>
{ noticeMessage }
<br />
{ __( 'Cloudflare is blocking REST API requests.' ) }
{ ' ' }
<a href={ cloudflareDetailsLink }>{ __( 'Learn More' ) } </a>
</p> :
noticeMessage;

dispatch( createErrorNotice( cloudflaredMessage, { id: SAVE_POST_NOTICE_ID } ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not loving how non-isolated the Cloudflare behaviors are.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

/**
Expand Down