238780 - Edit versions should reject newline characters

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[timeless]

This is a serious problem. my browser is rather drunk and it just made a big mess.

Comment 1

[Dave Miller [:justdave]]

agreed, it's basic data validation here... :)

Comment 2

[Dave Miller [:justdave]]

... but it's an edge case, and I'm not going to hold 2.18 for it.  If someone
fixes this prior to the 2.18 release I'll be happy to take it on the branch though.

Comment 3

[Max Kanat-Alexander]

Oh, man, this should be easy. Somebody ought to be able to do this...

Comment 4

[GavinS]

(In reply to comment #3)
> Oh, man, this should be easy. Somebody ought to be able to do this...

Yes, but I'm waiting for the editversions templatisation patch to be checked in
before doing this (and the other version related admin bugs like sortkeys) - so
I'd suggest anyone wanting to nab this to wait a bit longer maybe?

Comment 5

[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 6

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 7

[Paul]

Attachment: A change to editversions.cgi

A change to replace newline,vertical tabs,carriage returns, and formfeeds with a harmess space.

Comment 8

[GavinS]

Comment on attachment 204299 [details] [diff] [review]
A change to editversions.cgi

2 things:

1) Usually, a problem in the admin scripts results in a ThrowUserError(), and requires the user to fix and re-submit. This specific case is very simple, and so we may want do do what you propose, but someone more important than I should decide on that. (And the bug does say 'reject' :)

2) If we did decide we wanted to silently strip out the newlines etc (and I'll check on irc  tonight or tomorrow, if someone doesn't jump in with a response), then it should be done in a common routine, which we can use in all the other admin scripts (and maybe some of the bug fields as well where appropriate (title?)

Comment 9

[Frédéric Buclin]

(In reply to comment #8)
> 2) If we did decide we wanted to silently strip out the newlines etc (and I'll
> check on irc  tonight or tomorrow, if someone doesn't jump in with a response),
> then it should be done in a common routine, which we can use in all the other
> admin scripts (and maybe some of the bug fields as well where appropriate

/me jumps in...

justdave is working on a similar patch for bug 101380. I suggested him to write a routine in Util.pm to allow its reuse.

(22:23:18) ssdbot: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=238780 maj, --, ---, [e-mail], NEW, Edit versions should reject newline characters
(22:26:26) LpSolit: justdave: this bug makes me think that your patch about newlines in bug summary should use a function in Util.pm which removes all unwanted characters. This way, the bug above could use it too instead of duplicating the code
(22:26:47) justdave: hmm, good idea


And yes, we should silently remove undesired characters.

About the name of this function, maybe Bugzilla::Util::clean_text() ?

Comment 10

[Paul]

I guess this is dependent on bug# 101380 then.

Comment 11

[Paul]

Attachment: Added shared function clean_text() and called it

This will do the same thing and also help other code.

Comment 12

[Paul]

Attachment: Added shared function clean_text() and called it.

Added a shared function and called it in the same places.

Comment 13

[Paul]

Attachment: Remove unprintable characters from versions

Comment change

Comment 14

[Frédéric Buclin]

Comment on attachment 204873 [details] [diff] [review]
Remove unprintable characters from versions

>Index: Bugzilla/Util.pm

>+sub clean_text {
>+    my ($dtext) = shift;
>+    $dtext =~  tr/[\x00-\x1F\x7F]/ /;   # change control characters to spaces
>+    return $dtext;
>+}

Use s// instead of tr//. And as I said on IRC, this routine needs POD.

Comment 15

[Paul]

Attachment: Has shared function and POD

I think this one has everything you wanted in it.

Comment 16

[Frédéric Buclin]

Comment on attachment 204874 [details] [diff] [review]
Has shared function and POD

>+sub clean_text {
>+    my ($dtext) = shift;
>+    $dtext =~  s/[\x00-\x1F\x7F]/ /g;   # change control characters to spaces
>+    return $dtext;
>+}

Works fine! And prevents some strange behaviors of Bugzilla for versions having unprintable characters. r=LpSolit


One remark though: the input field for versions is limited to 64 characters, which is the maximum length allowed for versions. But unprintable characters are not considered in the count when entering text and so 64 printable characters + N unprintable characters will be converted to a string with 64 + N printable characters. This makes PostgreSQL to crash as you exceeded the maximum length allowed (MySQL silently truncates the string at 64 characters). So editversions.cgi should have a test to make sure the string isn't too long. But that's another bug.

Comment 17

[Frédéric Buclin]

I think a 2.20 backport would be fine....

Comment 18

[Frédéric Buclin]

tip:

Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.41; previous revision: 1.40
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.42; previous revision: 1.41
done

Comment 19

[Frédéric Buclin]

Reopening! This patch prevents us from editing or deleting existing versions having control characters in them.

Maybe should we only prevent to create *new* versions with control characters in then, same for renaming, but letting existing versions unaltered.

This problem is a mess.


Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.43; previous revision: 1.42
done

Comment 20

[Frédéric Buclin]

Comment on attachment 204874 [details] [diff] [review]
Has shared function and POD

Looks like this generates some problem when existing versions have control characters.

Comment 21

[Max Kanat-Alexander]

(In reply to comment #19)
> Maybe should we only prevent to create *new* versions with control characters
> in then, same for renaming, but letting existing versions unaltered.

  Just have checksetup fix existing versions, sort of like we did for flag names.

Comment 22

[Dave Miller [:justdave]]

(In reply to comment #19)
> Reopening! This patch prevents us from editing or deleting existing versions
> having control characters in them.

It does?  How?  I only see it calling clean_text on new and updates.  And it's only calling it on "version", not on "old_version".  So any update to a version will clean it, but I can't see how this prevents you from updating it.

Comment 23

[Frédéric Buclin]

(In reply to comment #22)
> It does?  How?  I only see it calling clean_text on new and updates.  And it's
> only calling it on "version", not on "old_version".  So any update to a version
> will clean it, but I can't see how this prevents you from updating it.


Here is a testcase (the admin would need to be drunk to do things like that, but that's a testcase anyway):

1) Assume your installation already has a version "foo(newline)bar";
2) Now enter a new version "foo  bar" <--- with two whitespaces on *nix (it doesn't matter that the patch has already been applied or not after step 1, because all characters are "valid");
3) Now apply the patch from this bug (if you haven't done it yet after step 1);
4) Try to edit the version "foo(newline)bar" and do nothing else than clicking on the "Update" button ---> Bugzilla complains that this version already exists, because the cleaned version of it is "foo  bar" already inserted in point 2.

And that's the reason we cannot just let checksetup.pl do the cleanup for us. We would get SQL errors due to duplicated entries (unless we explicity try to catch them). Another point about checksetup.pl is that if we are going to do it for *all* fields in the DB (product name, component name, real name, query name, etc..), checksetup.pl would probably require a huge amount of time to scan the whole DB, and everytime, we would have to make sure we are not going to introduce duplicated entries in the DB and make checksetup.pl to crash.


I admit that this scenario is more than unlikely, and my comments 19 and 20 were partially incorrect as you can still delete versions having control characters in them, and you can still edit them in most cases (but not all).


The reason I changed my mind (from r+ to r-) was also because bug 101380 and bug 177773, which I reviewed yesterday, introduced much more severe problems, where you couldn't edit a bug, respectively target milestones, *at all*. Those were clearly *major* regressions. And having again a testcase here (even minor and hard to reproduce) made me take this decision, even if the patch is 99.999% good and will work for 99.999% of cases.


I definitely want a discussion, e.g. during a "meeting" on IRC, and a clear decision about what to do with these control characters.

I will probably send an email to developers@ again.


NB: justdave, if you estimate that this testcase is not enough to deny review, then feel free to override my decision (well, paul did it already by removing my r-) and grant approval again. But be aware that such "side-effects" will occur all the time, even if they are very unlikely, especially in edit*.cgi pages.

Comment 24

[Max Kanat-Alexander]

We always deal with duplicates when we do things like that in checksetup. Things usually just get renamed to have an underscore after them.

Comment 25

[Frédéric Buclin]

Comment on attachment 204874 [details] [diff] [review]
Has shared function and POD

After some discussion on IRC, we (=justdave) have now decided that new and existing values shouldn't contain any control character unless these fields are in some given whitelist (such as bug comments and descriptions).

So the "right" fix is to alter files to prevent control characters in *new* values only, and to let checksetup.pl fix existing values.

My testcase is so unlikely that I now change my mind again.

So r=LpSolit

Comment 26

[Frédéric Buclin]

Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.44; previous revision: 1.43
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.44; previous revision: 1.43
done