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

s3boto: saving media leads to infinite loop since 1.12 #1084

Closed
syphar opened this issue Oct 25, 2021 · 4 comments · Fixed by #1085
Closed

s3boto: saving media leads to infinite loop since 1.12 #1084

syphar opened this issue Oct 25, 2021 · 4 comments · Fixed by #1085

Comments

@syphar
Copy link

syphar commented Oct 25, 2021

We discovered an issue with django-storages 1.12 where saving new files to S3 ends up in an infinite loop.
1.11.1 still works fine. A bisect between the releases pinpoints the problem to 3222c23 reverting this commit on master fixes the issue for us.

What could be the issue here? It looks like Storage.save in Django uses get_available_name, which uses exists, which is why our new file-save calls this method.

The error S3 returns here is:

{'Error': {'Code': '403', 'Message': 'Forbidden'},
 'ResponseMetadata': {'HTTPHeaders': {'content-type': 'application/xml',
                                      'date': 'Mon, 25 Oct 2021 15:25:12 GMT',
                                      'server': 'AmazonS3',
                                      'x-amz-id-2': '(redacted)',
                                      'x-amz-request-id': '(redacted)'},
                      'HTTPStatusCode': 403,
                      'HostId': '(redacted)',
                      'RequestId': '(redacted)',
                      'RetryAttempts': 1}}

Which ends up leading to True for exists(), so Django searches forever for a new filename.

I don't know enough about the reasoning behind 3222c23 to know what the solution here is. What information do you need? More about the config? Is our bucket configuration the problem?

I'm happy to provide a PR with the fix, when I know what the possible solution could be.

@jschneier
Copy link
Owner

403 is some kind of permissions error so it seems like a misconfigurartion.

In general it doesn’t seem like returning either is the correct thing to do, maybe the error needs to be re-raised.

@syphar
Copy link
Author

syphar commented Oct 26, 2021

403 is some kind of permissions error so it seems like a misconfigurartion.

I digged further and I tracked it down to this part of the boto documentation

A HEAD request has the same options as a GET action on an object. The response is identical to the GET response except that there is no response body. Because of this, if the HEAD request generates an error, it returns a generic 404 Not Found or 403 Forbidden code. It is not possible to retrieve the exact exception beyond these error codes.

and

You need the relevant read object (or version) permission for this operation. For more information, see Specifying Permissions in a Policy . If the object you request does not exist, the error Amazon S3 returns depends on whether you also have the s3:ListBucket permission.

  • If you have the s3:ListBucket permission on the bucket, Amazon S3 returns an HTTP status code 404 ("no such key") error.
  • If you don’t have the s3:ListBucket permission, Amazon S3 returns an HTTP status code 403 ("access denied") error.

So adding s3:ListBucket to the policies would solve the issue. Is there already a page in the docs about needed permissions in the access policies? This would be important to add to prevent the same question coming up again.

In general it doesn’t seem like returning either is the correct thing to do, maybe the error needs to be re-raised.

There I agree, this would have been the better experience.

@syphar
Copy link
Author

syphar commented Oct 26, 2021

Ah wait, I already found the docs about this: https://django-storages.readthedocs.io/en/latest/backends/amazon-S3.html#iam-policy

So what remains is making this an actual error

@jschneier
Copy link
Owner

@syphar can you try out #1085

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants