Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add auth0 env vars to replace-environment-config script #1332

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

schroerbrian
Copy link
Contributor

Note: as part of this PR I could also replace line 16's sed command slash delimiters with a hash or pipe, etc. Any thoughts on this? The only downside is if we ever use an env var that includes a hash or pipe, etc, we'd have to escape it out, just as we needed to do with url slashes.

@lgarofalo
Copy link
Member

Note: as part of this PR I could also replace line 16's sed command slash delimiters with a hash or pipe, etc. Any thoughts on this? The only downside is if we ever use an env var that includes a hash or pipe, etc, we'd have to escape it out, just as we needed to do with url slashes.

Up to you, not too worried about it.

Copy link
Member

@richardxia richardxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: as part of this PR I could also replace line 16's sed command slash delimiters with a hash or pipe, etc. Any thoughts on this? The only downside is if we ever use an env var that includes a hash or pipe, etc, we'd have to escape it out, just as we needed to do with url slashes.

I think it'd be good to do this at some point to avoid the weird workaround in https://github.com/ShelterTechSF/gcloud/pull/33#discussion_r1554047145, but it doesn't necessarily need to be in this PR.

Hash might not be the best replacement symbol, since that's legal in URLs as well. Pipe is probably better, but we still run the risk of some other kind of environment variable having it. In general, simply changing the delimiter symbol isn't sufficient, since we don't know what variable values we may see in the future.

I think the safest thing to do would be to properly escape all special characters in the replacement variable, which I think the monster of an answer in https://stackoverflow.com/a/2705678 will do.

Another option that would be significantly more work but be a lot less janky than using sed to replace magic strings in our already-built webpack bundle would be to properly inject variables into our app. I couldn't actually find many good articles on it, but this one roughly captures the rough idea and a few solutions and tradeoffs, despite being written in 2016 and containing some outdated code examples: http://ryanogles.by/on-injecting-a-javascript-environment/

In any case, I think it's up to you what you want to do here. I think it's fine to continue to do nothing, and I also think using | as a delimiter would be a slight improvement over the status quo.

@schroerbrian
Copy link
Contributor Author

Note: as part of this PR I could also replace line 16's sed command slash delimiters with a hash or pipe, etc. Any thoughts on this? The only downside is if we ever use an env var that includes a hash or pipe, etc, we'd have to escape it out, just as we needed to do with url slashes.

I think it'd be good to do this at some point to avoid the weird workaround in ShelterTechSF/gcloud#33 (comment), but it doesn't necessarily need to be in this PR.

Hash might not be the best replacement symbol, since that's legal in URLs as well. Pipe is probably better, but we still run the risk of some other kind of environment variable having it. In general, simply changing the delimiter symbol isn't sufficient, since we don't know what variable values we may see in the future.

I think the safest thing to do would be to properly escape all special characters in the replacement variable, which I think the monster of an answer in https://stackoverflow.com/a/2705678 will do.

Another option that would be significantly more work but be a lot less janky than using sed to replace magic strings in our already-built webpack bundle would be to properly inject variables into our app. I couldn't actually find many good articles on it, but this one roughly captures the rough idea and a few solutions and tradeoffs, despite being written in 2016 and containing some outdated code examples: http://ryanogles.by/on-injecting-a-javascript-environment/

In any case, I think it's up to you what you want to do here. I think it's fine to continue to do nothing, and I also think using | as a delimiter would be a slight improvement over the status quo.

Thanks for the feedback, as well as the links! For expediency's sake, I'm merging this without further changes.

@schroerbrian
Copy link
Contributor Author

Thanks for the reviews!

@schroerbrian schroerbrian merged commit eee78f9 into master Apr 11, 2024
5 checks passed
@schroerbrian schroerbrian deleted the auth0_env_vars branch April 11, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants