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

bug: http_to_https ret_code 307 not working #2257

Closed
TigerGxl opened this issue Sep 18, 2020 · 7 comments · Fixed by #2311
Closed

bug: http_to_https ret_code 307 not working #2257

TigerGxl opened this issue Sep 18, 2020 · 7 comments · Fixed by #2311
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@TigerGxl
Copy link

Issue description

Environment

  • apisix version (cmd: apisix version):1.5
  • OS:ubuntu16.04

i use plugin http_to_https and set ret_code 307, like below.

{
  "priority": 0,
  "plugins": {
    "redirect": {
      "ret_code": 307,
      "http_to_https": true
    }
  },
  "uris": [
    "/*"
  ],
  "id": "1a5f4337d",
  "hosts": [
    "demo-apisix.xxxxxxx.com"
  ],
  "upstream": {
    "nodes": {
      "172.16.3.2:80": 100
    },
    "hash_on": "vars",
    "type": "roundrobin"
  }
}

when client sends POST reqeust with http.it's not working, apisix should return 307 and client sends a new request with https again, in fact, apisix return 301 and client sends a new request with https and the method is GET

image
image

@TigerGxl
Copy link
Author

TigerGxl commented Sep 21, 2020

启用http跳转https后,设置状态码为307不生效.
etcd中存储的状态码是307,但是实际跳转的时候是301,导致原本POST的method在301跳转后变为GET

@membphis
Copy link
Member

that is a bug. here is the test case. welcome PR to fix it

=== TEST 1: add plugin with new uri: /test/add
--- config
    location /t {
        content_by_lua_block {
            local t = require("lib.test_admin").test
            local code, body = t('/apisix/admin/routes/1',
                ngx.HTTP_PUT,
                [[{
                    "plugins": {
                        "redirect": {
                            "http_to_https": true,
                            "ret_code": 307
                        }
                    },
                    "uri": "/hello"
                }]]
                )

            if code >= 300 then
                ngx.status = code
            end
            ngx.say(body)
        }
    }
--- request
GET /t
--- response_body
passed
--- no_error_log
[error]



=== TEST 2: redirect
--- request
GET /hello
--- response_headers
Location: /test/add
--- error_code: 307
--- no_error_log
[error]

@membphis membphis changed the title request help: http_to_https ret_code 307 not working bug: http_to_https ret_code 307 not working Sep 24, 2020
@membphis membphis added bug Something isn't working good first issue Good for newcomers labels Sep 24, 2020
@apache apache deleted a comment from membphis Sep 24, 2020
@moonming
Copy link
Member

@liuhengloveyou please take a look

@gy09535
Copy link
Contributor

gy09535 commented Sep 25, 2020

I thinks when we send redirect to 301 ,some browser default change the request to get, so we should add some logical to handle post request , when the request is post we should send the ret_code to 307, according to https://github.com/apache/apisix/blob/master/apisix/plugins/redirect.lua#L136

@liuhengloveyou
Copy link
Contributor

liuhengloveyou commented Sep 25, 2020

If the user has explicitly configured ret_code, we should not change it.
Unless the configuration is wrong, we should indicate the range in the documentation.

I think the range of should be 400 > ret_code >= 300?

@spacewander
Copy link
Member

@gy09535
I think you mean 308 but not 307: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/308
Code 307 is 'Temporary'.

@gy09535
Copy link
Contributor

gy09535 commented Sep 25, 2020

@gy09535
I think you mean 308 but not 307: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/308
Code 307 is 'Temporary'.

yeah, should be 308

starsz added a commit to starsz/apisix that referenced this issue Oct 29, 2020
starsz added a commit to starsz/apisix that referenced this issue Oct 29, 2020
starsz added a commit to starsz/apisix that referenced this issue Oct 29, 2020
starsz added a commit to starsz/apisix that referenced this issue Oct 29, 2020
starsz added a commit to starsz/apisix that referenced this issue Oct 29, 2020
starsz added a commit to starsz/apisix that referenced this issue Oct 29, 2020
starsz added a commit to starsz/apisix that referenced this issue Oct 29, 2020
starsz added a commit to starsz/apisix that referenced this issue Oct 30, 2020
spacewander pushed a commit that referenced this issue Oct 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants