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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

grendzy’s picture

Status: Active » Needs review
FileSize
1023 bytes
marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good thanks.

kylebrowning’s picture

Status: Reviewed & tested by the community » Needs work

Needs a test.

iLLin’s picture

Why 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?

kylebrowning’s picture

Still needs a test :/

kylebrowning’s picture

Status: Needs work » Fixed

heykarthikwithu’s picture

Status: Fixed » Closed (fixed)

Reviewed and Tested the patch +1

tyler.frankenstein’s picture

Status: Closed (fixed) » Active

Copying 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 ;)

kylebrowning’s picture

Tyler, 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?

  • kylebrowning committed 0a0d486 on 7.x-3.x
    Merge pull request #3 from signalpoint/7.x-3.x
    
    Issue #2288775. Skip...
  • tyler.frankenstein committed 7d69d55 on 7.x-3.x
    Issue #2288775. Skip JSON validation when the request's body is empty.
    
kylebrowning’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.