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.

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

get_parameters() is ignoring data params sent as application/json #95

Closed
kosso opened this issue Nov 23, 2015 · 10 comments
Closed

get_parameters() is ignoring data params sent as application/json #95

kosso opened this issue Nov 23, 2015 · 10 comments

Comments

@kosso
Copy link
Contributor

kosso commented Nov 23, 2015

get_parameters() is still ignoring extra post data parameters sent as application/json.
Is this on purpose?

When merging all $_GET and $_POST parameters, along with Authorization headers, since PHP 5.6 data sent as application/json is ignored in $_POST, so we need to also collect parameters from posted JSON data using php://input

After line 93 of class-wp-json-authentication-oauth1.php

// ... 
        $params = array_merge( $_GET, $_POST );
        $params = wp_unslash( $params );

        if($_SERVER['CONTENT_TYPE']=='application/json'){
            $raw_post_data_params = json_decode(file_get_contents('php://input'), true);
            if ( ! empty( $raw_post_data_params ) ) {
                $raw_post_data_params = wp_unslash( $raw_post_data_params );
                $params = array_merge( $params, $raw_post_data_params );
                ksort($params);
            }
        }

// .....

This fixed my Missing OAuth parameter oauth_verifier error.

Now, I can finally connect a client to my server and get an access_token for its user, since my OAuth client was sending the oauth_verifier as application/json. (And likely to interact with the API in this way for all other POST requests).

I have also successfully tested creating a new post with my newly acquired access_token/secret after making these changes, when sending the data as application/json.

One thing, however... the OAuth1.0a signing requests spec does state:

The request parameters are collected, sorted and concatenated into a normalized string:
Parameters in the OAuth HTTP Authorization header excluding the realm parameter.
Parameters in the HTTP POST request body (with a content-type of application/x-www-form-urlencoded).
HTTP GET parameters added to the URLs in the query part (as defined by [RFC3986] section 3).

note: "(with a content-type of application/x-www-form-urlencoded)"

but this ignores the PHP5.6+ issue when sending applicaton/json formatted data.

also note: When I tested uploading a media file, I found I needed to use : multipart/form-data (in a PHP cUrl request)

@kosso kosso mentioned this issue Nov 23, 2015
@rmccue
Copy link
Member

rmccue commented Nov 23, 2015

This is annoying, but we can't simply read php://input - this can only be read from once for most requests. We can get this from (new WP_REST_Server())->get_raw_data() though, which saves it into a superglobal for reuse.

@kosso
Copy link
Contributor Author

kosso commented Nov 23, 2015

Ahhh. I see. It caters for the deprecation of $HTTP_RAW_POST_DATA in there. Nice.

@rmccue
Copy link
Member

rmccue commented Nov 24, 2015

Hmm, so, thinking over this again: I don't think we're meant to mix in any of the JSON data at all. The OAuth RFC explicitly states:

The HTTP request entity-body, but only if all of the following
conditions are met:

*  The entity-body is single-part.

*  The entity-body follows the encoding requirements of the
   "application/x-www-form-urlencoded" content-type as defined by
   [W3C.REC-html40-19980424].

*  The HTTP request entity-header includes the "Content-Type"
   header field set to "application/x-www-form-urlencoded".

Based on this, I don't think we should be parsing out any of the data at all, so the current behaviour seems correct; although we're lacking the Content-Type header check, I think this is handled by PHP when parsing out $_POST. It sounds like the issue above is that we need an extra header check there for PHP <5.6?

@kosso
Copy link
Contributor Author

kosso commented Nov 24, 2015

Yeah, I quoted that bit above. ;)
Though, I do think that when it was written over 5 years ago, there weren't as many JS based apps and servers around as there are these days, happily posting and digesting JSON and dealing with application/json payloads. Heck, it took long enough to wrestle people away to an alternative to XML and SOAP etc :)

I see the WP_REST_Server get_raw_data() method just says, hey if no $HTTP_RAW_POST_DATA is available (ie: post 5.6), then fill it with whatever's coming in via php://input. Which is basically what the PHP manual says to do.

I've built apps against some APIs which allow for sending extra params in either GET POST or via json/application payloads. I've also seen some which only allow oauth_... params in the Auth header.

OAuth can be a mess for some (and also why an author since walked away from it), so why not err on the side of flexibility? :) Unless there's a gaping security hazard which I haven't seen yet?

Go JSON, I say! Especially since the WPI-API is all about the JSON, eh? ;)

@rmccue
Copy link
Member

rmccue commented Nov 24, 2015

Though, I do think that when it was written over 5 years ago, there weren't as many JS based apps and servers around as there are these days, happily posting and digesting JSON and dealing with application/json payloads.

Indeed, it makes sense to sign the body given that's where most of the data actually is, but we're best off sticking to the spec.

I've built apps against some APIs which allow for sending extra params in either GET POST or via json/application payloads. I've also seen some which only allow oauth_... params in the Auth header.

Of course, I forgot about the OAuth meta parameters themselves, that makes sense. That said, it's a bit of a bad idea to send those in the JSON data, as it might interfere with the actual payload itself. There's no cases in the core REST API where the payload is used wholesale, but it's possible that could happen; this could lead to your OAuth parameters accidentally getting saved to the database as part of an object.

As far as I know, there's no standard libraries which send the OAuth params in JSON data in the body, so I think we're fine not supporting that. Either GET parameters or the Authorization header (with a preference for the latter) are the best spots to send the data.

why not err on the side of flexibility?

The parameters are used for two things: getting the OAuth meta parameters, and building the signature base string. We can't do one without the other, and accepting JSON here would make our implementation non-conforming per the first point.

Going to close this out as wontfix in light of the above; if anyone needs the ability to send OAuth parameters in JSON, please note here why and we can reconsider this in the future. :)

@kosso Thanks for filing the issue! 🍍

@rmccue rmccue closed this as completed Nov 24, 2015
@kosso
Copy link
Contributor Author

kosso commented Nov 24, 2015

OK. I guess I'll just have to live with that. ;)

I just irks me a little that we can't send pure JSON data (not the oauth_.. headers) to a JSON based API server. Naturally, multi-depth structured JSON might be problematic. But as long as a client and server both parse each param/property, and format, order and concatenate the base string the same way, it could work ;)

Is it something a developer could write a plugin/hook to override if they wanted to? I haven't looked too deeply yet into how I could 'enhance' the server (plugin, for now) with my own plugin if I wanted to. eg: I might want my consumer apps to have more fields or extra functionality in the wp-admin forms, etc.

@rmccue
Copy link
Member

rmccue commented Nov 24, 2015

The OAuth stuff is built almost entirely separately from the API itself, so there's only a loose binding between the two.

I just irks me a little that we can't send pure JSON data (not the oauth_.. headers) to a JSON based API server.

Realistically, the OAuth meta-parameters belong in the Authorization header; with those being sent there, you should be able to send entirely JSON requests, unless I've missed something?

Is it something a developer could write a plugin/hook to override if they wanted to?

I suspect it's possible if you need to :)

@kosso
Copy link
Contributor Author

kosso commented Nov 24, 2015

unless I've missed something?

Possibly. That's the thing, neither $_POST nor $HTTP_RAW_POST_DATA can read data from an incoming application/json POST without using php://input.

eg: Say I want to create a new post: (sorry, I go on a bit here - just for my own clarity ;) )

I need to make a signature for a POST to : /wp-json/wp/v2/posts with a title, content and status.

Let's say this is the JSON I want to send:

{
    status:'publish',
    title:'Oh wow! It Worked!!',
    content:'Absolutely incredible. Tomorrow, the world!'
}

I then get my oauth_consumer_key, oauth_nonce, oauth_signature_method, oauth_timestamp, oauth_token and oauth_version meta-parameters, then the status, content and title from the JSON payload, put them all in alphabetical order , then concatenate them properly with the method, according to the spec to create my base string. With that, the app uses the consumer_secret and my user's token_secret to generate the signature....

.. breathes.. ;)

.. then the app will build the Authorization header to use, using just the oauth_ meta-parameters, along with the freshly generated oauth_signature, and then POST the JSON with a Content-Type of application/json.

Currently, this fails for me. Without the additional php://input I suggested, the server never sees the json payload parameters to assist in rebuilding the same base string to then sign itself and compare signatures to authenticate.

The current system can't accept an Authorized POST of application/json Content-Type at all.
Without php://input, after PHP5.6, it simply can't read the data.


It's quite possible though, that OAuth has fried my brain and I'm vastly over-thinking this. ;)

@rmccue
Copy link
Member

rmccue commented Nov 24, 2015

I then get my oauth_consumer_key, oauth_nonce, oauth_signature_method, oauth_timestamp, oauth_token and oauth_version meta-parameters, then the status, content and title from the JSON payload, put them all in alphabetical order , then concatenate them properly with the method, according to the spec to create my base string.

Right, so per the OAuth spec, you shouldn't be adding the parameters from the payload unless they're URL-encoded. The spec also notes this:

   The signature base string does not cover the entire HTTP request.
   Most notably, it does not include the entity-body in most requests,
   nor does it include most HTTP entity-headers.

The payload parameters are only included when the Content-Type is application/x-www-form-urlencoded, not for anything else.

@rmccue
Copy link
Member

rmccue commented Nov 24, 2015

That is: if you have the following request:

POST /wp-json/wp/v2/posts
Host: example.com
Authorization: OAuth
               oauth_consumer_key="key"
               oauth_token="token"
               oauth_signature_method="HMAC-SHA1"
               oauth_timestamp="123456789",
               oauth_nonce="nonce",
               oauth_signature="..."

{
    "title": "Hello World!"
}

Then the signature pieces are:

Method: POST
URL:    http://example.com/wp-json/wp/v2/posts
Params: oauth_consumer_key=key&oauth_nonce=nonce&oauth_signature_method=HMAC-SHA1&oauth_timestamp=123456789&oauth_token=token

And the signature base string is

POST&http%3A%2F%2Fexample.com%2Fwp-json%2Fwp%2Fv2%2Fposts&oauth_consumer_key%3Dkey%26oauth_nonce%3Dnonce%26oauth_signature_method%3DHMAC-SHA1%26oauth_timestamp%3D123456789%26oauth_token%3Dtoken

Note: no title parameter in there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants