feat(transaction-controller): gate internal validations on explicit isInternal flag

packages/transaction-controller/CHANGELOG.md

@@ -11,9 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Export `decodeAuthorizationSignature` utility that decodes a 65-byte EIP-7702 authorization signature into RLP-canonical `r`, `s`, and `yParity` ([#8656](https://github.com/MetaMask/core/pull/8656))
   - All `eth_sendRawTransaction` failures are prefixed `RPC submit:` for failure-surface attribution in error metrics
+- Add optional `isInternal` flag to `addTransaction` and `addTransactionBatch`, persisted on `TransactionMeta` and `TransactionBatchMeta` ([#8633](https://github.com/MetaMask/core/pull/8633))
 
 ### Changed
 
+- **BREAKING:** Internal-only validations are now gated by `isInternal` instead of `origin === ORIGIN_METAMASK`; callers that relied on `ORIGIN_METAMASK` to skip them must pass `isInternal: true` ([#8633](https://github.com/MetaMask/core/pull/8633))
 - Bump `@metamask/accounts-controller` from `^37.2.0` to `^38.0.0` ([#8665](https://github.com/MetaMask/core/pull/8665))
 - Bump `@metamask/messenger` from `^1.1.1` to `^1.2.0` ([#8632](https://github.com/MetaMask/core/pull/8632))
 - Bump `@metamask/network-controller` from `^30.0.1` to `^30.1.0` ([#8636](https://github.com/MetaMask/core/pull/8636))

packages/transaction-controller/src/TransactionController.test.ts

@@ -1656,7 +1656,7 @@ describe('TransactionController', () => {
 
         await controller.addTransaction(
           { from: ACCOUNT_MOCK, to: ACCOUNT_MOCK, ...extraTxParamsToSet },
-          { networkClientId: NETWORK_CLIENT_ID_MOCK },
+          { isInternal: true, networkClientId: NETWORK_CLIENT_ID_MOCK },
         );
 
         expect(controller.state.transactions[0].txParams.type).toBe(type);
@@ -3714,7 +3714,7 @@ describe('TransactionController', () => {
         ).rejects.toThrow('Batch ID already exists');
       });
 
-      it('does not throw if duplicate but internal origin', async () => {
+      it('does not throw if duplicate but isInternal', async () => {
         const { controller } = setupController({
           options: {
             state: {
@@ -3735,6 +3735,7 @@ describe('TransactionController', () => {
 
         await controller.addTransaction(txParams, {
           batchId: BATCH_ID_MOCK,
+          isInternal: true,
           networkClientId: NETWORK_CLIENT_ID_MOCK,
         });
       });

packages/transaction-controller/src/TransactionController.ts

@@ -1198,6 +1198,7 @@ export class TransactionController extends BaseController<
       excludeNativeTokenForFee,
       isGasFeeIncluded,
       isGasFeeSponsored,
+      isInternal = false,
       isStateOnly,
       method,
       nestedTransactions,
@@ -1234,6 +1235,7 @@ export class TransactionController extends BaseController<
       data: txParams.data,
       from: txParams.from,
       internalAccounts,
+      isInternal,
       origin,
       permittedAddresses,
       txParams,
@@ -1262,7 +1264,7 @@ export class TransactionController extends BaseController<
         (tx) => tx.batchId?.toLowerCase() === batchId?.toLowerCase(),
       );
 
-    if (isDuplicateBatchId && origin && origin !== ORIGIN_METAMASK) {
+    if (isDuplicateBatchId && !isInternal) {
       throw new JsonRpcError(
         ErrorCode.DuplicateBundleId,
         'Batch ID already exists',
@@ -1272,6 +1274,7 @@ export class TransactionController extends BaseController<
     const dappSuggestedGasFees = this.#generateDappSuggestedGasFees(
       txParams,
       origin,
+      isInternal,
     );
 
     const transactionType =
@@ -1307,6 +1310,7 @@ export class TransactionController extends BaseController<
         ? {}
         : { excludeNativeTokenForFee }),
       isFirstTimeInteraction: undefined,
+      isInternal,
       isStateOnly,
       nestedTransactions,
       networkClientId,
@@ -3621,8 +3625,9 @@ export class TransactionController extends BaseController<
   #generateDappSuggestedGasFees(
     txParams: TransactionParams,
     origin?: string,
+    isInternal?: boolean,
   ): DappSuggestedGasFees | undefined {
-    if (!origin || origin === ORIGIN_METAMASK) {
+    if (isInternal || !origin) {
       return undefined;
     }
 

packages/transaction-controller/src/types.ts

@@ -325,6 +325,9 @@ export type TransactionMeta = {
    */
   origin?: string;
 
+  /** Whether the transaction was added by trusted internal MetaMask code. */
+  isInternal?: boolean;
+
   /**
    * The original dapp proposed token approval amount before edit by user.
    */
@@ -613,6 +616,9 @@ export type TransactionBatchMeta = {
    */
   origin?: string;
 
+  /** Whether the batch was added by trusted internal MetaMask code. */
+  isInternal?: boolean;
+
   /**
    * ID of the JSON-RPC request from DAPP.
    */
@@ -1866,6 +1872,9 @@ export type TransactionBatchRequest = {
   /** Origin of the request, such as a dApp hostname or `ORIGIN_METAMASK` if internal. */
   origin?: string;
 
+  /** Whether the batch was added by trusted internal MetaMask code. */
+  isInternal?: boolean;
+
   /** Whether to overwrite existing EIP-7702 delegation with MetaMask contract. */
   overwriteUpgrade?: boolean;
 
@@ -2251,6 +2260,9 @@ export type AddTransactionOptions = {
   /** Origin of the transaction request, such as a dApp hostname. */
   origin?: string;
 
+  /** Whether the transaction was added by trusted internal MetaMask code. */
+  isInternal?: boolean;
+
   /** Custom logic to publish the transaction. */
   publishHook?: PublishHook;
 

packages/transaction-controller/src/utils/batch.ts

@@ -131,6 +131,7 @@ export async function addTransactionBatch(
 
   validateBatchRequest({
     internalAccounts: getInternalAccounts(),
+    isInternal: transactionBatchRequest.isInternal,
     request: transactionBatchRequest,
     sizeLimit,
   });
@@ -296,6 +297,7 @@ async function addTransactionBatchWith7702(
     from,
     gasFeeToken,
     gasLimit7702,
+    isInternal,
     networkClientId,
     origin,
     overwriteUpgrade,
@@ -438,6 +440,7 @@ async function addTransactionBatchWith7702(
     excludeNativeTokenForFee,
     isGasFeeIncluded,
     isGasFeeSponsored,
+    isInternal,
     nestedTransactions,
     networkClientId,
     origin,
@@ -724,7 +727,7 @@ async function processTransactionWithHook(
     updateTransaction,
   } = request;
 
-  const { from, networkClientId, origin } = userRequest;
+  const { from, isInternal, networkClientId, origin } = userRequest;
 
   if (existingTransaction) {
     const { id, onPublish } = existingTransaction;
@@ -797,6 +800,7 @@ async function processTransactionWithHook(
       assetsFiatValues,
       batchId,
       disableGasBuffer: true,
+      isInternal,
       networkClientId,
       origin,
       publishHook,
@@ -941,6 +945,7 @@ async function prepareApprovalData({
 
   const {
     from,
+    isInternal,
     origin,
     networkClientId,
     transactions: nestedTransactions,
@@ -966,6 +971,7 @@ async function prepareApprovalData({
     from,
     gas: gasLimit,
     id: batchId,
+    isInternal,
     networkClientId,
     origin,
     transactions: nestedTransactions,

packages/transaction-controller/src/utils/gas-fees.test.ts

@@ -1,4 +1,3 @@
-import { ORIGIN_METAMASK } from '@metamask/controller-utils';
 import type { NetworkClientId } from '@metamask/network-controller';
 
 import type { TransactionControllerMessenger } from '../TransactionController';
@@ -506,9 +505,9 @@ describe('gas-fees', () => {
         );
       });
 
-      it('to custom if request gas price but no request maxFeePerGas or maxPriorityFeePerGas and origin is metamask', async () => {
+      it('to custom if request gas price but no request maxFeePerGas or maxPriorityFeePerGas and isInternal', async () => {
         updateGasFeeRequest.txMeta.txParams.gasPrice = GAS_HEX_MOCK;
-        updateGasFeeRequest.txMeta.origin = ORIGIN_METAMASK;
+        updateGasFeeRequest.txMeta.isInternal = true;
 
         await updateGasFees(updateGasFeeRequest);
 
@@ -517,7 +516,7 @@ describe('gas-fees', () => {
         );
       });
 
-      it('to medium if request gas price but no request maxFeePerGas or maxPriorityFeePerGas and origin not metamask', async () => {
+      it('to dappSuggested if request gas price but no request maxFeePerGas or maxPriorityFeePerGas and not isInternal', async () => {
         updateGasFeeRequest.txMeta.txParams.gasPrice = GAS_HEX_MOCK;
         updateGasFeeRequest.txMeta.origin = ORIGIN_MOCK;
 
@@ -538,8 +537,8 @@ describe('gas-fees', () => {
         );
       });
 
-      it('to medium if origin is metamask', async () => {
-        updateGasFeeRequest.txMeta.origin = ORIGIN_METAMASK;
+      it('to medium if isInternal', async () => {
+        updateGasFeeRequest.txMeta.isInternal = true;
 
         await updateGasFees(updateGasFeeRequest);
 
@@ -548,7 +547,7 @@ describe('gas-fees', () => {
         );
       });
 
-      it('to dappSuggested if origin is not metamask', async () => {
+      it('to dappSuggested if not isInternal', async () => {
         updateGasFeeRequest.txMeta.origin = ORIGIN_MOCK;
 
         await updateGasFees(updateGasFeeRequest);

packages/transaction-controller/src/utils/gas-fees.ts

@@ -1,8 +1,4 @@
-import {
-  ORIGIN_METAMASK,
-  gweiDecToWEIBN,
-  toHex,
-} from '@metamask/controller-utils';
+import { gweiDecToWEIBN, toHex } from '@metamask/controller-utils';
 import type {
   FetchGasFeeEstimateOptions,
   GasFeeState,
@@ -291,7 +287,7 @@ function getUserFeeLevel(request: GetGasFeeRequest): UserFeeLevel | undefined {
     !initialParams.maxPriorityFeePerGas &&
     initialParams.gasPrice
   ) {
-    return txMeta.origin === ORIGIN_METAMASK
+    return txMeta.isInternal
       ? UserFeeLevel.CUSTOM
       : UserFeeLevel.DAPP_SUGGESTED;
   }
@@ -313,7 +309,7 @@ function getUserFeeLevel(request: GetGasFeeRequest): UserFeeLevel | undefined {
     return UserFeeLevel.MEDIUM;
   }
 
-  if (txMeta.origin === ORIGIN_METAMASK) {
+  if (txMeta.isInternal) {
     return UserFeeLevel.MEDIUM;
   }
 

packages/transaction-controller/src/utils/validation.test.ts

@@ -1,4 +1,3 @@
-import { ORIGIN_METAMASK } from '@metamask/controller-utils';
 import { rpcErrors } from '@metamask/rpc-errors';
 import type { Hex } from '@metamask/utils';
 
@@ -792,6 +791,25 @@ describe('validation', () => {
         }),
       ).toBeUndefined();
     });
+
+    it('does not throw if isInternal is true regardless of origin', async () => {
+      expect(
+        await validateTransactionOrigin({
+          data: DATA_MOCK,
+          from: FROM_MOCK,
+          internalAccounts: [TO_MOCK],
+          isInternal: true,
+          origin: ORIGIN_MOCK,
+          permittedAddresses: ['0x123'],
+          selectedAddress: '0x123',
+          txParams: {
+            authorizationList: [],
+            to: TO_MOCK,
+            type: TransactionEnvelopeType.setCode,
+          } as TransactionParams,
+        }),
+      ).toBeUndefined();
+    });
   });
 
   describe('validateParamTo', () => {
@@ -842,7 +860,7 @@ describe('validation', () => {
       ).not.toThrow();
     });
 
-    it('does not throw if no origin and any transaction target is internal account with data', () => {
+    it('throws if no origin and any transaction target is internal account with data', () => {
       expect(() =>
         validateBatchRequest({
           ...VALIDATE_BATCH_REQUEST_MOCK,
@@ -852,18 +870,19 @@ describe('validation', () => {
             origin: undefined,
           },
         }),
-      ).not.toThrow();
+      ).toThrow(
+        rpcErrors.invalidParams(
+          'External calls to internal accounts cannot include data',
+        ),
+      );
     });
 
-    it('does not throw if internal origin and any transaction target is internal account with data', () => {
+    it('does not throw if internal and any transaction target is internal account with data', () => {
       expect(() =>
         validateBatchRequest({
           ...VALIDATE_BATCH_REQUEST_MOCK,
           internalAccounts: ['0x123', TO_MOCK],
-          request: {
-            ...VALIDATE_BATCH_REQUEST_MOCK.request,
-            origin: ORIGIN_METAMASK,
-          },
+          isInternal: true,
         }),
       ).not.toThrow();
     });
@@ -877,14 +896,11 @@ describe('validation', () => {
       ).toThrow(rpcErrors.invalidParams('Batch size cannot exceed 1. got: 2'));
     });
 
-    it('does not throw if transaction count is internal and greater than limit', () => {
+    it('does not throw if internal and transaction count is greater than limit', () => {
       expect(() =>
         validateBatchRequest({
           ...VALIDATE_BATCH_REQUEST_MOCK,
-          request: {
-            ...VALIDATE_BATCH_REQUEST_MOCK.request,
-            origin: ORIGIN_METAMASK,
-          },
+          isInternal: true,
           sizeLimit: 1,
         }),
       ).not.toThrow();