-
Notifications
You must be signed in to change notification settings - Fork 80
Description
Description
#63 introduced a new safe parsing function NewGitHubHost which ensures that the URL parsing is valid. Consumers of this library can use it in order to pass in Host in the OauthFlow configuration struct:
Line 68 in afffc8e
| Host *Host |
However, it was possible to provide the deprecated Hostname string and both the device code and oauth flows would do the parsing internally. .
In #63, this parsing was changed to use the new function (which is great) but in doing so, some unintentional variable shadowing happened. Previously, the result of GitHubHost was assigned to the host variable which was used later on. Now the result of NewGitHubHost is assigned to host, err via short variable declaration (:=). This results in a new host variable being declared in the inner scope. Although we set oa.Host = host, the rest of the functions still use host, which results in a nil pointer deference.
Reproduction
Use these tests:
package oauth_test
import (
"testing"
"github.com/cli/oauth"
)
func TestDeviceFlowHostname(_ *testing.T) {
flow := &oauth.Flow{
Hostname: "github.com",
}
_, err := flow.DeviceFlow()
if err != nil {
panic(err)
}
}
func TestWebFlowHostname(_ *testing.T) {
flow := &oauth.Flow{
Hostname: "github.com",
}
_, err := flow.WebAppFlow()
if err != nil {
panic(err)
}
}They will panic on:
github.com/cli/oauth.(*Flow).DeviceFlow(0x1400007aeb0)
/Users/williammartin/workspace/oauth/oauth_device.go:42 +0x188
Line 42 in afffc8e
| code, err := device.RequestCode(httpClient, host.DeviceCodeURL, oa.ClientID, oa.Scopes) |
github.com/cli/oauth.(*Flow).WebAppFlow(0x140001462c0)
/Users/williammartin/workspace/oauth/oauth_webapp.go:37 +0x16c
Lines 36 to 37 in afffc8e
| } | |
| browserURL, err := flow.BrowserURL(host.AuthorizeURL, params) |
Note that these tests aren't otherwise intended to pass (e.g. Device flow will panic for other reasons on an earlier commit), just to demonstrate the issue.