fix CORS on s3 #92

wants to merge 3 commits into


None yet

2 participants

dstrek commented Apr 3, 2013

Currently putBucketCors fails because xml builder throws a TypeError and getBucketCors fails because cors isn't included in the signers subResources.

This patch addresses these issues.


Can you fix the whitespace on this (make sure to remove tabs)? Thanks!


I'm curious why this behaviour is necessary. If we are not finding the rules, that seems like a bigger issue and we shouldn't mask it like this.

dstrek properly fix the s3 CORS api
added AllowedHeaders
fixed the api service instead of monkey patching the xml builder
dstrek commented Apr 4, 2013

Ok I have fixed it, the s3 API definition was incorrect so that's why xml was failing to build, removed the monkey patch.

I also added AllowedHeaders to CORS as defined in the spec.

lsegal commented Apr 4, 2013

Thanks, this makes much more sense. I'll work on some integration tests and merge this if everything looks good.

dstrek commented Apr 4, 2013

yeah that needs a test, as it was passing tests with the wrong xml just then :(

dstrek commented Apr 17, 2013

Is there something I can do to help push this along?

@lsegal lsegal pushed a commit that referenced this pull request Apr 17, 2013
Loren Segal Add tests for #92 and update output members 58f1791
lsegal commented Apr 17, 2013

I merged this via eb3b8ea and then added some tests and fixed a missing output change to getBucketCors in the above commit. Sorry about taking so long for this, had to find some time to write tests.

Thank you for the contribution!

@lsegal lsegal closed this Apr 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment