Skip to content

Commit

Permalink
Removal of CORS for API on 9.x
Browse files Browse the repository at this point in the history
  • Loading branch information
@Fmstrat
Fmstrat committed Mar 11, 2016
1 parent fef71b6 commit b0254fc
Showing 1 changed file with 0 additions and 11 deletions.
11 changes: 0 additions & 11 deletions controller/ownnoteapicontroller.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ public function __construct($appName, IRequest $request){

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function index() {
Expand All @@ -48,7 +47,6 @@ public function index() {

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function mobileindex() {
Expand All @@ -58,7 +56,6 @@ public function mobileindex() {

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function remoteindex() {
Expand All @@ -68,7 +65,6 @@ public function remoteindex() {

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function create($name, $group) {
Expand All @@ -79,7 +75,6 @@ public function create($name, $group) {

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function del($name, $group) {
Expand All @@ -90,7 +85,6 @@ public function del($name, $group) {

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function edit($name, $group) {
Expand All @@ -100,7 +94,6 @@ public function edit($name, $group) {

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function save($name, $group, $content) {
Expand All @@ -111,7 +104,6 @@ public function save($name, $group, $content) {

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function ren($name, $group, $newname, $newgroup) {
Expand All @@ -122,7 +114,6 @@ public function ren($name, $group, $newname, $newgroup) {

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function delgroup($group) {
Expand All @@ -133,7 +124,6 @@ public function delgroup($group) {

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function rengroup($group, $newgroup) {
Expand All @@ -144,7 +134,6 @@ public function rengroup($group, $newgroup) {

/**
* @NoAdminRequired
* @CORS
* @NoCSRFRequired
*/
public function version() {
Expand Down

9 comments on commit b0254fc

@BernhardPosselt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gz, now your app is vulernable to CSRF ;)

@butcher211
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BernhardPosselt Why?

@BernhardPosselt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because CORS annotations explicitely disallow session authentication which is the root issue of CSRF (without csrf token). There's a ticket in core which I can't work on anymore due to working a full time job which would introduce a new external API interface which does this out of the box. The hotfix for now was to disallow session auth when using @cors (since its an API annotation). I don't know why it's not yet in the docs, but here's the code https://github.com/owncloud/core/blob/master/lib/private/appframework/middleware/security/corsmiddleware.php#L85

@Fmstrat
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I'm not sure what to change here. I feel like every new OC release I've had to change something with the API, which is really frustrating since I'm in the same boat as you with a full time role.

Is it possible to include CORS in ownNote with this outstanding in core?

@BernhardPosselt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, just revert the commit and add back the annotations.

@Fmstrat
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this happens in OC 9: #286

@BernhardPosselt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since 8.2.3, same issue for news app. Could also be an android lib issue that tries to use cookies no matter what

@BernhardPosselt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also does not happen for everyone

@BernhardPosselt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce it btw

Please sign in to comment.