From c0d1fca2e94c4e912183fad2c5f2976bdbcc5bfc Mon Sep 17 00:00:00 2001 From: Jose Antonio Insua Date: Fri, 27 Feb 2026 16:24:39 +0100 Subject: [PATCH 1/3] Split AuthorizeUser into AuthorizeUser and AuthorizeUserPKCE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove PKCE field from Config — it was not user configuration but a command-level decision. The pkce command now calls AuthorizeUserPKCE() directly instead of mutating the shared Config pointer. --- cmd/authz/pkce.go | 3 +-- ims/authz_user.go | 14 +++++++++++--- ims/config.go | 1 - 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/cmd/authz/pkce.go b/cmd/authz/pkce.go index 463e286..36d4e76 100644 --- a/cmd/authz/pkce.go +++ b/cmd/authz/pkce.go @@ -27,8 +27,7 @@ func UserPkceCmd(imsConfig *ims.Config) *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { cmd.SilenceUsage = true - imsConfig.PKCE = true - resp, err := imsConfig.AuthorizeUser() + resp, err := imsConfig.AuthorizeUserPKCE() if err != nil { return fmt.Errorf("error in user authorization: %w", err) } diff --git a/ims/authz_user.go b/ims/authz_user.go index b02cba2..83e252e 100644 --- a/ims/authz_user.go +++ b/ims/authz_user.go @@ -55,9 +55,17 @@ func (i Config) validateAuthorizeUserConfig() error { return nil } -// AuthorizeUser uses the standard Oauth2 authorization code grant flow. The Oauth2 configuration is -// taken from the Config struct. +// AuthorizeUser uses the standard OAuth2 authorization code grant flow. func (i Config) AuthorizeUser() (string, error) { + return i.authorizeUser(false) +} + +// AuthorizeUserPKCE uses the OAuth2 authorization code grant flow with PKCE. +func (i Config) AuthorizeUserPKCE() (string, error) { + return i.authorizeUser(true) +} + +func (i Config) authorizeUser(pkce bool) (string, error) { // Perform parameter validation err := i.validateAuthorizeUserConfig() if err != nil { @@ -80,7 +88,7 @@ func (i Config) AuthorizeUser() (string, error) { ClientID: i.ClientID, ClientSecret: i.ClientSecret, Scope: i.Scopes, - UsePKCE: i.PKCE, + UsePKCE: pkce, RedirectURI: fmt.Sprintf("http://localhost:%d", port), OnError: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprintln(w, ` diff --git a/ims/config.go b/ims/config.go index 5c9dd82..371f468 100644 --- a/ims/config.go +++ b/ims/config.go @@ -41,7 +41,6 @@ type Config struct { Cascading bool Token string Port int - PKCE bool FullOutput bool Guid string AuthSrc string From d6badce3faaaf87c26b3f5d798c41ddc1a6d3528 Mon Sep 17 00:00:00 2001 From: Jose Antonio Insua Date: Fri, 27 Feb 2026 17:13:10 +0100 Subject: [PATCH 2/3] Miscellaneous hardening: zero private key, port validation, struct JSON, URL slash - Zero private key bytes after use in JWT exchange to prevent key material lingering in memory until GC (6.1.7) - Add port validation to validateAuthorizeUserConfig, remove defaultPort fallback so the ims package validates its own parameters (N10) - Replace map[string]interface{} with anonymous struct in refresh command for deterministic JSON field order and better performance (N11) - Trim trailing slash from IMS URL before building JWT claim keys to prevent double-slash in metascope URIs (6.2.10) - Standardize orgs API version error message to match profile style (6.2.12) - Document PersistentPreRunE override risk in README.md (N7) Co-Authored-By: Claude Opus 4.6 --- README.md | 6 ++++++ cmd/refresh.go | 8 ++++---- ims/authz_user.go | 18 ++++++------------ ims/config_test.go | 2 ++ ims/jwt_exchange.go | 9 ++++++++- ims/organizations.go | 2 +- 6 files changed, 27 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 6b9f030..5652c8a 100644 --- a/README.md +++ b/README.md @@ -34,6 +34,12 @@ imscli authorize user help The complete documentation of the project is available in the [DOCUMENTATION.md](DOCUMENTATION.md) file. +## Development Notes + +### PersistentPreRunE and subcommands + +The root command defines a `PersistentPreRunE` that loads configuration from flags, environment variables, and config files (see `cmd/root.go`). In cobra, if a subcommand defines its own `PersistentPreRunE`, it **overrides** the parent's — the root's `PersistentPreRunE` will not run for that subcommand or its children. If you need to add a `PersistentPreRunE` to a subcommand, you must explicitly call the parent's first. + ## Contributing Contributions are welcomed! Read the [Contributing Guide](CONTRIBUTING.md) for more information. diff --git a/cmd/refresh.go b/cmd/refresh.go index 648b59b..877dfb7 100644 --- a/cmd/refresh.go +++ b/cmd/refresh.go @@ -33,10 +33,10 @@ func refreshCmd(imsConfig *ims.Config) *cobra.Command { return fmt.Errorf("error during the token refresh: %w", err) } if imsConfig.FullOutput { - data := map[string]interface{}{ - "access_token": resp.AccessToken, - "refresh_token": resp.RefreshToken, - } + data := struct { + AccessToken string `json:"access_token"` + RefreshToken string `json:"refresh_token"` + }{resp.AccessToken, resp.RefreshToken} jsonData, err := json.MarshalIndent(data, "", " ") if err != nil { return fmt.Errorf("error marshalling full JSON response: %w", err) diff --git a/ims/authz_user.go b/ims/authz_user.go index 83e252e..07685e2 100644 --- a/ims/authz_user.go +++ b/ims/authz_user.go @@ -24,8 +24,6 @@ import ( "github.com/pkg/browser" ) -const defaultPort = 8888 - // Validate that: // - the ims.Config struct has the necessary parameters for AuthorizeUser // - the provided environment exists @@ -42,6 +40,8 @@ func (i Config) validateAuthorizeUserConfig() error { return fmt.Errorf("missing client id parameter") case i.Organization == "": return fmt.Errorf("missing organization parameter") + case i.Port <= 0: + return fmt.Errorf("missing or invalid port parameter") case i.ClientSecret == "": if i.PublicClient { log.Println("all needed parameters verified not empty") @@ -72,12 +72,6 @@ func (i Config) authorizeUser(pkce bool) (string, error) { return "", fmt.Errorf("invalid parameters for login user: %w", err) } - // Use default port if not specified - port := i.Port - if port == 0 { - port = defaultPort - } - c, err := i.newIMSClient() if err != nil { return "", fmt.Errorf("error creating the IMS client: %w", err) @@ -89,7 +83,7 @@ func (i Config) authorizeUser(pkce bool) (string, error) { ClientSecret: i.ClientSecret, Scope: i.Scopes, UsePKCE: pkce, - RedirectURI: fmt.Sprintf("http://localhost:%d", port), + RedirectURI: fmt.Sprintf("http://localhost:%d", i.Port), OnError: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprintln(w, `

An error occurred

@@ -107,15 +101,15 @@ func (i Config) authorizeUser(pkce bool) (string, error) { return "", fmt.Errorf("create authorization server: %w", err) } - listener, err := net.Listen("tcp", fmt.Sprintf(":%d", port)) + listener, err := net.Listen("tcp", fmt.Sprintf(":%d", i.Port)) if err != nil { - return "", fmt.Errorf("unable to listen at port %d", port) + return "", fmt.Errorf("unable to listen at port %d", i.Port) } defer listener.Close() log.Println("Local server successfully launched and contacted.") - localUrl := fmt.Sprintf("http://localhost:%d/", port) + localUrl := fmt.Sprintf("http://localhost:%d/", i.Port) // Suppress "Opening in existing browser session." messages from chromium-based // browsers. The CLI token output goes to stdout, so stray browser messages diff --git a/ims/config_test.go b/ims/config_test.go index ced8041..c628844 100644 --- a/ims/config_test.go +++ b/ims/config_test.go @@ -224,6 +224,7 @@ func TestValidateAuthorizeUserConfig(t *testing.T) { ClientSecret: "s", Organization: "org", Scopes: []string{"openid"}, + Port: 8888, } tests := []struct { name string @@ -239,6 +240,7 @@ func TestValidateAuthorizeUserConfig(t *testing.T) { {name: "missing clientID", config: withField(validConfig, func(c *Config) { c.ClientID = "" }), wantErr: "missing client id"}, {name: "missing organization", config: withField(validConfig, func(c *Config) { c.Organization = "" }), wantErr: "missing organization"}, {name: "missing secret non-public", config: withField(validConfig, func(c *Config) { c.ClientSecret = "" }), wantErr: "missing client secret"}, + {name: "missing port", config: withField(validConfig, func(c *Config) { c.Port = 0 }), wantErr: "missing or invalid port"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/ims/jwt_exchange.go b/ims/jwt_exchange.go index a30277b..a1d7ba2 100644 --- a/ims/jwt_exchange.go +++ b/ims/jwt_exchange.go @@ -13,6 +13,7 @@ package ims import ( "fmt" "os" + "strings" "time" "github.com/adobe/ims-go/ims" @@ -29,14 +30,20 @@ func (i Config) AuthorizeJWTExchange() (TokenInfo, error) { if err != nil { return TokenInfo{}, fmt.Errorf("error read private key file: %s, %w", i.PrivateKeyPath, err) } + defer func() { + for i := range key { + key[i] = 0 + } + }() // Metascopes are passed as generic claims with the format map[string]interface{} // where the strings are in the form: baseIMSUrl/s/metascope // and the interface{} is 'true' + baseURL := strings.TrimRight(i.URL, "/") claims := make(map[string]interface{}) for _, metascope := range i.Metascopes { - claims[fmt.Sprintf("%s/s/%s", i.URL, metascope)] = true + claims[fmt.Sprintf("%s/s/%s", baseURL, metascope)] = true } r, err := c.ExchangeJWT(&ims.ExchangeJWTRequest{ diff --git a/ims/organizations.go b/ims/organizations.go index 5853c9b..c9fadc4 100644 --- a/ims/organizations.go +++ b/ims/organizations.go @@ -22,7 +22,7 @@ func (i Config) validateGetOrganizationsConfig() error { switch i.OrgsAPIVersion { case "v1", "v2", "v3", "v4", "v5", "v6": default: - return fmt.Errorf("invalid API version parameter, use something like v5") + return fmt.Errorf("invalid API version parameter, latest version is v6") } switch { From 8a68d34ec3cd85ea7950c8bf27b8cff0c51d6e5f Mon Sep 17 00:00:00 2001 From: Jose Antonio Insua Date: Fri, 27 Feb 2026 17:26:42 +0100 Subject: [PATCH 3/3] Remove dead TokenInfo.Expires field and broken calculations The Expires field was set in jwt_exchange.go, exchange.go, and refresh.go but never read by any caller. The calculations were also wrong: duration * duration produces nanoseconds-squared (overflow), and refresh.go used time.Second while the others used time.Millisecond. Remove the field entirely rather than fixing dead code. Co-Authored-By: Claude Opus 4.6 --- ims/config.go | 1 - ims/exchange.go | 2 -- ims/jwt_exchange.go | 1 - ims/refresh.go | 3 +-- 4 files changed, 1 insertion(+), 6 deletions(-) diff --git a/ims/config.go b/ims/config.go index 371f468..8500eca 100644 --- a/ims/config.go +++ b/ims/config.go @@ -52,7 +52,6 @@ type Config struct { // Access token information type TokenInfo struct { AccessToken string - Expires int //(response.ExpiresIn * time.Millisecond), Valid bool Info string } diff --git a/ims/exchange.go b/ims/exchange.go index 6bd1d43..c1405d4 100644 --- a/ims/exchange.go +++ b/ims/exchange.go @@ -12,7 +12,6 @@ package ims import ( "fmt" - "time" "github.com/adobe/ims-go/ims" ) @@ -62,6 +61,5 @@ func (i Config) ClusterExchange() (TokenInfo, error) { return TokenInfo{ AccessToken: r.AccessToken, - Expires: int(r.ExpiresIn * time.Millisecond), }, nil } diff --git a/ims/jwt_exchange.go b/ims/jwt_exchange.go index a1d7ba2..42a91f4 100644 --- a/ims/jwt_exchange.go +++ b/ims/jwt_exchange.go @@ -61,6 +61,5 @@ func (i Config) AuthorizeJWTExchange() (TokenInfo, error) { return TokenInfo{ AccessToken: r.AccessToken, - Expires: int(r.ExpiresIn * time.Millisecond), }, nil } diff --git a/ims/refresh.go b/ims/refresh.go index 185a258..82ce08e 100644 --- a/ims/refresh.go +++ b/ims/refresh.go @@ -12,8 +12,8 @@ package ims import ( "fmt" + "github.com/adobe/ims-go/ims" - "time" ) func (i Config) validateRefreshConfig() error { @@ -56,7 +56,6 @@ func (i Config) Refresh() (RefreshInfo, error) { return RefreshInfo{ TokenInfo: TokenInfo{ AccessToken: r.AccessToken, - Expires: int(r.ExpiresIn * time.Second), }, RefreshToken: r.RefreshToken, }, nil