8000 [FrameworkBundle][HttpKernel] Restrict stateless reporting to exception only by mtarld · Pull Request #36321 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle][HttpKernel] Restrict stateless reporting to exception only #36321

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
wants to merge 1 commit into from
Closed

[FrameworkBundle][HttpKernel] Restrict stateless reporting to exception only #36321

wants to merge 1 commit into from

Conversation

mtarld
Copy link
Contributor
@mtarld mtarld commented Apr 2, 2020
Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
License MIT

This PR is related to #35732 which is reporting session usage when stateless is specified.
In debug mode, it throws an UnexpectedSessionUsageException whereas in "no debug mode", it only logs that session is unexpectedly used.

The current PR proposes to replace the log by an exception in the "no debug mode".
In that way, if session is unexpectedly used in production mode, it will throw an exception and prevent more serious problems such as serving a cached page with personal data.

WDYT ?

@mtarld mtarld force-pushed the feature/stateless-only-exception branch from bf3ee5d to 369872a Compare April 2, 2020 16:13
@nicolas-grekas
Copy link
Member

I was wondering if we shouldn't rename "debug" to "strict".
But removing the mode altogether prevents seamless migrations.

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 2, 2020
@mtarld
Copy link
Contributor Author
mtarld commented Apr 2, 2020

I like the strict idea.

Maybe we could add a strict configuration parameter with a default fallback too true ?

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 2, 2020 •
edited
Loading

yes, a config entry looks like an idea to consider to me
it would default to %kernel.debug% actually

@mtarld
Copy link
Contributor Author
mtarld commented Apr 2, 2020

yes, a config entry looks like an idea to consider to me
it would default to %kernel.debug% actually

Even better yes, I'll give a try

@mtarld mtarld force-pushed the feature/stateless-only-exception branch from 369872a to 2d20edb Compare April 3, 2020 05:51
@mtarld
Copy link
Contributor Author
mtarld commented Apr 3, 2020 •
edited
Loading

With this implementation, you'll have the following configuration available:

session:
    strict_stateless: true

With default configuration to %kernel.debug%

@mtarld mtarld force-pushed the feature/stateless-only-exception branch from 2d20edb to 3f38bdf Compare April 3, 2020 06:11
@javiereguiluz
Copy link
Member
javiereguiluz commented Apr 3, 2020 •
edited
Loading

Thanks for creating this PR. The thing I don't like about the current implementation is that stateless is unreliable. If you do this:

if (true === $request->attributes->get('stateless')) {
    $response->setSharedMaxAge(3600);
}

The problem is that, if the route uses the session, you may be caching a page with private information and serving it to anyone. That may be a very serious problem.

I don't like the idea of making this behavior configurable either. You said you weren't going to use the session ... so if you use it by mistake, the application should fail.

I think the transition will be OK, because developers will see the exceptions locally and fix them. So, in the remote case that you use the session in production when you shouldn't, I think that throwing an exception is better than silencing this error.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 3, 2020 •
edited
Loading

The problem is that, if the route uses the session, you may be caching a page with private information and serving it to anyone. That may be a very serious problem.

How can this happen? That's not possible to me, we have a mechanism that prevents it already.

We know by experience that it's very easy to use the session by mistake. For ppl that inadvertently render() a controller that uses the session, would they prefer a broken prod, or an automatic "cache-control: private"? The current way (since always) is the 2nd option and this should remain to me. With a line in the logs ftw. In dev, sure, I want an exception.

@javiereguiluz
Copy link
Member

After discussing privately with Nicolas ... my main concern is now gone. In the previous example, Symfony will set "cache private" automatically because the session was used, so it will ignore our "smaxage" setting.

So, can anyone spot another issue for using the session in a controller that said that it was not going to use the session? Thanks!

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 3, 2020 •
edited
Loading

Shall we close this PR then? One less option FTW.

Here is an excerpt of our conversation for the record:

Q:

if your app is not ready for this option ... you shouldn't need to worry about strict or not ... you just don't add stateless to your routes. do that only when you are ready, right?

A:

Nope: You declare it first, then you achieve it. And you know when you failed at it by logs or exceptions on dev. The only outcome of failing to comply is that cache-control private will be sent, exactly like today, which means you're safe.
A performance loss is a much better outcome than a broken prod.

Q:

In this code:
return $response->setSharedMaxAge(3600);
the cache private is automatically set?

A:

Yes: the session listener comes after and forces a private cc when the session is used, because nobody is able to correctly manage cc headers without privacy risks. That is the point of the new feature: Symfony protects privacy by default, but how do you achieve great cacheability? By patiently following the reports of the stateless mode; without breaking your prod; business is more important than cacheability all the time.

@mtarld
Copy link
Contributor Author
mtarld commented Apr 3, 2020

IMHO, some Symfony users may want to actually throw an exception even on production. In this way they'll be a hundred percent sure that session isn't used.

@nicolas-grekas
Copy link 8000
Member

@mtarld my stand:

business is more important than cacheability all the time.

@mtarld
Copy link
Contributor Author
mtarld commented Apr 3, 2020

Fair enough. So if it shouldn't be configurable, we can close this PR I guess.

@mtarld mtarld closed this Apr 3, 2020
@mtarld mtarld deleted the feature/stateless-only-exception branch April 3, 2020 17:45
@wouterj wouterj mentioned this pull request Apr 4, 2020
Merged
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

Assignees
No one assigned
Labels
Feature FrameworkBundle HttpKernel Status: Needs Review
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
4 participants
@mtarld @nicolas-grekas @javiereguiluz @carsonbot
0