Add cors middleware #8504
Add cors middleware #8504
Conversation
|
Sorry but we don´t want to dumb more libraries in the ownCloud core only because they might be useful. Apps are free to ship components like that if they want to use it. At the later stage if 4-5 apps ship the same component in the exact same version then we might consider to add the to the ownCloud core, or not. |
|
Well i see a need in particular for the music app that also exposes an API (shiva), concering other apps thats why im currently mentioning them at the top. |
|
I certainly see a reason for a properly implemented CORS middleware. Such security related components shouldn't get implemented by every single application because it's too easy to do it wrong (TM). As this middleware in the AppFramework namespace it shouldn't hurt core itself too much ;-) That said, I didn't took a proper look at the source yet but if @karlitschek reconsiders his vote I'm more than happy to review this properly together with @Raydiation. |
|
Without the test its basically 15 lines of code and does not affect any other part of owncloud btw. |
|
It does affect us because if it is in core then we always have to maintain it, fix potential security things, maintain compatibility because it is part of the ownCloud public app api then. And so on. So adding things like that comes with a cost. Please use it in the apps directly for now. And if it becomes common practice that everyone is using it then we can check if it is worth to save a few bytes of code in ownCloud and add it to the core. At the moment there is no need. |
|
@karlitschek can i leave the PR open until enough people vote for it in here then? |
|
Mhm, this middleware and the one in notes/news allows any website to access the API. This is very bad. |
And this is exactly why we should have this in core. The proposed solution suffers from a security flaw as Without this pull request this would most likely also propagated into other apps (e.g. the music player). Thanks to it we had the chance to review this and discuss the security implications together. If even @Raydiation makes this fault it's likely that others are doing it to and we should try to prevent that. With a centralised solution we can at least try to make the life easier for developers so that they won't unintentional introduce critical security bugs. Regarding the fact that this is really not much code and that unit tests are provided I'd advocate for the inclusion of this into core. |
|
guys, if there are security problems in apps then the next step is to fix them. this is a way later discussion if we include _another_library in the owncloud core which should be small and not bloated |
|
@karlitschek middleware is now private and always registered and prevents CSRF via web api if an app implements cors by itself. Apart from that it should offer a sane/secure (needs review) default way to do this PS: Test fail because I need a previous PR to go in beforehand because I cache an expensive operation now which speeds up execution |
|
@LukasReschke please review if the current implementation is safe :) (will fix it for notes and news afterwards then) @karlitschek is the appstore problem fixed? I can't release a security update otherwise |
|
To adress the 'bloat' argument: CORS is the only way to make ownCloud API's work on web plattforms such as:
Since ownCloud is essentially a giant API that is used to sync stuff across devices I think providing a default implementation for CORS is at least as valid of an argument as providing a default CSRF and OAuth implementation. Leaving this to app developers (like me) will lead to potential security problems (like news and notes). Adding this to core will allow us to make it easy for developers to make their ownCloud API available on web platforms and allow us to further spread the acceptance of ownCloud as a synchronization service alternative. The current implementation is one additional function that gets called, and only called if you use @cors (which would happen anyways if you do it on your own) and ensures that the request is handled in a secure manner. In addition there is a default controller method implementation that allows you to define your access rules (headers, verbs) for each resource seperately. |
| $middleware = new CORSMiddleware($request, $this->reflector); | ||
|
|
||
| $response = new Response(); | ||
| $response->addHeader('AcCess-control-Allow-Credentials ', 'TRUE'); |
There was a problem hiding this comment.
I dont know how browsers handle http headers, I think theyre treatet in a case insensitive way, thats why this test tries to that edge case ;)
|
This seems a very important part of making it possible for external app developers (especially web apps) to integrate with ownCloud. Also see the »ownCloud as a backend for web apps« concept at https://github.com/owncloud/core/wiki/Project-ideas#owncloud-as-backend-for-web-apps |
|
@Raydiation wrote:
Yes! I fully agree. Cross-origin user data is very important indeed. I'll help you in any way I can to add cors headers to your API. I can mentor a GSOC student for instance, or come to a hackathon to work on it, or whatever is useful. |
|
I'm not saying this is not useful, secure, good code or in other ways helpful. But this doesn't mean that we should dump all useful code into the core. The design goal of the owncloud core is to be as small as possible. In fact it is already too big. So if this is useful then we should use it for the news app API first and perhaps other apps. This doesn't mean that we have to dump it into the core to make it heavier, difficult to maintain and to keep stable |
|
@karlitschek I'm fully aware of that. For me personally it is in fact easier to maintain my own version since i don't have to go through the whole PR process for adding and maintaining the feature. BUT: If I propose a feature via a PR I expect from the reviewer to seriously look at the problem and give me proper feedback. I don't want to start a basic discussion about code in general and why less code is better than more code, I want to solve a problem or share things with fellow developers. Now if, in any case, you'd depend or use the App Framework for any of your apps I'd be very grateful for your opinion and if you think it would fix your problem, if you think this should be easy enough and trivial enough to not do it in core and let it implement each individual app author. But this kind of argumentation will get us nowhere and is VERY frustrating for me as a contributor. If you don't want to read into the code or documentation by yourself, please let someone else that you trust take a look at it. |
|
As in please dont: but rather: Edit: If of course you're interested in it |
|
💣 Test Failed. 💣 |
|
As it has been - wisely - decided to move the AppFramework to core, I am interested in having it both lean, but more importantly usable and secure. Therefore I prefer to have great implementations which are closely scrutinized for security issues. |
remove methodannotationreader namespace fix namespace for server container fix tests fail if with cors credentials header is set to true, implement a reusable preflighted cors method in the controller baseclass, make corsmiddleware private and register it for every request remove uneeded local in cors middleware registratio dont uppercase cors to easily use it from routes fix indention comment fixes explicitely set allow credentials header to false dont depend on better controllers PR, fix that stuff later split cors methods to be in a seperate controller for exposing apis remove protected definitions from apicontroller since controller has it
|
🚀 Test Passed. 🚀 |
|
In case my point didn't come clearly thru, as I may sometimes try to soften my views when putting them into words.
Recommendable, but what does that exactly mean? What is not needed in core? archive, migration, memcache, ocs, vobject, activitymanager, api, avatar, backgroundjob, cache, contactsmanager, tags, eventsource or files for that matter? Plus everything else under /apps. Fine, lets scrap it and start from scratch. I can actually mention a few that I wouldn't be missed (including some of my contributions), but that's another matter. Design-wise it would probably be good, but we would be left with basically nothing. Some of the good decisions that have been made during the past ~1½ year - from my PoV - are
Those - and other - changes can make it possible to actually move towards a mature, trustworthy codebase. [*] With exceptions of course. No one mentioned, no one forgotten. PS: I've tried to keep this rant in a sober tone, although my state might not be so (otherwise I probably wouldn't have written it) and I expect any responses to it to be made in public. |
|
@tanghus Thanks a lot. I don´t think this has anything to do with Inc. or anything else. |
|
O.K. Let´s put this into core under this conditions:
Additionally we should also regularly check which code, frameworks or dependencies in core can be removed. Otherwise we run into a long term problem. |
|
Reasonable and preferable.
I believe that should be the case for oC7 or oC8. Longterm dev is of essence.
I think this should be rephrased to "there must always be an active, responsible maintainer." which is documented in the source file. I don't know if there's a
Meaning what? If it's correct it doesn't have to change. This correlates with the maintainer or other developers related to this.
Agreed.
This should be addressed in a separate issue and I'm sorry @Raydiation that I kinda hijacked this issue for that. |
thanks a lot
In KDE for example is the rule that a library has to be used by at least two independent apps before it can go into kdelibs. I think this makes sense. It is not enough to just merge it into core and then later depend on someone else to actually use it. This hould be done in parallel also as a reality check to see if the API works.
O.K. Great. And welcome to the wonderful world of being a maintainer. We have to feel responsible for the software and also fix bugs from other people. ;-)
Sorry. Formating fuck up. :-)
Sure. It is fine to keep old code for compatibility reasons. But a commitment to keep on using this code and porting other pieces would be nice of course |
To be very forthright I'm not not sure which ownCloud you mean here? Is it to be sure independent developers can produce efficient and secure code, or is it to secure that the Inc. have less dependencies when they use their paid work force to make solid apps for paying customers? @Raydiation again for misusing this PR. |
|
@tanghus I´m not really sure what you mean here. Like I said this has nothing to do with Inc. or anything else. |
| private $request; | ||
|
|
||
| /** | ||
| * @param string $request the name of the method that will be called on |
There was a problem hiding this comment.
Shouldn't this be a IRequest instead of a string?
|
A new inspection was created. |
|
The inspection completed: 8 new issues, 28 updated code elements |
|
🚀 Test Passed. 🚀 |
|
👍 once the mentioned requirements are fulfilled. |
|
As for the two independent apps: Notes and News will depend on this. The music shiva API should also benefit from this, that is if @MorrisJobke wants to make the API work with web applications. Any requirements left? |
|
Another reviewer is left ;-) |
|
👍 from me as discussed |
|
Sorry. I didn't get to read thorugh the whole thread. I like to have a security-wise review of such critical code. I will also use this for shiva and the other parts of the API because @jbtbnl started to build a mobile app (with phonegap AFAIK) which will need to have this. Thanks @Raydiation |
Guess who did that ;-) |
That's what I meant. ;) I read through the discussions and would also like to have that in core. |
|
Ahh - misunderstood you. Sorry 🍺 |
|
Thank you everyone! :) |
This adds another middleware that adds a preflighted CORS request to your base controller class and enforces strict security for CORS requests in your app to not allow CSRF
To make your controller method respond to a CORS request, extend from ApiController and simply annotate it with @cors
To enable the preflighted CORS OPTIONS request for all urls starting with /api/v0.2/, add something like this to your routes where ApiController is present
CORS is used to allow webapps and websites to access your API methods and can be a potential threat for CSRF.
More information on that https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS
@LukasReschke @MorrisJobke @LEDfan @DeepDiver1975 @tanghus @cosenal