Skip to content
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

fix: route limit to subdomains does not work #5961

Merged
merged 3 commits into from May 8, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented May 5, 2022

Description
Fixes #5959

// Limit to any sub-domain
$routes->add('from', 'to', ['subdomain' => '*']); // not work

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • [] User guide updated
  • Conforms to style guide

kenjis added 2 commits May 5, 2022 09:22
`example.com` does not match`'subdomain' => '*'`, so the route is not registered.
@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label May 5, 2022
@iRedds
Copy link
Collaborator

iRedds commented May 5, 2022

Not sure if this will work correctly if the domain name is a first level domain.
The first-level domain can be used in internal (corporate) networks.

@kenjis
Copy link
Member Author

kenjis commented May 5, 2022

@iRedds What do you mean by "first level domain"?
Example?

@iRedds
Copy link
Collaborator

iRedds commented May 5, 2022

The local resource can be named mycompany.
Then the behavior will be incorrect for the api.mycompany subdomain.

@kenjis
Copy link
Member Author

kenjis commented May 5, 2022

It seems Brand top-level domains.
https://en.wikipedia.org/wiki/List_of_Internet_top-level_domains#Brand_top-level_domains

This method is not perfect from the beginning.

* Examines the HTTP_HOST to get a best match for the subdomain. It
* won't be perfect, but should work for our needs.

@iRedds
Copy link
Collaborator

iRedds commented May 5, 2022

Not perfect and not correct are two different cases.
I think subdomain parsing should be based on app.baseURL

@szajens
Copy link

szajens commented May 5, 2022

Tomorrow will show my solution, I have been using for several years. Because I'm leaving today

@szajens
Copy link

szajens commented May 5, 2022

        /**
         * domainFilter()
         *
         *
         * @param mixed $allowed_domain - allowed domain format ['example.net','subdomain.example.com'] , don't use http://
         * @param mixed $domain_to_check - input domain to check e.g. www.www.www.www.a1.g.h.r.r.e.e.e.e.e.e.d.subdomain.example.com
         *
         * @return array associates
         *
         *               if not found:
         *    ['matches'] type (int) == 0
         *    [0] == empty array()
         *
         *               if checked:
         *
         *    ['matches'] type (int) == 1
         *    [0][0] => www. - prefix
         *    [0][1] => a1.g.h.r.r.e.e.e.e.e.e.d - subdomain or null if not check
         *    [0][2] => subdomain.example.com - domain   <br>
         *
         *
         *
         */

        function domainFilter(array $allowed_domain, string $domain_to_check):array {

            $result = [];//only for IDE (warning undefined variable)

            $allowed_domain = implode('|', $allowed_domain);

            $result['matches'] = preg_match('/^(?:(w{3})\.)*(?:((?:[a-z0-9\-\_]+\.)*[a-z0-9\-\_]+)\.)*('.$allowed_domain.')$/', $domain_to_check, $result[0]);

            //remove first element from array, but first array return input param $allowed_domain
            array_shift($result[0]);

            return $result;
        }



        d(
            domainFilter(['crazy.sub.domain.com', 'ci.loc'], 'www.usersubdomain.crazy.sub.domain.com')
        );

        d(
            domainFilter(['crazy.sub.domain.com', 'ci.loc'], 'aaa.bbb.usersubdomain.crazy.sub.domain.com')
        );

domain-screenshottt

@szajens
Copy link

szajens commented May 5, 2022

link to source domain filter function

I use this function in other scripts.
I've been using CI for about a week or two.
I don't know him well yet,
issomuch I am not able to write this function for it myself.

p.s.
could be added Placeholders and Regular Expression to domain and subdomain

edit2: using hostname and subdomain together does not work at the moment
$routes->get('test','Test::index', ['subdomain' => '*', 'hostname' => 'ci.loc']);
*.ci.loc - not work

@kenjis
Copy link
Member Author

kenjis commented May 6, 2022

I believe this PR fixed the bug #5959. So this PR is done.

@iRedds The Bug with Brand top-level domains is another bug that exists already.

@szajens Thank you for sample code. Adding like that code is an enhancement. It is better to create another PR.
From the beginning, using $_SERVER['HTTP_HOST'] without validation is kind of vulnerability.

I think it is better to add hostname validation and logic that is more reliable to determine subdomains.
Contributions are welcome.

@szajens
Copy link

szajens commented May 6, 2022

That's right, because someone can dynamically replace the host.

The above code eliminates the Bug with Brand top-level domains

@szajens
Copy link

szajens commented May 6, 2022

@kenjis maybe you will create new PR,because my English no perfect 😁

@kenjis
Copy link
Member Author

kenjis commented May 6, 2022

I am not going to send any more PR on this matter.
It seems not so simple.
Ref #5807

@MGatner
Copy link
Member

MGatner commented May 7, 2022

Open to other enhancements and bugfixes, but I'm with kenjis that this addresses the issue at hand.

@kenjis kenjis merged commit a7fe439 into codeigniter4:develop May 8, 2022
@kenjis kenjis deleted the fix-route-subdomain branch May 8, 2022 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Limit to subdomains, not work
4 participants