From c66d9da5e5c7264ed0b6abda24e57c225003fee1 Mon Sep 17 00:00:00 2001 From: Arpit Jain Date: Wed, 19 Aug 2026 04:34:20 -0400 Subject: [PATCH] Report a malformed --account file instead of panicking Every value read out of the --account JSON used a bare type assertion, so step oauth panics on the file's contents before any network call: step oauth --account sa.json --bare {"type":"service_account"} panic: interface conversion: interface {} is nil, not string {"installed":{}} panic: interface conversion: interface {} is nil, not string {"installed":"notamap"} panic: interface conversion: interface {} is string, not map[string]interface {} Any partial, truncated, or non-Google account file does this. Without STEPDEBUG the panic handler reports it as "Something unexpected happened" and asks the user to mail the output in. The unsupported-account-type branch had a separate bug: it wrapped err, which is nil at that point, and errors.Wrapf(nil, ...) returns nil. So an unrecognised file reported no error at all and the flow continued with empty endpoints. Confirmed: that path printed nothing and hung. Moved the parsing into readAccountCredentials so it can be tested without starting an OAuth flow, and read each value with the two-value form, naming the key that is missing or of the wrong type. Signed-off-by: Arpit Jain --- command/oauth/cmd.go | 114 ++++++++++++++++++++++++++-------- command/oauth/cmd_test.go | 125 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 215 insertions(+), 24 deletions(-) create mode 100644 command/oauth/cmd_test.go diff --git a/command/oauth/cmd.go b/command/oauth/cmd.go index 655637c5..9aa1f8bf 100644 --- a/command/oauth/cmd.go +++ b/command/oauth/cmd.go @@ -348,6 +348,88 @@ func (o *options) Validate() error { return nil } +// accountCredentials holds the OAuth endpoints and client credentials read from +// an --account file. +type accountCredentials struct { + authzEp string + tokenEp string + clientID string + clientSecret string + issuer string + do2lo bool +} + +// accountString reads a string value out of a decoded account file, naming the +// key when it is absent or of the wrong type. The values come straight from the +// file, so a type assertion here would panic on any partial or non-Google JSON. +func accountString(m map[string]interface{}, key, filename string) (string, error) { + v, ok := m[key].(string) + if !ok { + return "", errors.Errorf("error reading %s: missing or invalid %q", filename, key) + } + return v, nil +} + +// readAccountCredentials parses an --account file. It supports the "installed" +// shape written by the Google console and Google service accounts. +func readAccountCredentials(filename string) (*accountCredentials, error) { + b, err := os.ReadFile(filename) + if err != nil { + return nil, errors.Wrapf(err, "error reading account from %s", filename) + } + + account := make(map[string]interface{}) + if err := json.Unmarshal(b, &account); err != nil { + return nil, errors.Wrapf(err, "error reading %s: unsupported format", filename) + } + + creds := &accountCredentials{} + if _, ok := account["installed"]; ok { + details, ok := account["installed"].(map[string]interface{}) + if !ok { + return nil, errors.Errorf("error reading %s: %q must be an object", filename, "installed") + } + for _, f := range []struct { + dst *string + key string + }{ + {&creds.authzEp, "auth_uri"}, + {&creds.tokenEp, "token_uri"}, + {&creds.clientID, "client_id"}, + {&creds.clientSecret, "client_secret"}, + } { + if *f.dst, err = accountString(details, f.key, filename); err != nil { + return nil, err + } + } + return creds, nil + } + + if accountType, ok := account["type"]; ok && accountType == "service_account" { + for _, f := range []struct { + dst *string + key string + }{ + {&creds.authzEp, "auth_uri"}, + {&creds.tokenEp, "token_uri"}, + {&creds.clientID, "private_key_id"}, + {&creds.clientSecret, "private_key"}, + {&creds.issuer, "client_email"}, + } { + if *f.dst, err = accountString(account, f.key, filename); err != nil { + return nil, err + } + } + creds.do2lo = true + return creds, nil + } + + // The original code wrapped a nil err here, and errors.Wrapf(nil, ...) is + // nil, so an unsupported file reported nothing and the flow continued with + // empty endpoints. + return nil, errors.Errorf("error reading %s: unsupported account type", filename) +} + func oauthCmd(c *cli.Context) error { opts := &options{ Provider: c.String("provider"), @@ -432,32 +514,16 @@ func oauthCmd(c *cli.Context) error { // This code supports Google service accounts. Probably maybe also support JWKs? if c.IsSet("account") { opts.Provider = "" - filename := c.String("account") - b, err := os.ReadFile(filename) + creds, err := readAccountCredentials(c.String("account")) if err != nil { - return errors.Wrapf(err, "error reading account from %s", filename) - } - account := make(map[string]interface{}) - if err = json.Unmarshal(b, &account); err != nil { - return errors.Wrapf(err, "error reading %s: unsupported format", filename) - } - - if _, ok := account["installed"]; ok { - details := account["installed"].(map[string]interface{}) - authzEp = details["auth_uri"].(string) - tokenEp = details["token_uri"].(string) - clientID = details["client_id"].(string) - clientSecret = details["client_secret"].(string) - } else if accountType, ok := account["type"]; ok && accountType == "service_account" { - authzEp = account["auth_uri"].(string) - tokenEp = account["token_uri"].(string) - clientID = account["private_key_id"].(string) - clientSecret = account["private_key"].(string) - issuer = account["client_email"].(string) - do2lo = true - } else { - return errors.Wrapf(err, "error reading %s: unsupported account type", filename) + return err } + authzEp = creds.authzEp + tokenEp = creds.tokenEp + clientID = creds.clientID + clientSecret = creds.clientSecret + issuer = creds.issuer + do2lo = creds.do2lo } scope := "openid email" diff --git a/command/oauth/cmd_test.go b/command/oauth/cmd_test.go new file mode 100644 index 00000000..182484c3 --- /dev/null +++ b/command/oauth/cmd_test.go @@ -0,0 +1,125 @@ +package oauth + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// The --account file is read and unmarshalled into map[string]interface{}, and +// every value used to be pulled out with a bare type assertion. Each case below +// panicked, except the unsupported-type one, which returned a nil error because +// errors.Wrapf(nil, ...) is nil. +func TestReadAccountCredentialsErrors(t *testing.T) { + dir := t.TempDir() + + for _, tt := range []struct { + name string + content string + wantErr string + }{ + { + name: "installed is not an object", + content: `{"installed":"notamap"}`, + wantErr: `"installed" must be an object`, + }, + { + name: "installed is missing its keys", + content: `{"installed":{}}`, + wantErr: `missing or invalid "auth_uri"`, + }, + { + name: "installed has a non-string value", + content: `{"installed":{"auth_uri":1}}`, + wantErr: `missing or invalid "auth_uri"`, + }, + { + name: "service account is missing its keys", + content: `{"type":"service_account"}`, + wantErr: `missing or invalid "auth_uri"`, + }, + { + name: "unsupported account type", + content: `{"other":1}`, + wantErr: "unsupported account type", + }, + { + name: "not json", + content: `not json at all`, + wantErr: "unsupported format", + }, + } { + t.Run(tt.name, func(t *testing.T) { + path := filepath.Join(dir, strings.ReplaceAll(tt.name, " ", "_")+".json") + if err := os.WriteFile(path, []byte(tt.content), 0o600); err != nil { + t.Fatal(err) + } + + creds, err := readAccountCredentials(path) + if err == nil { + t.Fatalf("expected an error, got credentials %+v", creds) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("expected error containing %q, got %v", tt.wantErr, err) + } + }) + } +} + +func TestReadAccountCredentialsMissingFile(t *testing.T) { + if _, err := readAccountCredentials(filepath.Join(t.TempDir(), "nope.json")); err == nil { + t.Fatal("expected an error for a missing file") + } +} + +func TestReadAccountCredentialsInstalled(t *testing.T) { + path := filepath.Join(t.TempDir(), "installed.json") + content := `{"installed":{"auth_uri":"https://accounts.example.com/auth",` + + `"token_uri":"https://oauth2.example.com/token",` + + `"client_id":"cid","client_secret":"secret"}}` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + + creds, err := readAccountCredentials(path) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if creds.authzEp != "https://accounts.example.com/auth" { + t.Errorf("unexpected authzEp %q", creds.authzEp) + } + if creds.tokenEp != "https://oauth2.example.com/token" { + t.Errorf("unexpected tokenEp %q", creds.tokenEp) + } + if creds.clientID != "cid" || creds.clientSecret != "secret" { + t.Errorf("unexpected client credentials %q / %q", creds.clientID, creds.clientSecret) + } + if creds.do2lo { + t.Error("do2lo should be false for an installed account") + } +} + +func TestReadAccountCredentialsServiceAccount(t *testing.T) { + path := filepath.Join(t.TempDir(), "sa.json") + content := `{"type":"service_account","auth_uri":"https://accounts.example.com/auth",` + + `"token_uri":"https://oauth2.example.com/token","private_key_id":"kid",` + + `"private_key":"pk","client_email":"svc@example.com"}` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + + creds, err := readAccountCredentials(path) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if creds.clientID != "kid" || creds.clientSecret != "pk" { + t.Errorf("unexpected client credentials %q / %q", creds.clientID, creds.clientSecret) + } + if creds.issuer != "svc@example.com" { + t.Errorf("unexpected issuer %q", creds.issuer) + } + if !creds.do2lo { + t.Error("do2lo should be true for a service account") + } +}