Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#1991 closed Bug (fixed)

Goal matching regex - test for regression

Reported by: matt Owned by:
Priority: critical Milestone: Piwik 1.2
Component: Core Keywords:
Cc: Sensitive: no

Description

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

Change History (10)

comment:1 Changed 3 years ago by matt (mattab)

  • Priority changed from major to critical

comment:2 Changed 3 years ago by matt (mattab)

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."

comment:4 Changed 3 years ago by matt (mattab)

  • Type changed from Task to Bug

comment:5 in reply to: ↑ description Changed 3 years ago by forextrading11

comment:6 Changed 3 years ago by matt (mattab)

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

comment:7 Changed 3 years ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from new to closed

(In [3901]) Fixes #1991

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

comment:8 Changed 3 years ago by vipsoft (robocoder)

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.

comment:9 Changed 3 years ago by vipsoft (robocoder)

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).

comment:10 Changed 3 years ago by vipsoft (robocoder)

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

Note: See TracTickets for help on using tickets.