Skip to content

Draft proposal of allowed origins cors list - #37896

Closed
aleixq wants to merge 8 commits into
nextcloud:masterfrom
Communia:cors_allowed_origins
Closed

Draft proposal of allowed origins cors list#37896
aleixq wants to merge 8 commits into
nextcloud:masterfrom
Communia:cors_allowed_origins

Conversation

@aleixq

Copy link
Copy Markdown

Summary

This is a WIP draft PR, that partially implements allowed origins list, to not open everywhere the nextcloud ocs and external api, and allowing to open some parts as the interaction will be limited. This is planned in a way that the desired endpoints will use the apicontroller instead of the base controller, so they can use the inherited preflighted_cors method.

When using so and adding the annotation @cors in desired method, filter will be done, The definition of the allowed origins will be done in system and in the (to be defined) app in a general and in a app manner, so for example in case of files_sharing will check in config.php for list of allowed_origins, then in (tbd) app settings, then will check also in config.php the value of files_sharing.allowed_origins and in (tbd) app settings the value of files_sharing.allowed_origins.

If http_origin header matches any entry in these settings will allow the use of api from this origin url.

As example I am allowing the preview endpoint only but it culd be opened to other endpoints easily, just grab the code and define

 'allowed_origins' => array(
'https://example.com',
),

in config.php

then check if you can get a preview when calling from the allowed origin url (a simple js fetch can be done).

The frontend to control allowed_origins is yet to be done depending on how accepted is this draft or if you think it's a bad and ugly way of achieving the filtering.

todo in this pr

  • Tests (unit, integration, api and/or acceptance) are included
  • Screenshots before/after for front-end changes
  • Documentation (manuals or wiki) has been updated or is not required

@szaimenszaimen added the 2. developing Work in progress label Apr 23, 2023
@szaimenszaimen added this to the Nextcloud 27 milestone Apr 23, 2023
@aleixq

Copy link
Copy Markdown
Author

Adding an application to be used as frontend to set the allowed origins, it's in separated repo at: https://github.com/Communia/cors_origin_filter_settings . I am marking this PR as it needs review right now although it needs to add and modify tests, let me know if it's better to set it back to DRAFT.

@aleixq
aleixq marked this pull request as ready for review April 28, 2023 10:49
Signed-off-by: Aleix Quintana Alsius <kinta@communia.org>
@szaimenszaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 29, 2023
@vitormattos

Copy link
Copy Markdown

Hi @aleixq is this PR WIP, as mentioned at body of descriptio?

If is WIP, I think that would be good convert to draft and change the label to 2. developing
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request

@aleixq

Copy link
Copy Markdown
Author

Thank you for caring, vitor. Yes, the code could be reviewed and its main idea evaluated:

  • Adding a way to control the origins which can interoperate with nextcloud endpoints and how it's achieved: applying whitelists to all @cors annotated controller methods.

But there are parts not done yet:

  • Tests.
  • Documentation.
  • Also, as there will be more control over the origins, opening more endpoints could be proposed (like search endpoints, the activity api or everything that could be used by a 3rd party web) as it's already done in this pr with preview endpoint just to show how it could be done.

Just looking for feedback before continue the work on this. As I don't want to waste everyone's time with a lot of code if it's futile. Said this, if it's better to set back to draft until everything is done, no problem.

This was referenced May 3, 2023
@blizzzblizzz mentioned this pull request May 17, 2023
@blizzzblizzz modified the milestones: Nextcloud 27, Nextcloud 28May 23, 2023
@skjnldsvskjnldsv mentioned this pull request Nov 1, 2023
@blizzzblizzz mentioned this pull request Nov 6, 2023
Signed-off-by: Aleix Quintana Alsius <kinta@communia.org>
@aleixq
aleixq requested a review from Altahrim as a code ownerJanuary 14, 2024 19:46
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsvskjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsvskjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsvskjnldsv mentioned this pull request Aug 13, 2024
@skjnldsvskjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@aleixq

Copy link
Copy Markdown
Author

@skjnldsv I suppose that is closed in favor of #40537 , these are good news! btw a simple comment to note this would help understand the close.

@skjnldsv

Copy link
Copy Markdown
Member

@aleixq thanks for the ping!
Yes and no! We did a cleanup pass on old PRs that looked stale. I wasn't able to comment on all of the, but nothing being definitive on Github, I counted on devs pinging me or the nc team if they needed extra help! So thank you for this! 🤗

#40537 looks more active indeed, if you think this is also a good approach to the initial PR, then great news, we'll try to move the other one forward.

If you think this one still apply, maybe you're willing to take over and push it forward?
It's not always easy to get a PR to completion amongst the wide variety of other tickets :)
Hope you can understand
Cheers, John

@aleixq

Copy link
Copy Markdown
Author

Thank's for your comment. You are true about the activity, and tbh it's more broadly implemented, so better let's close this and wait and test how this will land in nextcloud and then suggest new features if it's still needed (in fact I am apporting my impressions there right now).

@skjnldsv

Copy link
Copy Markdown
Member

Thank you for taking the time to reply! :)
We'll try our best to make things happen and move forward!!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. developingWork in progressstaleTicket or PR with no recent activity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@aleixq@vitormattos@skjnldsv@github-advanced-security@blizzz@szaimen