Should the overrides on env files really be VUE_APP_S3D_XXXX?

[guillemmateos]

Considering the overrides defined in the .env files are not really required at runtime of the Vue app, but essentially just to deploy it, is there really any need to use VUE_APP_ in front of them? This means they get bundled in the chunk-vendors file which seems far from ideal.

I really do not want my bucket name, aws profile or cloudfront ID to be embedded in the final app i am building and I would say it really isn't necessary unless i'm missing something.

Shouldn't the vars be overriden in some way so that they do not get embedded? maybe just S3D_XXXX

[nicekiwi]

.env files are not bundled at all thier values are used in the deployment process. And elsewhere in your application. Out of the box vue does not bundle those variables.

[nicekiwi]

VUE_APP_S3D following the vue plugin naming convention.

[guillemmateos]

I've been doing some research and there is definitely something weird with the app that i'm building (i guess it's in some of the packages that i'm using although it's only very few of them) as most env vars do definitely get included in the form of VUE_APP_S3D_ENABLE_CLOUDFRONT:"true" in the vendor-chunk and not even a single time but multiple times. I will investigate this further and try and figure out what module is creating those.

In any case, as far as I have been able to find, VueJS does not mention any requirement for env vars to start with VUE_APP_ unless you want them to be statically embedded into the client bundle, which I understand, being vue-cli-plugin-s3-deploy just used for deployment and not at app runtime, is not necessary at all. Details on env vars usage according to VueJS are detailed here: https://cli.vuejs.org/guide/mode-and-env.html#environment-variables.

I will also try and figure out why the env vars are getting included in my bundle and report results in case they are relevant.

[nicekiwi]

I've checked the chunked code and there's no record of the plugin values from the .env files or vue.config.js file.

As far as I know VUE_APP_* entries will only be included if they are explicitly referenced in your app when it's built. So if you include in your app let apiKey = process.env.VUE_APP_S3D_AWS_API_KEY, it will be uploaded as let apiKey = 12345678ASDFGHJ for example.

[nicekiwi]

Note that the CLI docs are directed towards using the CLI with your app, not developing plugins for the CLI.

[nicekiwi]

It looks like you're right about the env naming though. Unless we want it to be available vue code there's no requirement it be prefixed with VUE_APP_.

[guillemmateos]

I've been checking the code of the app that is producing the mentioned behaviour and turns out this is a much broader issue than I initially anticipated, which I just found by 'accident' using the vue-cli-plugin-s3-deploy. There are certain situations (which won't be uncommon) where a Vue app may unexpectedly leak env vars. In most of the cases it will only be the ones with VUE_APP_ prefix, but it won't always be limited to that.

The env vars will be leaked no matter if they are actually being used in the code or not for the client app. So as an example, if you have a VUE_APP_SECRET (which is a bad idea anyway) that you expect won't get included in the client bundle because you are not referencing it in your code, that may actually not be true and your secret may end up in the client bundle.

I'm trying to narrow down the specific conditions in which this happens to notify the relevant devs to fix the security issue.

In regards to the prefix discussion, I still think it makes sense to avoid the VUE_APP_ prefix for the plugin, as the vars are not really intended to be part of the client bundle and this may cause issues at some point and may leak potentially sensitive information like awsProfile, endpoint, bucket, aclor cloudfrontId. Those aren't what I would consider 'highly sensitive' information, but better to keep them private.

[multiplegeorges]

Hi @guillemmateos,

Thanks very much for this report and tracking down the issue.

Including the env vars in a bundle when the developer doesn't expect it is very undesirable behaviour, IMO. We should default to safe behaviour just in case a developer puts some sensitive info into one of our vars.

So, I agree, we don't need to have the VUE_APP_ prefix on the vars. I think S3D is probably sufficient.

As 4.0 is currently being worked on, that is probably the best time to make the switch.

What do you think @nicekiwi ?

[guillemmateos]

Hi @multiplegeorges

Happy to help! I agree that considering 4.0 is on the way, it's probably reasonable to consider doing the change for 4.0.

I've also seen that 4.0 has the prefix as a var, so it should be a really easy change. Also keen on knowing what @nicekiwi thinks about the change.

Let me know if you'd like me to submit a PR with the change to the prefix.

I have also reported the issue with the env vars to the relevant devs via their security contact information so hopefully that bit will be addressed soon. I will post any updates about it that are relevant for this.

[nicekiwi]

I'm not sure removing the prefix would resolve the issue. But we can certainly try :) I've been meaning to tidy up the ENV vars for 4.0.0 so this is even more of a reason to. :)

[nicekiwi]

@guillemmateos you could submit a PR if you like, against the devel branch if possible.

[guillemmateos]

Cool, I'll do that @nicekiwi. I cloned the dev branch and there seems to be a couple of tests failing, related to a missing helper referenced from bucket.js. I guess that is expected at this stage on the devel branch, correct?

I will also update the README to reflect the changes in the env var names.

[guillemmateos]

I'm not sure removing the prefix would resolve the issue. But we can certainly try :) I've been meaning to tidy up the ENV vars for 4.0.0 so this is even more of a reason to. :)

Sorry, I forgot to answer to this. At least in the specific issue I have experienced, changing the env vars to not have VUE_APP_ as a prefix will avoid them being exposed in the client bundle.

[nicekiwi]

@guillemmateos yeah don't worry about the tests at this stage :) cheers

[guillemmateos]

Ok, cool. PR submitted with a reference to this discussion.

@nicekiwi @multiplegeorges let me know if you'd like something else addressed there.

I did also remove a .DS_Store file that was in src. I think the syntax in gitignore for it is not ideal, and should be moved to something like **/.DS_Store