I'm getting "NOT Check an order's state" (note the double-encoded html entities) on "admin/config/workflow/rules/reaction/manage/" page when I negate a condition that reads properly "Check an order's state" without negation.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ptmkenny’s picture

Status: Active » Postponed (maintainer needs more info)

Are you using Commerce or Ubercart? If so, you should report this to that module's issue queue, not Rules.

TR’s picture

Version: 7.x-2.2 » 7.x-2.x-dev
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Active

Confirmed that this still happens in the latest 7.x-2.x release, but it is absolutely NOT a problem with Ubercart - this is a problem with Rules (or, more specifically, the '#type' => 'link' render element) improperly double-encoding the condition name when the the negate text is added. The only reason it shows up with an Ubercart condition and not any of the built-in core Rules conditions is because the core Rules conditions don't use special characters like ' which can be HTML encoded.

To see that this really is a Rules problem, you can simply edit one of the core Rules conditions (such as 'Data comparison' in modules/data.rules.inc) and change the name to include an apostrophe. Then clear your caches and use that condition (negated) in a rule - you will see the exact same behavior as described in the original post.

The problem does not exist in Rules 8.x-3.x, just in 7.x-2.x.

TR’s picture

Priority: Normal » Minor
Status: Active » Needs review
FileSize
516 bytes

Here's a patch that fixes this problem. I would appreciate it if someone would apply this patch and confirm that.

Patch looks like:

   public function label() {
     $label = parent::label();
-    return $this->negate ? t('NOT @condition', array('@condition' => $label)) : $label;
+    return $this->negate ? t('NOT !condition', array('!condition' => $label)) : $label;
   }
 }

The problem was that using @ here prematurely sanitizes $label when the NOT is prepended. Note that in the original code $label is not sanitized here except in the case where NOT is prepended. Thus, this is not introducing a security problem - the patch is just treating NOT $label the same as we were previously treating $label. The security problem would only be if $label wasn't sanitized before output, which occurs elsewhere in the code. Drupal policy is to sanitize before output, and Rules DOES do that. And we know Rules does that, because that's why we were getting a DOUBLE escape when we sanitized $label here.

TR’s picture

Issue tags: +Needs tests
TR’s picture

Assigned: Unassigned » TR

Status: Needs review » Needs work

The last submitted patch, 3: 1945006-3-htmlescape.patch, failed testing. View results

TR’s picture

Status: Needs work » Needs review
FileSize
515 bytes

Testbot does not like fuzz. Re-roll against current HEAD.

(EDIT: It seems I named the patch incorrectly. But the contents are as intended - it's just the name that's wrong)

TR’s picture

Okay, here's a test-only patch that demonstrates and reproduces the error described by the original poster. This patch should FAIL because of the bug.

Also, here's a patch with the same test but also with the fix posted in #7. This patch should PASS because the fix solves the problem.

The last submitted patch, 8: 1945006-8-htmlescape-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

TR’s picture

Tests results in #8 are as expected - the test-only patch adds tests to demonstrate the problem in the current -dev, and the other patch fixes the problem and proves the fix with the new tests.

  • TR committed dd86c9f on 7.x-2.x
    Issue #1945006 by TR: Negated condition in rules does double...
TR’s picture

Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.