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

sanitizeInputValue() - improvements #2185

Closed
robocoder opened this issue Mar 14, 2011 · 10 comments
Closed

sanitizeInputValue() - improvements #2185

robocoder opened this issue Mar 14, 2011 · 10 comments
Labels
Bug
Milestone

Comments

@robocoder
Copy link
Contributor

@robocoder robocoder commented Mar 14, 2011

setDocumentTitle() expects an un-encoded string because piwik.js uses encodeURIComponent to encode parameters in the request.

@robocoder
Copy link
Contributor Author

@robocoder robocoder commented Mar 14, 2011

(In [4080]) fixes #2185

@mattab
Copy link
Member

@mattab mattab commented Mar 14, 2011

the sanitizeInputValue is called for all input values, generally very often, I think charset detection is pretty slow...

is the piwik.js fix not enough to get the page titles right?

@robocoder
Copy link
Contributor Author

@robocoder robocoder commented Mar 14, 2011

I'll rework it.

@robocoder
Copy link
Contributor Author

@robocoder robocoder commented Mar 15, 2011

(In [4087]) refs #2185 - revert r4080

  • for performance, move html_entity_decode out of sanitizeInputValue() since it's only used when we getRequestVar('action_name')
  • add unit tests

@robocoder
Copy link
Contributor Author

@robocoder robocoder commented Mar 15, 2011

(In [4092]) refs #2185 - sanitizeInputValue() returned '' if input wasn't valid UTF-8

@robocoder
Copy link
Contributor Author

@robocoder robocoder commented Mar 15, 2011

Re-think:

  • re r4087, moved html_entity_decode out of sanitizeInputValue(). Matt wonders if we should also use html_entity_decode for custom variables and referer.
  • re forum post, passing an already encoded URL results in double encoding.

@robocoder
Copy link
Contributor Author

@robocoder robocoder commented Mar 15, 2011

Attachment:
2185.patch

@robocoder
Copy link
Contributor Author

@robocoder robocoder commented Mar 15, 2011

The attached patch moves html_entity_code() back to sanitizeInputValue(), and tries to detect/fix double encoding.

I'll come back to this problem after I've thought more about the implications are.

@mattab
Copy link
Member

@mattab mattab commented Mar 15, 2011

do we need to handle this use case though? It has never been a problem so far, and I really don't want to complicate the sanitize function because it is heavily used, and security related. It must stay simple and fast. So I vote for updating the doc and clarify that we don't accept encoded values, and leave the sanitize as is on trunk (with your new test in the function)

@robocoder
Copy link
Contributor Author

@robocoder robocoder commented Mar 16, 2011

(In [4096]) fixes #2185

@robocoder robocoder added this to the Piwik 1.2.1 milestone Jul 8, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug
Projects
None yet
Development

No branches or pull requests

2 participants