remove the 'public-read' acl as the default.

[clarkie]

Prefer to use bucket policy permissions over acls.

I have some permissions set at at a bucket level and don't want my uploading application to be concerned about the permissions for a given file. That should be managed elsewhere.

This still leaves in place the ability to override permissions with a file level acl.

[clarkie]

I'd suggest this is a major version bump.

[seanadkinson]

@clarkie Looks like you could get what you want by specifying uploadRequestHeaders={}, since it will only apply the public-read acl if those parameters aren't specified.

Are you suggesting that we get rid of the public-read as just a general best practice? Makes sense to me, but this library has provided public-read by default this whole time, so yes, we'd need a major version bump, and I just wonder if it is worth it, since there is a mechanism to get the behavior you want.

Thanks for the PR regardless!

[clarkie]

I'm happy to take that approach but it does feel a bit wrong to be explicitly setting acls by default. I'm not sure what the AWS best practise says for this but I'd rather handle security globally than granularly by default.

Also, to someone coming across the code I don't think uploadRequestHeaders={} means 'use the default permissions'.

[seanadkinson]

I agree the ability to change the default isn't well documented, and someone would need to look at the source code. But to remain backward-compatible, I'd love it if this PR could provide the default existing behavior, and just allow it to be overridden easily to do what you want. So maybe another option labeled useBucketAcl, which if true, would not set the acl header. Then existing users still get the same behavior, and hopefully it is clear to see how to NOT set the public-read by default.

[seanadkinson]

Just thinking of other solutions that wouldn't break existing usage. If you have other ideas, please let me know.

[clarkie]

That could be an option I guess but I think it's worth updating the library to match 'modern' aws standards:

What about S3 ACLs?

As a general rule, AWS recommends using S3 bucket policies or IAM policies for access control. S3 ACLs is a legacy access control mechanism that predates IAM. However, if you already use S3 ACLs and you find them sufficient, there is no need to change.

http://blogs.aws.amazon.com/security/post/TxPOJBY6FE360K/IAM-policies-and-Bucket-Policies-and-ACLs-Oh-My-Controlling-Access-to-S3-Resourc

As long as it goes with a major version bump and some docs explaining the change I don't see that being a problem.

[eqyiel]

This is not quite right because undefined is not truthy: #54 (comment)

If you want to avoid setting the public-read ACL, you need to do this: uploadRequestHeaders={{}}

[PatSmuk360]

I'm trying to avoid having to add PutObjectAcl to the service's S3 role, but unfortunately it looks like the router always adds an ACL parameter to the signing request. This should probably be optional as well.

[tjdavey]

In 2021 this issue is still unpatched and results in the component producing a default insecure configuration. How many people are using this component without checking the defaults and have inadvertantly exposed files in their bucket to the public?

@seanadkinson I realise this is an outdated PR, would you accept a new PR (or an update to this one), and a major version bump to patch this today?

[seanadkinson]

@tjdavey I think your concern is quite valid, and I hope this library hasn't caused too much headache for folks not paying attention to the defaults (it was originally created as an Avatar upload for user profiles, so public ACL made sense for us).

Unfortunately, I no longer work for this company, and my Write permissions have been revoked, so sadly, I can't actually merge any changes, and I'm not sure how to reach anyone who can (the company was acquired, and I don't know anyone still working there). If any Admins are reading this, and want a maintainer, I'm happy to pick back up the reins, but if not, this repo may be dead unfortunately @tjdavey.

[hexpunk]

Whoops. I opened #246 before I looked to see what other PRs were around.