Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

sanitizeInputValue() - improvements #2185

Closed
robocoder opened this Issue · 10 comments

2 participants

Anthon Pang Matthieu Aubry
Anthon Pang
Collaborator

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

Anthon Pang
Collaborator

(In [4080]) fixes #2185

Matthieu Aubry
Owner

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?

Anthon Pang
Collaborator

I'll rework it.

Anthon Pang
Collaborator

(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
Anthon Pang
Collaborator

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

Anthon Pang
Collaborator

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.

Anthon Pang
Collaborator

Attachment:
2185.patch

Anthon Pang
Collaborator

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.

Matthieu Aubry
Owner

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)

Anthon Pang
Collaborator

(In [4096]) fixes #2185

Anthon Pang robocoder added this to the Piwik 1.2.1 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.