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

Combining redirectors #1774

Closed
Mis1eader-dev opened this issue Sep 11, 2023 · 9 comments
Closed

Combining redirectors #1774

Mis1eader-dev opened this issue Sep 11, 2023 · 9 comments

Comments

@Mis1eader-dev
Copy link
Member

Is your feature request related to a problem? Please describe.
Currently there are two redirector plugins, SSL Redirector and Slash Remover. I plan on adding another redirector for prepending www. to the url if it isn't present, and redirect IP address url bar to the same domain. This makes total redirects consist of:

  1. HTTP -> HTTPS
  2. /home/ -> /home
  3. 1.2.3.4 -> www.example.com
  4. example.com -> www.example.com

However, each one acts independently from one another if used in the form of plugins. Navigating to the URL 1.2.3.4/home/ would require too many redirects:

  1. 1.2.3.4/home/ -> https://1.2.3.4/home/ (SSL)
  2. https://1.2.3.4/home/ -> https://1.2.3.4/home (Slash)
  3. https://1.2.3.4/home -> https://www.example.com/home (IP rewrite)

This creates lots of unnecessary requests and redirect responses when it could have done so in one pass.

Describe the solution you'd like
A way to pipeline the request through all redirector rules and each that match the criteria can process the URL and pass it to the next redirector in line, until all redirectors have finished processing the URL, then do one response for redirection.

Describe alternatives you've considered
To take the code of SSL Redirector and Slash Remover plugins and put them into one big plugin that handles all of these with a single point of response.

@an-tao
Copy link
Member

an-tao commented Sep 11, 2023

@Mis1eader-dev Thank you for your feedback. I think you can set the "redirect" option of the SlashRemover plugin to false to avoid redirecting twice.

@Mis1eader-dev
Copy link
Member Author

Mis1eader-dev commented Sep 11, 2023

Yes you are right, the use case is for cookies though, adding more redirect rules requires more redirects, if the user has logged in on example.com, then the cookie is only visible on example.com, if they go to www.example.com then they are treated as a different site.
If we want to have a single point of entry for all users, then redirecting is needed.

The Slash Remover plugin isn't an issue for cookies, but it does add one redirect if we want a clean looking URL.

I'm thinking of doing something similar to filters, or combine all redirector plugins under the name Redirector, what do you think?

@Mis1eader-dev
Copy link
Member Author

Adding to the www. prefix redirector, there seems to be two different preferences, either your website prefers www. or it prefers non-www., two good examples are:

  1. Google prefers prefixing with www., google.com redirects to www.google.com
  2. GitHub prefers non-www., github.com redirects to github.com

I plan on adding this redirector as a PR, but we have to decide whether it is made as a separate plugin or the pull request combines all existing redirectors and adds the subdomain redirector all within one plugin.

@Mis1eader-dev
Copy link
Member Author

Mis1eader-dev commented Sep 11, 2023

Google redirects http://google.com:443 to https://www.google.com, but drogon's SSL Redirector doesn't do redirects on http://www.example.com:443, it is an edge case worth mentioning. The response is ERR_EMPTY_RESPONSE

@an-tao
Copy link
Member

an-tao commented Sep 12, 2023

Yes you are right, the use case is for cookies though, adding more redirect rules requires more redirects, if the user has logged in on example.com, then the cookie is only visible on example.com, if they go to www.example.com then they are treated as a different site. If we want to have a single point of entry for all users, then redirecting is needed.

The Slash Remover plugin isn't an issue for cookies, but it does add one redirect if we want a clean looking URL.

I'm thinking of doing something similar to filters, or combine all redirector plugins under the name Redirector, what do you think?

This is a really nice idea and we can discuss the implementation details further.

@Mis1eader-dev
Copy link
Member Author

Here are two ways of implementing this optimization

  1. The easiest way is to make it all into one plugin called Redirector, it can have its config look like the following:
"enable_ssl_redirection": true,
"ssl_redirector_config": {
    "exempt": ...
},
"enable_slash_removal": true,
"slash_remover_config": {
    "redirect": false
},
"enable_subdomain_redirection": true,
"subdomain_redirector_config": {
    // if subdomain is empty, then redirect to non-www
    "preferred_subdomain": "www",
    // if set to true, it will match all subdomains that don't match against registered controller paths and regexes
    // e.g. any.example.com -> www.example.com
    "redirect_invalid_subdomains": true
},
"enable_ip_rewrite": true,
"ip_rewriter_config": {
    // replace IP with domain
    "domain": "example.com"
}

With the above, it is easy to chain the redirect process so each enabled redirector will modify the response and at the end send it as one response.
However, we must throw out regexes so as to not decrease the checker performance, as we will be dealing with multiple check points.
This also allows us to know the order in which the chain goes through, with room for more optimization.
I'm thinking of its implementation in the following fashion:

registerSyncAdvice([](req)
{
    HttpResponsePtr resp = nullptr;

    const auto &host = req->getCookie("host");

    if(host.starts_with("http"))
    {
        resp = newRedirectResp(...);
    }

    if(isIP(host) && ipRewriter.domain.length() < host.length())
    {
        if(resp)resp = resp->location().replace(..., ipRewriter.domain);
        else resp = newRedirect..
    }

    ...
});

Another implementation is to have an empty string at the beginning, we reserve some space for it by knowing which redirector is enabled and what the final string may look like, and start appending different strings to it as if it is going through a car manufacturer assembly line.
At the end check if string is not empty, then return a redirect with that string.
I prefer this one, it will perform fastest.

  1. We can have a std::vector of redirect processors, with a processor being a std::function<void (const HttpRequestPtr&, string&)>, what these functions do is check for certain parts of the request based on their individual criterias, and set the string to what they see fit, or a responsePtr& to set or initialize if they are the first to match the criteria.

We can go with the first one, and keep each redirector as a separate file, with the redirect and checking logic done inside of them, and the redirector plugin will call these sequentially and come to a conclusion at the end whether the string variable is empty, if not, then redirect to that string.

@Mis1eader-dev
Copy link
Member Author

This is an idea:

string url;
url.reserve(
    sslRedirector.len + // 8 (https://)
    subdomainRedirector.len + // subdomain.length()
    ipRewriter.len + // domain.length()
    req->path().length() // can't assume slash remover yet
);

SecureSSLRedirector::process(req, url);
SubdomainRedirector::process(req, url);
IPRewriter::process(req, url);
SlashRemover::process(req, url);

return url.empty() ? nullptr : HttpResponse::newRedirectResponse(url);

@an-tao
Copy link
Member

an-tao commented Sep 12, 2023

This is an idea:

string url;
url.reserve(
    sslRedirector.len + // 8 (https://)
    subdomainRedirector.len + // subdomain.length()
    ipRewriter.len + // domain.length()
    req->path().length() // can't assume slash remover yet
);

SecureSSLRedirector::process(req, url);
SubdomainRedirector::process(req, url);
IPRewriter::process(req, url);
SlashRemover::process(req, url);

return url.empty() ? nullptr : HttpResponse::newRedirectResponse(url);

Considering that plugins need to be able to be loaded and unloaded based on configuration, we need to think about how to determine which AOP advice is the last one.

@an-tao
Copy link
Member

an-tao commented Sep 13, 2023

@Mis1eader-dev
I made PR for this issue, please check it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants