Steps to reproduce: 1. Go to bug 33830. 2. Try to add <u>@netscape.com to the CC list. Result: The name @netscape.com is not a valid username. -------------------------------------- Expected: The name <u>@netscape.com is not a valid username. This bug happens with all e-mail address fields, not just with CC. I think it affects the new-bug form as well as the form for modifying existing bugs. This is a security hole. Fixing bug 26257 would make the security hole less exploitable, but it would still be a bug.

This patch looks to me like it will put HTML escapes in the database... not necessarily a good thing. Based on the code surrounding the line you added I think the quoting actually needs to be done in globals.pl in the DBIDtoNameAndCheck() routine.

Yes, I can do it there. In that case, escapes wouldn't be (potentially) added to the database unless ForceOK was true. Is that acceptable? CGI.pl isn't available to globals.pl so I just copied the three lines. Gerv

CGI.pl and globals.pl are both required in all of bugzilla's CGI's. Also, I don't think this solves the problem at hand. The CC ($name) is passed to the DBNameToIdAndCheck() routine, but it is not returned, so quoting it there will not have any effect. What was done for other bugs of the nature is running the variable through html_quote() before it is printed. my $cc = html_quote($::FORM{newcc}); print "$cc doesn't exist"; Of course, this has to be done anywhere that user input is echoed in an error message (as any other time).

> CGI.pl and globals.pl are both required in all of bugzilla's CGI's. Well, I tried calling the html_quote function and it didn't like it... > so quoting it there will not have any effect. The error message which is the source of this bug report is there, and my code does correct that error message. If you are saying there's a problem with invalid characters getting into the DB, then this should be caught by emailregexp, shouldn't it? Gerv

My bad. DBNameToIdAndCheck() also prints the error message, so quoting it there does work. Also, |$name = html_quote($name);| does work where you copied the regular expressions from CGI.pl.

Yes... that looks good. r=jake on attachment 40452 [details] [diff] [review].

Hmm, this has a r= on it and has been sitting here. I'm glad it hasn't been checked in yet, because I'm still going to veto this one. You're HTML-encoding data going to the database still. DBname_to_id() is safe, because it's using SqlQuote() on the data before it goes to the database. That'll prevent anything nasty from happening there. So there's no need to encode the data prior to that point. The next patch I'll post here in a minute should be a bit safer (it only encodes data getting sent back to the browser, and not data that's getting send to the database)

> You're HTML-encoding data going to the database still. There's something wrong here. It's asking the DB whether <string> is a valid email address in the Bugzilla DB. (DBNameToIDAndCheck()). Who cares what <string> is? & < and > are not valid characters in an email address, as far as I am aware, so munging them to other non-valid characters is not a wrong thing to do. The match will fail anyway. Gerv

ok, guess that's true... unless $forceok is set, then it writes it. Then again, the emailregexp should stop that. :-) OK, I have no preference. Someone besides me re-review this and pick one of the two. Either one works :-)

Well, my patch has r= and you said they are the same, so I'll just check it in :-) Gerv

I prefer Dave's patch, plus a check when you create a new account to disallows <>& in the e-mail address. Currently I can create an account with <> in the e-mail address and then CC that account on a bug. (See test bug 88448.)

> plus a check when you create a new account to disallows > <>& in the e-mail address. We don't do this already? If we don't, that's a separate (serious, 2.14) bug. This one can be fixed by either my patch or Dave's, which are totally equivalent. Gerv

OK, new patch coming up. It does the following: - Adds a regexp to match all illegal chars in an email address (list gathered from http://javascript.internet.com/forms/check-email.html) to the CheckEmailSyntax function. It will check that it matches the emailregexp param and that it doesn't match any of those characters. - Adds an explanation to the error that this list of chars are not allowed. - Simplifies the default emailregexp so it doesn't exclude spaces and commas, as we are doing that in the new regexp. - Change the emailregexpdesc to match. - Fixes the original problem Review me, please. Gerv

r= justdave checked in.

*** Bug 93385 has been marked as a duplicate of this bug. ***

Moving to Bugzilla product