Access checking broken!

I added now a "access recent changes" permission to view the "recent changes" page. Also I added the db_rewrite_sql statements. Can you please test if it works?

Use the DRUPAL-5 branch for the fixed version.

I'm afraid this is not the right solution. My idea about the recent changes module is that users that access the recent changes feed should be able to see changes to all content, even if they have no access to that content or content type. This behavior can of course be enabled or disabled in the administrative page. To prevent security problems, the headers and content of each node or comment in the feed can be replaced by 'access denied' messages if "List inaccessible nodes" is enabled. If certain nodes are already discarded in the database query (because of db_rewrite_sql), it will not be possible to list those inaccessible nodes with 'access denied' messages in feeds.

Whereas for the recent changes page using db_rewrite_sql statements is the right solution, this is not how it should be done for the recent changes feed. Therefore in this patch, the db_rewrite_sql is optional for the recent changes feed, depending on the "List inaccessible nodes" setting in the administration page. A separate "access recent changes" permission is not necessary anymore, so I removed it. Instead, I changed the "List inaccessible nodes" and "List inaccessible revisions" default options to FALSE, so that users who upgrade to 5.x-1.2 won't compromise their website's security.

Furthermore, I implemented db_rewrite_sql in _recent_changes_access_check. It now takes care of sql statements in the form of WHERE n.type NOT IN ('node_type_1', 'node_type_2', ...) (for multiple node types and WHERE clauses), and removes those node types from the list of accessible node types.

Also, note that for the recent changes feed, the following code is used to check whether the headers of a node should be shown in feeds, or be replaced by access denied messages:

$node_access = node_access('view', $node) && ($item->is_current_revision || user_access('view revisions'));

I don't know whether this bypasses modules like Forum Access.

The patch contains the changes relative to 1.1.2.8 in CVS.

And don't forget to change the sentence

The recent changes page and the RSS feed are accessible to everyone with the view revisions permission.

on the recent changes project overview page when this is fixed.

@salvis: when I access the recent changes feed with an RSS reader, I am not logged in to the website, so the RSS reader is treated as anonymous user. I want to be able to use an RSS reader to track which content has been updated, which includes content that is not accessible to the anonymous user. However, I don't want to compromise security, so headers of inaccessible content (that is inaccessible to the anonymous user) can be replaced with 'access denied' messages. Content of inaccessible nodes or comments is always replaced by an 'access denied' message. If you don't want to list inaccessible content in feeds, you can simply disable this feature in the administrative page. In my latest patch the options "List inaccessible nodes" and "List inaccessible revisions" are disabled by default.

On the recent changes page, it is not necessary to list content to which the anonymous user has no access; the user could simply log in to see the changes in content to which he or she has access.

I know the problem with the RSS feed, I had it too. I just removed the access check for the feed locally since the recent_changes page was not accessible for the anonymous user the url of the feed was never shown. Yes, it's a hack.

But I had another idea now. What about generating authentication tokens. If you log in and go to the recent changes feed, the url of the feed has a token appended (recent_changes/feed/x7fJ32lk3jaS) with a mapping of token strings to users. So now if you access the recent changes feed with the token it checks which user the token belongs to and uses the appropriate permission settings. The token is in the url, so I'm not sure how much of a security issue that would be.

@leop: I had some problems with my email account. I didn't receive any emails from drupal.org lately. So now I changed the email address to another one and it works again. If you sent me a message through my contact form about co-maintaining recent_changes module please send it again.

@salvis Maybe you misunderstood my previous comments; I was merely describing the behavior of my patches to the recent changes module instead of defending them. I do agree however, that access to RSS feeds is a problem. My approach for the recent changes is a somewhat ugly solution, but the other solutions you mention (HTTP authentication and Secure Site) are also not perfect. In my case I prefer the solution of listing headers of nodes (content of nodes is never shown). Note that this feature can be disabled (and is disabled by default), and that headers can be replaced by 'access denied' messages. You are right about security being compromised in the current version of recent changes, but my patch in #2 completely takes care of that. No inaccessible content will be shown to unauthorized users. If somebody wants to list headers of inaccessible nodes in feeds, he or she can enable this feature in the administrative page. This behavior is clearly described as "List inaccessible nodes".

The recent changes feed does check access, be it with node_access instead of db_rewrite_sql. This might be a bit slower, but it certainly does not violate the basic contract of Drupal.

@rötzi I don't think there is a security problem for the recent changes feed (after my patch in #2), so adding tokens to the URL is a solution to a nonexistent problem. If there were a security problem, adding the token might help a little bit, but it is not a real solution (in any case it is much better than using user:pass@example.com/recent_changes/feed as is done in the Secure Site module). If the website administrator wants to list the headers of inaccessible content in recent changes feeds, it is a decision he or she makes, and it is clearly described. Moreover, even headers can be replaced by 'access denied' messages. If the website administrator does not want to list headers of inaccessible content, he doesn't have to do anything, since this feature is disabled by default as of #2. The same holds for listing older revisions when the anonymous user has no view revisions permission.

To view RSS feeds as an authenticated user one can use the HTTP authentication or Secure Site modules as explained by salvis. I have tried these, and found them not suitable for my website, but that is something we don't need to discuss here. For the recent changes module I implemented the solution of listing the headers of inaccessible content, listing inaccessible content with access denied messages instead of headers, or not listing inaccessible content at all. It's up to the website administrator to decide which option to use. Given the discussion about recent changes in previous months, I do feel that enabling the listing of headers of inaccessible content in RSS feeds is a solution to many users. You might consider this a hack, but hey, you don't have to enable listing headers of inaccessible content.

An access recent changes permission makes a lot of sense, thank you.

I think I was too fast removing the access recent changes permission. Although it won't give you additional security (all security aspects can be handled in the administrative page of recent changes), you might want to disable the listing of recent changes for certain users or roles. So we need to put the access recent changes permission back in the module. Sorry for removing it.

But if we add a way to identify which user's feed it is (e.g. with a token), we can show in the feed the exact same list as if that user is logged in and goes to the recent changes page. That way we don't need to distinguish the feed listing from the normal page view.

Indeed, that is possible. But granting access to the recent changes feed, which includes content of (normally inaccessible) recently changed nodes at a fixed URL is kind of a security problem. URLs get logged, and can easily leak. If somebody has found out (e.g. by looking in the browser or server log files) what that URL is, he or she has access to the recent changes feed, including the content that the user for which the token was generated, can access.

Using tokens for logins is only secure for one-time logins.

But isn't this the same problem with HTTP authentication? If you have to send your username and password in plaintext to get the feed, this could be logged as well.

I guess as long as you don't use https you always have this problem.

Exactly. That is why don't like the way Secure Site handles access to RSS. And that is why I took the trouble to implement listing of inaccessible content in feeds in a secure fashion.

Whereas passwords typed in an input form might not always be logged, URLs of the form user:pass@example.com/recent_changes/feed or example.com/recent_changes/feed/token most certainly are logged. So that is a big security problem.

@salvis I took a closer look at the Secure Site module, and you are right. The much sought after solution is in that module, so showing inaccessible items in RSS feeds can be disabled. I'll create a patch that removes listing inaccessible items in feeds. No loaded guns in the recent changes module anymore. Thanks for your comments.

Here is the patch, relative to 5.x-1.2. Listing of inaccessible content and revisions is removed. I can recommend the Secure Site module for providing a method to log in to RSS feeds.

@rötzi: it seems we are working with different versions of the diff module; I solved this for now (see _recent_changes_get_diff), but maybe we need to specify which version is needed for integration.

@salvis I found out about that nasty part of Secure Site rather quickly. My solution to this is in #227988: login form after logout. But I'll try yours as soon as I have time. Also I will take a look at the problem with the patch in #15, but I won't have time for a week or so.

I just read through all this ... and there is a basic premise that I disagree with.

Somewhere at the head of this it's stated that "Recent Changes" is only for administrators. Uh, this is where I disagree. Rather, you guys as the project owner can define the scope as you wish. However I want to use Recent Changes so that my user base can have an easier way to view the recent activity on the site. Recent Changes is excellent in this regard since it digs up recent comments .. most of the activity on a forum is in the comments as people chat back and forth. So to see recent activity the feed for recent activity must have the the comments. The "comment RSS" module doesn't quite do it because that's only for comments.

The content in the Recent Changes page & feed should be relevant for the user ID (or anonymous) currently in play.

Returning "access denied" messages in place of content disallowed to the current user ID would be very user-unfriendly-to-the-max. It would rightly confuse the typical user into thinking they had done something wrong.

Of course as y'all noted an RSS feed is likely going to be anonymous. And things like Google will find the RSS feed even if you are careful about not advertising it.

I agree with reikiman that Recent Changes shouldn't be only for administrators. The feed is great for showing all recent activity on the site. The tracker doesn't show anything about all the page edits, and the best you can do to come up with something like this in views will only show the most recent edit of a page - all previous edits of that page are disregarded.

On my site I push everyone towards using either the recent changes page or feed as their way of keeping up with the site otherwise they don't really know what's going on. That's kind of the community wiki approach. Our biggest problem though is if we want to discuss something private - we can use access control to stop roles seeing a page or its comments - but then the recent changes module pushes out the entire page edit with a diff, or comment in a RSS feed to anon users.

So with that in mind I would have thought what should be in the recent changes page should be only what the user has privileges to see. So if you have pages that are private using access control, edits of those pages, and comments on those pages shouldn't show up for users that don't have privileges. I'm using 5.x.1.1, and was the reporter of http://drupal.org/node/217262. From what I can see 5.x-1.2 just shows access denied for those you're not able to see. It would be better if nothing was shown for those updates to those users.

Some complete solution for this to work with authentication for the RSS feed needs to be possible too, so we can't be scared from having the page and feed have less items in it for anonymous users. The HTTP Auth module, and the Secure Site module seem to deal with this in some regards. HTTP Auth allows you to manually put ?authenticate onto the end of your rss feed and then authenticate at the time of logging into your feed reader. What would be better is if it automatically appended that to the feed. That would be enough, as long as the recent changes feed was outputting the right thing (like the rest of the automatically generated Drupal feeds are). I couldn't stick with Secure Site long enough to figure out if it worked any better as no matter what settings I used, or workarounds it always gave me the login form when logging out is just a pain.

All in all, I'd like to echo reikiman said - you guys are the ones putting the effort into maintaining this and it's your call. I just hope you consider above when you're coming up with your solution.

PS. I"m setting up my test environment properly so I can help test Recent Changes bugs to hopefully make your lives easier :)

I try to recap, so please correct me if I'm wrong (or support me if I'm right ;)

- Recent changes page is fine as it is (no one complained about it ;)
- Offer "Access recent changes" permission for role-based access (possibility for anonymous access or admin's only)
- Feed:
-- No "access denied" messages in feed (done in leop's patch)
-- Per default, only what anonymous can access is shown (done in leop's patch)
-- Add a remark in help section to use "Secure site" or "HTTP Auth" module if desired
-- If HTTP Auth module enabled, offer setting to add "?authenticate" automatically (or don't offer an option and always do it?)

What about offering the "token"-based solution as an option? (Explaining the security problem and recommending https). I still think it's not that bad a solution as you "only" leak the recent changes page and don't send your username and password with every feed-access (even if username and password don't have to be in the URL itself, they still get sent with every access).

Recent changes page is fine as it is (no one complained about it :)

Sure is. I do love a good recent changes page :)

Offer "Access recent changes" permission for role-based access (possibility for anonymous access or admin's only)

Yes, exactly. That gives the user the control over how they want to use it. The feed outputted should match the choice chosen. So unless it's everyone can see everything, some form of authentication should be forced on the feed.

Feed:
-- No "access denied" messages in feed (done in leop's patch)

Yep agree.

-- Per default, only what anonymous can access is shown (done in leop's patch)

Yep. If users want anonymous to see everything they can configure that as per the "Access recent changes permission"

-- Add a remark in help section to use "Secure site" or "HTTP Auth" module if desired

Possibly need to add some documentation on how to get either of these set up with Recent Changes (if it's any different to with other feeds).

-- If HTTP Auth module enabled, offer setting to add "?authenticate" automatically (or don't offer an option and always do it?)

I would suggest offering to add it, and have it turned on by default. Giving the option to turn it off I'm sure would be helpful to someone.

What about offering the "token"-based solution as an option? (Explaining the security problem and recommending https). I still think it's not that bad a solution as you "only" leak the recent changes page and don't send your username and password with every feed-access (even if username and password don't have to be in the URL itself, they still get sent with every access).

I'd consider leaking the recent changes page a major security risk. I'm not sure how easy it would be for that token to be intercepted. Obviously if you were looking at the server logs you could see the token on access logs of pages hit - but if you're reading the server logs you're an admin anyway. I agree it has the advantage over HTTP auth of not having to send the username and password on each access. Not really sure on this one - will leave it to someone with a bit more expertise on authentication. I'd have no issue with it being added as an option - but not in place of HTTP Auth or secure site.

Everyting discussed in #20, exept for "Add a remark in help section to use 'Secure site' or 'HTTP Auth' module if desired" and "'?authenticate' automatically" is implemented in the patch in #15.

It doesn't really matter whether you see recent changes as admin tool or not; you can choose to grant "access recent changes" permission to the site administrator, anonymous users, or anything in between as of #15. Recent changes page or feed will only list items that are accessible to the current (or anonymous) user. Also db_rewrite_sql() statements are back in place as of #15. When I have more time, I'll write an explanation for setting up Secure Site to provide a login method for the recent changes feed. I think Secure Site is the way to do it right. If users care about their passwords, they can choose to access the https version of the website, in my case at least.

subscribe...

#133676 has been marked as a duplicate of this issue.

#217262 has been marked as a duplicate of this issue.

The patch adds a call to diff_default_cols() but I don't see that function in diff.module. I don't think this will work with Diff installed and the patched recent_changes.module's RSS feeds set to Full Text.

the tokenauth module does the token based solution that rotzi mentions ... protected rss feeds are a serious hassle for users and site admins. no way around that. the solutions mentioned here are the best available option.

meanwhile, the patch here should be committed soon so that access control iworks proprely on a site like groups.drupal.org

What is the status of this issue? Is a patch suitable enough to be applied?

@iamwhoiam: Please feel welcome to try the patch. From what I can tell, it's fine with the exception of what I said in my earlier comment.

In that case, shouldn't this be committed by rötzi if it fixes the issue? And then we might be able to think about D6 port?

+1

I'd love to start using this module as soon as this issue is resolved.

Cheers,

Tobias

Hi,

Any progress on this patch please?
greetings,
Martijn

Subscribing.

subscribe

So no progresss ?

I just want to keep it for admin, so can I put some if function there that if user == 1 only then show recent changes to it ?

subscribing

sub

According to the project page, the issue isn't limited to D6 as it globally states: "Please note that the access check is broken at the moment. So if you have sensitive information, don't use the module until this bug is fixed."

The project isn't marked as "Abandoned project"; however, very sad if the module is actually abandoned :-(

I am closing this issue, as Drupal 6 is now not supported.