Add a shim to override wp.apiRequest, allowing HTTP/1.0 emulation #5741

pento · 2018-03-22T05:26:46Z

Description

#4396 switched API requests to using HTTP/1.0, using Backbone's emulateHTTP setting. #5253 changed to using wp.apiRequests() for API requests, which uses jQuery. Unfortunately, jQuery doesn't have a setting similar to emulateHTTP, so we need to manually do it, instead.

It seems that a lot of web application firewalls have stricter rules for PUT requests, so the merge of #5253 manifests itself as posts sporadically failing to save. For example: #5675, #5632, #5660.

I've also opened a core ticket to address this behaviour.

How Has This Been Tested?

Using your browser's dev tools, watch the network requests, and confirm that the save request for a post is sent as a POST, with the X-HTTP-Method-Override: PUT header. The post should save correctly.

If you have a WAF that currently blocks saving posts in Gutenberg 2.4, this PR should fix that behaviour.

Checklist:

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

pento · 2018-03-23T01:26:13Z

This is ready to merge, with review. It's been tested against a previously failing WAF.

gziolo · 2018-03-23T07:58:23Z

lib/compat.php

+	var oldApiRequest = wp.apiRequest;
+	wp.apiRequest = function ( options ) {
+		if ( options.method ) {
+			if( [ 'GET', 'PUT', 'DELETE' ].indexOf( options.method.toUpperCase() ) >= 0 ) {


Space after if is missing :)

gziolo · 2018-03-23T08:04:37Z

lib/compat.php

+
+	$scripts->add_inline_script( 'wp-api-request', $api_request_fix, 'after' );
+}
+add_action( 'wp_default_scripts', 'gutenberg_shim_api_request_emulate_http' );


The previous shim was called inside gutenberg_editor_scripts_and_styles. This code uses wp_default_scripts action. Not sure what is the difference as I can't find what this action does.

No practical difference, I just copy/pasta-ed from the gutenberg_shim_fix_api_request_plain_permalinks shim above it.

gziolo · 2018-03-23T08:08:09Z

lib/compat.php

+	var oldApiRequest = wp.apiRequest;
+	wp.apiRequest = function ( options ) {
+		if ( options.method ) {
+			if ( [ 'GET', 'PUT', 'DELETE' ].indexOf( options.method.toUpperCase() ) >= 0 ) {


One thing I noticed which differs from Backbone:

(type === 'PUT' || type === 'DELETE' || type === 'PATCH')

https://github.com/jashkenas/backbone/blob/master/backbone.js#L1587

gziolo

I haven't tested but I think it looks good. It would be the best idea to ask someone who has issues with the regular HTTP methods to confirm.

aduth · 2018-03-23T13:00:36Z

lib/compat.php

+		if ( options.method ) {
+			if ( [ 'PATCH', 'PUT', 'DELETE' ].indexOf( options.method.toUpperCase() ) >= 0 ) {
+				if ( ! options.headers ) {
+					options.headers = [];


This should be assigned as an object, not array.

http://api.jquery.com/jquery.ajax/ (settings.headers)

Missed that, but array is also an object in JS... so might work, but obviously should be updated.

aduth · 2018-03-23T13:04:25Z

lib/compat.php

+	var oldApiRequest = wp.apiRequest;
+	wp.apiRequest = function ( options ) {
+		if ( options.method ) {
+			if ( [ 'PATCH', 'PUT', 'DELETE' ].indexOf( options.method.toUpperCase() ) >= 0 ) {


Alternatively, we could inverse the check to better reflect that we support only GET and POST:

if ( [ 'GET', 'POST' ].indexOf( options.method.toUpperCase() ) === -1 ) {

Aside: Curiosity got me wondering: https://jsperf.com/indexof-array-vs-regexp (tl;dr: This is good)

OPTIONS shouldn't be converted, either. (Neither should HEAD, CONNECT, or TRACE, even though we don't specifically use them anywhere.) I'm inclined to go with the same conversion as Backbone, as that's the one that is most likely to be supported.

Hmm. I'm not clear what exactly we're doing in this pull request then. Are we targeting HTTP/1.0 ? There are only 3 defined verbs (previously overlooked HEAD):

https://www.w3.org/Protocols/HTTP/1.0/spec.html#Methods

It makes me a bit nervous that we target Backbone, as it's not obvious why we target Backbone (aside from anecdotal evidence that it appears to be well-supported).

aduth · 2018-03-26T17:18:23Z

lib/compat.php

+	var oldApiRequest = wp.apiRequest;
+	wp.apiRequest = function ( options ) {
+		if ( options.method ) {
+			if ( [ 'PATCH', 'PUT', 'DELETE' ].indexOf( options.method.toUpperCase() ) >= 0 ) {


Hmm. I'm not clear what exactly we're doing in this pull request then. Are we targeting HTTP/1.0 ? There are only 3 defined verbs (previously overlooked HEAD):

https://www.w3.org/Protocols/HTTP/1.0/spec.html#Methods

It makes me a bit nervous that we target Backbone, as it's not obvious why we target Backbone (aside from anecdotal evidence that it appears to be well-supported).

pento · 2018-03-27T00:58:57Z

Hmm. I'm not clear what exactly we're doing in this pull request then. Are we targeting HTTP/1.0?

Not exactly. More like "simulate HTTP/1.0 for the HTTP/1.1 verbs that have problematic support".

It makes me a bit nervous that we target Backbone, as it's not obvious why we target Backbone (aside from anecdotal evidence that it appears to be well-supported).

Backbone just happens to be the previous implementation of the same thing, via the Backbone.emulateHTTP setting. There have been various proposals over the years for a standard way to tunnel HTTP/1.1 verbs through a POST request, X-HTTP-Method-Override is the one that is usually used in any new REST API implementations.

jutta-livingmiracles · 2018-12-25T21:16:01Z

Hi, this issue is still occurring for me. I'm not a developer in any way, so if someone can help me troubleshoot, that would be really lovely. I have a SiteGround-hosted WordPress site; WP 5.0.2 and cannot update pages. I get the "Updating failed" error in the editor, and the following console errors in the Chrome Inspector:

POST /wp-json/wp/v2/pages/1242?_locale=user 403
POST /wp-json/wp/v2/pages/1242/autosaves?_locale=user 403

nerrad · 2018-12-31T15:48:23Z

@jutta-livingmiracles I'm not sure this pull addresses the same issue you were experiencing because this old pull references wp.apiRequest which is no longer used by Gutenberg (now uses wp.apiFetch).

I would recommend creating a new issue documenting your problems and it can be addressed there. Please make sure you can reproduce this without any themes/plugins active on your site as well.

pento added 2 commits March 22, 2018 15:49

Add a shim to override wp.apiRequest, allowing HTTP/1.0 emulation

c3c7f97

options.headers is an associative array.

543bb37

pento added [Type] Bug An existing feature does not function as intended Core REST API Task Task for Core REST API efforts labels Mar 22, 2018

pento self-assigned this Mar 22, 2018

pento requested review from youknowriad and aduth March 22, 2018 05:26

Add a link to the core ticket.

3154754

Remove the now unnecessary Backbone.emulateHTTP setting

7c7ada0

pento requested a review from a team March 23, 2018 01:24

gziolo reviewed Mar 23, 2018

View reviewed changes

\s++

7ba2dda

gziolo reviewed Mar 23, 2018

View reviewed changes

PATCH !== GET

a7abd50

gziolo approved these changes Mar 23, 2018

View reviewed changes

aduth reviewed Mar 23, 2018

View reviewed changes

Headers shoud be an object, not an array.

d22b177

aduth approved these changes Mar 26, 2018

View reviewed changes

pento merged commit b99137d into master Mar 27, 2018

pento deleted the fix/http-10-emulation branch March 27, 2018 00:59

DanReyLop mentioned this pull request Mar 27, 2019

Avoid using "PUT" or "DELETE" HTTP methods woocommerce/woocommerce-admin#1934

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a shim to override wp.apiRequest, allowing HTTP/1.0 emulation #5741

Add a shim to override wp.apiRequest, allowing HTTP/1.0 emulation #5741

pento commented Mar 22, 2018 •

edited

Loading

pento commented Mar 23, 2018

gziolo Mar 23, 2018

pento Mar 23, 2018

gziolo Mar 23, 2018

pento Mar 23, 2018

gziolo Mar 23, 2018 •

edited

Loading

pento Mar 23, 2018

gziolo left a comment

aduth Mar 23, 2018

gziolo Mar 23, 2018 •

edited

Loading

pento Mar 25, 2018

aduth Mar 23, 2018

pento Mar 25, 2018

aduth Mar 26, 2018

aduth Mar 26, 2018

pento commented Mar 27, 2018

jutta-livingmiracles commented Dec 25, 2018

nerrad commented Dec 31, 2018

Add a shim to override wp.apiRequest, allowing HTTP/1.0 emulation #5741

Add a shim to override wp.apiRequest, allowing HTTP/1.0 emulation #5741

Conversation

pento commented Mar 22, 2018 • edited Loading

Description

How Has This Been Tested?

Checklist:

pento commented Mar 23, 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

gziolo Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Mar 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pento commented Mar 27, 2018

jutta-livingmiracles commented Dec 25, 2018

nerrad commented Dec 31, 2018

pento commented Mar 22, 2018 •

edited

Loading

gziolo Mar 23, 2018 •

edited

Loading

gziolo Mar 23, 2018 •

edited

Loading