search

joshkosmala / silverstripe-tenon

Check the accessibility of your SilverStripe site with this module that integrates with Tenon
Other
16 stars 3 forks source link

Does not check for URL domain #2

Closed chillu closed 9 years ago

chillu commented 9 years ago

Hey Josh, great module, that's going to come in handy very soon for us! I haven't installed it yet, but looking through the code it appears that there's no domain checks around the URL submitted to tenon. Combined with the fact that the controller endpoint does not require authentication, does this mean anybody can use a website with tenon installed to fire off checks to random URLs and use up quota for the installed API key? You could also use this for (limited) DDos attacks, since the CURL call is synchronous and potentially long-running. I would recommend to limit tenon runs to sessions where a CMS author is logged in.

camfindlay commented 9 years ago

@leighelse good practice is to ensure you leave at least a trail to your improvements when suggested by the community.

Is the issue fixed in https://github.com/joshkosmala/silverstripe-tenon/commit/91b5d47f49aec5a32ebc3705573e774aa5b1dc09 ? @chillu ?

Another option would have been to raise a pull request with the fix which helps improve the peer review process :)

chillu commented 9 years ago

Thanks Leigh! That fixes the most pressing issue (URLs from other domains), not the authentication though. As a developer looking into using this module, leaving the controller endpoint open to the world just introduces unnecessary security surface in my opinion.

camfindlay commented 9 years ago

@leighelse what I think @chillu is referring to is that adding the controller for the ajax can be triggered by anyone. Perhaps the js and the ajax controller can only fire by a CMS user (unless having the public trigger the calls to Tenon api is the intension?).

leighelse commented 9 years ago

The intention was indeed that the first view of any updated page should trigger a Tenon review. We could make that configuration-dependent if doing so would meet a wider range of use cases. For us, completely automating the accessibility reports makes it an "install and forget" solution, and ensures the reports are there whenever they're required.

leighelse commented 9 years ago

The controller has now been locked down to a single method, which reduces the liability.