Bug: SMTP password field does not escape comment sign (hash)


(Joe Seyfried) #1

Since todays rebuild, it’s official (I thought I read about this somewhere, but cannot find it). Since this can really be a show-stopper, especially for new Discourse-admins, I will file it officially:

Docker Manager does not escape anything in the SMTP password. If you use quotes, they will be stripped. Therefore, any hash sign (and probably some other special characters) will be fatal for your SMTP connectivity…


Discourse app.yml doesn't like email password contain "#"
(Jeff Atwood) #2

Not really following here. How does docker manager get affected by SMTP password in the app.yml exactly?


(Joe Seyfried) #3

Not sure about DM’s role here myself, but here’s what happens:
I place my

DISCOURSE_SMTP_USER_NAME: 'discourse@example.com'      # (optional)
DISCOURSE_SMTP_PASSWORD: 'qaz123WSX!@#edc'               # (optional)

into container/app.yaml,
run ./launcher rebuild app, and some magic creates a file /var/www/discourse/config/discourse.conf inside the container, which contains the specified password - with no quotes or whatever protection for the special chars.


(Jeff Atwood) #4

Those fields should not have single quotes around them.


(Joe Seyfried) #5

Worked anyway - the config inside the container looks alright! And without the quotes, the problem with hash-sign-containing passwords is even more obvious - note the “# (optional)” beside it!


(Jeff Atwood) #6

Delimited by spaces or starting the line… you are saying the hash mark in the password field causes problems somehow? I don’t see why that would be.


(Joe Seyfried) #7

I had one account which I did not get working - the difference between them is a hash sign in the password. Maybe the parsers inside the containers get thrown off by this?


(Marcin Rataj) #8

I have the same issue. Did some preliminary investigation and:

DISCOURSE_SMTP_PASSWORD: 'password@#?'

is correctly placed in /var/www/discourse/config/discourse.conf:

smtp_password = password@#?

Note that the password it is not wrapped in quotes when placed in discourse.conf – may this be the source of issue?


(Joe Seyfried) #9

Yeah, still a bug. The rebuild does not properly escape, or at least quote the relevant strings…


(Jeff Atwood) #10

I personally feel the confusion of having quotes in this field that “are not part of the password” outweighs the marginal benefit of allowing # – maybe a comment above the field would suffice for now.


(Marcin Rataj) #11

@codinghorror Quotes are already ignored during parsing of discourse.conf:

I was about to prepare a PR for discourse_docker, but it seems that @malexw did it in 2014 :confused:

@sam could you take a second look at Add quotes around values in the generated discourse.conf file by malexw · Pull Request #118 · discourse/discourse_docker · GitHub (I left a comment there).


(Jeff Atwood) #12

I still think a comment above about # in the password is preferable to force including leading and trailing quotes in the password field that magically disappear, because the possibility of confusion is severe.


(Marcin Rataj) #13

The comment should also include " and '
It is possible to have these characters in password and currently both are ignored by global_setting.rb#L69.


(Joe Seyfried) #14

I understand Jeff’s hesitation here… but the only place where the quotes are really important are discourse.conf - where users normally don’t edit around! So I agree with Marcin’s comment on that PR, since it does not change any user-facing moving parts…


(Alex W) #15

I can explain my reasoning for opening the PR in the first place. I figure there are three different ways to proceed:

  1. Make no change, except for a comment in the config file noting that passwords containing # won’t be parsed properly
  2. Change the config parser to allow hashes in SMTP passwords without quotes - possibly by requiring whitespace before the # for it to be interpreted as a comment.
  3. Add quotes around the values in the generated discourse.conf file.

I ended up going with option 3 since it seemed the least of 3 evils.

I didn’t like option 1 because I think it’s important for security reasons to allow as many different characters as possible. In addition, the SMTP password I was using was given to me by our ops guy, and he was reluctant to change it for other business reasons.

Option 2 seemed the most difficult to implement correctly, and I was also worried about usability issues with having a commenting system that operated differently from a standard system. I was worried that someone would add a comment to the end of the SMTP password and then get confused when the comment was interpreted as part of the password.

Option 3 isn’t great either, for the reasons that @sam and @codinghorror have mentioned. But if every value in the .conf file is wrapped in quotes like my PR does, I think that someone looking at the generated .conf file would notice that pattern.

Anyway, we’ve got to do something, at least. It took me a long time to figure out why my password wasn’t working, and clearly I’m not the only one.

Unfortunately I don’t have access to the system I was using for development and testing, so I can’t contribute any more code.


(Marcin Rataj) #16

FYI I came up with a custom template that fixes this issue locally, without the need for changing Discourse code.

Create a new template file (eg. /var/discourse/templates/password_field_fix.yml) with:

run:
  # fix for a bug discussed at https://goo.gl/M4KU6Y
  - replace:
     filename: /etc/runit/1.d/copy-env
     from: 'puts "#{$1.downcase} = #{v}" if'
     to: 'puts "#{$1.downcase} = \"#{v}\"" if'

And add it in app.yml after templates/web.template.yml:

templates:
(...)
  - "templates/web.template.yml"
  - "templates/password_field_fix.yml"
(...)

It solved problem for me.


(Jeff Atwood) #17

I added a comment warning

https://github.com/discourse/discourse_docker/commit/7439768eb05dcdb101677e913015dff811897093

Like so

#DISCOURSE_SMTP_PASSWORD: pa$$word      # (optional, WARNING the char '#' in pw can cause problems!)

(Sam Saffron) #18

I might as well just fix the conf parser, this is an odd restriction, bookmarked will fix


(Kane York) #19

The colon can also cause yml problems. Better to enclose it in quotes.

#DISCOURSE_SMTP_PASSWORD: "pa$$word"      # (optional, keep the quotes)

https://github.com/discourse/discourse_docker/pull/191


(Jeff Atwood) #20

No, then people will think the password contains quotes. This was discussed…