openannotation / annotateit

The web application running AnnotateIt
http://annotateit.org
Other
27 stars 11 forks source link

Access-Control-Allow-Credentials #9

Closed tilgovi closed 12 years ago

tilgovi commented 12 years ago

I think this header should be set to false. At worst, you could accidentally respond to requests that have been improperly exposed via CORS that you would not want a consumer to make (such as requesting account details). Another reason is that ACAC with ACAO=* is forbidden by the spec. Granted, no browser should be making a cross-domain request without an Origin header, and non-browser clients that might access these endpoints maybe won't care about this detail, but it's nevertheless not the right combination of headers.

Perhaps this relates to #7 in that, even if cookie authentication is added for the annotator, cookies should only be used for requests where the consumer is annotateit.

nickstenning commented 12 years ago

Yep, I think you're right. It should only be set to "true" for the annotateit-specific token generator endpoint, and we should enforce a hard requirement on the Origin header for CORS requests.

The use case for #7 is (I think) the ability to browse the API. We can satisfy this simply by refusing to send any CORS headers if Origin is missing, and if it is missing, attempting to authenticate the user's cookies.

tilgovi commented 12 years ago

I would leave the * default for Origin if you intend to have non-browser clients accessing the API.

The best argument for enforcing Origin would be if you tied the consumer key to specific origins, as ReCAPTCHA and many OAuth providers do with their consumer keys. E.g., you make an account and then request a key that's valid for X.Y.Z domain, at which point the backend would enforce the origin for that.

I think it's probably not necessary to enforce the existence of an Origin header, especially if cookie credentials are turned off.

nickstenning commented 12 years ago

There are two different places where ACAC is mentioned:

1) In the annotator-store. Here I agree there should be no hard requirement on Origin, but ...Allow-Credentials should be false or unspecified.

2) In the annotateit token generator. In this case, ...Allow-Credentials must be true, otherwise the token generator can't function, so according to the spec I must place a hard requirement on the Origin header being set.

I think we're in agreement, right?

tilgovi commented 12 years ago

Yep! On Mar 22, 2012 1:25 AM, "Nick Stenning" < reply@reply.github.com> wrote:

There are two different places where ACAC is mentioned:

1) In the annotator-store ( https://github.com/okfn/annotator-store/blob/master/annotator/store.py#L25). Here I agree there should be no hard requirement on Origin, but ...Allow-Credentials should be false or unspecified.

2) In the annotateit token generator: https://github.com/okfn/annotateit/blob/master/annotateit/main.py#L61. In this case, ...Allow-Credentials must be true, otherwise the token generator can't function, so according to the spec I must place a hard requirement on the Origin header being set.

I think we're in agreement, right?


Reply to this email directly or view it on GitHub: https://github.com/okfn/annotateit/issues/9#issuecomment-4634290

nickstenning commented 12 years ago

Oh, and on 2): I'm pretty sure this is enforced by conforming browsers already.

nickstenning commented 12 years ago

Fixed and pushed. annotateit v2.1.2 and annotator-store v0.7.2.