Skip to content

[SecurityBundle] Don't break security.http_utils service decoration #60803

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

Merged
merged 1 commit into from
Jun 27, 2025

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Jun 16, 2025

Q A
Branch? 7.4
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

Actually, this service is really hard to override. My use case: I want to preserve a parameter in the URL. I ended with the following code, and I'm not happy with it

<?php

namespace App\Security\HttpUtils;

use App\Site\SiteContext;
use Symfony\Component\DependencyInjection\Attribute\AsDecorator;
use Symfony\Component\DependencyInjection\Attribute\Autowire;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestMatcherInterface;
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
use Symfony\Component\Routing\Matcher\UrlMatcherInterface;
use Symfony\Component\Security\Http\HttpUtils as SymfonyHttpUtils;

#[AsDecorator('security.http_utils')]
final class HttpUtils extends SymfonyHttpUtils
{
    public function __construct(
        private readonly SiteContext $siteContext,
        ?UrlGeneratorInterface $urlGenerator = null,
        #[Autowire(service: 'router')]
        UrlMatcherInterface|RequestMatcherInterface|null $urlMatcher = null,
        // See https://github.com/symfony/symfony/pull/60803
        ?string $domainRegexp = null,
        ?string $secureDomainRegexp = null,
    ) {
        parent::__construct($urlGenerator, $urlMatcher, $domainRegexp, $secureDomainRegexp);
    }

    public function generateUri(Request $request, string $path): string
    {
        $uri = parent::generateUri($request, $path);

        $site = $this->siteContext->currentSite;
        if (!$site) {
            return $uri;
        }

        $position = strpos($uri, '?');
        if ($position === false) {
            return $uri . '?site=' . $site->id;
        }

        parse_str(parse_url($uri, PHP_URL_QUERY) ?? '', $queryParams);
        if (array_key_exists('site', $queryParams)) {
            return $uri;
        }

        return substr($uri, 0, $position + 1) . 'site=' . $site->id . '&' . substr($uri, $position + 1);
    }

}

This is not a real decorator. And I cannot do that, since the original service call itself


Caution

I believe my patch is good, but with it I cannot achieve anymore what I need to do 😬

Instead, I could change the class with a compiler pass

@nicolas-grekas
Copy link
Member

I believe my patch is good, but with it I cannot achieve anymore what I need to do 😬

I don't understand what this means, can you expand a bit please?

@lyrixx
Copy link
Member Author

lyrixx commented Jun 19, 2025

It's about this line. The decoration does not work here.

@nicolas-grekas
Copy link
Member

It's about this line. The decoration does not work here.

So this PR won't fix your issue, isn't it?

Instead, I could change the class with a compiler pass

Indeed.

So let's close here?

@lyrixx
Copy link
Member Author

lyrixx commented Jun 19, 2025

So this PR won't fix your issue, isn't it?

I confirm it won't fix. I did not chose decoration, but inheritance to fix it.


We could close it, but I think the current code is wrong because it you decorate the service, everything is badly wired. This patch fix it

@nicolas-grekas
Copy link
Member

Why don't you decorate the url-generator BTW?

@lyrixx
Copy link
Member Author

lyrixx commented Jun 19, 2025

Of course I did. But I had to change the http utils because of this, may be the error is there. I don't understand this comment TBH

@nicolas-grekas
Copy link
Member

Let's do this on 7.4 then since that's not really a bugfix but more of an improved behavior.

@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.4 Jun 27, 2025
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me on 7.4

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for 7.4 as this is likely to break some not-that-edge cases

@lyrixx lyrixx changed the base branch from 6.4 to 7.4 June 27, 2025 15:55
@lyrixx lyrixx force-pushed the security-http-utils branch from 896828e to a099ba2 Compare June 27, 2025 15:57
@lyrixx
Copy link
Member Author

lyrixx commented Jun 27, 2025

I updated the base branch and rebased

@nicolas-grekas
Copy link
Member

Thank you @lyrixx.

@nicolas-grekas nicolas-grekas merged commit be5ec1a into symfony:7.4 Jun 27, 2025
11 checks passed
@lyrixx lyrixx deleted the security-http-utils branch June 30, 2025 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants