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: the query string is repeated twice when enable http-to-https and append-query-string (#7394) #7433

Conversation

monkeyDluffy6017
Copy link
Contributor

Description

Fix bug that the query string is repeated twice when enable http-to-https and append-query-string

Fixes #7394

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

apisix/plugins/redirect.lua Show resolved Hide resolved
@@ -34,7 +34,7 @@ The `redirect` Plugin can be used to configure redirects.

| Name | Type | Required | Default | Valid values | Description |
|---------------------|---------------|----------|---------|--------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| http_to_https | boolean | False | false | | When set to `true` and the request is HTTP, it will be redirected to HTTPS with the same URI with a 301 status code. |
| http_to_https | boolean | False | false | | When set to `true` and the request is HTTP, it will be redirected to HTTPS with the same URI with a 301 status code, contains the query string. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| http_to_https | boolean | False | false | | When set to `true` and the request is HTTP, it will be redirected to HTTPS with the same URI with a 301 status code, contains the query string. |
| http_to_https | boolean | False | false | | When set to `true` and the request is HTTP, it will be redirected to HTTPS with the same URI with a 301 status code. Note the querystring from the raw URI will also be contained in the Location header. |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 46 to 47
Only one of `http_to_https`, `uri` and `regex_uri` can be configured.
Only one of `http_to_https` and `append_query_string` can be configured.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Only one of `http_to_https`, `uri` and `regex_uri` can be configured.
Only one of `http_to_https` and `append_query_string` can be configured.
* Only one of `http_to_https`, `uri` and `regex_uri` can be configured.
* Only one of `http_to_https` and `append_query_string` can be configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

--- no_error_log
[error]



=== TEST 26: enable http_to_https with upstream
=== TEST 26: wrong configure, enable http_to_https with append_query_string
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change the test case position to the end of the file (so there won't' have many test number changes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

oneOf = {
{required = {"uri"}},
{required = {"regex_uri"}},
{required = {"http_to_https"}}
Copy link
Member

Choose a reason for hiding this comment

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

["not"] = {required = {"client_cert_id"}}

We can use not operation to show the exclusive relationship?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried, but not succeed

["not"] = {
    allOf = {
        {required = {"append_query_string"}},
        {required = {"http_to_https"}},
    }
}  

and

["not"] = {required = {"append_query_string", "http_to_https"}},

Copy link
Member

Choose a reason for hiding this comment

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

http_to_https = {
    ["not"] = {required = {"uri", "regex_uri", "append_query_string"}}
},

uri = {
    ["not"] = {required = {"regex_uri", "http_to_https"}}
},

regex_uri = {
    ["not"] = {required = {"uri", "http_to_https"}}
},

what about this form?

Copy link
Member

Choose a reason for hiding this comment

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

Give it a try:

    oneOf = {
        {required = {"uri"}},
        {required = {"regex_uri"}},
        {required = {"http_to_https"}, ["not"] = {
           required = {"append_query_string"}
        }}
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Give it a try:

    oneOf = {
        {required = {"uri"}},
        {required = {"regex_uri"}},
        {required = {"http_to_https"}, ["not"] = {
           required = {"append_query_string"}
        }}
    }

It does work, i have updated, please check @spacewander

@spacewander spacewander merged commit b47317d into apache:master Jul 14, 2022
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants