Closed Bug 238875 Opened 16 years ago Closed 15 years ago

remove %FORM from post_bug.cgi

Categories

(Bugzilla :: Bugzilla-General, defect, P2)

2.17.7
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: justdave, Assigned: wicked)

References

Details

Attachments

(1 file, 4 obsolete files)

This is going to be a medium to hard level (lots of them, but not as bad as
attachment.cgi.

This will also involve converting [% form.foo %] stuff to use the CGI stuff in
two templates:
   template/en/default/bug/create/comment-guided.txt.tmpl
   template/en/default/bug/create/comment.txt.tmpl
Blocks: 236678
No longer blocks: 225818
Depends on: 251933
No longer blocks: 236678
Doing COOKIE separately.
Blocks: 225818
No longer depends on: 251933
Summary: remove %FORM and %COOKIE from post_bug.cgi → remove %FORM from post_bug.cgi
Fred, Marc, did one of you want to do this one?
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.20
Assignee: nobody → wicked
Depends on: 287947
Attached patch %FORM removal, V1 (obsolete) — Splinter Review
This patch converts post_bug.cgi and the two comment text templates to use the
CGI object instead of the FORM hash.

I did test this with both normal and guided bug entry form and it seems to
work. Note that this patch requires the patch from the dependent bug before
field validation and default values work correctly for certain fields.
Attachment #178771 - Flags: review?(LpSolit)
Attached patch %FORM removal, V1.1 (obsolete) — Splinter Review
Removed bitrot and diffed against a patch in the bug 287947 (needs it to apply
cleanly). This now removes the temporary FORM compatibility code introduced in
that bug.
Attachment #178771 - Attachment is obsolete: true
Attachment #179465 - Flags: review?(LpSolit)
Attachment #178771 - Flags: review?(LpSolit)
Depends on: 238878
Comment on attachment 179465 [details] [diff] [review]
%FORM removal, V1.1

>--- post_bug.cgi-bug287947	2005-03-30 05:32:58.000000000 +0300
>+++ post_bug.cgi	2005-04-03 19:03:23.000000000 +0300
>-    if (!UserInGroup("editbugs") || trim($::FORM{'qa_contact'}) eq "") {
>+    if (!UserInGroup("editbugs") || trim($cgi->param('qa_contact')) eq "") {

Nit: $cgi->param('qa_contact') could be undefined.


>-if (defined $::FORM{'cc'}) {
>-    foreach my $person (split(/[ ,]/, $::FORM{'cc'})) {
>+if (defined $cgi->param('cc')) {
>+    foreach my $person (split(/[ ,]/, $cgi->param('cc'))) {
>         if ($person ne "") {
>             my $ccid = DBNameToIdAndCheck($person);
>             if ($ccid && !$ccids{$ccid}) {

This does not work. Only the first user in the CC list is considered. All
others are dropped from the list. This is due to $cgi->append() in
match_field().


>-foreach my $b (grep(/^bit-\d*$/, keys %::FORM)) {
>-    if ($::FORM{$b}) {
>+foreach my $b (grep(/^bit-\d*$/, $cgi->param())) {
>+    if ($cgi->param($b)) {
>         my $v = substr($b, 4);
>         $v =~ /^(\d+)$/
>           || ThrowCodeError("group_id_invalid");

GroupIsActive($v) fails due to the tainted parameter $v. detaint_natural()
fixes the problem. Don't ask me why keys %::FORM is not affected but
$cgi->param() is; I have no idea.

Else, it seems to works fine.
Attachment #179465 - Flags: review?(LpSolit) → review-
Attached patch %FORM removal, V2 (obsolete) — Splinter Review
Fixed review comments:
 - Added defined check for qa_contact field.
 - Changed the CC foreach loop to correctly process list of CC entries. Also
@cc is no longer created because it's unused due to emailprefs landing. It can
also be retrieved directly from CGI if needed.
 - Added detainting for the $v variable. I don't know either why it's now
tainted.

This patch is also based on both dependent patches. They are needed to apply
this cleanly. Tested and these changes seems to do the trick.
Attachment #179465 - Attachment is obsolete: true
Attachment #179794 - Flags: review?(LpSolit)
Comment on attachment 179794 [details] [diff] [review]
%FORM removal, V2

>--- post_bug.cgi-withdeps	2005-04-06 02:36:20.000000000 +0300
>+++ post_bug.cgi	2005-04-06 03:55:59.000000000 +0300

> # Retrieve the default QA contact if the field is empty
>-if (Param("useqacontact")) {
>+if (Param("useqacontact") && defined $cgi->param('qa_contact')) {
>     my $qa_contact;
>-    if (!UserInGroup("editbugs") || trim($::FORM{'qa_contact'}) eq "") {
>+    if (!UserInGroup("editbugs") || trim($cgi->param('qa_contact')) eq "") {
>         SendSQL("SELECT initialqacontact FROM components " .
>                 "WHERE id = $component_id");
>         $qa_contact = FetchOneColumn();
>     } else {
>-        $qa_contact = DBNameToIdAndCheck(trim($::FORM{'qa_contact'}));
>+        $qa_contact = DBNameToIdAndCheck(trim($cgi->param('qa_contact')));
>     }
> 
>     if ($qa_contact) {
>-        $::FORM{'qa_contact'} = $qa_contact;
>+        $cgi->param(-name => 'qa_contact', -value => $qa_contact);
>         push(@bug_fields, "qa_contact");
>     }
> }

WRONG! If $cgi->param('qa_contact') is undefined, we have to take it from the
DB!
Attachment #179794 - Flags: review?(LpSolit) → review-
Comment on attachment 179794 [details] [diff] [review]
%FORM removal, V2

>-foreach my $b (grep(/^bit-\d*$/, keys %::FORM)) {
>-    if ($::FORM{$b}) {
>+foreach my $b (grep(/^bit-\d*$/, $cgi->param())) {
>+    if ($cgi->param($b)) {
>         my $v = substr($b, 4);
>         $v =~ /^(\d+)$/
>           || ThrowCodeError("group_id_invalid");
>+        $v = $1; # Detaint

Also, replace

$v =~ /^(\d+)$/
$v = $1; # Detaint

by detaint_natural($v).
Attached patch %FORM removal, V2.1 (obsolete) — Splinter Review
Attachment #179794 - Attachment is obsolete: true
Attachment #179802 - Flags: review?(LpSolit)
OK, now both changes are done.
Attachment #179802 - Attachment is obsolete: true
Attachment #179805 - Flags: review?(LpSolit)
Attachment #179802 - Flags: review?(LpSolit)
Comment on attachment 179805 [details] [diff] [review]
%FORM removal, V2.2

All is ok now. :)

r=LpSolit
Attachment #179805 - Flags: review?(LpSolit) → review+
Note to checker-in: this patch as #3. Next one will be bug 238870.
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval? → approval+
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.112; previous revision: 1.111
done
Checking in template/en/default/bug/create/comment-guided.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/comment-guided.txt.tmpl,v
 <--  comment-guided.txt.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/bug/create/comment.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/comment.txt.tmpl,v
 <--  comment.txt.tmpl
new revision: 1.3; previous revision: 1.2
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.