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

net/http: redirect code is not handled for DELETE requests. #13994

Closed
harshavardhana opened this issue Jan 18, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@harshavardhana
Copy link
Contributor

commented Jan 18, 2016

redirection is only handled for GET, HEAD, PUT and POST requests. But not for 'DELETE' , this leads to an issue where in a 'DELETE' request with proper response from the server doesn't honor redirect.

Following code reproduces this problem.

$ cat server.go
package main

import (
    "fmt"
    "log"
    "net/http"
)

func main() {
    http.HandleFunc("/bucket/redirect", func(w http.ResponseWriter, r *http.Request) {
        msg := fmt.Sprintf("Successful redirect. for method %s", r.Method)
        w.Write([]byte(msg))
    })
    http.HandleFunc("/bucket", func(w http.ResponseWriter, r *http.Request) {
        w.Header().Set("Location", "http://localhost:8080/bucket/redirect")
        w.WriteHeader(http.StatusTemporaryRedirect)
    })

    log.Fatal(http.ListenAndServe(":8080", nil))
}
$ cat client.go
package main

import (
    "bytes"
    "fmt"
    "log"
    "net/http"
)

func main() {
    clnt := &http.Client{}

    req, err := http.NewRequest("GET", "http://localhost:8080/bucket", nil)
    if err != nil {
        log.Fatalln(err)
    }

    resp, err := clnt.Do(req)
    if err != nil {
        log.Fatalln(err)
    }

    // Write response.
    var bufferGet bytes.Buffer
    resp.Write(&bufferGet)

    fmt.Println("--- GET RESPONSE ---")
    fmt.Println(string(bufferGet.Bytes()))
    fmt.Println("")

    req, err = http.NewRequest("DELETE", "http://localhost:8080/bucket", nil)
    if err != nil {
        log.Fatalln(err)
    }

    resp, err = clnt.Do(req)
    if err != nil {
        log.Fatalln(err)
    }

    // Write response.
    var bufferDelete bytes.Buffer
    resp.Write(&bufferDelete)

    fmt.Println("--- DELETE RESPONSE ---")
    fmt.Println(string(bufferDelete.Bytes()))
}

Now running this client against the server.go

$ go run client.go
--- GET RESPONSE ---
HTTP/1.1 200 OK
Content-Length: 35
Content-Type: text/plain; charset=utf-8
Date: Mon, 18 Jan 2016 09:33:21 GMT

Successful redirect. for method GET

--- DELETE RESPONSE ---
HTTP/1.1 307 Temporary Redirect
Content-Type: text/plain; charset=utf-8
Date: Mon, 18 Jan 2016 09:33:21 GMT
Location: http://localhost:8080/bucket/redirect
Content-Length: 0

The problem seems to be in client.Do()

func (c *Client) Do(req *Request) (resp *Response, err error) {
        method := valueOrDefault(req.Method, "GET")
        if method == "GET" || method == "HEAD" {
                return c.doFollowingRedirects(req, shouldRedirectGet)
        }
        if method == "POST" || method == "PUT" {
                return c.doFollowingRedirects(req, shouldRedirectPost)
        }
        return c.send(req, c.deadline())
}

Is there a specific reason why DELETE is not handled?.

Tested with curl seems to work fine

$ curl -i -L -X DELETE localhost:8080/bucket
HTTP/1.1 307 Temporary Redirect
Location: http://localhost:8080/bucket/redirect
Date: Mon, 18 Jan 2016 09:39:00 GMT
Content-Length: 0
Content-Type: text/plain; charset=utf-8

HTTP/1.1 200 OK
Date: Mon, 18 Jan 2016 09:39:00 GMT
Content-Length: 38
Content-Type: text/plain; charset=utf-8

Successful redirect. for method DELETE

Also verified in RFC7231 - https://tools.ietf.org/html/rfc7231#section-4.3.5, doesn't talk anything specific about redirects for 'DELETE'.

Thanks for your inputs.

@gopherbot

This comment has been minimized.

Copy link

commented Jan 18, 2016

CL https://golang.org/cl/18706 mentions this issue.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented May 11, 2016

I don't think CL 18706 is correct.

DELETE is not a safe method: https://tools.ietf.org/html/rfc7231#section-4.2.1

Per https://tools.ietf.org/html/rfc7231#section-6.4 ...

Automatic redirection needs to done with
care for methods not known to be safe, as defined in Section 4.2.1,
since the user might not wish to redirect an unsafe request

It's not correct to treat DELETE the same as a GET or HEAD.

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Go1.7 May 11, 2016

@harshavardhana

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2016

DELETE is not a safe method: https://tools.ietf.org/html/rfc7231#section-4.2.1

Per https://tools.ietf.org/html/rfc7231#section-6.4 ...

Automatic redirection needs to done with
care for methods not known to be safe, as defined in Section 4.2.1,
since the user might not wish to redirect an unsafe request
It's not correct to treat DELETE the same as a GET or HEAD.

Such is the case for PUT and POST as well, wouldn't it be safe to allow a way to set unsafe Methods to be redirected if necessary? and by default only do "GET" and "HEAD".

gopherbot pushed a commit that referenced this issue Sep 26, 2016

net/http: add Client tests for various 3xx redirect codes
Updates #13994
Updates #16840

Change-Id: Ia3cad5c211e0c688a945ed6b6277c2552592774c
Reviewed-on: https://go-review.googlesource.com/29760
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Oct 3, 2016

CL https://golang.org/cl/29852 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.