New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACL checks in the media file details ajax calls only for root/arbitrary namespace #765

Closed
michitux opened this Issue Jun 25, 2014 · 11 comments

Comments

Projects
None yet
6 participants
@michitux
Collaborator

michitux commented Jun 25, 2014

A comment in our old bug tracker alerted me of this issue: in the media manager ACLs are broken for all views for individual files if you have access to the root namespace or - in the case of the media diff ajax call - an arbitrary namespace.

I have reproduced this issue in my local DokuWiki installation, I can simply open an image I have no permission for in the media manager, I get a permission denied message in the media details tab and when I click on the detail tabs they load via ajax with the real content. The media diff ajax call is a bit more difficult to test as the ns parameter (but only that one) is a post parameter, but after changing the code to accept a get parameter as well I can clearly see that it uses the ns parameter for the permission check (and nothing else).

No actual file content is exposed, just the metadata, but the metadata can contain a lot of information (title, caption etc. from the exif metadata) and the full history is displayed, too. This also requires knowing the actual media file id.

So far I can see the following problems in the code:

  • in ajax_mediadetails(), $NS is not set, but in tpl_mediaFileDetails() $NS is used and its value is only checked when it is set (if(isset($NS) && getNS($image) != $NS) return;)
  • $AUTH is a cache for the permissions of the current media ns (wtf?) which is set by ajax_mediadiff() using the "ns" parameter as namespace without any checks if this is actually the namespace of the current media file. This check is done in tpl_mediaFileDetails() but not in media_diff() which is called in ajax_mediadiff().

I think a first fix could be to ignore the ns parameter in all ajax calls and instead set it based on the supplied image id.

I think we should fix this and release a hotfix release ASAP.

@michitux michitux added the Bug label Jun 25, 2014

@splitbrain

This comment has been minimized.

Show comment
Hide comment
@splitbrain

splitbrain Jun 25, 2014

Owner

ouch. Can you provide a quick fix?

Owner

splitbrain commented Jun 25, 2014

ouch. Can you provide a quick fix?

michitux added a commit that referenced this issue Jun 25, 2014

Quick fix for #765 - ACL checks in the media manager ajax calls
This should be superseded by a proper rewrite of the media manager code
@Chris--S

This comment has been minimized.

Show comment
Hide comment
@Chris--S

Chris--S Jun 25, 2014

Collaborator

I noticed this same issue with media manager & acl just today, but from the other way around. When a user has full access to a namespace but no access to the parent namespace. The media manager shows the images but gives a denied access message when viewing details.

Was the $NS global 'rationalised' recently?

A side question - how can you check the user's permissions to a namespace? medialist plugin does a quick_aclcheck() using the namespace, but gets the actual permissions for the page (named as namespace) in the parent namespace.

Collaborator

Chris--S commented Jun 25, 2014

I noticed this same issue with media manager & acl just today, but from the other way around. When a user has full access to a namespace but no access to the parent namespace. The media manager shows the images but gives a denied access message when viewing details.

Was the $NS global 'rationalised' recently?

A side question - how can you check the user's permissions to a namespace? medialist plugin does a quick_aclcheck() using the namespace, but gets the actual permissions for the page (named as namespace) in the parent namespace.

@splitbrain

This comment has been minimized.

Show comment
Hide comment
@splitbrain

splitbrain Jun 25, 2014

Owner

Namespace permissions are checked by adding ':*' to the namespace usually.

Owner

splitbrain commented Jun 25, 2014

Namespace permissions are checked by adding ':*' to the namespace usually.

@Chris--S

This comment has been minimized.

Show comment
Hide comment
@Chris--S

Chris--S Jun 25, 2014

Collaborator

@michitux your fix resolves my issue.

Collaborator

Chris--S commented Jun 25, 2014

@michitux your fix resolves my issue.

@splitbrain

This comment has been minimized.

Show comment
Hide comment
@splitbrain

splitbrain Jun 25, 2014

Owner

I will be home in an hour or so and build the hotfix release.

Owner

splitbrain commented Jun 25, 2014

I will be home in an hour or so and build the hotfix release.

@Klap-in

This comment has been minimized.

Show comment
Hide comment
@Klap-in

Klap-in Jun 25, 2014

Collaborator

What about including 59d6be9 in the hotfix as well. This will improve the usability of the extension manager significantly, but is of course not urgent.

Collaborator

Klap-in commented Jun 25, 2014

What about including 59d6be9 in the hotfix as well. This will improve the usability of the extension manager significantly, but is of course not urgent.

splitbrain added a commit that referenced this issue Jun 25, 2014

Merge pull request #766 from splitbrain/media_acl_fix
Quick fix for #765 - ACL checks in the media manager ajax calls

michitux added a commit that referenced this issue Jun 25, 2014

Quick fix for #765 - ACL checks in the media manager ajax calls
This should be superseded by a proper rewrite of the media manager code

splitbrain added a commit that referenced this issue Jun 25, 2014

michitux added a commit that referenced this issue Jun 25, 2014

Quick fix for #765 - ACL checks in the media manager ajax calls
This should be superseded by a proper rewrite of the media manager code

Conflicts:
	inc/template.php

splitbrain added a commit that referenced this issue Jun 25, 2014

@splitbrain

This comment has been minimized.

Show comment
Hide comment
@splitbrain

splitbrain Jun 25, 2014

Owner

Hotfix is out.

Owner

splitbrain commented Jun 25, 2014

Hotfix is out.

@ginsterbusch

This comment has been minimized.

Show comment
Hide comment
@ginsterbusch

ginsterbusch Jul 2, 2014

Does that issue only apply to the current release or are all (some) older releases are affected as well?

Does that issue only apply to the current release or are all (some) older releases are affected as well?

@splitbrain

This comment has been minimized.

Show comment
Hide comment
@splitbrain

splitbrain Jul 2, 2014

Owner

It applies to at least the last two releases, maybe even older.

Owner

splitbrain commented Jul 2, 2014

It applies to at least the last two releases, maybe even older.

@mprins

This comment has been minimized.

Show comment
Hide comment
@mprins

mprins Jul 4, 2014

Contributor

Will you push a new tag (eg. release_stable_2014-05-05a)?
I know this fix is in the stable branch as well.

Contributor

mprins commented Jul 4, 2014

Will you push a new tag (eg. release_stable_2014-05-05a)?
I know this fix is in the stable branch as well.

@splitbrain

This comment has been minimized.

Show comment
Hide comment
@splitbrain

splitbrain Jul 4, 2014

Owner

tags pushed. I think I just learned something new about tags :-)

Owner

splitbrain commented Jul 4, 2014

tags pushed. I think I just learned something new about tags :-)

@splitbrain splitbrain closed this Jul 4, 2014

splitbrain added a commit that referenced this issue Sep 29, 2014

Merge branch 'stable' into old-stable
* stable: (474 commits)
  hotfix release for #765
  Quick fix for #765 - ACL checks in the media manager ajax calls
  Use git attributes to exclude some files from exported archives
  Release 2014-05-05 "Ponder Stibbons"
  Release preparation
  no fancy quotes in user manager import description
  add defaults to phpdocs of search universal
  update deprecation stuff for dw_qearch
  translation update
  translation update
  translation update
  added another test for arrays
  fixed some test inheriting from the wrong parent
  use new $INPUT->valid() method in feed.php
  add new valid() method to $INPUT #667
  some updates on phpunit docs and settings
  Fix https proxy authentication, the header was missing a colon so that the auth info was not working.
  translation update
  translation update
  translation update
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment