138456 - adding invalid addresses to CC list results in html error displayed as text

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P3)

Description

[John Vandenberg]

When entering an invalid email address in either the Assigned To or CC input
boxes, the error message returned in DBNameToIdAndCheck is displayed without a
Content-type header preceeding it.

Comment 1

[John Vandenberg]

Within post_bug.cgi, calls to CheckFormField and CheckFormFieldDefined which
result in an error, and the two direct calls to PuntTryAgain, all result in Bad
Header errors.

Steps to reproduce:
1) Simulate a enter_bug.cgi page, and remove the hidden product field, or set
the product field to a value which is not valid.
2) Submit form

Expected Results:
Error messages indicating that a product field must be defined, and must have a
valid value.

Actual Results:
500 Internal Server Error

Comment 2

[Dave Miller [:justdave]]

John, this sounds like two separate issues, can you file a separate bug for the
second one?  Thanks.

Making a 2.16 blocker, entering a bad CC address is a pretty common thing to do,
the error messages for it need to work right.

Comment 3

[John Vandenberg]

As far as I can see this is the same basic problem as bug 109054.

The simple fix for this is to move the Content-type up higher to be just
underneath the last DisplayError.  The 500's and plain text then disappear and a
page without a Bugzilla header results.  A header can not be displayed at this
point as the title is not known until later, as it displays the newly created
bug number in the title.

A better solution would be to templatise DBNameToIdAndCheck() and resolve bug
136770 at the same time.

Comment 4

[Gervase Markham [:gerv]]

Where we are heading with Content-Types is that they should get printed just
before templates; all error messages up to that point should use DisplayError(),
which prints a Content-Type of its own.

This presents a slight problem, because the admin CGIs, which are not yet
templatised, may get double content-types. But admins can live with that :-)

So, change the calls from PuntTryAgain to ThrowUserError, with the
$unlock_tables parameter set to true.

Gerv

Comment 5

[John Vandenberg]

In this case, DBNameToIdAndCheck is in globals.pl , whereas ThrowUserError and
friends are in CGI.pl  
The 500 errors due to PuntTryAgain are being addressed in bug 138879, however I
will try and get a patch up for it based on your suggestion.

Comment 6

[Gervase Markham [:gerv]]

> In this case, DBNameToIdAndCheck is in globals.pl , whereas ThrowUserError and
> friends are in CGI.pl 

Oops. Does this mean that we can't use them in globals.pl? If so, that's a bit
bad - we'll need to move them to globals.pl. We want to be able to use them
everywhere.

Gerv

Comment 7

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Puts correct error handling into DBNameToIDAndCheck, and fixes bug 136770 along
the way.

Gerv

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.2

<cough> This one doesn't have Mail stuff mixed up in it...

Gerv

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 80847 [details] [diff] [review]
Patch v.2

works, trivial, 2xr=myk

Comment 10

[Gervase Markham [:gerv]]

Fixed.

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.158; previous revision: 1.157
done

Gerv