Skip to content

fix: the query string is repeated twice when enable http-to-https and append-query-string (#7394)#7433

Merged
spacewander merged 3 commits intoapache:masterfrom
monkeyDluffy6017:fix-the-query-string-to-be-repeated-twice
Jul 14, 2022
Merged

fix: the query string is repeated twice when enable http-to-https and append-query-string (#7394)#7433
spacewander merged 3 commits intoapache:masterfrom
monkeyDluffy6017:fix-the-query-string-to-be-repeated-twice

Conversation

@monkeyDluffy6017
Copy link
Copy Markdown
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)

| 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done



=== TEST 26: enable http_to_https with upstream
=== TEST 26: wrong configure, enable http_to_https with append_query_string
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

bug: enable both http_to_https and append_query_string of the redirect plugin will cause the query string to be repeated twice

4 participants