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

Issue with JSON POST calls #80

Closed
Rick84 opened this Issue May 24, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@Rick84

Rick84 commented May 24, 2017

In csrfprotector.js in the new_send method (line 293) the following method is executed:

data.append(CSRFP.CSRFP_TOKEN, CSRFP._getAuthKey());

When I execute the ajax call below, the CSRF protector correctly identifies the payload as an object and tries to append the token. This, however, does not seem to do anything nor trigger an error, and log messages below this line are not executed (Chrome 58 under Windows 7).

        $.ajax({
            dataType: 'json',
            type: "POST",
            url: '[someURL]',
            contentType: "application/json",
            data: {foo: "bar"},
            processData: false,
            success: function(resultData) {
                console.log(1);
            }
        });

If I replace this line by the following code:

data[CSRFP.CSRFP_TOKEN] = CSRFP._getAuthKey();

At least the token is appended to the payload object as a property. It still isn't correctly passed through the protector, but this seems like a step in the right direction.

Is there a known issue with using JSON objects in jQuery AJAX calls with the CSRF protector, or am I missing something obvious here?

@mebjas

This comment has been minimized.

Owner

mebjas commented May 25, 2017

Thanks for posting, will look into it asap

@mebjas mebjas self-assigned this May 25, 2017

@arnavchaudhary

This comment has been minimized.

arnavchaudhary commented Aug 10, 2017

I have used this for our Ajax calls.
`
var csrf_token = $.cookie('csrfp_token');
$.ajaxPrefilter(function(options, originalOptions, jqXHR){
if (options.type.toLowerCase() === "post") {
// initialize data to empty string if it does not exist
options.data = options.data || "";

    // add leading ampersand if `data` is non-empty
    options.data += options.data?"&":"";

    // add _token entry
    options.data += "csrfp_token=" + csrf_token;
}

});

`
sorted the issue for us.

@Liwoj

This comment has been minimized.

Liwoj commented Sep 13, 2017

@arnavchaudhary workaround looks like content type is application/x-www-form-urlencoded and it will not work for content type application/json
Imho main reason is that server side validation code is using $_POST variable which is empty when POST body contains JSON payload.

Both client side (new_send() method) and server side (authorizePost()) needs to be updated to detect content type of the request and change data handling logic appropriately...

@jisol

This comment has been minimized.

jisol commented Sep 13, 2017

Making the library work for content type application / json is essential for it to work with the EXT JS framework.

As the library recommended by OWASP for PHP and Apache, it would be very important and appreciated if the updates suggested by Liwoj were implemented, or in the meantime, a functional workaround was provided.

As far as I can tell CSRFProtector is a fantastic idea and a precious help for CSRF defense. So, thanks to all who work on this project.

@Liwoj

This comment has been minimized.

Liwoj commented Sep 13, 2017

Well, there is workaround - use content type application/x-www-form-urlencoded instead of application/json. The later one is really not needed unless the data POSTed are complex - like nested list\objects.
But I agree that proper handling of JSON payload would be really useful for cases where JSON is necessary....not just for ExtJS users but really for any RIA framework out there...

@mebjas

This comment has been minimized.

Owner

mebjas commented Sep 13, 2017

Thanks for more context around the bug, I'll try reproduce this and send a fix.
As far as I get here, the JS code is not able to attach token when condition like this

 contentType: "application/json",
 data: {foo: "bar"},

is observed; which in turn leads to false negative on server side;

@jisol

This comment has been minimized.

jisol commented Sep 15, 2017

The master version available for download here https://github.com/mebjas/CSRF-Protector-PHP/wiki/Setting-up-CSRF-Protector-PHP-in-your-web-application seems outdated and does not work with EXTJS.

The new release available here https://github.com/mebjas/CSRF-Protector-PHP/releases apparently works fine (no error is triggered)

@mebjas

This comment has been minimized.

Owner

mebjas commented Sep 17, 2017

@Rick84 - I could reproduce the issue and here are some points:

  • As @Liwoj suggested, the library doesn't seem to support pulling data from POST payload as in case of application/json contentType. I'm working on this, also client side js need to support adding tokens to JSON Objects.

However there are some issues with the way you have used $.ajax as well I believe, data field should be in this format:

$(document).ready(function() {
    $.ajax({
        dataType: 'json',
        type: 'POST',
        url: './index.php',
        contentType: "application/json",
        data: JSON.stringify({foo: "bar"}),
        processData: false,
        success: function(resultData) {
            console.log(1);
        }
    });
});

As per: https://stackoverflow.com/questions/12693947/jquery-ajax-how-to-send-json-instead-of-querystring

And it should be called inside $(document).ready method as library itself is initialized after that;

mebjas added a commit that referenced this issue Sep 17, 2017

fix for issue 80
#80
Added support for content-type = application json in both client and
server side.
@mebjas

This comment has been minimized.

Owner

mebjas commented Sep 17, 2017

@Rick84 @arnavchaudhary @jisol @Liwoj - can you validate the fix on https://github.com/mebjas/CSRF-Protector-PHP/tree/dev-master-issue80 branch.

Fix was pushed in this commit - a14bb9e

For now consider this a hotfix, I have to look at all other type of content types as well.

@mebjas mebjas added this to the v1.0.0 milestone Sep 17, 2017

@Liwoj

This comment has been minimized.

Liwoj commented Sep 18, 2017

@mebjas I'm not PHP guy at all so i can't try it. Take this with grain of salt...

  1. Appending token in JS - taking stringified JSON, decode it, add token and stringify again doesn't look very efficient.
  2. I have a feeling that php://input is non seekable. Which means if you read it like this, request handler has no access to JSON payload anymore
  3. I don't like the fact that token is left as part of JSON object after the request is validated.
  4. XMLHttpRequests appends charset to Content-Type - if you set Content-type: application/json it ends up sending Content-type: application/json; charset=utf-8. Is I said, i'm not PHP guy so maybe the server side detection logic if fine as it is...

Not sure how to solve 1.-3.
One way could be switching from appending token (and decoding\encoding JSON) to prepending. You can handle data as string (it should be stringified JSON right?) and just prepend token with some delimiter. On Server side you read from input stream until you hit delimiter and validate. Everything whats left in input stream is just user's data (encoded JSON).
Only issue with this approach is data transferred is not valid JSON but i think its minor one...

Another way - and probably simpler - is to transfer token in headers. It's content-type agnostic and you avoid all problems with reading input stream in PHP. What do you think ?

@mebjas

This comment has been minimized.

Owner

mebjas commented Sep 19, 2017

@Liwoj - interesting points. Let's look at them one by one;

Problems:

  1. Sure there is efficiency issue, stringify the object is always gonna be there; So CSRFP adds an additional overhead of decoding JSON string to JSON Object, adding the token and encoding it back to JSON string; - is it too high? it could be yes!
  2. Yeah this is there!
  3. It's the same with data being left in $_POST array. Seems like a solvable problem though.
  4. Yeah, this is there too. Will have to check, been a while with PHP myself 👴

I must say these are really good points, I had overlooked considering problem to be easy. Thanks for pointing it out.

Coming to solutions:

  1. So token length is fixed and known to both server and client. So we append the token to JSON string and load that set of info from server and use it for validation; Sounds awesome!

PROS: overheads removed in problem 1, problem 2 solved to a good extend, 3 solved;
CONS: schema of JSON string is not followed, so if someone else has written wrappers in JS or php expecting payload to be JSON string, the breaks. If CSRFP is there in caller script and not in receiver script flow breaks (json decode fails).

  1. Send token as header

PROS: problem 1 solved, 2 solved, 3 solved;
CONS: I guess headers can only be set in XMLHttpRequests (POST). So server has to fetch the token from headers if request is POST - application/json * - this doesn't seem to be much over head.

of these two I like 2nd more, let's do little more research and go for a solution.

Thanks for both pointing out issues and clear solutions.

@Liwoj

This comment has been minimized.

Liwoj commented Sep 19, 2017

@mebjas Agree with yours pros\cons analysis. Solution no 2 seems superior in every way.
I suggest to use headers for all XHR POST requests (no logic testing content type etc needed) + on server side just try to read header 1st and if its not there, fallback to current implementation (reading from $_POST superglobal).
You are right about setting custom headers - only JS can do it....

@mebjas

This comment has been minimized.

Owner

mebjas commented Sep 20, 2017

yeah makes sense, will send out a new iteration.

@mebjas mebjas added bug todo and removed question repro needed labels Sep 23, 2017

@mebjas mebjas referenced this issue Sep 23, 2017

Merged

Fix to issue #80 #88

@mebjas

This comment has been minimized.

Owner

mebjas commented Sep 23, 2017

@Liwoj - check this out if it works, fb5c208

mebjas added a commit that referenced this issue Oct 6, 2017

@mebjas

This comment has been minimized.

Owner

mebjas commented Oct 7, 2017

Marking as done.

@mebjas mebjas closed this Oct 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment