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

fix: Fix special char handling in pre-define env #11799

Merged
merged 1 commit into from Mar 11, 2022
Merged

Conversation

sharat87
Copy link
Member

When starting the fat container with APPSMITH_MONGODB_URI variable set to a value that has query params, like mongodb+srv://user:pass@appsmith.abcde.mongodb.net/testdb?retryWrites=true&w=majority, then the fat container ignores it and goes ahead with using a local MongoDB server.

The problem is that we generated a pre-define.env file, that gets the following line (among many others like it):

APPSMITH_MONGODB_URI=mongodb+srv://user:pass@appsmith.abcde.mongodb.net/testdb?retryWrites=true&w=majority

This file is then sourced with . pre-define.env. When this runs, this file is evaluated like a shell script, and the shell's glob based file-name expansions take affect.

So for this line, the shell looks for a file that looks like mongodb+srv://user:pass@appsmith.abcde.mongodb.net/testdb?retryWrites=true&w=majority, where there's any letter in place of the ?, which is the glob definition of ? character. Since it doesn't find any such files, it will turn this line into just APPSMITH_MONGODB_URI=, indicating that there's no files to expand the glob pattern into.

Now since the APPSMITH_MONGODB_URI is set to empty value, the fat container will proceed to use it's local MongoDB just fine.

This can be avoided by wrapping the value in quotes. Like

APPSMITH_MONGODB_URI='mongodb+srv://user:pass@appsmith.abcde.mongodb.net/testdb?retryWrites=true&w=majority'

In this case, the shell will take the value verbatim and won't attempt to do glob expansion (or any potential variable expansion, if the value has any $ characters in it).

This PR adds a sed transformation to the generation of pre-define.env that:

  1. Replaces ' in each line with '"'"', which is kind of an escaped single-quote, in a single-quoted string.
  2. Adds a ' just after the = sign on each line.
  3. Adds a ' at the end of each line.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

> export z="a'b=d?x"
> printenv | sed "s/'/'\"'\"'/; s/=/='/; s/$/'/" | grep '^z'
z='a'"'"'b=d?x'

@sharat87 sharat87 added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Mar 11, 2022
@vercel
Copy link

vercel bot commented Mar 11, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/get-appsmith/appsmith/BLm8Lt1dUakypPTxHLB4EEnGWRYn
✅ Preview: https://appsmith-git-sharat87-patch-1-get-appsmith.vercel.app

@github-actions github-actions bot removed the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Mar 11, 2022
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@mohanarpit mohanarpit changed the title Fix special char handling in pre-define env fix: Fix special char handling in pre-define env Mar 11, 2022
@github-actions github-actions bot added the Bug Something isn't working label Mar 11, 2022
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@mohanarpit mohanarpit merged commit e732190 into release Mar 11, 2022
@mohanarpit mohanarpit deleted the sharat87-patch-1 branch March 11, 2022 15:51
@WernerEngelsB
Copy link

even though this PR is referened in Changelog of v1.6.15, the actual entrypoint.sh within v1.6.15 (https://github.com/appsmithorg/appsmith/blob/v1.6.15/deploy/docker/entrypoint.sh) does not seem to have the changes of this PR merged...

@WernerEngelsB
Copy link

even though this PR is referened in Changelog of v1.6.15, the actual entrypoint.sh within v1.6.15 (https://github.com/appsmithorg/appsmith/blob/v1.6.15/deploy/docker/entrypoint.sh) does not seem to have the changes of this PR merged...

@sharat87 could you have a look, why this happend?

@sharat87
Copy link
Member Author

Hey @WernerDreierB, thanks for bringing my attention to this. This fix wasn't part of the last release, it was erroneously reported in the release notes. We are correcting the release notes now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants