Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Goal matching regex - test for regression #1991

Closed
mattab opened this Issue · 7 comments

2 participants

@mattab
Owner

A few users have been complaining that Goals are not matched since upgrade to 1.1.

We should add unit tests to test that goal matching works as expected in all supported use cases, if not fix the issue

@mattab
Owner

From Joachim: "the fault is the uncommented / in the regex. With
http://dev.piwik.org/trac/changeset/3344/trunk/core/Tracker/GoalManager.php,
the / are commented by piwik, but it is not checked, if they are already
commented out and also the example in piwik itself is not updated to the new
behaviour."

@mattab
Owner

vipsoft, is r3344 doing something in particular or fixing a bug? it seems it broke the regex format. shall I revert?

@mattab
Owner

(In [3901]) Fixes #1991

  • Adding test triggering a goal with regex
  • revert r3344 as the documentation examples are escaped regular expressions
@robocoder
Collaborator

r3344 is a security best practice because the regex is user input. (If this was a preg_replace, it would have been a potentially serious vulnerability.)

Instead of an unescaped regex failing silently, we should have fixed the docs/examples. Vote to revert 3901.

@robocoder
Collaborator

Either way, existing goals are affected. Can we add an update script?

Also, there's a UI inconsistency in the input handling of regular expressions (SitesManager/Controller.php).

@robocoder
Collaborator

(In [3946]) refs #1991 - runtime detect unescaped "/" in patterns, and escape if needed

@mattab mattab added this to the Piwik 1.2 milestone
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.