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

Handle JSON POSTs in CSRF #2240

Merged
merged 1 commit into from
Sep 26, 2019
Merged

Conversation

nowackipawel
Copy link
Contributor

Important change to handle JSON POSTS in CSRF.
I wanted to send JSONS instead on normal POST but CSRF filter did not allow me to to this even if token existed in my JSON.

{"usr_id":1322189,"ctn":"3a111012714a58b53260ede89380d412","token":"ctn","parent":17031,"mode":{"main":"test","additional":"exam"},"time":{"start":1568992305,"end":1568993826},"result":{"2103":["T"]}}

my token name is: ctn

I think this is important, without this we are limiting a lot.

Important change to handle JSON POSTS in CSRF.
I wanted to send JSONS instead on normal POST but CSRF filter did not allow me to to this even if token existed in my JSON.
```
{"usr_id":1322189,"ctn":"3a111012714a58b53260ede89380d412","token":"ctn","parent":17031,"mode":{"main":"test","additional":"exam"},"time":{"start":1568992305,"end":1568993826},"result":{"2103":["T"]}}
```
my token name is: ctn

I think this is important, without this we are limiting a lot.
@jim-parry
Copy link
Contributor

@lonnieezell Are you ok with this? It looks good to me.

@MGatner
Copy link
Member

MGatner commented Sep 26, 2019

It looks solid to me but I’m a CSRF noob. :(

@jim-parry
Copy link
Contributor

I spent too much time in the CSRF unit tests, so maybe just a bit better than noob.
Using JSON paykiad for the token is a thing.

@jim-parry jim-parry merged commit 3dfedfe into codeigniter4:develop Sep 26, 2019
@michalsn
Copy link
Member

Personally, I would rather see a fallback to the request header than decoding whole json. json_* methods are really slow in php.

@jim-parry
Copy link
Contributor

They may be slow, but they are an allowed thing for CSRF token passing :-/
CSRF looks messy to begin with, and I am sure that there are some subtle things we might have overlooked (outside of performance), that might come up on hacker1 once we add CodeIgntier4 to it.

@lonnieezell
Copy link
Member

I was just getting ready to say seeing it in a header was likely a better solution, also. I've seen Laravel use X-CSRF-TOKEN. Not sure how others handle that.

@jim-parry
Copy link
Contributor

I think there are a bunch of ways, and that we don't handle them all yet!

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

Successfully merging this pull request may close these issues.

5 participants