181797 - View All link shows Edit link for attachments even when user does not have enough perms

Status: RESOLVED WONTFIX

(Bugzilla :: Attachments & Requests, defect)

Description

[Cesar Eduardo Barros]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021123
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021123

I don't have enough priviledges to edit the patch at
http://bugzilla.mozilla.org/show_bug.cgi?id=181791 . So, the Edit link in the
Action column does not even appear. However, if I chose View All, the Edit link
is there.

Reproducible: Always

Steps to Reproduce:
1.Go to the bug with an account without enough priviledges
2.Click View All
3.Click the Edit link for the patch
Actual Results:  
The link is there (and shows that ugly big red error message)

Expected Results:  
The link should not appear

Comment 1

[Bradley Baetz (:bbaetz)]

The attachment.cgi code should really change to using Attachment.pm. That way we
get to show bug flags too

Comment 2

[timeless]

see also bug 140999

Comment 3

[Vlad Dascalu]

Attachment: V1

What this patch doesn't do (I'd prefer to do that in other opened bugs):

- a security review of attachment.cgi and its safety againest exploits, and if
some are detected, patches to cover up those holes.
- code redundancy elimination (we already do a lot of this with the current
patch; there is room for more, especially between
template/en/default/attachment/list.html.tmpl and
template/en/default/attachment/show-multiple.html.tmpl; I'd like to investigate
this more closely in another bug; in addition, the code in question is not
exactly identically, but this patch brings them a little bit closer to each
other).

Comment 4

[Vlad Dascalu]

Attachment: V2

Comment 5

[Christian Reis]

Comment on attachment 145847 [details] [diff] [review]
V2

>Index: attachment.cgi
>-    my $privacy = "";
>-    if (Param("insidergroup") && !(UserInGroup(Param("insidergroup")))) {
>-        $privacy = "AND isprivate < 1 ";
>-    }

Was this code obsolete? I just don't see any references to insidergroup in
Attachment.pm, but maybe I'm blind.

>-    $a{'isviewable'} = isViewable($a{'contenttype'});

This I'm also left wondering about..

template/en/default/attachment/show-multiple.html.tmpl
>+[% canseeprivate = !Param("insidergroup") || UserInGroup(Param("insidergroup")) %]
> [% FOREACH a = attachments %]
>-
>+      [% IF !a.isprivate || canseeprivate %]

Oh, I see this here now. Hmmmm. Don't you think it might be a better idea to
pass a flag to query instead of omitting the data here in the template?

Comment 6

[Vlad Dascalu]

> Was this code obsolete? I just don't see any references to insidergroup in
> Attachment.pm, but maybe I'm blind.

That's because we pass the whole list to the template, and the template displays
only the attachment for which the security tokens are valid.

> >-    $a{'isviewable'} = isViewable($a{'contenttype'});
> 
> This I'm also left wondering about..

This makes non-viewable attachments for which the content-type doesn't allow to
show them in an iframe.

> Oh, I see this here now. Hmmmm. Don't you think it might be a better idea to
> pass a flag to query instead of omitting the data here in the template?

Keeping the things at the template-level is a nice approach in my opinion.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

It would be better to pass canseeprivate to Bugzilla::Attachment::query for
performance (unnecessary data wouldn't be getting retrieved from the database
and sent to the template processor); simplicity of template logic (which makes
it easier for less experienced Bugzilla hackers to modify the look and feel);
and security (is consistent with other places in the codebase where we do the
same thing, exposes secure data in fewer places, and doesn't let it reach the
template, which might get edited by less experienced Bugzilla hackers).

Comment 8

[Christian Reis]

It also ensures that other templates and clients of that form are aware of the
privacy issue.

Comment 9

[Dave Miller [:justdave]]

I was going to push this off, but it looks like it has current activity on it. 
Anyone planning to finish this up soon?

Comment 10

[Gervase Markham [:gerv]]

Seems not :-| Dave: time to push this off?

Gerv

Comment 11

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

[Frédéric Buclin]

Now that bug 274906 is fixed, all links in comments point to the "Edit
attachment" page and this page is now accessible to everybody thanks to the fix
from bug 274724. So we are perfectly consistent now. Users without sufficient
privs can access the page but cannot edit the attachment, in the same way
show_bug.cgi works (it let you see the bug, but not do changes if you haven't
editbugs privs).

Marking wontfix!