Closed Bug 234879 Opened 16 years ago Closed 16 years ago

Remove %FORM from editkeywords.cgi

Categories

(Bugzilla :: Bugzilla-General, defect)

2.17.6
defect
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: goobix, Assigned: goobix)

References

Details

Attachments

(1 file, 1 obsolete file)

Use $cgi->param in editkeywords.cgi.
Attached patch Version 1 (obsolete) — Splinter Review
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.18
Attachment #141739 - Flags: review?(kiko)
Comment on attachment 141739 [details] [diff] [review]
Version 1

>Index: >@@ -173,7 +173,10 @@
> #
> 
> if ($action eq 'edit') {
>-    my $id  = trim($::FORM{id} || 0);
>+    my $id = cgi->param('id');
>+    detaint_natural($id);
>+    $id  = trim($id || 0);

Make sure you still need the trim after detaint_natural; I'm not sure myself.
Is zero used for anything special here? If so, it would be nice to add a
comment...
Attachment #141739 - Flags: review?(kiko) → review+
Comment on attachment 141739 [details] [diff] [review]
Version 1

>@@ -173,7 +173,10 @@
>-    my $id  = trim($::FORM{id} || 0);
>+    my $id = cgi->param('id');
>+    detaint_natural($id);
>+    $id  = trim($id || 0);

kiko's got a point... this won't work.	If the value had spaces around it and
thus needs to be trimmed, it's going to fail the detaint_natural.  Need to trim
it first before calling detaint_natural on it.
Attachment #141739 - Flags: review-
It used to get trimmed after detaining it in the old code. But good point, 
*nods*.
Attached patch Version 2Splinter Review
0 is not a special case, so I removed that as well.
Attachment #141739 - Attachment is obsolete: true
Attachment #142619 - Flags: review?(kiko)
Attachment #142619 - Flags: review?(kiko) → review+
Flags: approval?
Flags: approval? → approval+
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.20; previous revision: 1.19
done
Status: ASSIGNED → RESOLVED
Closed: 16 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.