Use the URL origin instead of hostname to identify subject websites

[rekmarks]

Due to some error made in the days of yore, the extension originally used the URL hostname component to identify dapps / websites/ (In the language of the permission controller, "subjects" that we identify by a part of their URLs.) This was changed to the more appropriate origin component in MetaMask/metamask-extension#8717. This change never made it to mobile, creating a confusing and potentially dangerous discrepancy in permission enforcement between the two applications. The offending line in mobile is here, although this may have implications for the SDK as well. Note that this may require a state migration.

[owencraston]

I suspect we are seeing the below error because of this line in the SSK Snap. The pre defined permissions map is here. Since I am testing with the published dapp (https://metamask.github.io/snap-simple-keyring/latest/) I figured that it would work but somewhere in the chain of commands the prefix (https://) is being stripped so the permissions are not accepted. Is there a reason we are stripping the prefix on mobile and not on extension?

[owencraston]

I tried changing this line to ensure that we are using the raw url in the snapMethodMiddlewareBuilder but it did not completely trickle down to the snap.

from

app/core/BackgroundBridge/BackgroundBridge.js

snapMethodMiddlewareBuilder(
        Engine.context,
        Engine.controllerMessenger,
        origin,
        // We assume that origins connecting through the BackgroundBridge are websites
        SubjectType.Website,
      ),

to

snapMethodMiddlewareBuilder(
        Engine.context,
        Engine.controllerMessenger,
        this.url,
        // We assume that origins connecting through the BackgroundBridge are websites
        SubjectType.Website,
      ),

[owencraston]

I also tried this change but got the following error.

WARN  [MetaMask DEBUG]: {"code": -32603, "message": "Must specify non-empty string origin."}
 LOG  [MetaMask DEBUG]: RPC (https://metamask.github.io): {"id": 771516241, "jsonrpc": "2.0", "method": "wallet_requestSnaps", "origin": "https://metamask.github.io", "params": {"npm:@metamask/snap-simple-keyring-snap": {"version": "1.1.2"}}, "toNative": true} -> {"error": {"code": -32603, "message": "Must specify non-empty string origin."}, "id": 771516241, "jsonrpc": "2.0"}

[rekmarks]

@owencraston assuming that this.url is a URL, that last one has to be const { origin } = this.url.

[Daniel-Cross]

@owencraston I'm looking into this but I'm getting the following message.

Does anyone know what version I need to be on to connect first before I can debug?

{"version": "1.1.6"}}, "toNative": true} -> {"error": {"code": -32603, "data": {"originalError": [Object]}, "message": "Installing Snaps is currently disabled in this version of MetaMask."}, "id": 1160102261, "jsonrpc": "2.0"}

[FrederikBolding]

Does anyone know what version I need to be on to connect first before I can debug?

@Daniel-Cross You'll need to set METAMASK_BUILD_TYPE=flask when building, which will enable installation of all types of Snaps.

[Daniel-Cross]

@owencraston @FrederikBolding

Has this been fixed or is there currently some kind of workaround in place?

Tried to replicate and got a success message:

[owencraston]

I created a temporary work around by passing the entire URL as the snap origin. This is working with the current code. To truly test this we will need to swap current implementation of keyringSnapPermissionsBuilder for the one on the extension that uses the subject metadata controller..

Mobile:

export function keyringSnapPermissionsBuilder(
  origin: string,
): () => string[] {
  return () => {
    if (origin === 'metamask') {
      return METAMASK_ALLOWED_METHODS;
    }

    if (PORTFOLIO_ORIGINS.includes(origin)) {
      return PORTFOLIO_ALLOWED_METHODS;
    }
    return isProtocolAllowed(origin) ? WEBSITE_ALLOWED_METHODS : [];
  };
}

Extension:

export function keyringSnapPermissionsBuilder(
  controller: SubjectMetadataController,
  origin: string,
): () => string[] {
  return () => {
    if (origin === 'metamask') {
      return METAMASK_ALLOWED_METHODS;
    }

    if (PORTFOLIO_ORIGINS.includes(origin)) {
      return PORTFOLIO_ALLOWED_METHODS;
    }

    const originMetadata = controller.getSubjectMetadata(origin);
    if (originMetadata?.subjectType === SubjectType.Website) {
      return isProtocolAllowed(origin) ? WEBSITE_ALLOWED_METHODS : [];
    }

    return [];
  };
}

The current issue is that the subject metadata controller uses the snapId as the key so passing in an origin that resembles a url (https://www.snap.com) will not work for referencing the preferences controller. solution could be passing in the snap id to this function (then we lose the https check) or modifying the subject metadata to use the url as the key.

[Cal-L]

@Daniel-Cross it seems like a more general issue across subjects that use permissions controller. Did the fix apply to those areas as well? For example, connecting to dapps, WC, snaps, etc?

[FrederikBolding]

@Cal-L As far as I know there has been no fix. Owen has worked around the previously blocking issue, but as the original ticket states we should migrate from using hostname to origin everywhere to truly fix the problem.