Refactor so we validate before auth on the vault

core/capabilities/vault/authorizer.go

@@ -11,6 +11,7 @@ import (
 	"github.com/smartcontractkit/chainlink-common/pkg/logger"
 
 	"github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes"
+	"github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaultutils"
 )
 
 // AuthResult is the normalized authorization output shared by
@@ -110,7 +111,7 @@ func (a *authorizer) AuthorizeRequest(ctx context.Context, req jsonrpc.Request[j
 		a.lggr.Debugw("replay guard rejected request", "method", req.Method, "requestID", req.ID, "owner", authResult.AuthorizedOwner(), "digest", authResult.Digest(), "expiresAt", authResult.ExpiresAt(), "hasAuth", req.Auth != "", "error", err)
 		return nil, err
 	}
-	if ownerErr := validatePreparedVaultOwners(req, authResult.AuthorizedOwner()); ownerErr != nil {
+	if ownerErr := validateSecretOwnersMatchAuthorized(req, authResult.AuthorizedOwner()); ownerErr != nil {
 		a.lggr.Errorw("owner binding rejected request", "method", req.Method, "requestID", req.ID, "owner", authResult.AuthorizedOwner(), "hasAuth", req.Auth != "", "error", ownerErr)
 		return nil, ownerErr
 	}
@@ -145,82 +146,84 @@ func (a *authorizer) authorizeJWTBasedAuth(ctx context.Context, req jsonrpc.Requ
 	return a.jwtBasedAuth.AuthorizeRequest(ctx, req)
 }
 
-// validatePreparedVaultOwners checks that secret identifiers in the request payload
-// belong to workflowOwner. It runs after allowlist or JWT authentication so neither
-// path can be exploited to mutate another owner's secrets.
-//
-// Param shape validation (empty batch, nil entries, parse errors) is left to the
-// gateway handler validators so clients receive the same InvalidParamsError codes
-// as before.
-func validatePreparedVaultOwners(req jsonrpc.Request[json.RawMessage], workflowOwner string) error {
-	// If the request has no params, there are no secret identifiers to validate.
-	// The gateway handler validates this case and returns InvalidParamsError.
-	if req.Params == nil {
-		return nil
-	}
-
+// validateSecretOwnersMatchAuthorized checks that secret identifiers in the request payload
+// match the authorized workflow owner. This is read-only validation; owner prefixing and
+// param stamping happen later in GatewayVaultRequestProcessor.
+func validateSecretOwnersMatchAuthorized(req jsonrpc.Request[json.RawMessage], workflowOwner string) error {
 	switch req.Method {
+	case vaulttypes.MethodPublicKeyGet:
+		return nil
 	case vaulttypes.MethodSecretsCreate:
-		parsed := &vaultcommon.CreateSecretsRequest{}
-		if err := json.Unmarshal(*req.Params, parsed); err != nil {
-			// InvalidParamsError is returned by the gateway handler for this case.
-			return nil
+		if req.Params == nil {
+			return errors.New("request params must not be nil")
+		}
+		var createReq vaultcommon.CreateSecretsRequest
+		if err := json.Unmarshal(*req.Params, &createReq); err != nil {
+			return err
 		}
-		return validateEncryptedSecretOwnerMismatch(parsed.EncryptedSecrets, workflowOwner)
+		return validateEncryptedSecretOwnerMismatch(createReq.EncryptedSecrets, workflowOwner)
 	case vaulttypes.MethodSecretsUpdate:
-		parsed := &vaultcommon.UpdateSecretsRequest{}
-		if err := json.Unmarshal(*req.Params, parsed); err != nil {
-			// InvalidParamsError is returned by the gateway handler for this case.
-			return nil
+		if req.Params == nil {
+			return errors.New("request params must not be nil")
 		}
-		return validateEncryptedSecretOwnerMismatch(parsed.EncryptedSecrets, workflowOwner)
+		var updateReq vaultcommon.UpdateSecretsRequest
+		if err := json.Unmarshal(*req.Params, &updateReq); err != nil {
+			return err
+		}
+		return validateEncryptedSecretOwnerMismatch(updateReq.EncryptedSecrets, workflowOwner)
 	case vaulttypes.MethodSecretsDelete:
-		parsed := &vaultcommon.DeleteSecretsRequest{}
-		if err := json.Unmarshal(*req.Params, parsed); err != nil {
-			// InvalidParamsError is returned by the gateway handler for this case.
-			return nil
+		if req.Params == nil {
+			return errors.New("request params must not be nil")
+		}
+		var deleteReq vaultcommon.DeleteSecretsRequest
+		if err := json.Unmarshal(*req.Params, &deleteReq); err != nil {
+			return err
 		}
-		return validateSecretIdentifierOwnerMismatch(parsed.Ids, workflowOwner)
+		return validateSecretIdentifierOwnerMismatch(deleteReq.Ids, workflowOwner)
 	case vaulttypes.MethodSecretsList:
-		parsed := &vaultcommon.ListSecretIdentifiersRequest{}
-		if err := json.Unmarshal(*req.Params, parsed); err != nil {
-			// InvalidParamsError is returned by the gateway handler for this case.
-			return nil
+		if req.Params == nil {
+			return errors.New("request params must not be nil")
 		}
-		if normalizeOwner(parsed.Owner) != normalizeOwner(workflowOwner) {
-			return fmt.Errorf("list secrets owner %q does not match authorized workflow owner %q", parsed.Owner, workflowOwner)
+		var listReq vaultcommon.ListSecretIdentifiersRequest
+		if err := json.Unmarshal(*req.Params, &listReq); err != nil {
+			return err
+		}
+		if vaultutils.NormalizeOwner(listReq.Owner) != vaultutils.NormalizeOwner(workflowOwner) {
+			return fmt.Errorf("list secrets owner %q does not match authorized workflow owner %q", listReq.Owner, workflowOwner)
 		}
-	case vaulttypes.MethodPublicKeyGet:
 		return nil
 	default:
-		// Fail open: this check only binds secret identifiers to the authorized owner.
-		// Unknown methods are rejected later with UnsupportedMethodError in the gateway
-		// handler (HandleJSONRPCUserMessage) and on vault nodes (GatewayHandler.HandleGatewayMessage).
-		return nil
+		return fmt.Errorf("owner validation not implemented for method %q", req.Method)
 	}
-	return nil
 }
 
 func validateEncryptedSecretOwnerMismatch(encryptedSecrets []*vaultcommon.EncryptedSecret, workflowOwner string) error {
+	if len(encryptedSecrets) == 0 {
+		return errors.New("request batch must contain at least 1 item")
+	}
 	for idx, encryptedSecret := range encryptedSecrets {
-		if encryptedSecret == nil || encryptedSecret.Id == nil {
-			// InvalidParamsError is returned by the gateway handler for this case.
-			continue
+		if encryptedSecret == nil {
+			return fmt.Errorf("encrypted secret must not be nil at index %d", idx)
+		}
+		if encryptedSecret.Id == nil {
+			return fmt.Errorf("secret ID must not be nil at index %d", idx)
 		}
-		if normalizeOwner(encryptedSecret.Id.Owner) != normalizeOwner(workflowOwner) {
+		if vaultutils.NormalizeOwner(encryptedSecret.Id.Owner) != vaultutils.NormalizeOwner(workflowOwner) {
 			return fmt.Errorf("encrypted secret owner at index %d %q does not match authorized workflow owner %q", idx, encryptedSecret.Id.Owner, workflowOwner)
 		}
 	}
 	return nil
 }
 
 func validateSecretIdentifierOwnerMismatch(ids []*vaultcommon.SecretIdentifier, workflowOwner string) error {
+	if len(ids) == 0 {
+		return errors.New("request batch must contain at least 1 item")
+	}
 	for idx, id := range ids {
 		if id == nil {
-			// InvalidParamsError is returned by the gateway handler for this case.
-			continue
+			return fmt.Errorf("secret ID must not be nil at index %d", idx)
 		}
-		if normalizeOwner(id.Owner) != normalizeOwner(workflowOwner) {
+		if vaultutils.NormalizeOwner(id.Owner) != vaultutils.NormalizeOwner(workflowOwner) {
 			return fmt.Errorf("secret identifier owner at index %d %q does not match authorized workflow owner %q", idx, id.Owner, workflowOwner)
 		}
 	}

core/capabilities/vault/authorizer_test.go

@@ -240,7 +240,186 @@ func TestAuthorizer_AllowListPath_RejectsListOwnerMismatch(t *testing.T) {
 	require.ErrorContains(t, err, "list secrets owner \"0xother\" does not match authorized workflow owner \"0xauthorized\"")
 }
 
-func TestAuthorizer_SkipsOwnerBindingWhenParamsMissing(t *testing.T) {
+func TestAuthorizer_JWTPath_RejectsOwnerMismatch(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		method      string
+		buildParams func(mismatchedOwner string) json.RawMessage
+		errContains string
+	}{
+		{
+			name:   "create",
+			method: vaulttypes.MethodSecretsCreate,
+			buildParams: func(mismatchedOwner string) json.RawMessage {
+				params, err := json.Marshal(vaultcommon.CreateSecretsRequest{
+					EncryptedSecrets: []*vaultcommon.EncryptedSecret{
+						{Id: &vaultcommon.SecretIdentifier{Owner: mismatchedOwner, Namespace: "ns", Key: "k"}, EncryptedValue: "cipher"},
+					},
+				})
+				require.NoError(t, err)
+				return params
+			},
+			errContains: "encrypted secret owner at index 0",
+		},
+		{
+			name:   "update",
+			method: vaulttypes.MethodSecretsUpdate,
+			buildParams: func(mismatchedOwner string) json.RawMessage {
+				params, err := json.Marshal(vaultcommon.UpdateSecretsRequest{
+					EncryptedSecrets: []*vaultcommon.EncryptedSecret{
+						{Id: &vaultcommon.SecretIdentifier{Owner: mismatchedOwner, Namespace: "ns", Key: "k"}, EncryptedValue: "cipher"},
+					},
+				})
+				require.NoError(t, err)
+				return params
+			},
+			errContains: "encrypted secret owner at index 0",
+		},
+		{
+			name:   "delete",
+			method: vaulttypes.MethodSecretsDelete,
+			buildParams: func(mismatchedOwner string) json.RawMessage {
+				params, err := json.Marshal(vaultcommon.DeleteSecretsRequest{
+					Ids: []*vaultcommon.SecretIdentifier{
+						{Owner: mismatchedOwner, Namespace: "ns", Key: "k"},
+					},
+				})
+				require.NoError(t, err)
+				return params
+			},
+			errContains: "secret identifier owner at index 0",
+		},
+		{
+			name:   "list",
+			method: vaulttypes.MethodSecretsList,
+			buildParams: func(mismatchedOwner string) json.RawMessage {
+				params, err := json.Marshal(vaultcommon.ListSecretIdentifiersRequest{
+					Owner:     mismatchedOwner,
+					Namespace: "ns",
+				})
+				require.NoError(t, err)
+				return params
+			},
+			errContains: "list secrets owner",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			mismatchedOwner := "0xother"
+			authorizedOwner := "0xauthorized"
+			params := tt.buildParams(mismatchedOwner)
+
+			req := jsonrpc.Request[json.RawMessage]{
+				ID:     "1",
+				Method: tt.method,
+				Params: &params,
+				Auth:   "jwt-token",
+			}
+
+			jwtBasedAuth := vaultmocks.NewAuthorizer(t)
+			jwtBasedAuth.EXPECT().AuthorizeRequest(mock.Anything, req).Return(vault.NewAuthResult("org-1", authorizedOwner, "digest-1", time.Now().Add(time.Minute).Unix()), nil).Once()
+
+			a := vault.NewAuthorizer(nil, jwtBasedAuth, logger.TestLogger(t))
+
+			authResult, err := a.AuthorizeRequest(t.Context(), req)
+			require.Nil(t, authResult)
+			require.ErrorContains(t, err, tt.errContains)
+			require.ErrorContains(t, err, authorizedOwner)
+		})
+	}
+}
+
+func TestAuthorizer_RejectsOwnerBindingOnMalformedBatches(t *testing.T) {
+	t.Parallel()
+
+	authorizedOwner := "0xauthorized"
+	tests := []struct {
+		name        string
+		method      string
+		buildParams func() json.RawMessage
+		errContains string
+	}{
+		{
+			name:   "create empty batch",
+			method: vaulttypes.MethodSecretsCreate,
+			buildParams: func() json.RawMessage {
+				params, err := json.Marshal(vaultcommon.CreateSecretsRequest{EncryptedSecrets: []*vaultcommon.EncryptedSecret{}})
+				require.NoError(t, err)
+				return params
+			},
+			errContains: "request batch must contain at least 1 item",
+		},
+		{
+			name:   "create nil secret id",
+			method: vaulttypes.MethodSecretsCreate,
+			buildParams: func() json.RawMessage {
+				params, err := json.Marshal(vaultcommon.CreateSecretsRequest{
+					EncryptedSecrets: []*vaultcommon.EncryptedSecret{
+						{Id: nil, EncryptedValue: "ab"},
+					},
+				})
+				require.NoError(t, err)
+				return params
+			},
+			errContains: "secret ID must not be nil at index 0",
+		},
+		{
+			name:   "update nil encrypted secret",
+			method: vaulttypes.MethodSecretsUpdate,
+			buildParams: func() json.RawMessage {
+				params, err := json.Marshal(vaultcommon.UpdateSecretsRequest{
+					EncryptedSecrets: []*vaultcommon.EncryptedSecret{nil},
+				})
+				require.NoError(t, err)
+				return params
+			},
+			errContains: "encrypted secret must not be nil at index 0",
+		},
+		{
+			name:   "delete nil secret identifier",
+			method: vaulttypes.MethodSecretsDelete,
+			buildParams: func() json.RawMessage {
+				params, err := json.Marshal(vaultcommon.DeleteSecretsRequest{
+					Ids: []*vaultcommon.SecretIdentifier{nil},
+				})
+				require.NoError(t, err)
+				return params
+			},
+			errContains: "secret ID must not be nil at index 0",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			params := tt.buildParams()
+			req := jsonrpc.Request[json.RawMessage]{
+				ID:     "1",
+				Method: tt.method,
+				Params: &params,
+			}
+
+			allowListBasedAuth := vaultmocks.NewAuthorizer(t)
+			allowListBasedAuth.EXPECT().AuthorizeRequest(mock.Anything, req).Return(vault.NewAuthResult("", authorizedOwner, "digest-1", time.Now().Add(time.Minute).Unix()), nil).Once()
+
+			a := vault.NewAuthorizer(allowListBasedAuth, nil, logger.TestLogger(t))
+
+			authResult, err := a.AuthorizeRequest(t.Context(), req)
+			require.Nil(t, authResult)
+			require.ErrorContains(t, err, tt.errContains)
+		})
+	}
+}
+
+func TestAuthorizer_RejectsOwnerBindingWhenParamsMissing(t *testing.T) {
+	t.Parallel()
+
 	allowListBasedAuth := vaultmocks.NewAuthorizer(t)
 	allowListBasedAuth.EXPECT().AuthorizeRequest(mock.Anything, mock.Anything).Return(vault.NewAuthResult("", "0xauthorized", "digest-1", time.Now().Add(time.Minute).Unix()), nil).Once()
 
@@ -250,6 +429,6 @@ func TestAuthorizer_SkipsOwnerBindingWhenParamsMissing(t *testing.T) {
 		ID:     "1",
 		Method: vaulttypes.MethodSecretsCreate,
 	})
-	require.NoError(t, err)
-	require.Equal(t, "0xauthorized", authResult.AuthorizedOwner())
+	require.Nil(t, authResult)
+	require.ErrorContains(t, err, "request params must not be nil")
 }