Mopidy: Missing CORS response header for static file assets

[weilbith]

Describe the bug
The static file handler of the http plugin is missing the extra headers for Access-Control-Allow-*. This is already correctly done for the JSON-RPC handler.
The issue is that is not possible to fetch static assets due to CORS issues even the request origin is whitelisted.

How to reproduce
Make sure that the domain you are using is whitelisted in the configuration:

[http]
allowed_origins = localhost:8080

Launch the client, open the console and try to do a fetch of an asset you know:

fetch('http://localhost:6080/local/feecccb956b1764b8245244611a61e15-600x600.jpeg')

This will give the following type error:

Access to fetch at 'http://localhost:6680/data/local/feecccb956b1764b8245244611a61e15-600x600.jpeg' from origin 'http://localhost:8080' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

Expected behaviour
Cross origin requests should work for static assets if the request origin is whitelisted.

Environment
Please complete the following information:

• Operating system: 5.9.4-arch1-1
• Running Mopidy as a service or in the terminal? service
• Your config (output of sudo mopidyctl config): command fails
• Software versions (output of sudo mopidyctl deps): command fails

Additional context
I think the issue should be quite easy to solve by extending the set_extra_headers() function of the StaticFileHandler as it is already done here. Is that correct?

[kingosticks]

Unfortunately the Access-Control-Allow-Origin header only allows a single value. It gets a bit trickier when we want to support multiple values from our configured list:

Limiting the possible Access-Control-Allow-Origin values to a set of allowed origins requires code on the server side to check the value of the Origin request header, compare that to a list of allowed origins, and then if the Origin value is in the list, to set the Access-Control-Allow-Origin value to the same value as the Origin value.

And it's even worse because, as I understand it, you can't rely on their being an Origin request header unless it's a CORS request. And you also can't rely on there being a Referer header for none HTTPS requests (which would be the alternate header to use). This is what led to the quite complicated CSRF protection code that ensures everything does a preflight CORS dance (note: I think this has since changed and all browser POSTs now get an Origin header?). Doubt we want that here.

Since the StaticFileHandler only deals with 'safe' GET requests, maybe it's fine to just set response header "Access-Control-Allow-Origin: *"?

[kingosticks]

Thinking about this more (despite not wanting to), it's safe to say that any browser concerned with upholding the CORS policy will have sent an Origin header. So we can just conditionally do check_origin() based on the Origin header being set, and then if allowed, copy the header value to the Access-Control-Allow-Origin response header like usual. If no Origin header, our behaviour can be unchanged.

[weilbith]

Thinking about this more (despite not wanting to)

I'm sorry. :see_no_evil:

---

Okay, so what is now so special about the static assets? Why can't we just re-use all the cool logic that already exists. I mean all the checks for the origin header etc pp are already there if I'm not mistaken. Can't we make use of it an conditionally set the headers? :thinking:

[kingosticks]

Only that GET requests and CORS policy is not the same as CRSF protection for 'unsafe' methods with side-effects like POST. They are different problems and the solution is not exactly the same.

But we can reuse most of it yes, that's what I was trying to say in my 2nd message.

[kingosticks]

Did my last message make sense? Do you fancy implementing the fix or shall I add it to the list of things to do?

[weilbith]

Yes it make sense. :smiley:

Would be cool if you could add it to the list. Although I was able to look into the code and get an understanding whats missing, I have too less knowledge of the framework that gets used here and what I would have to take care of. Sorry. :see_no_evil:

[kingosticks]

I think it's quite an isolated fix but yes, does require a look at Tornado so no worries, thanks for reporting it.