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

Check response for avoid inject debugbar on json ajax #1558

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

erikn69
Copy link
Contributor

@erikn69 erikn69 commented Mar 5, 2024

Closes #1275

@erikn69 erikn69 mentioned this pull request Mar 5, 2024
@AforDesign
Copy link

@erikn69 Thanks a lot for the quick reaction with commit.

This method is clearly the place to fix the issue, but I'm a bit confused by the if-else tree here.
Looks like the isJsonResponse condition on line 797 should indeed fix the issue mentioned in #1275.

This injection also happens on plain text responses.
Could a condition to check this also be added after line 797?
To get to: // Just collect + store data, don't inject it.

I don't have a test case right now, but that's how I've stumbled unto this undesired inject issue.
Using FilePond I had to create a controller that returns a serverId as string when a file is stored on the server.
This string also receives the debugBar injection.

@erikn69
Copy link
Contributor Author

erikn69 commented Mar 5, 2024

This injection also happens on plain text responses.

Not possible, look

} elseif (
!$app['config']->get('debugbar.inject', true) ||
($response->headers->has('Content-Type') &&
strpos($response->headers->get('Content-Type'), 'html') === false) ||

strpos($response->headers->get('Content-Type'), 'html') === false
means that text/plain must be true, demo: https://onlinephp.io/c/67d34
maybe your response is not setting the right Content-Type, https://laravel.com/docs/10.x/responses#response-objects

return response('Hello World', 200)
     ->header('Content-Type', 'text/plain');

Anyway, if you show me a way to reproduce the bug, I will try to upload a fix

@AforDesign
Copy link

Thanks again for the followup erik69.
I copied an example code form a tutorial and overlooked that the controller's method return type was set to string.
This is propably causing the problem. I just assumed it would still be returned as a response with plain text header settings.

Your example should work as expected.
I will try it again with tomorrow with the proper settings.

Thanks again.
Much appreciated!

@barryvdh
Copy link
Owner

barryvdh commented Mar 6, 2024

I'm not sure if this is the best way. Maybe we can use https://www.php.net/manual/en/function.json-validate.php when available?

@erikn69
Copy link
Contributor Author

erikn69 commented Mar 6, 2024

I also thought about json_validate but it can only be used in PHP 8.3, and that's why I discarded it
I will add it to the early returns
image

@barryvdh
Copy link
Owner

barryvdh commented Mar 6, 2024

Yeah, but it would be best if people just set the json header.


    json_decode($string);

    return json_last_error() === JSON_ERROR_NONE;

would be the alternative to json_validate pre PHP8.3

@barryvdh
Copy link
Owner

barryvdh commented Mar 6, 2024

an alternative would be to ONLY show the debugbar on actual HTML (eg if it includes HTML or something), but that's also not really ideal.

@erikn69 erikn69 force-pushed the patch-61 branch 2 times, most recently from b323fb2 to e7763a5 Compare March 6, 2024 13:53
@erikn69
Copy link
Contributor Author

erikn69 commented Mar 6, 2024

but it would be best if people just set the json header.

Yes, but what happens in the case that request does not expect application/json, and response have it?

@erikn69 erikn69 force-pushed the patch-61 branch 2 times, most recently from a11602d to 8a8138a Compare March 6, 2024 14:30
@barryvdh
Copy link
Owner

Yeah I'm a bit worried about the validate impact. Do we really need to be sure it's json, or just looks like json?

@erikn69
Copy link
Contributor Author

erikn69 commented Sep 20, 2024

Do we really need to be sure it's json, or just looks like json?

It is an extreme case, it happened to me with some plugin,
it injected html on a json string, anyway I fix the plugin
but others could waste time until they discover what happens,
it can be closed if it causes performance problems, these are cases that should not happen

Also, if some of the previous conditions are false, this condition never gets executed

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.

Breaks ajax
3 participants