97976 - Tests to do ...

Status: RESOLVED FIXED

(Bugzilla :: Testing Suite, defect, P2)

Description

[Matthew Tuck [:CodeMachine]]

We should check for the following at least:

1.  Single parameter exec/system calls.
2.  Jake's regexp problem.
3.  Characters in filenames that are illegal on some platforms.
4.  \$::M?FORM\s*[^{]

Comment 1

[Matthew Tuck [:CodeMachine]]

<justdave> -w in the shebang line and use strict at the top

Comment 2

[Matthew Tuck [:CodeMachine]]

zach it would be good if 1.t became compile.t and these other tests because
filename.t for (3), systemexec for (1) and regexp.t for everything else
(apparently systemexec can't be done with regexp).

Comment 3

[Zach Lipton [:zach]]

actually:

1compile.t
2regexp.t
3filename.t
4etc

so that we can ensure that it compiles before we go on to other tests.

Comment 4

[Zach Lipton [:zach]]

I renamed 1.t to be 1compile.t, working on 2goodperl.t to check for -w, use 
strict, etc.

Comment 5

[Zach Lipton [:zach]]

2goodperl.t has been created and checked in to test for -w and use strict; 
Next stop, single parameter exec and system calls.

Comment 6

[Matthew Tuck [:CodeMachine]]

Dave can you paste your systemexec code here?

Comment 7

[Dave Miller [:justdave]]

I've tried to upload it three or four times, but Mozilla keeps core dumping on
me when I try to attach a file lately.

Guess cut & paste it is.

This file is called safesystem.pm, and is included via the -M flag on the
$perlapp command line.

----8<---- cut here
#!/usr/bin/perl
package safesystem;
require Exporter;
@ISA = qw(Exporter);
@EXPORT = qw(system exec);
@EXPORT_OK = qw();
sub system($$@) {
  1;
}
sub exec($$@) {
  1;
}
1;----8<---- cut here

Comment 8

[Matthew Tuck [:CodeMachine]]

INNER JOIN can't be used in Bugzilla code as it is unsupported by versions of
MySQL we support.

Comment 9

[Zach Lipton [:zach]]

I'm not sure what that INNER JOIN line is doing here, but the safesystem 
code is in. Leaving:

a. \$::M?FORM\s*[^{]
b. Characters in filenames that are illegal on some platforms.
c. Jake's regexp problem.

I'll get to work on b soon.

Comment 10

[Dave Miller [:justdave]]

Zach: he means we should test to make sure INNER JOIN isn't being used.

And safesystem is not in yet, at least not in the copy I just checked out.  The
test is there, but the .pm file that goes with it is not, and it isn't watching
for errors involving exec() in the output, only system()  (both should be
checked for as both have the same security concerns)

Comment 11

[Zach Lipton [:zach]]

safesystem.pm is now fully running. The pm file is there, just in t/Support. 
I now test for system() and exec() being used incorrectly. The fix was 
checked in today. 

Also, I have gutted the Files.pm code and replaced it with something that 
automatically generates the list of files to test. I still need to do some work 
on this involving modules which I will adress shortly. 

For the INNER JOIN tests, what would everyone think about looking for:

/SendSQL\(INNER JOIN\)/i

Or would that do something stupid?

Comment 12

[Jacob Steenhagen]

"INNER JOIN" may not be inside the SendSQL() call.  For instance,

my $query = "SELECT ... FROM ... INNER JOIN ... ON ...";
SendSQL($query);

Is still a violation.

I just tried to come up w/a regexp to check for this possibility (w/out just
blatently looking for m/INNER JOIN/) and it was quite lenghty w/out catching
every possiblity.  Not sure what we should do here...

Comment 13

[Zach Lipton [:zach]]

Hmm. I guess we could do something like we do for system() and exec() 
where we define SendSQL() and then whine if it's args contain INNER 
JOIN (this assumes that I don't send INNER JOIN as a value or 
something other than a command). Something like this should work:

sub SendSQL {
   $input = $_[0];
   if ($input =~ /SendSQL\(INNER JOIN\)/i) {
       print "INNER JOIN found\n";
   } else {
       print "no inner join found\n";
   }
}

This also has the advantage of being useful for other SQL tests as well. 
What do people think about this?

Comment 14

[David D. Kilzer (ddk)]

I would recommend adding more numeric digits to the beginning of the
test filenames so that you don't run into silly sorting issues down
the road:

001compile.t
002goodperl.t
003safesys.t

Comment 15

[Zach Lipton [:zach]]

I have renamed *.t to be 00*.t

Comment 16

[Dave Miller [:justdave]]

Jake: overriding SendSQL() like that won't due any good for the tests, since all
you can test with an override at compile-time is whether the prototype matches.
 You'd actually have to RUN the code to test the contents of the variables
passed to it.

Comment 17

[Jacob Steenhagen]

I added a test (004template.t) that scans Bugzilla's code to come up w/a list of
templates used and then makes sure that
a) They all exist in the proper subdirectory in template/default.
b) They all have proper syntax (by running them through TT's process() method,
   discarding all output and checking the return value).

http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/t/004template.t
http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/t/Support/Templates.pm

Comment 18

[Jacob Steenhagen]

Guess I never mentioned the addition of notabs.t which enforces the rule from
the hacker's guide about not using tabs in the source code :)

http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/t/005no_tabs.t

Comment 19

[Matthew Tuck [:CodeMachine]]

There doesn't seem to be too much traction here.  Now we have the component
perhaps it's time to mark this resolved fixed and file separate bugs on the
residuals.  I suspect INNER JOIN and regexp interpolation are gonna be dependent
on bug #104679.

Comment 20

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

[Zach Lipton [:zach]]

1 is done, 2 is already in another bug, 3 is odd enough and I think we 
know what not to do that we shouldn't need to test for it (since the tests 
will choke if there are invalid chars anyway, and 4 is in another bug iirc as 
well. Marking fixed. Please open a new bug in the testing suite 
component for any new issues

Comment 22

[Matthew Tuck [:CodeMachine]]

Spun off bug #123193, bug #123194, bug #123195, bug #123198.

Comment 23

[Dave Miller [:justdave]]

fixing incorrect milestones on fixed bugs.