Getting Warned for Passing a URI Variable Directly into http.Get

This is a typical small tip.

Well, until recently, I was writing code like this too, so I can't really judge others.

func get(rawuri string) ([]byte, error) {
    resp, err := http.Get(rawuri)
    if err != nil {
        return nil, err
    }
    defer resp.Body.Close()
    return ioutil.ReadAll(resp.Body)
}

If you write code that passes a URI string into the [http].Get() function as an unvalidated variable like this:

$ golangci-lint run --enable gosec
sample.go:11:15: G107: Potential HTTP request made with variable url (gosec)
        resp, err := http.Get(rawuri)
                     ^

You'll get scolded by the lint tool^[1]. By the way, if it's a literal string, you won't be scolded (regardless of the string's content)^[2].

Replacing it with a fixed string is out of the question as it would change the design, so there are two ways to avoid this.

One is to evaluate the URI string before using it.

If minimal evaluation is sufficient, you can revise it using the [url].Parse() function like this:

func get(rawuri string) ([]byte, error) {
-   resp, err := http.Get(rawuri)
+   u, err := url.Parse(rawuri)
+   if err != nil {
+       return nil, err
+   }
+   resp, err := http.Get(u.String())
    if err != nil {
        return nil, err
    }
    defer resp.Body.Close()
    return ioutil.ReadAll(resp.Body)
}

The other is to rewrite it to generate an [http].Request type using the [http].NewRequest() function or similar^[3]. For example, like this:

func get(rawuri string) ([]byte, error) {
-   resp, err := http.Get(rawuri)
+   req, err := http.NewRequest(http.MethodGet, rawuri, nil)
+   if err != nil {
+       return nil, err
+   }
+   resp, err := (&http.Client{}).Do(req)
    if err != nil {
        return nil, err
    }
    defer resp.Body.Close()
    return ioutil.ReadAll(resp.Body)
}

The reason why [http].NewRequest() doesn't trigger the warning seems to be because it internally uses the [url].Parse() function to convert the URI string into a [url].URL type.

But you know, if we actually look at the implementation of the [http].Get() function:

client.go

var DefaultClient = &Client{}

func Get(url string) (resp *Response, err error) {
	return DefaultClient.Get(url)
}

func (c *Client) Get(url string) (resp *Response, err error) {
    req, err := NewRequest("GET", url, nil)
    if err != nil {
        return nil, err
    }
    return c.Do(req)
}

It calls the [http].NewRequest() function internally anyway.

It's not a bad idea for a linter to treat strings like paths, filenames, or URIs passed as arguments as "tainted data" and issue warnings for code that uses them without evaluation. However, if that's the case, I can't help but feel the design should have avoided passing the URI as a string to functions like [http].Get() or [http].NewRequest() in the first place.

Taking a URI that has been constructed or validated using the [url].URL type, expanding it back into a string with the String() function to pass it to [http].Get(), only for that function to [url].Parse() it back into a [url].URL type internally, feels like an endlessly redundant process.

Well, I suppose it's too late to change that now, though.

Related Links

https://zenn.dev/spiegel/articles/20210113-fetch

Reference Books

https://www.amazon.co.jp/dp/4621300253

脚注

1. In this example, I'm specifically pointing out the [http].Get() function, but it's the same for the [http].Post() function.

2. As a way to avoid using "tainted data" directly within a function, defining and using it as a constant instead of passing it as an argument (variable) from outside the function is a common secure coding tactic.

3. If you need to include [context].Context information, use the [http].NewRequestWithContext() function.