diff --git a/cmd/admin/organizations.go b/cmd/admin/organizations.go index 53c86cf..5be7b21 100644 --- a/cmd/admin/organizations.go +++ b/cmd/admin/organizations.go @@ -13,8 +13,8 @@ package admin import ( "fmt" + "github.com/adobe/imscli/cmd/pretty" "github.com/adobe/imscli/ims" - "github.com/adobe/imscli/output" "github.com/spf13/cobra" ) @@ -33,7 +33,7 @@ func OrganizationsCmd(imsConfig *ims.Config) *cobra.Command { if err != nil { return fmt.Errorf("error in get admin organizations cmd: %w", err) } - output.PrintPrettyJSON(resp) + fmt.Println(pretty.JSON(resp)) return nil }, } diff --git a/cmd/admin/profile.go b/cmd/admin/profile.go index b285447..9bc545d 100644 --- a/cmd/admin/profile.go +++ b/cmd/admin/profile.go @@ -13,8 +13,8 @@ package admin import ( "fmt" + "github.com/adobe/imscli/cmd/pretty" "github.com/adobe/imscli/ims" - "github.com/adobe/imscli/output" "github.com/spf13/cobra" ) @@ -31,7 +31,7 @@ func ProfileCmd(imsConfig *ims.Config) *cobra.Command { if err != nil { return fmt.Errorf("error in get admin profile cmd: %w", err) } - output.PrintPrettyJSON(resp) + fmt.Println(pretty.JSON(resp)) return nil }, } diff --git a/cmd/decode.go b/cmd/decode.go index 518c5ce..4ae70d7 100644 --- a/cmd/decode.go +++ b/cmd/decode.go @@ -13,6 +13,7 @@ package cmd import ( "fmt" + "github.com/adobe/imscli/cmd/pretty" "github.com/adobe/imscli/ims" "github.com/spf13/cobra" ) @@ -32,9 +33,9 @@ func decodeCmd(imsConfig *ims.Config) *cobra.Command { return fmt.Errorf("error decoding the token: %w", err) } - fmt.Println(decoded.Header) + fmt.Println(pretty.JSON(decoded.Header)) fmt.Println() - fmt.Println(decoded.Payload) + fmt.Println(pretty.JSON(decoded.Payload)) return nil }, diff --git a/cmd/organizations.go b/cmd/organizations.go index 4929f3c..c15e3bf 100644 --- a/cmd/organizations.go +++ b/cmd/organizations.go @@ -13,8 +13,8 @@ package cmd import ( "fmt" + "github.com/adobe/imscli/cmd/pretty" "github.com/adobe/imscli/ims" - "github.com/adobe/imscli/output" "github.com/spf13/cobra" ) @@ -33,7 +33,7 @@ func organizationsCmd(imsConfig *ims.Config) *cobra.Command { if err != nil { return fmt.Errorf("error in get organizations cmd: %w", err) } - output.PrintPrettyJSON(resp) + fmt.Println(pretty.JSON(resp)) return nil }, } diff --git a/output/output.go b/cmd/pretty/json.go similarity index 62% rename from output/output.go rename to cmd/pretty/json.go index 12d43cf..00fb1ee 100644 --- a/output/output.go +++ b/cmd/pretty/json.go @@ -8,23 +8,19 @@ // OF ANY KIND, either express or implied. See the License for the specific language // governing permissions and limitations under the License. -package output +package pretty import ( "bytes" "encoding/json" - "fmt" ) -// PrintPrettyJSON prints a JSON string with indentation. -// If the input is not valid JSON, it prints the original string. -func PrintPrettyJSON(jsonStr string) { - var prettyBuf bytes.Buffer - err := json.Indent(&prettyBuf, []byte(jsonStr), "", " ") - if err != nil { - // Not valid JSON, print as-is - fmt.Println(jsonStr) - return +// JSON returns the input with JSON indentation applied. +// If the input is not valid JSON, it returns the original string. +func JSON(jsonStr string) string { + var buf bytes.Buffer + if err := json.Indent(&buf, []byte(jsonStr), "", " "); err != nil { + return jsonStr } - fmt.Println(prettyBuf.String()) + return buf.String() } diff --git a/output/output_test.go b/cmd/pretty/json_test.go similarity index 63% rename from output/output_test.go rename to cmd/pretty/json_test.go index 73005f1..d8acc7b 100644 --- a/output/output_test.go +++ b/cmd/pretty/json_test.go @@ -8,49 +8,25 @@ // OF ANY KIND, either express or implied. See the License for the specific language // governing permissions and limitations under the License. -package output +package pretty import ( - "bytes" - "io" "os" "path/filepath" + "strings" "testing" ) -// captureStdout captures the output of a function that writes to stdout. -func captureStdout(t *testing.T, fn func()) string { - t.Helper() - old := os.Stdout - r, w, err := os.Pipe() - if err != nil { - t.Fatalf("failed to create pipe: %v", err) - } - os.Stdout = w - - fn() - - w.Close() - os.Stdout = old - - var buf bytes.Buffer - if _, err := io.Copy(&buf, r); err != nil { - t.Fatalf("failed to read pipe: %v", err) - } - return buf.String() -} - -// loadExpected reads the expected output from a golden file in testdata/. func loadExpected(t *testing.T, filename string) string { t.Helper() data, err := os.ReadFile(filepath.Join("testdata", filename)) if err != nil { t.Fatalf("failed to read golden file %s: %v", filename, err) } - return string(data) + return strings.TrimSuffix(string(data), "\n") } -func TestPrintPrettyJSON(t *testing.T) { +func TestJSON(t *testing.T) { tests := []struct { name string input string @@ -62,20 +38,16 @@ func TestPrintPrettyJSON(t *testing.T) { {name: "empty object", input: `{}`, file: "empty_object.json"}, {name: "empty array", input: `[]`, file: "empty_array.json"}, {name: "nested objects", input: `{"a":{"b":{"c":1}}}`, file: "nested_objects.json"}, - {name: "non-JSON is printed as-is", input: "this is not JSON", file: "non_json.txt"}, - {name: "empty string is printed as-is", input: "", file: "empty_string.txt"}, + {name: "non-JSON is returned as-is", input: "this is not JSON", file: "non_json.txt"}, + {name: "empty string is returned as-is", input: "", file: "empty_string.txt"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { expected := loadExpected(t, tt.file) - - got := captureStdout(t, func() { - PrintPrettyJSON(tt.input) - }) - + got := JSON(tt.input) if got != expected { - t.Errorf("PrintPrettyJSON(%q)\ngot:\n%s\nexpected output is in testdata/%s", tt.input, got, tt.file) + t.Errorf("JSON(%q)\ngot:\n%s\nexpected:\n%s", tt.input, got, expected) } }) } diff --git a/output/testdata/empty_string.txt b/cmd/pretty/testdata/empty_string.txt similarity index 100% rename from output/testdata/empty_string.txt rename to cmd/pretty/testdata/empty_string.txt diff --git a/output/testdata/non_json.txt b/cmd/pretty/testdata/non_json.txt similarity index 100% rename from output/testdata/non_json.txt rename to cmd/pretty/testdata/non_json.txt diff --git a/cmd/profile.go b/cmd/profile.go index 7324205..4ab29ba 100644 --- a/cmd/profile.go +++ b/cmd/profile.go @@ -13,8 +13,8 @@ package cmd import ( "fmt" + "github.com/adobe/imscli/cmd/pretty" "github.com/adobe/imscli/ims" - "github.com/adobe/imscli/output" "github.com/spf13/cobra" ) @@ -32,7 +32,7 @@ func profileCmd(imsConfig *ims.Config) *cobra.Command { if err != nil { return fmt.Errorf("error in get profile cmd: %w", err) } - output.PrintPrettyJSON(resp) + fmt.Println(pretty.JSON(resp)) return nil }, } diff --git a/cmd/validate/access_token.go b/cmd/validate/access_token.go index c8ee4a5..70f4428 100644 --- a/cmd/validate/access_token.go +++ b/cmd/validate/access_token.go @@ -13,8 +13,8 @@ package validate import ( "fmt" + "github.com/adobe/imscli/cmd/pretty" "github.com/adobe/imscli/ims" - "github.com/adobe/imscli/output" "github.com/spf13/cobra" ) @@ -35,7 +35,7 @@ func AccessTokenCmd(imsConfig *ims.Config) *cobra.Command { if !resp.Valid { return fmt.Errorf("invalid token: %v", resp.Info) } - output.PrintPrettyJSON(resp.Info) + fmt.Println(pretty.JSON(resp.Info)) return nil }, } diff --git a/cmd/validate/authorization_code.go b/cmd/validate/authorization_code.go index 9625aa1..66337bc 100644 --- a/cmd/validate/authorization_code.go +++ b/cmd/validate/authorization_code.go @@ -13,8 +13,8 @@ package validate import ( "fmt" + "github.com/adobe/imscli/cmd/pretty" "github.com/adobe/imscli/ims" - "github.com/adobe/imscli/output" "github.com/spf13/cobra" ) @@ -35,7 +35,7 @@ func AuthzCodeCmd(imsConfig *ims.Config) *cobra.Command { if !resp.Valid { return fmt.Errorf("invalid token: %v", resp.Info) } - output.PrintPrettyJSON(resp.Info) + fmt.Println(pretty.JSON(resp.Info)) return nil }, } diff --git a/cmd/validate/device_token.go b/cmd/validate/device_token.go index 99cd5f6..7209bfc 100644 --- a/cmd/validate/device_token.go +++ b/cmd/validate/device_token.go @@ -13,8 +13,8 @@ package validate import ( "fmt" + "github.com/adobe/imscli/cmd/pretty" "github.com/adobe/imscli/ims" - "github.com/adobe/imscli/output" "github.com/spf13/cobra" ) @@ -35,7 +35,7 @@ func DeviceTokenCmd(imsConfig *ims.Config) *cobra.Command { if !resp.Valid { return fmt.Errorf("invalid token: %v", resp.Info) } - output.PrintPrettyJSON(resp.Info) + fmt.Println(pretty.JSON(resp.Info)) return nil }, } diff --git a/cmd/validate/refresh_token.go b/cmd/validate/refresh_token.go index dd6d1c6..e149111 100644 --- a/cmd/validate/refresh_token.go +++ b/cmd/validate/refresh_token.go @@ -13,8 +13,8 @@ package validate import ( "fmt" + "github.com/adobe/imscli/cmd/pretty" "github.com/adobe/imscli/ims" - "github.com/adobe/imscli/output" "github.com/spf13/cobra" ) @@ -35,7 +35,7 @@ func RefreshTokenCmd(imsConfig *ims.Config) *cobra.Command { if !resp.Valid { return fmt.Errorf("invalid token: %v", resp.Info) } - output.PrintPrettyJSON(resp.Info) + fmt.Println(pretty.JSON(resp.Info)) return nil }, } diff --git a/docs/oauth-serve-error.md b/docs/oauth-serve-error.md new file mode 100644 index 0000000..4e1f09e --- /dev/null +++ b/docs/oauth-serve-error.md @@ -0,0 +1,84 @@ +# OAuth Local Server: Unhandled Serve() Error + +## The Problem + +`go server.Serve(listener)` (`ims/authz_user.go:130`) launches a goroutine. The `error` +return from `Serve` is discarded. + +`http.Server.Serve` (`ims-go/login/server.go:136-138`) delegates to the standard +library's `http.Server.Serve`. If the listener fails, `Serve` returns an error +immediately. No HTTP handler ever runs. + +Since no handler runs, nothing writes to `resCh` or `errCh` +(`ims-go/login/server.go:91-92`). + +The `select` (`ims/authz_user.go:137-145`) is waiting on `server.Error()`, +`server.Response()`, and a 5-minute timeout. Neither channel will ever receive a value. + +The user waits 5 minutes for the timeout to fire, then gets a generic "user timed out" +error instead of the actual listener failure. + +## Practical Likelihood + +In practice, this is hard to trigger. The most obvious cause — port already in use — is +caught earlier at `net.Listen` (`ims/authz_user.go:112-114`), before `Serve` is called. +Once `net.Listen` succeeds, `Serve` with a valid listener will block on `Accept()` until +`Shutdown()` is called. A failure would require something unusual like the file descriptor +being invalidated between `Listen` and `Serve`. + +The fix is applied as a defensive pattern: capturing goroutine errors and reacting to +them is good practice regardless of how likely the failure is, and it has no cost. + +## The Fix + +Capture the `Serve` error via a buffered channel and add it as a case in the `select`: + +```go +serveCh := make(chan error, 1) +go func() { + serveCh <- server.Serve(listener) +}() +``` + +The new `select` case: + +```go +case serr = <-serveCh: + log.Println("The local server stopped unexpectedly.") +``` + +This gives the user an immediate, accurate error instead of a 5-minute wait. + +## Why the Channel Must Be Buffered + +In the normal flow, `Serve` runs in the background while the `select` +(`ims/authz_user.go:137-145`) waits for one of three things: response, error, or +timeout. + +The browser callback arrives. The handler writes to `resCh` +(`ims-go/login/result.go:35-36`). The `select` reads from `server.Response()` and +proceeds. + +We reach `Shutdown()` (`ims/authz_user.go:159-163`). `Shutdown` closes the listener +and waits for handlers to finish, then returns. + +Once the listener is closed, `server.Serve(listener)` (`ims-go/login/server.go:136-138`) +returns `http.ErrServerClosed`. + +Our goroutine tries to write this error to `serveCh`: + +```go +go func() { + serveCh <- server.Serve(listener) +}() +``` + +Nobody is reading from `serveCh` — we already left the `select` back when the browser +callback arrived. + +If `serveCh` were **unbuffered**, the goroutine blocks on the write forever — a goroutine +leak (the same class of problem described in `docs/oauth-shutdown-deadlock.md`). + +With `serveCh` buffered at capacity 1, the write succeeds immediately into the buffer. +The goroutine exits. The channel and its buffered value are cleaned up by GC when nothing +references them anymore. diff --git a/ims/authz_user.go b/ims/authz_user.go index 310e2f5..7229ab4 100644 --- a/ims/authz_user.go +++ b/ims/authz_user.go @@ -127,7 +127,13 @@ func (i Config) AuthorizeUser() (string, error) { fmt.Fprintf(os.Stderr, "error launching the browser, open it and visit %s\n", localUrl) } - go server.Serve(listener) + // Capture Serve errors via a buffered channel. Buffered so the goroutine + // can always write and exit, even if nobody reads (e.g., a response arrived + // first). See docs/oauth-serve-error.md for a detailed explanation. + serveCh := make(chan error, 1) + go func() { + serveCh <- server.Serve(listener) + }() var ( serr error @@ -139,6 +145,8 @@ func (i Config) AuthorizeUser() (string, error) { log.Println("The IMS HTTP handler returned an error message.") case resp = <-server.Response(): log.Println("The IMS HTTP handler returned a message.") + case serr = <-serveCh: + log.Println("The local server stopped unexpectedly.") case <-time.After(time.Minute * 5): fmt.Fprintf(os.Stderr, "Timeout reached waiting for the user to finish the authentication ...\n") serr = fmt.Errorf("user timed out") diff --git a/ims/decode.go b/ims/decode.go index 50f94dc..991f1f4 100644 --- a/ims/decode.go +++ b/ims/decode.go @@ -11,18 +11,15 @@ package ims import ( - "bytes" "encoding/base64" - "encoding/json" "fmt" "strings" ) // DecodedToken represents the decoded parts of a JWT token. type DecodedToken struct { - Header string - Payload string - Signature string + Header string + Payload string } func (i Config) validateDecodeTokenConfig() error { @@ -44,40 +41,21 @@ func (i Config) DecodeToken() (*DecodedToken, error) { return nil, fmt.Errorf("the JWT is not composed by 3 parts") } - // Decode header and payload (not signature since it's binary) - decoded := &DecodedToken{ - Signature: parts[2], - } + decoded := &DecodedToken{} - // Decode and prettify header + // Decode header headerBytes, err := base64.RawURLEncoding.DecodeString(parts[0]) if err != nil { return nil, fmt.Errorf("error decoding token header: %w", err) } - decoded.Header, err = prettyJSON(headerBytes) - if err != nil { - return nil, fmt.Errorf("error formatting token header: %w", err) - } + decoded.Header = string(headerBytes) - // Decode and prettify payload + // Decode payload payloadBytes, err := base64.RawURLEncoding.DecodeString(parts[1]) if err != nil { return nil, fmt.Errorf("error decoding token payload: %w", err) } - decoded.Payload, err = prettyJSON(payloadBytes) - if err != nil { - return nil, fmt.Errorf("error formatting token payload: %w", err) - } + decoded.Payload = string(payloadBytes) return decoded, nil } - -// prettyJSON formats JSON bytes with indentation. -func prettyJSON(data []byte) (string, error) { - var prettyBuf bytes.Buffer - err := json.Indent(&prettyBuf, data, "", " ") - if err != nil { - return "", err - } - return prettyBuf.String(), nil -}