Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The REST server accepts invalid JSON, and passes NULL to the service callback. Expected behavior would be to return a 400 error.
Proposed resolution
Patch ServicesParserJSON
to check the return value of json_decode().
Remaining tasks
Patch needs review.
User interface changes
N/A
API changes
HTTP response changes from 200 to 400 when invalid JSON is sent.
Comment | File | Size | Author |
---|---|---|---|
#1 | 0001-Issue-2288775-Reject-invalid-JSON.patch | 1023 bytes | grendzy |
Comments
Comment #1
grendzy CreditAttribution: grendzy commentedComment #2
marcingy CreditAttribution: marcingy commentedLooks good thanks.
Comment #3
kylebrowning CreditAttribution: kylebrowning commentedNeeds a test.
Comment #4
iLLin CreditAttribution: iLLin commentedWhy does services force an Array instead of an Object for the JSON? Is there a valid reason why the TRUE parameter is passed to json_decode?
Comment #5
kylebrowning CreditAttribution: kylebrowning commentedStill needs a test :/
Comment #6
kylebrowning CreditAttribution: kylebrowning commentedComment #8
heykarthikwithuReviewed and Tested the patch +1
Comment #9
tyler.frankenstein CreditAttribution: tyler.frankenstein commentedCopying what was placed here: https://github.com/kylebrowning/services/commit/6969e5645b513144bb0ea50c...
===================
I don't fully understand this yet, but returning an error is pretty drastic here. For example, a typical system connect call (for years now) hasn't required any request body be sent AFAIK (it's always just been a POST with the X-CSRF-Token header).
Can we please take a lighter approach here? e.g. a watchdog warning...
I say this because jDrupal and DrupalGap are very dependant on this not throwing an error, and this will break thousands and thousands of app installs for users if this gets rolled into the next recommended Services release.
I'm definitely open to learning why we need this, but I think we should at least allow a recommended release or two for apps to get caught up to this new requirement. What do you guys think?
====================
To follow up, it sounds like if we don't send a request body up to Services, that is considered invalid JSON, correct? Like I said, I'm definitely open to learning why it is invalid and how to fix it, but am concerned at the havoc this will cause some folks ;)
Comment #10
kylebrowning CreditAttribution: kylebrowning commentedTyler, yeah thats true, we nuked the problem. how about we refactor and say
If you sent a request body, and it is invalid, then we say invalid JSON Request Body?
Comment #12
kylebrowning CreditAttribution: kylebrowning commented