Navigation Menu

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
Closed

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

harshavardhana opened this issue Jan 18, 2016 · 5 comments
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@harshavardhana
Copy link
Contributor

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
Copy link

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

@bradfitz
Copy link
Contributor

Related: #10767 #9348 #8384 #7912 #4800 #4385

@bradfitz
Copy link
Contributor

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

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
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
Copy link

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

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016
@golang golang locked and limited conversation to collaborators Oct 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants