[bug] CORS headers are not following specification

[akamensky]

This is copy of socketio/socket.io#1778

In my code:

io.origins('http://a.local:80');

If I do request from http://b.local:80, the response headers are:

400 Bad Request
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: http://b.local
Connection: close
Content-Length: 5
Content-Type: application/octet-stream
Date: Fri, 12 Sep 2014 04:24:46 GMT
Server: nginx/1.4.6 (Ubuntu)
Set-Cookie: io=***

Which is wrong because Origin header tells us that we actually are allowed there. This is also incorrect from specification point of view.
From www.w3.org:

The Access-Control-Allow-Origin header indicates whether a resource can be shared based by returning the value of the Origin request header, "*", or "null" in the response.

Same time the standard does not specify response code.

Possible solution:

• Either return Access-Control-Allow-Origin: null (as per specs), or completely drop CORS headers as it will trigger browser to display console error of missing header (latter is my preferred option)
• Suggestion is to change from 400 Bad Request to 403 Forbidden as it more precisely reflects actual status of response (request itself may be not _bad_, but it is not allowed there).

Please vote if you agree and I will provide PR for this issue.

[akamensky]

As per comments in socketio/socket.io#1778 I will provide PR based on 403 response code and not setting CORS headers at all

[mokesmokes]

@akamensky your PR #282 will cause this issue to reopen: #211
If we don't set CORS headers correctly (i.e. for an invalid request from a.local when a.local is indeed allowed) then we will get a CORS error when in fact the origin was valid. The way to handle this should be different in my opinion: perhaps origins should be set on engine.io and not socket.io (or passed from socket.io to engine.io), and in the engine.io verify function check the headers, act on CORS first and only if that passes do the rest of the checks.
EDIT: I'm convinced that origins should be an engine.io property. This is basic network configuration and not high level sugar.

[akamensky]

@mokesmokes From #211 it looks like this piece of code was made for the purpose of only easier debugging, while by introducing this code it in fact broke CORS specifications. IMO debugging of any application should not be done using CORS. It can be done from same domain.

The sole purpose of Access-Control-Allow-Origin is to show which domain/domains are allowed to access this resource. Therefore just writing error code and still sending this header is wrong as a developer who does not know about this trick in engine.io will think something is wrong with his application.

If it is really necessary to give ability to put this header even for not allowed domain - this can be done as optional debugging mode and leave standard (production) mode to follow specifications.

[mokesmokes]

@akamensky no, the purpose is not necessarily just for debugging. If a client makes a request which is valid from an origin standpoint, but invalid from an engine.io protocol standpoint (could happen in production due to many reasons), he should get a 4XX error and not get a response with screwed up CORS headers which will not enable the client app to understand the true nature of the error (which is what #211 complained about). If the CORS headers are wrong the engine.io error is not even seen by the client app (blocked by the browser).
A full and correct solution should be to move the origin handling from socket.io to engine.io where it belongs, and the engine.io verify function should verify the entire request - from a CORS perspective (first)and as well as engine.io protocol perspective.

[akamensky]

@mokesmokes Thanks for clarification. I agree that in case request coming from actually allowed origin must not get that response. For such request headers should be set correctly. However I believe that in such situations function of processing error and setting CORS headers should not be combined in one (which as it seem is the case here). CORS headers is a one things and processing error is completely different. I guess most correct approach is to move CORS headers handling into engine.io completely (as you proposed) and same time split headers setting and error processing into two separate functions.

I can look into that, however this will also take time as two packages need to be changed together to fix this thing.

[mokesmokes]

@akamensky whether to check CORS and protocol in one function or two is a matter of style. But the essence is the same: the request must be verified on both issues. Note it's not a "processing" issue but a protocol issue - e.g. verify checks that the request has a valid ID, etc. So yes, it needs to happen in engine.io and of course you also need to modify socket.io to account for this change.

[defunctzombie]

I am closing this as it seems no agreement is reached without potentially breaking something. Unless something is specifically broken here that we need to fix, I don't see an issue.