Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[WebProfilerBundle] Fix interception for non conventional redirects#59033
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[WebProfilerBundle] Fix interception for non conventional redirects#59033
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedNov 29, 2024
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
-1 for this PR for now
src/Symfony/Bundle/WebProfilerBundle/EventListener/WebDebugToolbarListener.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| @@ -99,13 +99,19 @@ public function onKernelResponse(ResponseEvent $event): void | |||
| return; | |||
| } | |||
| // do not capture redirects done in a non-conventional way (http-equiv="refresh" tag,...). | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
are you actually returning a response using the HTTP status code of the redirection without the header providing the redirection location ?
http-equiv="refresh" isnot a redirection (and so to me, it looks like it should be a 200 status code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
are you actually returning a response using the HTTP status code of the redirection without the header providing the redirection location ?`
yes that's it!
// Metatag template redirectionif (!is_null($cachedLink->metatag)) { // As we need to have custom meta tags, we need an intermediary template // Redirection will be done either by browser or in JS $response = $this->render('app/redirects/metatag.html.twig', [ 'metatag' => $cachedLink->metatag, 'redirectUrl' => $data['redirectUrl'], ]); $response->setStatusCode(Response::HTTP_FOUND); foreach ($data['headers'] as $key => $value) { $response->headers->set($key, $value); } return $response;}and the template has thehttp-equiv="refresh" tag.
for me it's because I develop an URL shortener, and this method allow to customize social media tags before the redirect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So maybe you have an idea on how to handle this on the Symfony side ? or maybe I'm wrong by doing this but it works well.
In all cases there is an exception in such case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm still wondering why you use a redirection status code in your response when you don't properly return a redirection (you return an HTML page that will trigger another navigation, which isnot a HTTP redirection)
Yes there's something wrong! Let me fix this PR :) |
d5a5363 to7954c8dCompareLooking at the HTTP specification, the |
What about patching WebDebugToolbarListener.php instead? and not render this template if there is no Location? |
Friendly ping@Huluti |
7954c8d to416aa0eCompareThank you@Huluti. |
068105e intosymfony:6.4Uh oh!
There was an error while loading.Please reload this page.
This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [symfony/framework-bundle](https://symfony.com)([source](https://redirect.github.com/symfony/framework-bundle)) |`7.2.3` -> `7.2.4` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|| [symfony/messenger](https://symfony.com)([source](https://redirect.github.com/symfony/messenger)) | `7.2.3` ->`7.2.4` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|| [symfony/stopwatch](https://symfony.com)([source](https://redirect.github.com/symfony/stopwatch)) | `7.2.2` ->`7.2.4` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|| [symfony/web-profiler-bundle](https://symfony.com)([source](https://redirect.github.com/symfony/web-profiler-bundle)) |`7.2.3` -> `7.2.4` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>symfony/framework-bundle (symfony/framework-bundle)</summary>###[`v7.2.4`](https://redirect.github.com/symfony/framework-bundle/releases/tag/v7.2.4)[CompareSource](https://redirect.github.com/symfony/framework-bundle/compare/v7.2.3...v7.2.4)**Changelog**(symfony/framework-bundle@v7.2.3...v7.2.4)- bug[symfony/symfony#59198](https://redirect.github.com/symfony/symfony/issues/59198)\[Messenger] Filter out non-consumable receivers when registering`ConsumeMessagesCommand` (@​wazum)- bug[symfony/symfony#59781](https://redirect.github.com/symfony/symfony/issues/59781)\[Mailer] fix multiple transports default injection([@​fkropfhamer](https://redirect.github.com/fkropfhamer))- bug[symfony/symfony#59829](https://redirect.github.com/symfony/symfony/issues/59829)\[FrameworkBundle] Disable the keys normalization of the CSRF form fieldattributes ([@​sukei](https://redirect.github.com/sukei))- bug[symfony/symfony#59728](https://redirect.github.com/symfony/symfony/issues/59728)\[Form]\[FrameworkBundle] Use auto-configuration to make the defaultCSRF token id apply only to the app; not to bundles([@​nicolas-grekas](https://redirect.github.com/nicolas-grekas))</details><details><summary>symfony/messenger (symfony/messenger)</summary>###[`v7.2.4`](https://redirect.github.com/symfony/messenger/releases/tag/v7.2.4)[CompareSource](https://redirect.github.com/symfony/messenger/compare/v7.2.3...v7.2.4)**Changelog**(symfony/messenger@v7.2.3...v7.2.4)- bug[symfony/symfony#59198](https://redirect.github.com/symfony/symfony/issues/59198)\[Messenger] Filter out non-consumable receivers when registering`ConsumeMessagesCommand` (@​wazum)</details><details><summary>symfony/stopwatch (symfony/stopwatch)</summary>###[`v7.2.4`](https://redirect.github.com/symfony/stopwatch/releases/tag/v7.2.4)[CompareSource](https://redirect.github.com/symfony/stopwatch/compare/v7.2.2...v7.2.4)**Changelog**(symfony/stopwatch@v7.2.3...v7.2.4)- no significant changes</details><details><summary>symfony/web-profiler-bundle(symfony/web-profiler-bundle)</summary>###[`v7.2.4`](https://redirect.github.com/symfony/web-profiler-bundle/releases/tag/v7.2.4)[CompareSource](https://redirect.github.com/symfony/web-profiler-bundle/compare/v7.2.3...v7.2.4)**Changelog**(symfony/web-profiler-bundle@v7.2.3...v7.2.4)- bug[symfony/symfony#59776](https://redirect.github.com/symfony/symfony/issues/59776)\[WebProfilerBundle] fix rendering notifier message options([@​xabbuh](https://redirect.github.com/xabbuh))- bug[symfony/symfony#59033](https://redirect.github.com/symfony/symfony/issues/59033)\[WebProfilerBundle] Fix interception for non conventional redirects([@​Huluti](https://redirect.github.com/Huluti))</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR is behind base branch, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about theseupdates again.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR was generated by [Mend Renovate](https://mend.io/renovate/).View the [repository joblog](https://developer.mend.io/github/Runroom/archetype-symfony).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xNzYuMiIsInVwZGF0ZWRJblZlciI6IjM5LjE3Ni4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
Uh oh!
There was an error while loading.Please reload this page.