Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#7978 - fix for user agent quotes issue #8050

Closed

Conversation

saleemkce
Copy link
Contributor

Fix for issue #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
Copy link
Member

sgiehl commented Jun 23, 2015

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()?

@mnapoli mnapoli added the Needs Review PRs that need a code review label Jun 23, 2015
@saleemkce
Copy link
Contributor Author

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
Copy link
Member

sgiehl commented Jun 24, 2015

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
Copy link
Contributor Author

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
#8050 (comment).

@mnapoli mnapoli self-assigned this Jul 27, 2015
@saleemkce saleemkce force-pushed the bugfix-user-agent-quotes-issue branch from 3bb26c9 to 7b57f0f Compare July 27, 2015 17:21
@saleemkce
Copy link
Contributor Author

@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
Copy link
Member

sgiehl commented Jul 27, 2015

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

@mnapoli
Copy link
Contributor

mnapoli commented Jul 27, 2015

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
Copy link
Contributor

mnapoli commented Jul 27, 2015

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 added a commit that referenced this pull request Jul 28, 2015
Before this commit, users can configure excluded user agents by separating them with commas. Enclosing quotes are not trimmed.

With this commit, users can configure excluded user agents one per line. Enclosing quotes are trimmed. This is meant to be simpler and more consistent. It also allows to exclude user agents containing both commas and quotes. This change applies both to global settings as well as site-specific settings.

This commit also changes the help text, which now explicitly says that user agents should be separated by new lines.

A migration script is included: it will replace commas by new lines.

Related: #7978, #8050
@mnapoli
Copy link
Contributor

mnapoli commented Jul 28, 2015

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

mnapoli added a commit that referenced this pull request Jul 29, 2015
…turns

Before this commit, users can configure excluded query parameters by separating them with commas.

With this commit, users can configure them one per line. This is meant to be simpler and more consistent (see #8448). This is now also consistent with form inline helps. This change applies both to global settings as well as site-specific settings.

A migration script is included: it will replace commas by new lines.

Related: #7978, #8050
mnapoli added a commit that referenced this pull request Jul 29, 2015
Before this commit, users can configure excluded IPs by separating them with commas.

With this commit, users can configure them one per line. This is meant to be simpler and more consistent (see #8448). This is now also consistent with form inline helps. This change applies both to global settings as well as site-specific settings.

A migration script is included: it will replace commas by new lines.

Related: #7978, #8050
@tsteur
Copy link
Member

tsteur commented Aug 13, 2015

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

@tsteur tsteur closed this Aug 13, 2015
@tsteur
Copy link
Member

tsteur commented Aug 13, 2015

actually because of #8482

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants