[Feature]: Make more strict regular expression not to expose unexpected environment variables

[t-jindai]

What area(s) will this request affect?

Extension

What type of change do you want to see?

Substantial change to existing feature

Overview

Overview

We need a limitation that restricts what environment variables are accessible in checkout ui extensions.

Details

The build step injects environment variables (e.g. process.env.APP_ENV) thanks to define option of esbuild. https://github.com/Shopify/cli/blob/a8dc21dabf0fbe3dc7da2e55796e5d02282d4687/packages/app/src/cli/services/extensions/bundle.ts#L134-L148

While the variables are filtered out, The regular expression is too loose. https://github.com/Shopify/cli/blob/a8dc21dabf0fbe3dc7da2e55796e5d02282d4687/packages/app/src/cli/services/extensions/bundle.ts#L124 https://github.com/Shopify/cli/blob/a8dc21dabf0fbe3dc7da2e55796e5d02282d4687/packages/app/src/cli/constants.ts#L38

Since injected variables are readable by user, the filtering rules must be restricted preventing from leaking unexpected values to users.

Solution

Environment variables which start with SHOPIFY_PUBLIC_ can be injected in built files.

export const EsbuildEnvVarRegexForPublic = /^SHOPIFY_PUBLIC_([a-zA-Z_$])([a-zA-Z0-9_$])*$/

Motivation

In my understanding, other meta-frameworks such as Next.js have the same mechanism.

Next.js injects environment variables which start with NEXT_PUBLIC_.

https://nextjs.org/docs/pages/building-your-application/configuring/environment-variables#bundling-environment-variables-for-the-browser

[amcaplan]

Hey @tomoyukikashiro, I'd like to understand your concern further. I believe only expressions like process.env.SOME_VARIABLE_NAME will be replaced. Is there a situation where you have in your frontend code process.env.SENSITIVE_INFORMATION and you don't want that to be replaced with the ENV value for SENSITIVE_INFORMATION?

[t-jindai]

@amcaplan

Is there a situation where you have in your frontend code process.env.SENSITIVE_INFORMATION?

No, there isn't.

---

I just want to point out that process.env.xxx is replaced with the actual value in not only shopify function (server side code) but also checkout ui extension (client side code)`.

To remind developers that environment variables become public (can be seen by users) in checkout ui extension, only variables which start with SHOPIY_PUBLIC_ can be used in shopify checkout ui extension.

This is the same rule as that of Next.js.

https://nextjs.org/docs/pages/building-your-application/configuring/environment-variables#bundling-environment-variables-for-the-browser

in order to make the value of an environment variable accessible in the browser, Next.js can "inline" a value, at build time, into the js bundle that is delivered to the client, replacing all references to process.env.[variable] with a hard-coded value. To tell it to do this, you just have to prefix the variable with NEXT_PUBLIC_. For example:

---

This is a just suggestion to make extension code more secure (IMO) but if you think that it isn't necessary, I would also agree with you!

[amcaplan]

Thanks for taking the time to explain your concerns and the basis for your proposed solution.

Given that it's a breaking change and would cause silent failures for thousands of apps, I don't think it would be wise to introduce this rule at this time.

Closing as wontfix.