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

Ignore request logging for upload POST requests #187

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amazeika
Copy link

The problem

Right now any POST request containing a file payload is failling on my end and ends up crashing the expose client due to memory limitations.

The cause

This is happening because all requests seem to go through a logging process that require parsing the whole request body.

On multipart/form-data POST content this is not a very good idea since the body is likely to contain a big amount of data which the parser won't be able to handle.

Proposed solution

Avoid logging POST upload requests.

This fixes the problem completely on my end and I'm now able to perform upload requests without issues while using expose.

If logging these requests is a necesity then another approach must be used as to only grab part of request and avoiding parsing the whole body and it is being done right now.

I hope this helps and ends up being useful for you.

Related tickets

Logging a POST request containing an upload payload ends up failing as the logging process involves parsing the whole body content.

It’s best to identify these request and just avoid logging them at all.
@mpociot
Copy link
Member

mpociot commented Mar 2, 2021

You can already customize the settings up to which payload size you want to log requests.
Isn't that what you want?

https://github.com/beyondcode/expose/blob/master/config/expose.php#L119

@amazeika
Copy link
Author

amazeika commented Mar 3, 2021

You can already customize the settings up to which payload size you want to log requests.
Isn't that what you want?

https://github.com/beyondcode/expose/blob/master/config/expose.php#L119

Hi, by the time those checks kick the request has already been parsed => https://github.com/beyondcode/expose/blob/1.5.1/app/Logger/LoggedRequest.php#L75

The call that's causing problems is this one => https://github.com/beyondcode/expose/blob/1.5.1/app/Logger/LoggedRequest.php#L69 which passes the whole request body through the CurlFormatter.

I see that in master it is a bit different but I still see the same problem happening. The calls go as follows:

  1. https://github.com/beyondcode/expose/blob/master/app/Client/Http/HttpClient.php#L52
  2. https://github.com/beyondcode/expose/blob/master/app/Logger/RequestLogger.php#L40
  3. https://github.com/beyondcode/expose/blob/master/app/Logger/RequestLogger.php#L77
  4. https://github.com/beyondcode/expose/blob/master/app/Logger/LoggedRequest.php#L66

My PR isn't complete, ideally you should remove the file part of the request as you certainly do not want on any circumstances passing the whole bin part of the body through any parser or logger.

Another thing on why I didn't consider that setting on config is that it says that's response related. Here the issue in question is the request, not the response. If this setting is also supposed to affect the request logging perhaps it should be called different. In any case and as I pointed out above, this setting won't currently have an effect on the problem.

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.

2 participants