Skip to content

[HttpFoundation] Route not found when URIs contains subdomain (virtualhosts) #39882

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
BafS opened this issue Jan 18, 2021 · 4 comments
Closed

[HttpFoundation] Route not found when URIs contains subdomain (virtualhosts) #39882

BafS opened this issue Jan 18, 2021 · 4 comments
Labels
Bug HttpFoundation Status: Needs Review

Comments

@BafS
Copy link
Contributor

BafS commented Jan 18, 2021 •
edited
Loading

Symfony version(s) affected: ^5.1.9

Description

#38614 introduce a BC when using URL rewriting and that the URI contains the same string as the subdomain.

How to reproduce

  1. Setup URL rewriting from domain.com/index.php/api to api.domain.com
  2. Add a route containing /api/ (for example @Route("/foo/api/bar"))
  3. Go to api.domain.com/foo/api/bar, you will have a 404

Possible Solution

Change the if in the PR to not change $basename when the URI contains the subdomain where strpos !== 0, basically revert the PR.
@BafS BafS added the Bug label Jan 18, 2021
@nicolas-grekas
Copy link
Member

/cc @mvorisek could you please have a look?

Alternatively @BafS maybe you know how to fix this and you can send a PR?

@BafS
Copy link
Contributor Author

BafS commented Jan 18, 2021

I don't think there is an easy way to have both behaviors.

This previous data used to pass (if you add it in RequestTest::getBaseUrlData() with Symfony <= 5.1.8)

            [
                '/foo/api/bar',
                [
                    'SCRIPT_FILENAME' => '/home/user/public_html/api/index.php',
                    'SCRIPT_NAME' => '/api/index.php',
                    'PHP_SELF' => '/api/index.php',
                ],
                '',
                '/foo/api/bar',
            ],

and since the PR it doesn't, the current workaround is simply to extends the Request and revert the prepareBaseUrl() changes.

@nicolas-grekas
Copy link
Member

OK, then I'd be for reverting #38614, and maybe adding a test case to prevent a regression and document the expected behavior. Can you submit one @BafS, please?

@mvorisek please let us know if you think there is a better way.

Right now, our policies would tell us to preserve BC with 4.4.

@BafS BafS mentioned this issue Jan 18, 2021
Merged
@mvorisek
Copy link
Contributor

It is always a guess. I think we should add support to some specific enviroment variable which webserver can set. As discussed in #38614, the standard server enviroment variables are not enough.

fabpot added a commit that referenced this issue Jan 18, 2021
@fabpot
bug #39886 [HttpFoundation] Revert #38614 and add assert to avoid reg…
6dd5078 
…ressions (BafS)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpFoundation] Revert #38614 and add assert to avoid regressions

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #39882
| License       | MIT

#38614 introduced a BC, this PR revert the PR, update tests and add an assert to avoid regressions.

Commits
-------

3058cd0 Revert #38614, add assert to avoid regression
@fabpot fabpot closed this as completed Jan 18, 2021
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 Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @nicolas-grekas @BafS @mvorisek @carsonbot