Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Obsolete S3FeedStorage instancing without AWS credentials #4411

wants to merge 4 commits into from


Copy link

@nyov nyov commented Mar 7, 2020

Question: does access_key is None and secret_key is None make sense here?
It's been the current behaviour, but is there a use-case for having no access_key but a secret_key? Otherwise this should possibly be or, instead of and?

Copy link

codecov bot commented Mar 7, 2020

Codecov Report

Merging #4411 into master will decrease coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4411      +/-   ##
- Coverage   84.78%   84.64%   -0.14%     
  Files         164      166       +2     
  Lines        9883     9862      -21     
  Branches     1469     1466       -3     
- Hits         8379     8348      -31     
- Misses       1248     1260      +12     
+ Partials      256      254       -2
Impacted Files Coverage Δ
scrapy/extensions/ 89.1% <100%> (+4.63%) ⬆️
scrapy/commands/ 75.36% <0%> (-8.25%) ⬇️
scrapy/utils/ 89.23% <0%> (-3.84%) ⬇️
scrapy/commands/ 26.66% <0%> (-1.91%) ⬇️
scrapy/spiders/ 98.43% <0%> (-1.57%) ⬇️
scrapy/utils/ 78.37% <0%> (-0.57%) ⬇️
scrapy/extensions/ 81.53% <0%> (-0.13%) ⬇️
scrapy/core/downloader/handlers/ 92.85% <0%> (-0.11%) ⬇️
scrapy/core/downloader/ 90.9% <0%> (-0.07%) ⬇️
scrapy/core/downloader/handlers/ 100% <0%> (ø) ⬆️
... and 14 more

Copy link

Gallaecio commented Mar 12, 2020

Actually, I wonder if we shouldn’t just remove the whole thing.

The previous code allows for the settings not to be defined at all, what it warns about is having them in settings. Your new code raises an exception in such an scenario, which is backward incompatible.

It looks like it’s possible not to use credentials at all:

Copy link
Contributor Author

nyov commented Mar 17, 2020

By "remove the whole thing", do you mean the whole S3FeedStorage? I think I've been advocating that in the past; well actually that was for S3FilesStore, I think. Moving those boto library users into a different project (as was done for scrapy-django-item) could "fix" this CVE-2017-14158.
But I have no strong opinion on that.

Copy link

Gallaecio commented Mar 17, 2020

I mean lines 94:112 of the first file.

Copy link
Contributor Author

nyov commented Mar 17, 2020

Ah okay :) That's easy enough. I can't say if it's correct or not as I don't use it. But I'll change it, so someone else can test the case without any credentials at all.

Copy link

kmike commented Aug 17, 2020

Thanks @nyov! Closed via #4688.

@kmike kmike closed this Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants