@saleemkce opened this Pull Request on June 6th 2015 Contributor

Fix for issue https://github.com/piwik/piwik/issues/7978.

  • Removes if double quotes present after checking each user-agent string. If quotes is not present, the user-agent is left as such.
@sgiehl commented on June 23rd 2015 Member

Simply replacing all quotes is imho not the best way. A quote is still a valid character for useragents and thus might occur in one. So we should not make it impossible to exclude useragents including a quote.

To better handle enclosing quotes maybe it might be better to use str_getcsv()?

@saleemkce commented on June 24th 2015 Contributor

A quote is still a valid character for useragents and thus might occur in one.

@sgiehl I studied "user agent" strings thoroughly before starting writing the fix. To my knowledge, no user agent contains single or double quotes. So, this might be a good solution. I suggest you guys to checkout the site http://www.useragentstring.com/pages/useragentstring.php which lists numerous user agent strings. As per Matt's suggestion, we do not user comma separated (user agent) strings for now. So, "str_getcsv" doesn't help in this case.

@sgiehl commented on June 24th 2015 Member

I'm the lead developer of device detector. Believe me or not, there are useragents containing quotes

some examples:

  • Mozilla/5.0 (Linux; U; Android 4.2.2; it-it; PocketBook SURFpad 3 (7,85") Build/JDQ39) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Safari/534.30
  • Mozilla/5.0 (Linux; Android 4.1.1; Family'TAB 8001 Build/JRO03C) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.114 Safari/537.36
  • RokaSpider/Nutch-1.9 ('webmaster at katzenmaier dot net')
  • Friendica 'Ginger' 3.3.2-1175; http://friendica.dszdw.net

The current implementation uses comma separated values, and your changes doesn't change that behaviour as checkAndReturnCommaSeparatedStringList is still used.
Btw. str_getcsv can also be used with custom separators such as "\n"

@saleemkce commented on June 24th 2015 Contributor

Okay, thanks for sharing such user agents which are quite new to me. We
have to rethink a different solution for this.

On Wed, Jun 24, 2015 at 3:59 PM, Stefan Giehl notifications@github.com
wrote:

I'm the lead developer of device detector
https://github.com/piwik/device-detector. Believe me or not, there are
useragents containing quotes

some examples:

  • Mozilla/5.0 (Linux; U; Android 4.2.2; it-it; PocketBook SURFpad 3
    (7,85") Build/JDQ39) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0
    Safari/534.30
  • Mozilla/5.0 (Linux; Android 4.1.1; Family'TAB 8001 Build/JRO03C)
    AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.114 Safari/537.36
  • RokaSpider/Nutch-1.9 ('webmaster at katzenmaier dot net')
  • Friendica 'Ginger' 3.3.2-1175; http://friendica.dszdw.net

The current implementation uses comma separated values, and your changes
doesn't change that behaviour as checkAndReturnCommaSeparatedStringList
is still used.
Btw. str_getcsv can also be used with custom separators such as "\n"


Reply to this email directly or view it on GitHub
https://github.com/piwik/piwik/pull/8050#issuecomment-114818476.

@saleemkce commented on July 27th 2015 Contributor

@mnapoli @mattab I have updated the quotes removal with trim() function as per suggestion and tested with single & more pair of double quotes in the end, works great. Please review and comment if we need any improvements here. Sorry for the extended delay, I had been quite busy in religious activities during Ramadan fasting period.

@sgiehl commented on July 27th 2015 Member

Isn't the goal of this PR the same as of #8368

@mnapoli commented on July 27th 2015 Contributor

Ah I started working on #7978 today, we have actually many problems on top of the original issue:

  • the inline help about excluded user agents in the form doesn't say anything about how to format user agent strings
  • other inline helps in the page (e.g. for query parameters) ask one item per line, whereas in the code it's also comma-separated, so it's clearly wrong
  • this PR fixes the problem of enclosing quotes, but doesn't solve the problem that user agent strings can contain commas too
  • all the problems that we see here also apply to the site-specific settings, so fixing it for the global list isn't enough

I started to work on a solution where all similar options work with one item per line (replacing the "comma-separated" formatting). Enclosing quotes are accepted and trimmed. The goal is to have consistency between all the different form fields, between the fields and their helps and between site-specific settings and global settings. Also it would allow any character in user agent strings (both quotes and commas).

@mnapoli commented on July 27th 2015 Contributor

Also @sgiehl I just noticed #8368 thanks to you, but I don't especially like it (at least the part concerning this issue) because it hides the "commas" problem by modifying request data in the tracker instead of fixing an escaping issue.

@mnapoli commented on July 28th 2015 Contributor

FYI I've started working on a new fix in #8482

@tsteur commented on August 13th 2015 Member

Really appreciate your work done here but have to close in favour of #8368 .

@tsteur commented on August 13th 2015 Member

actually because of #8482

This Pull Request was closed on August 13th 2015
Powered by GitHub Issue Mirror