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

[BUG]: getJsonRawBody can return false for valid json #15608

Closed
stdex opened this issue Aug 3, 2021 · 8 comments · Fixed by #16381
Closed

[BUG]: getJsonRawBody can return false for valid json #15608

stdex opened this issue Aug 3, 2021 · 8 comments · Fixed by #16381
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium

Comments

@stdex
Copy link

stdex commented Aug 3, 2021

Version | 3.4.3
Build Date | Apr 1 2019 13:32:28
Powered by Zephir | Version 0.10.14-975ad02db4

In php json_last_error reset error after every json_decode call.

<?php
$t = null;
$data1 = json_decode($t);
$err1 = json_last_error();
$errN1 = json_last_error_msg();

var_dump([$data1, $err1, $errN1]);

$t = '{}';
$data1 = json_decode($t);
$err1 = json_last_error();
$errN1 = json_last_error_msg();

var_dump([$data1, $err1, $errN1]);
array(3) {
  [0]=>
  NULL
  [1]=>
  int(4)
  [2]=>
  string(12) "Syntax error"
}
array(3) {
  [0]=>
  object(stdClass)#1 (0) {
  }
  [1]=>
  int(0)
  [2]=>
  string(8) "No error"
}

If I use getJsonRawBody (internal call json_last_error can return error from prev call json_decode).

For example: POST with body {}

<?php
$t = json_decode(null, true);

$errS = json_last_error();
$errNS = json_last_error_msg();

$data = $request->getJsonRawBody();

$err0 = json_last_error();
$errN0 = json_last_error_msg();

$d1 = file_get_contents('php://input');
$data1 = json_decode($d1);

$err1 = json_last_error();
$errN1 = json_last_error_msg();

var_dump([$errS, $errNS], [$data, $err0, $errN0], [$d1, $data1, $err1, $errN1]);
[3] => Array
	(
		[0] => 4
		[1] => Syntax error
	)

[4] => Array
	(
		[0] => 
		[1] => 4
		[2] => Syntax error
	)

[5] => Array
	(
		[0] => {}
		[1] => stdClass Object
			(
			)

		[2] => 0
		[3] => No error
	)
@stdex stdex added bug A bug report status: unverified Unverified labels Aug 3, 2021
@Jeckerson
Copy link
Member

You are using old and unsupported version of Phalcon. Please migrate your app to the latest.

@Jeckerson Jeckerson added wontfix The issue will not be fixed or implemented and removed bug A bug report status: unverified Unverified labels Aug 13, 2021
@niden niden added this to Unverified - Wont Fix - Duplicates in Phalcon Roadmap Aug 13, 2021
@madrian-es
Copy link

I think this one should be revisited, since we are experiencing a similar bug with phalcon 5.1.2 & php 8.1.13 on alpine 3.16:

We have some code in a base controller similar to this:

if ($this->request->getJsonRawBody()){
    $request = $this->request->getJsonRawBody();
} else if ($this->request->isPost()) {
    $request = (object) $this->request->getPost();
} else {
    $request = (object) $this->request->getPut();
}

This allows us to process the request using the same code regardless of the request type (JSON or formdata or whatever)

Note that the first check involves calling getJsonRawBody(). When the body is NOT json, this obviously fails and json_last_error() returns syntax error (4).

Now, in a particular controller, we try to validate a JWT token (passed using a form, and NOT json) using code similar to this:

$signer     = new Hmac('sha256');
$parser      = new Parser();
$tokenObject = $parser->parse($jwt);
//...

Since the parse() function needs to decode a JSON, for reasons unknown to us, it reaches this code path.

You would think that the call to json_decode() from a few lines before would reset the json_last_error(), but for some reason it does not. If we modify our code to add json_encode(null); if getJsonRawBody() fails, everything works as expected.

@niden niden reopened this Dec 20, 2022
@niden
Copy link
Sponsor Member

niden commented Dec 20, 2022

@madrian-es Do you happen to have an example or a failing test for this so that I can test it?

@niden niden self-assigned this Dec 20, 2022
@niden niden added bug A bug report status: medium Medium 5.0 The issues we want to solve in the 5.0 release and removed wontfix The issue will not be fixed or implemented labels Dec 20, 2022
@niden
Copy link
Sponsor Member

niden commented Dec 20, 2022

The reason it reaches that code path is because something is going on when decoding the headers. If you notice the parse method decodes headers also which in turn call the decode method and then the json equivalents.

To understand this better. What we need is anytime a json_decode runs and fails, right after that the json_last_error() should be reset.

Correct?

@madrian-es
Copy link

@niden I was able to create a small repo that you can use to reproduce this fully. I tried to remove all the superfluous code that is not relevant to this bug, but there may be some unnecessary use statements here and there.

@niden
Copy link
Sponsor Member

niden commented Dec 21, 2022

I will test it today.

Note you do not need PSR with Phalcon 5.x. Just FYI

@madrian-es
Copy link

I must have missed that when upgrading to 5, thanks for the heads up.

@niden niden mentioned this issue Jul 25, 2023
5 tasks
@niden niden linked a pull request Jul 25, 2023 that will close this issue
5 tasks
@niden
Copy link
Sponsor Member

niden commented Jul 25, 2023

Resolved in #16381

Thanks guys.

@niden niden closed this as completed Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium
Projects
Status: Released
Phalcon Roadmap
  
Unverified - Wont Fix - Duplicates
Development

Successfully merging a pull request may close this issue.

4 participants