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 $ not being escaped in .env.production file generated by mastodon:setup #23012

Merged

Conversation

ClearlyClaire
Copy link
Contributor

No description provided.

@@ -395,7 +395,7 @@ namespace :mastodon do
incompatible_syntax = false

env_contents = env.each_pair.map do |key, value|
if value.is_a?(String) && value =~ /[\s\#\\"]/
if value.is_a?(String) && value =~ /[\s\#\\"\$]/
Copy link
Member

Choose a reason for hiding this comment

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

Instead of trying to fix this manually, should we use Shellwords.escape

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, but IIRC dotenv doesn't exactly implement shell rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would need to check if it's still the case, but at the time the escaping was introduced, you made the same suggestion and the reason we could not do that was #13156 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it still behaves the same. Shellwords.escape('foo!#test') would return 'foo\!\#test', which would be parsed by dotenv as 'foo!\'.
That being said we probably can do something as simple as "\"#{Shellwords.escape(value)}\"".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From Shellwords::shellescape's documentation:

Note that a resulted string should be used unquoted and is not intended for use in double quotes nor in single quotes.

And indeed, this would result in double-escaping some characters…

I'm going to more thoroughly check Dotenv's behavior and better document that code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrote a documented escape function that I tested to be fairly robust. I don't think everything can be escaped, though.

@ClearlyClaire ClearlyClaire marked this pull request as draft January 9, 2023 18:08
@ClearlyClaire ClearlyClaire marked this pull request as ready for review January 9, 2023 20:53
@ClearlyClaire ClearlyClaire force-pushed the fixes/mastodon-setup-escape-dollar-sign branch from a9984ba to 254d5c2 Compare January 9, 2023 21:30
@Gargron Gargron merged commit a65f86a into mastodon:main Jan 11, 2023
ClearlyClaire added a commit to ClearlyClaire/mastodon that referenced this pull request Jan 12, 2023
Gargron pushed a commit that referenced this pull request Jan 13, 2023
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.

None yet

4 participants