| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | cron system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Example: CCK and Content Permissions modules enabled; some fields restricted to privileged users. Search index is automatically updated via normal invocation of cron.php (running as anonymous user).
Suppose I create/amend a node and manually invoke cron (/admin/status/reports/run-cron). The contents of the restricted fields will get into the search index. 2 problems with this:
1. If an unrestricted user searches for words in the restricted fields then the relevant nodes will be returned in the search results (though happily the content of the restricted fields won't be shown)
2. Some nodes have their restricted fields indexed, while others don't.
A simple fix would be to force $user to http://api.drupal.org/api/function/drupal_anonymous_user/6. However there may be situations in which cron needs to run with additional privileges - see comment #19 in #5380: introduce a "cron" user to fix various cron related issues. Though maybe such situations would be best handled via a contrib module.
Comments
#1
Clarification/improvement to suggestion: the "simple fix" is only needed when cron is run using the run-cron path (i.e. in http://api.drupal.org/api/function/system_run_cron/6). This avoids interfering with cron.php in any way.
Arguably content permissions module should implement the fix (e.g. by overriding http://api.drupal.org/api/function/theme_status_report/6), but IMO it would be better to fix it this in core, otherwise any module that modifies content depending on user roles/permissions could interfere with cron when invoked via the run-cron path and would therefore need to override theme_status_report().
#2
Changing title. This seems the simplest approach, contrib cron modules or special case sites can do what they want with regard to the user that "runs" cron.
The problem is potentially wider than content permissions module; having cron run by default as different users depending on whether invoked by cron.php or the run-cron link on the status report page hardly helps troubleshooting.
#3
subscribing
#4
This is obviously a bug in Drupal core, but seems like a serious design issue in content permission, too ;)
#5
Issue mentioned in content_permissions
http://drupal.org/node/510744
#6
Here's a patch, sets $GLOBALS['user'] = drupal_anonymous_user() in drupal_cron_run() and prevents session being saved.
Critical because this blocks #523894: Comment indexing does not include the fully rendered comment which affects indexing fields on comments, which in turn blocks comment body as field (and etc.).
#7
Spoke to DamZ in irc, inverted the two calls.
#8
The last submitted patch failed testing.
#9
HEAD was a couple of hours stale.
#10
The last submitted patch failed testing.
#11
That's a real bug - if run via the ui, system_run_cron() does a drupal_set_message() to report success/fail. So, we now restore the user after all jobs have run, and don't prevent session information being saved. The other option is to remove those drupal_set_message() calls, but having to look at the status table to see if it ran properly or not isn't ideal, so would rather keep them in.
#12
It's ok to restore the user at the end of the process, but you still have to prevent the session from being saved during the time the other is changed.
#13
Here.
#14
The last submitted patch failed testing.
#15
I guess I have a different perspective. Search should have access to index all the content that it is supposed to.
So whether cron is run as anonymous or as an authenticated user with privileges should be irrelevant.
Search results on the other hand should respect the access modules in place. Usually in the form of a contrib module, such as apachesolr_nodeaccess (in http://drupal.org/project/apachesolr) that will modify the query of search results to exclude those nodes the user does not have access to.
Because search results may show restricted content does not mean that that restricted to anonymous content should not get indexed at all, and therefore not be available to those that have access, to view them in search.
For apache solr field access control, I will start http://drupal.org/project/apachesolr_fieldaccess
#16
Moving to new cron system component.
#17
We should probably require #287292: Add function to impersonate a user to land in core first.
#18
Too late for D7. :(
#19
hrm. Actually, reading the OP once more, I wonder why this is a public security issue. Moving back.
#20
@aufumy
You concern is off-topic here. Cron normally runs as anonymous (from the command0line), so we need to ensure uniform behavior.
Here's a patch that passes, but I had to alter the verifyCron() test for dbLog to make it do so.
#21
#22
+++ includes/common.inc 26 Feb 2010 23:02:01 -0000@@ -4373,6 +4373,14 @@ function drupal_cron_run() {
+ // Prevent session information from being saved while the cron is running.
... while cron is running.
+++ includes/common.inc 26 Feb 2010 23:02:01 -0000@@ -4447,6 +4458,10 @@ function drupal_cron_cleanup() {
+ drupal_save_session(TRUE);
Usually, this is called right after reverting the user. Is there a specific reason for delaying this by putting it in
drupal_cron_cleanup()?+++ includes/common.inc 26 Feb 2010 23:02:01 -0000@@ -4447,6 +4458,10 @@ function drupal_cron_cleanup() {
+ return TRUE;
drupal_cron_cleanup()should not return any value.+++ modules/dblog/dblog.test 26 Feb 2010 23:02:04 -0000@@ -74,9 +74,7 @@ class DBLogTestCase extends DrupalWebTes
- $this->drupalGet('admin/reports/status/run-cron');
- $this->assertResponse(200);
- $this->assertText(t('Cron ran successfully'), t('Cron ran successfully'));
I don't think we can just get rid of the "Cron ran successfully" message when running cron manually through the interface. Users expect this kind of message when performing actions through the interface. Since the message is set in
system_cron()after callingdrupal_run_cron(), the message should be persisted if the user is reverted anddrupal_save_session(TRUE)called at the end of drupal_run_cron(), right?Powered by Dreditor.
#23
Could be. Let's try it.
#24
+++ includes/common.inc 10 Mar 2010 21:51:02 -0000@@ -4385,6 +4385,14 @@ function drupal_cron_run() {
+ // Prevent session information from being saved while the cron is running.
1. Is it 'the cron' or just 'cron'?
2. If we forced the user to the anonymous user, do we still care about sessions? I'm not sure the phpDoc provides enough context to understand why sessions need to be disabled (for the anonymous user).
#25
@Dries: IMO, yes, we care about the session: if we do not disable session saving during impersonation and the cron run is aborted half-way through, the user will logged out.
Thinking about this does bring up an interesting point though. What if an individual hook_cron implementation does impersonation itself. When reverting, it calls session_save_session(TRUE); and switches session saving back on...
#26
mr.baileys: that sounds like it would be a bug in the module then. I think this is RTBC. I'll fix the comment and commit it.
#27
Committed to CVS HEAD.
#28
Automatically closed -- issue fixed for 2 weeks with no activity.