Feedback

• the concept and solution looks good to me 👍
• there are a few unit tests failures: https://travis-ci.org/piwik/piwik/jobs/64028866
  • also some integration test failures but these could be random?

Would it make sense to have RequestSanitizer be just a Sanitizer? After all it isn't coupled to the request, it only sanitizes values (except for sanitizing API parameters and the blacklist).

Regarding putting the "blacklist" in the DI config, it works, but it's a bit "disconnected" from the controllers (i.e. I work on a controller, it's not easy to see if request parameters are escaped or not, which is a little critical). What about using an annotation for that? e.g.

class Controller
{
    /**
     * @Escape(false)
     */
    public function saveReport($name, $description, ...)
    ...
}

(could be @NoEscape or whatever else)

The other good thing that comes with that is that the Sanitizer would be entirely ignorant of Request/HTTP stuff and be a simple standalone service.

Would it make sense to have RequestSanitizer be just a Sanitizer? After all it isn't coupled to the request, it only sanitizes values (except for sanitizing API parameters and the blacklist).

The point of this class (which may not be mentioned in the issue description or class docs, the PR is still a WIP) is to centralize the logic so we can remove it easily in 3.0. We can call it Sanitizer, it's not really important to me what the name is. I went w/ RequestSanitizer since it is only used to sanitize query parameters in an HTTP request.

Regarding putting the "blacklist" in the DI config, it works, but it's a bit "disconnected" from the controllers (i.e. I work on a controller, it's not easy to see if request parameters are escaped or not, which is a little critical). What about using an annotation for that? e.g.

It is disconnected, and it is supposed to be disconnected. Using this class & setup, we can change one part of Piwik at a time to be "store unsanitized, sanitize on output" by adding the info to DI (maintaining existing behavior in other places and thus preventing unwanted xss vulnerabilities). Then, when we change the default handling of parameters to be unsanitized in 3.0, we can just remove the class and DI entries. If we use annotations or something else coupled with the plugins, we'd have to modify every API method and remove the useless annotation.

Also using DI means the code isn't a publicly available API, which is better in this case.

Finally, a quick note, this only affects APIs, so not Controllers (who don't actually get called w/ request parameters).

OK I see your point (it's all only temporary). FYI I am not very confident we will be able to fix this whole sanitization issue in 3.0 (and thus remove this class), I mean it's a huge-major thing with high risk of creating security issues (XSS) while refactoring… Somehow I think it won't be as fast as we would like to. That's why I was pushing with spending more time making this more "durable", but if you are confident then I don't want to slow you down :)

The ultimate goals is to do no sanitization on query parameters in API requests. That's a HUGE breaking change, so If we can't get to that point by 3.0, it will probably never happen. To make the process faster, maybe I can create an automated XSS test. The UI tests do this already to some extent, but it's certainly not complete. And maybe I can come up w/ a set of patterns for JS coding to avoid XSS (for example, some code builds HTML directly w/ the result of AJAX queries, this is not necessary w/ angularjs).

For an XSS test, we could potentially add <script>$('body').html('xss ?')</script> to every varchar/char/text/blob/etc. column we can find in the DB. Or just add the data by hand, and test that we've covered every potential table column. This would be hardening of what is currently done in UITestFixture. This would test having untrusted data in Piwik's DB.

I'm not sure how to test having untrusted input returned from Piwik (either because it is in the DB, or due to a man in the middle attack). We could fake XHR responses... but triggering specific code paths is impossible w/ the jquery soup much of the frontend consists of.

Note to merger: This PR could potentially introduce XSS, so it would be a good idea to do manual UI tests before merging.

For an XSS test, we could potentially add <script>$('body').html('xss ?')</script> to every varchar/char/text/blob/etc. column we can find in the DB.

Unfortunately automated tests for XSS is not realistic nor practical for now... we will need to manually check every instance of variable in all templates, and html generators, and manually sanitise the output for the context (js, html attribute, etc.). it's tedious work but needs to be done. Don't think we can do it by 3.0.0 as it's already huge scope https://github.com/piwik/piwik/milestones/3.0.0 but we shall discuss (#4231 is tentatively in 3.0.0)

Note to merger: This PR could potentially introduce XSS

I don't think we should rely on the reviewer to catch XSS but rather the original code author. You don't write in PR that you checked instances of the use of variables and check they are sanitised... did you, or is this PR really risky in this regard?

You don't write in PR that you checked instances of the use of variables and check they are sanitised...

I checked where the description property was used and where exception messages were used. Perhaps I checked every possible place, perhaps I missed something. Perhaps someone could double check.

I checked where the description property was used and where exception messages were used. Perhaps I checked every possible place, perhaps I missed something. Perhaps someone could double check.

Ok thanks for more details, good to know 👍

@diosmosis as you suspected, because this PR is quite complex, it didn't get merged in time. I guess it be a possibility to fix all sanitisations problems at once in #4231

Realistically I think this won't get merged. Instead we shall try to fix the issue at once everywhere, which should make things clearer and easier in the mid term.