8000 [HttpFoundation] [Runtime] Response can be written to during/after `KernelEvents::Terminate`-event in debug mode · Issue #59447 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] [Runtime] Response can be written to during/after KernelEvents::Terminate-event in debug mode #59447

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.

Sign up for GitHub

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

Jump to bottom
Closed
Benedikt-Brunner opened this issue Jan 9, 2025 · 4 comments
Closed

[HttpFoundation] [Runtime] Response can be written to during/after KernelEvents::Terminate-event in debug mode #59447

Benedikt-Brunner opened this issue Jan 9, 2025 · 4 comments
Labels
Bug HttpFoundation Runtime Status: Needs Review Status: Waiting feedback

Comments

@Benedikt-Brunner
Copy link
Benedikt-Brunner commented Jan 9, 2025 •
edited
Loading

Symfony version(s) affected

6.4.0-current

Description

With #52047 the behaviour of the HttpKernelRunner was adjusted, to not "close" the response before the KernelEvents::Terminate-event when in debug mode. It is therefore possible to write to both the response-content and -headers during/after the execution of KernelEvents::Terminate event-listeners when in debug mode.
While the need for a good debugging experience of code that is running during termination is valid, this does not suffice, in my opinion, to fundamentally break the behaviour of the HTTP stack when compared to a production environment.
As the response content has already been written to at this point, writing to it again will corrupt any response content, see below for a sanitized example. I have also attached an example of how this leads to differing reporting of, for example, HTTP status codes between the browser console and the symfony profiler.

How to reproduce

Following the contributors guide i have created a minimal reproduction: https://github.com/Benedikt-Brunner/Symfony_Write_After_Terminate_Reproduction

Possible Solution

As mentioned above i understand the need to debug code running in response to the Terminate event. I would still suggest disallowing writes to the response during/after the KernelEvents::Terminate-event, as this is the behaviour described in the documentation and should not be different in debug mode (See: https://symfony.com/doc/current/components/http_kernel.html#component-http-kernel-kernel-terminate).

Additional Context

Corrupted payload example:

# Written before KernelEvents::Terminate
{
  "SOME_ORIGINAL_PAYLOAD": "As there are two top level objects in this response, it is not valid JSON"
}
# Written during error handling of error in KernelEvents::Terminate
{
  "errors": [
    {
      "status": "400",
      "code": "SOME_ERROR_CODE",
      "title": "Bad Request",
      "detail": "Some error message",
      "meta": {
        "parameters": []
      }
    }
  ]
}

Inconsistent HTTP status code reporting example:

Browser:
Screenshot 2025-01-09 at 09 13 59

Profiler:
Screenshot 2025-01-09 at 09 12 29
@Benedikt-Brunner Benedikt-Brunner mentioned this issue Jan 9, 2025
Merged
6 tasks
@Benedikt-Brunner Benedikt-Brunner mentioned this issue Jan 18, 2025
Closed
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jan 21, 2025 •
edited
Loading

Can you provide a reproducer where the status code is different between browser and profiler?

For the current reproducer, it's precisely the behavior we allowed with #52047. Seeing the exception is still way better DX than silencing it IMHO.

Also, any other SAPIs than FPM/litespeed/frankenphp will exhibit the same behavior so there is no such guarantee as the one you mention about the terminate event IMHO. It's on apps to behave correctly (and Symfony to not make it hard to debug)

@Benedikt-Brunner
Copy link
Author

Hello @nicolas-grekas, thanks for taking the time to look into the issue.

I have added the mismatching-status-code-reporting-reproducer branch to my reproducer-repository. If you visit the test-page-route you can observe the mentioned mismatch in status code reporting between browser and profiler.

While i appreciate the insights into the inner workings of the response process, my objections to the behaviour stem mainly from this line in the documentation of the KernelEvents::Terminate-Event:

This event is dispatched after the response has been sent

This, at least to me, has the logical consequence (even if the implementation under the hood may not have actually sent the TCP-packages to the client), that the Response-object enters a sort of "Read-Only"-mode. Therefore preventing the behaviour described above.

As mentioned above i truly do appreciate the need for debugging opportunities in the termination stage, which as i am working in it i have also profited from. However I disagree that this is following the role symfony should take in the stack, it should present a simple and uniform interface for hooking into different parts of the request/response-lifecycle and not fundamentally change the behaviour of this lifecycle when entering the debug-mode.

I would be happy to explore other avenues of easing the debugging story in the termination environment, which to do interfere with the rest of the HTTP-stack.

Browser

Image

Profiler

Image

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jan 22, 2025 •
edited
Loading

OK, got it.

This event is dispatched after the response has been sent

That sentence is still true if you consider the response object returned by the controller.

If anything else happen in the terminate event that writes to the network stream, then we're back to what I said: we do want to see that error when debugging.

If an app does things with the response there, their problem, we cannot do anything but hide the problem. And that'd be the worse.

Let me close as I think we have the best possible behavior here, as explained.

@Benedikt-Brunner
Copy link
Author

Ok, thanks for taking the time to look into it anyway 👍

For anyone stumbling across this later on, we ended up working around this by effectively silencing away exceptions occurring at this stage of execution and implemented a separate system for giving the user insight into said errors. 👋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpFoundation Runtime Status: Needs Review Status: Waiting feedback
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nicolas-grekas @xabbuh @carsonbot @Benedikt-Brunner
0