scrapyd icon indicating copy to clipboard operation
scrapyd copied to clipboard

Sanitize project name

Open pawelmhm opened this issue 3 years ago • 2 comments

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.

pawelmhm avatar Nov 29 '21 11:11 pawelmhm

Codecov Report

Merging #421 (5980853) into master (2eb5409) will increase coverage by 0.24%. The diff coverage is 87.50%.

Impacted file tree graph

@@            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.

codecov[bot] avatar Dec 03 '21 09:12 codecov[bot]

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.

pawelmhm avatar Dec 14 '21 17:12 pawelmhm