101380 - Newlines, nulls, leading/trailing spaces are getting into summaries

Status: RESOLVED FIXED

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

Description

[Jacob Steenhagen]

When bug 101361 somehow a newline got embeded at the end of the summary
(Stephen, any idea how this happened? What browser were you using?)  This caused
the X-Bugzilla-Reason header to appear in the body.  Also, when I went to change
the bug, it said that I changed the summary, even though I never touched that
field (View Source on the Bug Activity page revealed that I removed the
newline).  We need to strip newlines in the summary (as well as any other one
line text fields) before we dump it to the database in post_bug.cgi.

Comment 1

[Stephen Rasku]

I have seen this a few times before (i.e. summary changed even though it hasn't
been touched).

When I originally posted the bug, I had started to file it under mozilla.org
instead of bugzilla.  I had quite extensive comments so I pasted the summary
into a terminal screen so that I could paste it back into the summary screen
without having to type it in again.  I am using Netscape 4.72 on Solaris 7.

Comment 2

[Matthew Tuck [:CodeMachine]]

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

Comment 3

[Matthew Tuck [:CodeMachine]]

We need to comprehensively deal with this.  I think nulls, newlines, tabs, etc
should be converted to spaces and then strip leading and trailing spaces.

We have seen one serious consequence of this problem here, there could well be
more.

Comment 4

[Jacob Steenhagen]

Without looking it up, I'm not 100% certain, but doesn't \s in a regexp match
all of those?  If so, we could just do:

$whatever =~ s/\s/ /g;
$whatever = trim($whatever);

If not, we'd have to build our own character class containing all those characters

Comment 5

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 6

[Matthew Tuck [:CodeMachine]]

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

Comment 7

[Matthew Tuck [:CodeMachine]]

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

Comment 8

[Matthew Tuck [:CodeMachine]]

We've had the permissions problem before with bad keywords cache, we need to
rearchitect the permissions to avoid this, so I filed a bug #132961.

See also new bug #132959 on sanity checking this.

Comment 9

[Jesse Ruderman]

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

Comment 10

[Jacob Steenhagen]

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

Comment 11

[Jacob Steenhagen]

Every now and then this happens and causes somebody w/out editbugs to be unable
to change the bug.  Bug 186001 was particularly nasty as the newlines were
embedded in the middle of the summary (one right after the first word).  This
would have made it so if somebody with editbugs would have loaded the bug and
clicked commit, changing nothing else, they would have inadvertently removed 90%
of the summary (the summary could use some trimming, but not quite that much :)

Granted the information would have still been there in the bug history, but
that's not the point.

Comment 12

[Dave Miller [:justdave]]

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

Comment 13

[Andreas Höfler]

Attachment: trims fields in post_bug.cgi

Trim fields in the "Enter Bug"-form to get rid of whitespaces at the end of
e.g. summary.

Comment 14

[Dave Miller [:justdave]]

All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20

Comment 15

[Christian Reis]

Comment on attachment 133247 [details] [diff] [review]
trims fields in post_bug.cgi

I'm for this change; I tried hard to think of a place where I don't want
leading and trailing whitespace to be murdered and I couldn't. 

(Having said that, I can't see how this would fix the more serious problems 
(being unable to change summaries, etc) here, but that's not the subject of
this bug :-)

Comment 16

[Christian Reis]

Comment on attachment 133247 [details] [diff] [review]
trims fields in post_bug.cgi

Actually, I'm thinking twice about this one. It may not be enough, and it may
just not be what we want :-)

There is some inconsistency with this patch between bugs and other tables
(since only fields being inserted into the bugs table are being trimmed).

Note that this patch doesn't fix the embedded newlines problem which was also
discussed here.

A lower-risk patch would just strip whitespace from summaries, but I think
embedded newlines would have to go too.

Comment 17

[Nick Rogers <wireboy>]

Attachment: trims all fields and removes newlines from summary

Comment 18

[Christian Reis]

Comment on attachment 147721 [details] [diff] [review]
trims all fields and removes newlines from summary

The comments aren't really necessary, but they can be removed upon checkin if
approved. However, this patch doesn't deal with tabs or nulls; wasn't that
suggested by MattyT?

Also, you're removing \r and \r, where AIUI we were going to replace them with
spaces.

Comment 19

[Matthew Tuck [:CodeMachine]]

Yeah we need to handle all kinds of weirdo characters, this possibly includes
ALL control characters.

I think conversion to spaces makes most sense since it preserves the separation
of whatever was there.

Comment 20

[Matthew Tuck [:CodeMachine]]

Actually this is probably a reasonable approach:

Convert miscellaneous control characters, nulls, tabs to spaces, regardless of
field.
If its a single line text field (status, wboard, URL, alias), convert newlines
to spaces, and trim.

Comment 21

[timeless]

erp. please leave my \t, \v, \f, \r, \n chars alone in comments, thank you very
much :)

Comment 22

[Matthew Tuck [:CodeMachine]]

The tabs in comments was an oversight, otherwise that's what I said.

Comment 23

[Marc Schumann [:Wurblzap]]

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

Comment 24

[Marc Schumann [:Wurblzap]]

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

Comment 25

[Marc Schumann [:Wurblzap]]

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

Comment 26

[Max Kanat-Alexander]

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to [e-mail] or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

Comment 27

[Martin Mokrejs]

I get with bugzilla-2.19.3 lines wrapped by a weird logic, even in the niddle of
the word. I think someone should inspect the code. This thread in another
project might help you comming up with a fast algorithm design.
http://rutherglen.ics.mq.edu.au/~alpha-bugzilla/show_bug.cgi?id=1398

Definitely, I'd like to disable line wrappings anyway, but it is not an option. :(

Comment 28

[Mark Loeser]

Another ramification of being able to put newlines, etc in the summary is that
it allows users to forge any header they want in the emails generated by
bugzilla.  That, in my opinion, makes it into a small security vulnerability.

Comment 29

[Frédéric Buclin]

Couldn't we use (and update if necessary) Util::value_quote() to replace undesirable characters by whitespaces (or remove them completely)? We should do that before inserting or updating bug summaries in the DB (post_bug.cgi and process_bug.cgi).

Comment 30

[Dave Miller [:justdave]]

Attachment: Patch v3 (2.20 and tip)

this version swaps all non-printable characters for spaces in the summary and trims all fields before going into the database.

Comment 31

[Dave Miller [:justdave]]

Attachment: Patch v3 (2.16 and 2.18)

Comment 32

[Frédéric Buclin]

Comment on attachment 201398 [details] [diff] [review]
Patch v3 (2.20 and tip)

shouldn't you include this conversion in process_bug.cgi too?

Comment 33

[Max Kanat-Alexander]

Comment on attachment 201398 [details] [diff] [review]
Patch v3 (2.20 and tip)

Yeah, this looks fine, but LpSolit is right, it needs to get process_bug, too, doesn't it?

Eventually this will move inside of Bugzilla::Bug.

Comment 34

[Max Kanat-Alexander]

Comment on attachment 201399 [details] [diff] [review]
Patch v3 (2.16 and 2.18)

Cancelling review based on r- of tip patch.

Comment 35

[Dave Miller [:justdave]]

Attachment: Patch v4 (2.20 and tip)

Comment 36

[Dave Miller [:justdave]]

Attachment: Patch v4 (2.16 and 2.18)

Comment 37

[Frédéric Buclin]

Comment on attachment 202909 [details] [diff] [review]
Patch v4 (2.20 and tip)

>Index: post_bug.cgi

> foreach my $field (@used_fields) {
>+    if ($field eq "short_desc") {
>+        my $temp = $cgi->param($field);
>+        $temp =~  s/[\x00-\x1F\x7F]/ /g;   # change control characters to spaces
>+        $cgi->param($field,$temp);
>+    }

This is not the right place to do this conversion IMO. You should do it when validating the title of the bug near line 110:

if (!defined $cgi->param('short_desc')
    || trim($cgi->param('short_desc')) eq "") {
    ThrowUserError("require_summary");
}



>Index: process_bug.cgi

>         if (!$cgi->param('dontchange')
>             || $cgi->param($field) ne $cgi->param('dontchange')) {
>             DoComma();
>+            my $value = $cgi->param($field);
>+            if ($field eq "short_desc") {
>+                $value =~  s/[\x00-\x1F\x7F]/ /g;   # change control characters to spaces
>+            }
>+            $::query .= "$field = " . SqlQuote(trim($value));
>         }

Same remark as above. This conversion should be done when validating the title of the bug near line 614:

    if (trim($cgi->param('short_desc')) eq "") {
        ThrowUserError("require_summary");
    }

Also, you should then update $cgi->param('short_desc') with this new value, as you did in post_bug.cgi.

Comment 38

[Frédéric Buclin]

Comment on attachment 202910 [details] [diff] [review]
Patch v4 (2.16 and 2.18)

same remarks here...

Comment 39

[Frédéric Buclin]

As there are several places where this cleanup could be welcome, I suggest to add a routine in Util.pm to avoid code duplication.

justdave, I hope your are still on it. ;)

Comment 40

[Frédéric Buclin]

You can now use the new Util::clean_text() function added by bug 238780 (landed on tip only, but the function can be taken as is for branches).

Comment 41

[Paul]

Attachment: using clean_text() v2.20

Comment 42

[Paul]

Attachment: white space cleaned v2.20 patch

Comment 43

[Paul]

Attachment: Patch for v2.16

Comment 44

[Paul]

Attachment: Patch for v2.18

Comment 45

[Paul]

Attachment: Patch for tip

Comment 46

[Frédéric Buclin]

I'm not sure I like this fix. What happens if a bug has some unwanted characters in his summary and a user without editbugs privs wants to CC himself or simply add a comment? I'm pretty sure that CheckCanChangeField() in process_bug.cgi will complain that he tried to change the bug summary. And the poor user cannot edit the bug anymore.

Comment 47

[Frédéric Buclin]

Comment on attachment 206397 [details] [diff] [review]
Patch for tip

>Index: post_bug.cgi

> if (!defined $cgi->param('short_desc')
>+    || trim(clean_text($cgi->param('short_desc'))) eq "") {
>     ThrowUserError("require_summary");
> }


> foreach my $field (@used_fields) {
>+    if ($field eq "short_desc") {
>+        # get rid of control characters
>+        $cgi->param($field, clean_text($cgi->param($field)));
>+    }
>     $sql .= SqlQuote($cgi->param($field)) . ",";
> }

As I said in comment 37, the foreach loop is not the right place to use clean_text(). Do it before testing the bug summary and leave this foreach loop alone.



>Index: process_bug.cgi

>+    if (trim(clean_text($cgi->param('short_desc'))) eq "") {
>         ThrowUserError("require_summary");
>     }

>+            my $value = $cgi->param($field);
>+            if (( $field eq "short_desc") || ($field eq "status_whiteboard")) {
>+                $value = clean_text($value);
>+            }
>+            $::query .= "$field = " . SqlQuote(trim($value));

The same comment as above applies here too, read again my comment 37. Moreover:

1) this bug is about the *bug summary* only. Cleaning the status whiteboard has nothing to do with this bug, which has to be backported to all branches. And we are not going to "clean" the status whiteboard on all branches. So doing it here only but not in backported patches is more than confusing.

2) with this patch applied, a user with no privs can no longer edit such bugs as CheckCanChangeField() is now complaining (I tested it right now). So existing bugs have to be checked by checksetup.pl.

Comment 48

[Max Kanat-Alexander]

I think we really just need to strip the newlines in the email headers, and leave everything else as-is. The email headers are the only place where this actually causes a *bug*, right?

Comment 49

[Frédéric Buclin]

For the record:

(19:27:14) LpSolit: justdave: the question is: do we want to prevent control characters on trunk?
(19:27:24) justdave: yes.
(19:27:30) justdave: there is no reason to allow them
(19:27:35) justdave: some browsers won't allow them
(19:27:53) justdave: so we stand a risk of data getting changed by a random submit that didn't touch it because someone's browser decided to clean it
(19:28:04) LpSolit: justdave: my second question is: do we want to prevent control characters for *new* values only or for existing ones too?
(19:30:00) LpSolit: justdave: if your answer is "existing values too", we would have to bypass some restrictions in process_bug.cgi for instance
(19:30:11) LpSolit: and that's something I would hesitate to r+
(19:30:30) LpSolit: I don't care about post_bug.cgi because it's a *new* bug
(19:30:35) justdave: fixing it for existing would be nice (on the trunk).  We've done it before for flag names, so there's precedence (and existing code for conflict handling)
(19:30:49) justdave: you don't do it in process_bug, you do it in checksetup.pl
(19:30:56) LpSolit: ok
(19:31:22) LpSolit: justdave: how long would it take to consider *all* columns of all tables???
(19:31:43) justdave: as long as we avoid attachment data and comments, probably not very long.
(19:31:55) justdave: couple minutes at the most.
(19:32:06) justdave: even for bmo



To summarize: 1) prevent control characters in *new* values from the different .cgi scripts, and 2) let checksetup.pl fix existing values in the DB (except fields being in some given whitelist).

So hacking post_bug.cgi to prevent control characters is fine; but hacking process_bug.cgi is not.

Comment 50

[Frédéric Buclin]

Comment on attachment 206388 [details] [diff] [review]
Patch for v2.16

post_bug.cgi should be fixed too. And as per my previous comment, existing bug summaries should be fixed in checksetup.pl, not process_bug.cgi.

Comment 51

[Paul]

Attachment: post bug changes as requested, process_bug same as before

post_bug processing was changed to where suggested.
process_bug processing needs to stay put so that it doesn't bother with security.
This has been tested on landfill with a user with no security and they were able to add comments on bugs with special characters in the summary.
I will wait for approval before submitting on tip and/or backporting.

Comment 52

[Paul]

Attachment: patch for v2.20 line 615 and do not call trim after clean_text

Comment 53

[Frédéric Buclin]

Comment on attachment 207209 [details] [diff] [review]
patch for v2.20 line 615 and do not call trim after clean_text

ok, per our discussion on IRC, it looks like this is the best solution. Tested, works fine.

r=LpSolit for 2.20

Comment 54

[Paul]

Attachment: patch for v2.16 post, process and global.

tested with unpriviledged user, works.

Comment 55

[Paul]

Attachment: new patch for v2.18

tested with unpriviledged user, works fine.

Comment 56

[Paul]

Attachment: new patch for tip.

tested with unpriviledged user, works fine.

Comment 57

[Frédéric Buclin]

Comment on attachment 207242 [details] [diff] [review]
new patch for tip.

We could even use clean_text() for the bug alias and the URL field, but that's definitely another bug.

Works fine. r=LpSolit

Comment 58

[Frédéric Buclin]

Comment on attachment 207239 [details] [diff] [review]
new patch for v2.18

r=LpSolit

Comment 59

[Dave Miller [:justdave]]

(In reply to comment #48)
> I think we really just need to strip the newlines in the email headers, and
> leave everything else as-is. The email headers are the only place where this
> actually causes a *bug*, right?

That is correct, and I don't see anything in any of the current patches that actually deals with that.

On the tip it'd be nice to fix the data and prevent it from being entered in the first place.  On the branches we just need to clean the headers.

There's basically two choices to resolve this bug:

1) Screen the email headers to ensure we aren't breaking the headers up by putting a newline in the middle of a header without folding it

- or -

2) Do what the current round of patches is doing, PLUS have checksetup.pl clean all the relevant existing data in the DB.

If the patch doesn't do one of the above two things, it's not actually fixing the real issue here (which is the email headers).

Comment 60

[Frédéric Buclin]

Comment on attachment 207237 [details] [diff] [review]
patch for v2.16 post, process and global.

Tested; works fine. r=LpSolit

Comment 61

[Frédéric Buclin]

(In reply to comment #59)
> 2) Do what the current round of patches is doing, PLUS have checksetup.pl clean
> all the relevant existing data in the DB.
> 
> If the patch doesn't do one of the above two things, it's not actually fixing
> the real issue here (which is the email headers).


When a bug is edited, its summary is automatically fixed, and the email header will contained the cleaned bug summary.

Comment 62

[Dave Miller [:justdave]]

(In reply to comment #61)
> When a bug is edited, its summary is automatically fixed, and the email header
> will contained the cleaned bug summary.

What about dependency and duplicate mail, where the bug having mail sent isn't actually the one being edited?

Comment 63

[Paul]

This bug's intention is to prevent newlines, etc. from getting into summaries.  Once this is in place, a single cleanup pass over summaries would fix the duplicates etc.
As discussed on IRC, that is a separate, secondary concern.

Comment 64

[Dave Miller [:justdave]]

(In reply to comment #63)
> This bug's intention is to prevent newlines, etc. from getting into summaries. 
> Once this is in place, a single cleanup pass over summaries would fix the
> duplicates etc.
> As discussed on IRC, that is a separate, secondary concern.

No, it's not.  Email headers getting messed up is the *main* concern, even according to the initial comment on this bug (see comment 0).  The problem with people editing existing bugs that had been hit by it wasn't brought up until comment 11.

Don't get me wrong, I love what you've done here, and you're doing great stuff, so I really don't want to scare you away. :)  But this doesn't fix the entire problem.

Maybe we shouldn't hold the release up for this.  This is a public bug after all, and the problem has been around for years, so it's not like it's a new problem.

Comment 65

[Colin Ogilvie [:cso]]

Attachment: Trunk Patch with Checksetup changes

Patch for Trunk, with checksetup.pl changes...

Apparently, justdave is going to review this.

Comment 66

[Colin Ogilvie [:cso]]

Attachment: Trunk Patch with Checksetup changes (v2)

Fix process_bug to work.

Comment 67

[Dave Miller [:justdave]]

Comment on attachment 207365 [details] [diff] [review]
Trunk Patch with Checksetup changes (v2)

ok, finally got a chance to try this out on a backup copy of the b.m.o database, and it works like a charm.  Had to heavily modify it to make it work on 2.20, and what I did is a hack and not suitable for backport, but the underlying code works and works great.

Comment 68

[Frédéric Buclin]

(In reply to comment #67)
> database, and it works like a charm.  Had to heavily modify it to make it work
> on 2.20, and what I did is a hack and not suitable for backport

What's so different between 2.20 and tip to require such hacks??? I thought that backporting the patch on branches would be straightforward.

Comment 69

[Colin Ogilvie [:cso]]

Attachment: 2.20 Patch with Checksetup changes

Patch for 2.20

Comment 70

[Colin Ogilvie [:cso]]

Attachment: 2.18 Patch with Checksetup changes

Patch for 2.18

Comment 71

[Colin Ogilvie [:cso]]

Attachment: 2.16 Patch with Checksetup changes

Patch for 2.16

Comment 72

[Colin Ogilvie [:cso]]

Commiting to 4 branches with 4 independant patches takes a while :(

2.16

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.149.2.28; previous revision: 1.149.2.27
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.169.2.31; previous revision: 1.169.2.30
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.52.2.10; previous revision: 1.52.2.9
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.125.2.14; previous revision: 1.125.2.13
done

2.18

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.35; previous revision: 1.289.2.34
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.88.2.10; previous revision: 1.88.2.9
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.205.2.24; previous revision: 1.205.2.23
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.12.2.5; previous revision: 1.12.2.4
done

2.20

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.412.2.21; previous revision: 1.412.2.20
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.118.2.5; previous revision: 1.118.2.4
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.263.2.6; previous revision: 1.263.2.5
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.28.2.4; previous revision: 1.28.2.3
done

Trunk

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.467; previous revision: 1.466
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.135; previous revision: 1.134
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.301; previous revision: 1.300
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.45; previous revision: 1.44
done