Edgewall Software
Modify

Opened 10 years ago

Closed 8 years ago

Last modified 4 years ago

#5640 closed defect (fixed)

SVN checkin comments float up past svn:authz barrier.

Reported by: Dave Abrahams <dave@…> Owned by: Remy Blank
Priority: normal Milestone: 0.12-multirepos
Component: version control/browser Version: devel
Severity: major Keywords: svnauthz, multirepos, authzsourcepolicy
Cc: osimons
Release Notes:
API Changes:

Description

Our SVN repo contains private client areas. With Trac and SVN logins using the same password file, it's awesome that the Trac browser won't show the private areas to those people who don't have the privileges to look at them. However, unfortunately, when browsing a public parent directory of a private area, one can always see the last checkin comment in its subtree, even if the changes were entirely in a subtree in which one doesn't have read privileges.

Attachments (7)

browser_comments_authz_0.10-stable.diff (669 bytes ) - added by ms@… 9 years ago.
poor-man's fix for this problem
5640-hide-forbidden-messages-r7802.patch (4.7 KB ) - added by Remy Blank 9 years ago.
Patch against 0.11-stable hiding changeset info for which the user doesn't have view permission
5640-hide-forbidden-changesets-r7825.patch (3.0 KB ) - added by Remy Blank 9 years ago.
Cleaner approach to hiding forbidden changeset data
5640-hide-forbidden-changesets-r8122.patch (5.6 KB ) - added by Remy Blank 9 years ago.
Updated patch fixing the "Next changeset" link.
5640-hide-forbidden-changesets-r8179.patch (5.6 KB ) - added by Remy Blank 8 years ago.
Updated patch for current 0.11-stable head.
5640-hide-forbidden-changesets-r8198.patch (11.8 KB ) - added by Remy Blank 8 years ago.
Even another approach, with its own issues
5640-hide-forbidden-changesets-r8198.2.patch (12.0 KB ) - added by Remy Blank 8 years ago.
Improved patch handling revisions 1 and 2 correctly

Download all attachments as: .zip

Change History (32)

comment:1 Changed 10 years ago by Emmanuel Blot

Milestone: 0.110.11.1
Severity: criticalmajor

(please stop assigning new ticket to 0.11)

comment:2 Changed 9 years ago by anonymous

revoking logview priviledge from the anonymous user is a workaround, but comes with the drawback that anonymous can't logview anything

this is *really* serious, after 18months nothing happend?

comment:3 Changed 9 years ago by Christian Boos

Keywords: verify added
Milestone: 0.11-retriage0.11.3

Changed 9 years ago by ms@…

poor-man's fix for this problem

comment:4 Changed 9 years ago by Remy Blank

Keywords: verify removed
Owner: changed from Christian Boos to Remy Blank

I can reproduce this. Even worse, the revision log exhibits the same symptom. I'll attach a patch shortly.

Changed 9 years ago by Remy Blank

Patch against 0.11-stable hiding changeset info for which the user doesn't have view permission

comment:5 Changed 9 years ago by Remy Blank

Keywords: review added

The patch above hides the changeset date, author and message in the browser and revision log views when the user doesn't have permission to view the changeset (e.g. due to authz).

I'm pushing into unknown territory here, so a review (and some user testing) would be most welcome.

comment:6 Changed 9 years ago by Remy Blank

Milestone: 0.11.40.11.3

I'd like this to be fixed in 0.11.3. Testing still welcome.

Changed 9 years ago by Remy Blank

Cleaner approach to hiding forbidden changeset data

comment:7 Changed 9 years ago by Remy Blank

My first patch was the wrong approach. The second one should be better:

  • The changeset date, author and message are hidden in the browser view when the user doesn't have permission to view the changeset.
  • Forbidden changesets are removed entirely from the log (instead of just hiding the metadata).

Christian, I'd appreciate if you could have a look at this patch and let me know what you think.

comment:8 Changed 9 years ago by Christian Boos

Yes, it looks fine, but I'd like to test it.

comment:9 Changed 9 years ago by Remy Blank

Milestone: 0.11.30.11.4

I don't feel confident pushing this shortly before 0.11.3. I'd rather apply the fix right after 0.11.3 is out, so that it has at least a chance to get some serious testing.

comment:10 Changed 9 years ago by Remy Blank

Milestone: 0.11.60.11.5

Changed 9 years ago by Remy Blank

Updated patch fixing the "Next changeset" link.

comment:11 Changed 9 years ago by Remy Blank

The updated patch above fixes the "Next changeset" link, which would previously link to unauthorized changesets and lead to a permission error. The "Previous changeset" was working well.

As authorization is quite critical, I'd appreciate if I could get a review of the patch before I apply it.

Changed 8 years ago by Remy Blank

Updated patch for current 0.11-stable head.

comment:12 Changed 8 years ago by Remy Blank

The patch has been applied to 0.11-stable in [8184].

comment:13 Changed 8 years ago by osimons

Cc: osimons added

I've got one empty changeset in my repos - see #5097. It contains the log message and meta data, but no record of paths affected by change.

Following [8184] I'm no longer able to resync this repos. The CachedRepository.sync() method turns off all authz information, and is not prepared for SubversionRepository.get_changeset() evaluating rev == 0 or any(cset.get_changes()) to false and throwing NoSuchChangeset(rev).

comment:14 Changed 8 years ago by Remy Blank

I didn't know you could have empty changesets. get_changeset() should obviously not throw NoSuchChangeset in that case. Thanks for testing!

comment:15 Changed 8 years ago by Remy Blank

This issue is even trickier than I thought. I have backed out [8184] in [8198], and I'll post a better fix here, as a patch.

Changed 8 years ago by Remy Blank

Even another approach, with its own issues

comment:16 Changed 8 years ago by Remy Blank

The patch above is a third variant that fixes this issue, as well as the new one reported in #8282. A few explanations:

  • SubversionChangeset.get_changes() and CachedChangeset.get_changes() now raise a ChangesetDenied exception if a changeset is not empty, but no changes are authorized. This is used in RealSubversionAuthorizer.has_permission_for_changeset() to determine if a changeset can be accessed or not.
  • The big change in SubversionRepository.sync() actually only moves the deactivation of the authorizer higher up in the function. Nothing else is changed there.

But it's still not complete. I get a "No changeset 0 in the repository" error when trying to view revisions 1 and 2 in my repository, which so far I could trace back to Node.get_previous() raising the exception.

Changed 8 years ago by Remy Blank

Improved patch handling revisions 1 and 2 correctly

comment:17 Changed 8 years ago by Remy Blank

The patch above handles revisions 1 and 2 correctly. AFAICT, it should now be complete and fix this issue and #8282.

Simon, would it be possible for you to test this patch with your repository containing empty revisions?

comment:18 in reply to:  17 Changed 8 years ago by osimons

Replying to rblank:

Simon, would it be possible for you to test this patch with your repository containing empty revisions?

Supert Remy, patch makes resync work as it should and it also displays the empty changeset without permission error. Haven't tested much else, but just noticed that when viewing my empty changeset 29 and hitting 'Next Changeset' I get 30. When viewing 30, 'Previous Changeset' will take me to 28 and not 29.

comment:19 Changed 8 years ago by Christian Boos

I should probably have joined the discussion on [8184] earlier, but as this things turns out to be more complex than the initial patch, I'm a bit worried of the direction it takes now.

The authz stuff is still a remnant of the 0.10 way to implement TracFineGrainedPermissions, which closely coupled the SubversionRepository and the SubversionAuthorizer. The plan was to transform the SubversionAuthorizer into a real PermissionPolicy and move the permission checks at the generic versioncontrol level. But we missed the opportunity to do so for 0.11, and now the old code still sits there, with its bugs biting us back ;-)

Perhaps a way to implement that "late" in the 0.11 cycle would be to just leave the authz parameter and property in the Repository, but not use it. So we wouldn't need a new ChangesetDenied exception and we could do the checks at a higher level, using the normal fine grained permission mechanism?

comment:20 in reply to:  19 ; Changed 8 years ago by Remy Blank

Replying to osimons:

just noticed that when viewing my empty changeset 29 and hitting 'Next Changeset' I get 30. When viewing 30, 'Previous Changeset' will take me to 28 and not 29.

That's weird, but not completely surprising. There's an asymmetry in the code between going back and going forward in the history. I will see if I can fix that, too (if we keep the code, see below).

Replying to cboos:

this things turns out to be more complex than the initial patch, I'm a bit worried of the direction it takes now.

Not quite. The number of changes between the last and second last patch is about the same. But yes, the code is quite complex and sometimes convoluted, so it does show a few surprises.

So we wouldn't need a new ChangesetDenied exception and we could do the checks at a higher level, using the normal fine grained permission mechanism?

Well, the ChangesetDenied exception is actually only a marker that allows differentiating between an empty changeset and a non-empty changeset where all changes are denied, in Changeset.get_changes(). It should never be visible to the user.

About migrating to the fine-grained permission mechanism, this is one of the areas in Trac I'm not yet familiar with, so I'm not sure how this would be done. But my feeling is that at least part of the last patch will be needed anyway, namely the missing permission checks in CachedRepository._next_prev_rev(), SubversionRepository._history() and get_changes() in web_ui/util.py. Or would all permission checks be removed from the repositories, and be moved to client code?

I'd be ok with dropping the fixes I propose here and going for the fine-grained permission mechanism (providing it solves this issue, obviously), but I would leave the implementation to someone else, as I would hate to mess up a security-sensitive area like this.

comment:21 in reply to:  20 Changed 8 years ago by Christian Boos

Keywords: svnauthz multirepos added; review removed
Milestone: 0.11.60.12
Owner: changed from Remy Blank to Christian Boos

Replying to rblank:

… my feeling is that at least part of the last patch will be needed anyway, namely the missing permission checks in CachedRepository._next_prev_rev(), SubversionRepository._history() and get_changes() in web_ui/util.py. Or would all permission checks be removed from the repositories, and be moved to client code?

I'd be ok with dropping the fixes I propose here and going for the fine-grained permission mechanism (providing it solves this issue, obviously), but I would leave the implementation to someone else, as I would hate to mess up a security-sensitive area like this.

Will be done in MultiRepos somewhat "soon".

comment:22 Changed 8 years ago by Christian Boos

Milestone: 0.120.12-multirepos

comment:23 Changed 8 years ago by Remy Blank

Resolution: fixed
Status: newclosed

This is fixed in [9082].

comment:24 Changed 8 years ago by Remy Blank

Owner: changed from Christian Boos to Remy Blank

comment:25 Changed 4 years ago by Ryan J Ollos

Keywords: authzsourcepolicy added

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .

 
Note: See TracTickets for help on using tickets.