scrapyd
scrapyd copied to clipboard
Sanitize project name
fixes #117
This fixes important security problem https://github.com/scrapy/scrapyd/issues/117 and also adds unit tests for webservice, increases coverage to 80%.
There is some discussion there in issue about allowing some characters, but notice that python won't allow you to build egg with illegal characters if you call your project name /etc/hosts and try to run python setup.py bdist_egg you'll get an error. This means IMO that there is no valid use case when someone sets project name to something that does not pass our validation. Which leaves us with only invalid cases, e.g. user errors or malicious attempts, for both we should reply with HTTP status 400 Bad Request. Regex here: https://github.com/scrapy/scrapyd/pull/421/files#diff-60f47eb24e330d5afdb3fd80dcee49bc31b957551483a97fa8edd59c222d59cbR27 is same as code in setuptools checking for valid package name https://github.com/pypa/setuptools/blob/d508e3841d3a40aeb602b0a9a883b227a27a3842/setuptools/_distutils/command/install_egg_info.py#L61
Probably it would be also good to make sure EggStorage is not storing invalid names. That's because resources can be configurable, so user can override our resource and put his own resource which is not safe. but I would prefer to do it on other pull request.
Another thing: version is already handled on eggstorage side, but we could also raise invalid request 400 HTTP code on invalid versions, but also would be nice to do on other PR, just from point of view of time management, to have separate issues, separate PR-s for separate things.
Codecov Report
Merging #421 (5980853) into master (2eb5409) will increase coverage by
0.24%
. The diff coverage is87.50%
.
@@ Coverage Diff @@
## master #421 +/- ##
==========================================
+ Coverage 79.63% 79.88% +0.24%
==========================================
Files 23 23
Lines 1051 1069 +18
==========================================
+ Hits 837 854 +17
- Misses 214 215 +1
Flag | Coverage Δ | |
---|---|---|
unittests | 79.88% <87.50%> (+0.24%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
scrapyd/webservice.py | 72.18% <86.27%> (+2.43%) |
:arrow_up: |
scrapyd/app.py | 91.37% <100.00%> (ø) |
|
scrapyd/utils.py | 88.61% <100.00%> (+0.38%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 2eb5409...5980853. Read the comment docs.
Tests unrelated to project name problem merged to master here: https://github.com/scrapy/scrapyd/commit/2eb5409ff2ed07e55543ed8babd58237780d29b7 this Pull Request will only contain sanitization of project name, to be merged later.