couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benoit Chesneau (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (COUCHDB-431) Support cross domain XMLHttpRequest (XHR) calls by implementing Access Control spec
Date Thu, 25 Aug 2011 22:08:29 GMT

    [ https://issues.apache.org/jira/browse/COUCHDB-431?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13091354#comment-13091354
] 

Benoit Chesneau commented on COUCHDB-431:
-----------------------------------------

Thanks a lot for the review. I will update the patch later this morning
(12:07am here) . Comments follow.

1) In check_origin/2, when * is in the AcceptOrigins would it be better to return * so that
we send * in the "Access-Control-Allow-Origin" header? 

Since we return Access-Allow-Credentials header the spec says we should
retutn the origin. WIthout it anyway authentication won't work.

2) could you pre-split Origin in check_origin/2 before passing it to check_origin1/2 so that
we don't call mochiweb_util:urlsplit/1 repeatedly? 

ok

3) Makes sense to me to join check_origin/2 and check_origin1/2 into a single function. Use
function guards to convert the accepted origins as you need them and to find the "*" case,
avoids looping through the accepted origins twice (once for couch_util:to_list and once for
lists:member). 

The problem is that someone could have put "*" anywhere in teh list. We
want to test it first.


4) Typo in set_prefilight_headers/4, clause 2, capitalization in Access-Control-ALlow-Headers


thanks will fix it

The rest are mostly aesthetics: 

5) set_prefilight_headers -> set_preflight_headers typo (extra i) 

will change that

6) You catch the case where erlang:get/1 in cors_headers/0 returns undefined, but that's impossible
because you call set_default_cors_headers/0. It makes more sense to me to take out set_default_cors_headers/0
and just set the defaults in the undefined erlang:get/1 case. That means cors_headers/0 becomes
cors_headers/1 and takes a mochiweb request record. 

7) Move cors_headers/0 up near default_origin_headers/1 

ok

8) I would get rid of set_preflight_headers/4 entirely. In preflight_cors_headers create PreflightHeaders0
after you get Origin1. Add {"Access-Control-ALlow-Headers", FinalReqHeaders} to PreflightHeaders0
in the last clause of the case and let PreflightHeaders be the result of the case. just call
erlang:set/2 from there. 

mmm i like the separation in 2 funs here, easier to read, at least for
me. But will see

9) whitespace change at couch_httpd_db (-212, +247)

didn't notice, thanks


> Support cross domain XMLHttpRequest (XHR) calls by implementing Access Control spec
> -----------------------------------------------------------------------------------
>
>                 Key: COUCHDB-431
>                 URL: https://issues.apache.org/jira/browse/COUCHDB-431
>             Project: CouchDB
>          Issue Type: New Feature
>          Components: HTTP Interface
>    Affects Versions: 0.9
>            Reporter: James Burke
>            Assignee: Benoit Chesneau
>            Priority: Minor
>             Fix For: 1.2
>
>         Attachments: 0001-cors-support.-should-fix-COUCHDB-431-2.patch, 0001-cors-support.-should-fix-COUCHDB-431.patch,
0001-cors-support.-should-fix-COUCHDB-431.patch, A_0001-Generalize-computing-the-appropriate-headers-for-any.patch,
A_0002-Send-server-headers-for-externals-responses.patch, A_0003-Usably-correct-w3c-CORS-headers-for-valid-requests.patch,
A_0004-Respond-to-CORS-preflight-checks-HTTP-OPTIONS.patch, cors.html, test_cors2-1.tgz, test_cors2.tgz
>
>
> Historically, browsers have been restricted to making XMLHttpRequests (XHRs) to the same
origin (domain) as the web page making the request. However, the latest browsers now support
cross-domain requests by implementing the Access Control spec from the W3C:
> http://dev.w3.org/2006/waf/access-control/
> In order to keep older servers safe that assume browsers only do same-domain requests,
the Access Control spec requires the server to opt-in to allow cross domain requests by the
use of special HTTP headers and supporting some "pre-flight" HTTP calls.
> Why should CouchDB support this: in larger, high traffic site, it is common to serve
the static UI files from a separate, differently scaled server complex than the data access/API
server layer. Also, there are some API services that are meant to be centrally hosted, but
allow API consumers to use the API from different domains. In these cases, the UI in the browser
would need to do cross domain requests to access CouchDB servers that act as the API/data
access server layer.
> JSONP is not enough in these cases since it is limited to GET requests, so no POSTing
or PUTing of documents.
> Some information from Firefox's perspective (functionality available as of Firefox 3.5):
> https://developer.mozilla.org/en/HTTP_access_control
> And information on Safari/Webkit (functionality in latest WebKit and Safari 4):
> http://developer.apple.com/safari/library/documentation/AppleApplications/Conceptual/SafariJSProgTopics/Articles/XHR.html
> IE 8 also uses the Access Control spec, but the requests have to go through their XDomainRequest
object (XDR):
> http://msdn.microsoft.com/en-us/library/cc288060%28VS.85%29.aspx
> and I thought IE8 only allowed GET or POST requests through their XDR.
> But as far as CouchDB is concerned, implementing the Access Control headers should be
enough, and hopefully IE 9 will allow normal xdomain requests via XHR.

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message