Closed
Bug 275904
Opened 20 years ago
Closed 20 years ago
Deleting a user when logged in as Editor blows away all user flags
Categories
(addons.mozilla.org Graveyard :: Developer Pages, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
1.0
People
(Reporter: Bugzilla-alanjstrBugs, Assigned: csthomas)
References
Details
Attachments
(1 file)
1.32 KB,
patch
|
wolf
:
first-review+
|
Details | Diff | Splinter Review |
I was logged in as an Editor and deleted a test account. The admin and editor flags were removed from all users that had them. Hendikins repaired with PMA. He was also unable to repeat this when logged in as an Admin.
Correction, PMA was not needed because Hendikins was still logged in, so the session variable was already set.
http://lxr.mozilla.org/mozilla/source/webtools/update/developers/usermanager.php#81 WHERE `UserID`='$i' You're executing UPDATE `userprofiles` SET `UserMode`= '$mode', `UserTrusted`= '$trusted' WHERE `UserID`='$i' for everyone, every time. And you're not matching it on userid, you're matching on some integer i
I think we need to split the single-user editor from this, too. There's more potential for something to get exploited if a low level user is in the same php.
Ok, I see how the names are being generated. The iteration itself looks ok.
$selecteduser = escape_string($_POST["selected$i"]); Maybe this is the problem: This is a checkbox now. Checkboxes have values of true and false, checked or unchecked. So I don't think its passing in the actual value of the user in question. For that, you'd just want to use $i Since the user is selected, you're doing WHERE `UserID`='TRUE' I'm impressed that I didn't accidentally delete all users when I went to delete only 1. It probably went through and disabled instead of deleting that user.
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [cst: active, r?]
Are you sure that's the right patch? The only thing that hits is Trusted, and I think it would break it. $i is the userid we're currently processing. $selected is the checkbox that tells us if the box on the left is checked. Compare lines 81 and 91. 91 should look like 81, not the other way around. Line 98 should also be like 81. 117, in turn, would be wrong. etc. I'm not sure why we keep doing checkformkey() like that. Shouldn't we be doing if ($selected =="TRUE") ? $selected == $selecteduser == escape_string($_POST["selected$i"]) If you have to rewrite a large chunk of code so that it makes sense, please do it. Put the SQL into a separate file, indent, whatever.
Assignee | ||
Comment 9•20 years ago
|
||
(In reply to comment #8) > Are you sure that's the right patch? The only thing that hits is Trusted, and I > think it would break it. It seems to have solved the problem - you can try it in my area: http://update.ctho.ath.cx:9305/developers > $i is the userid we're currently processing. $selected is the checkbox that > tells us if the box on the left is checked. Compare lines 81 and 91. 91 should > look like 81, not the other way around. Line 98 should also be like 81. 117, > in turn, would be wrong. etc. Are you sure you read the code correctly? It seems to me like all of those lines are using effectively the same WHERE clause (plus or minus an escape_strings call) > > I'm not sure why we keep doing checkformkey() like that. In the form keys bug, it was decided to check form keys before every DB access, instead of at the top of the PHP. > If you have to rewrite a large chunk of code so that it makes sense, please do > it. Put the SQL into a separate file, indent, whatever. I think it would be better to quickly fix the bugs that are blocking use of /developers first, but I'll start working on that too.
Assignee | ||
Comment 10•20 years ago
|
||
I looked more carefully at what's going on, and this patch does seem to be correct: <INPUT NAME="selected81" TYPE="CHECKBOX" VALUE="81" TITLE="Selected User"> ...so grabbing the value would give us the user ID
Reporter | ||
Comment 11•20 years ago
|
||
Removing security flag with permission from cbeard.
Group: webtools-security
Reporter | ||
Comment 12•20 years ago
|
||
Comment on attachment 169782 [details] [diff] [review] patch After explanation on IRC, I approve of this patch. Now just get someone to check it in for you.
Attachment #169782 -
Flags: first-review?(psychoticwolf) → first-review+
Attachment #169782 -
Flags: first-review+ → first-review?(cbeard)
Comment 14•20 years ago
|
||
After a first look, it seems checkbox handling culd be more orthodox, simple, clear and efficient. More on this later (maybe a patch).
Reporter | ||
Comment 15•20 years ago
|
||
mao - I'm satisfied with the fix now. We just need cbeard to approve the checkin.
Comment 16•20 years ago
|
||
Comment on attachment 169782 [details] [diff] [review] patch Looks good to me. r=wolf
Attachment #169782 -
Flags: first-review?(cbeard) → first-review+
Reporter | ||
Comment 18•20 years ago
|
||
Fixed in CVS.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•20 years ago
|
Whiteboard: [cst: active, r?]
Updated•9 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•