Make sure the administration area can't be broken

@i.chris.jacob: I'm glad you were finally able to figure it out.

This seems like an important bug/feature that may end up being rather complex.

With power comes responsibility... You can also delete your whole DB with the execute PHP action. I don't think it's possible to anticipate which rules will cause errors. IMO, this issue should be won't fix.

I don't think this use case applies to those who would use the PHP action. #323312: Add PHP execution permissions is an issue where we can help prevent undesirable events with the execute PHP action.

There are a wide range of possibilities where normal Rules usage can bring down Drupal. As a general rule of thumb, a proper action at that point would be to deactivate the offending rule and display a message, 'Rules deactivated rule_# because it would cause x, y, or z.'

To do this, we could further integrate rules with itself:
- Event: rule is saved
- Condition: rule is unsafe
- Action: inactivate rule

I don't think it's possible to anticipate which rules will cause errors.

Thankfully, i.chris.jacob's reported a rule signature that breaks stuff, much like a virus definition. Obviously we can't predict all user behaviors, but we can at least interpret some events and act on them, so as to avoid giving users "the dangerous module" :-)

Similarly, in #445084-11: Adjust module weight from 0:

Could not the weight of the module be dynamically changed? In that way, if some future modules will use a higher weight, the module is able to adapt itself to the environment conditions (it seems like the module is getting alive ;-)).

We could further integrate rules with core:
- Event: module is enabled
- Condition: rules's weight is not right
- Action: set module weight.

Just a thought on this one... It seems that some rules, such as drupal_goto() calls, may need to be turned off if the user is in the admin screen. That, I think, would be enough.

As I have now been playing with the Rules module, I have noticed quite a few side effects with rules calling other rules calling other rules... Here you can easily get recursivity and I don't recall seeing anything that will prevent an infinite loop of the Rules system, although I hope I'm wrong. (i.e. Save User data, generates an update, runs another Rule which Saves a Taxonomy, that wakes up another Rule which happens to be the first rule and that again Saves User data, etc.)

So, what I'm saying is that we should have internal conditions that prevent silly things from happening. In this instance, we could even totally prevent a redirect if there are no conditions since it would make no sense anyway.

Thank you.
Alexis

Update: The recursivity is being checked. I ran into it once and got a message about a loop being broken.

>I don't recall seeing anything that will prevent an infinite loop of the Rules system
oh that's in there, else you would run into loops quite fast.

>So, what I'm saying is that we should have internal conditions that prevent silly things from happening. In this instance, we could even totally prevent a redirect if there are no conditions since it would make no sense anyway.

I don't think it's realistic to try to prevent "bad configurations". That's the users responsibility, however the UI should not immediately apply the changes. Thus if you just want to remove the condition to add another one, it shouldn't cause troubles. However, this is quite a major change, that won't happen for rules 1.x any more.

thanks
had the same problem and resolved it by following your instructions to the letter.
so did you finally get the rule to work?

You had:

Original Rule:
Event: User is going to view a page
Condition: Path has alias(es)
- Existing system path: "events"
Action: System > Page Redirect
- To: "/calendar"

My question is did you get the rule to work as above? Or what did you use as above is not working for me?
Your help is most welcome!

I do think we should just disable redirects for the whole admin area. Also see #732542: system_goto_action breaks core APIs.

We cannot use hook_admin_paths() as this might contain stuff like node/X/edit too, so I guess best we just disable dangerous actions like the page redirect action for /admin/* ?

fago,

Couldn't we limit it to admin/rules/* ?

Thank you.
Alexis

That would work too, but why not to "admin" at all? Are there use-cases where one wants to use rules-driven page redirects in the admin area?

I guess that if someone needs such a tweak they can always do it in they .htaccess or directly in the code. I do not know of a case myself, although I have a rather heavily transformed version of Drupal since there are many problems that can easily be solved (i.e. missing blocks on 404 pages for instance!)

I guess that a redirect within the admin would mean you want a customer to never go to page A and redirect him/her to page B instead where he/she can do the setup without breaking anything. That probably means you programmed a new page (B) that replaced the old page (A) and thus can do the redirect in software too.

Will see if anyone else comes up with something.

Thank you.
Alexis

We should include a built-in check for the drupal_goto action, or better for all events firing on rules-admin screens. Thoughts?

adding tag

Here is a patch that just tackles the page redirect action. It works for GET requests when visiting the Rules administration pages, but it does not fully work on form submissions (saving changes of a rules configuration). I guess the form redirects somehow interfere.

+    if (isset($_GET['q']) && strpos($_GET['q'], 'admin/config/workflow/rules') === 0) {
+      rules_log('Prevented page redirect action on Rules administration page.', array(), RulesLog::WARN);
+    }

It won't work for POST requests because it doesn't need hook_page_build() to work then. We should throw the warning directly in the action and bail out, then it should work for POST too.

Of course, I was blind. Patch attached.

patch looks good.

Somehow related, should we also add a check for batches as in the proposed core patch?

+  // Do not redirect during batch processing.
+  if (!$batch =& batch_get() || !isset($batch['current_set'])) {

I think it is enough to protect Rules administration pages, so that people are always able to revert/disable/delete their malicious redirect actions. But otherwise we are not their parents: they can shoot them in the foot, if they want to.

I think it's our job to assist users doing so + we I'd argue no one wants page-directs to happen during batches. Thus, I've added the check too.

I've committed your patch + added the check in a follow-up. thanks.

Is the patch applicable to 6.x too? If so, should we have it ported there too?

Thank you.
Alexis Wilke

Yep, porting this to 6.x is probably a good idea.