-
Notifications
You must be signed in to change notification settings - Fork 2k
Clarify the description of REST vs OCS in accordance to sugestions discussed #12264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@provokateurin I started to rephrase the "do not use OCS" in this PR. Does this sound reasonable or am I going completely the wrong direction? I know I need to eventually write a few more words and I for sure have to check the table entries (and wanted to explain them a bit more). What do you think, makes this sense? Is this to be discussed with some guys from the core team before merging (as it reverts some basic strategic statement)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, this is already super helpful!
The RST syntax for the table really messed with my brain, I need to take a closer look at it in proper table shape.
The following combinations of attributes might be useful for various scenarios: | ||
|
||
#. Plain frontend route: No special attribute related to CSRF or CORS | ||
#. Plain Frontend with CRSF checks disabled: Add the ``#[NoCSRFRequired]`` attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CSRF checks should only be disabled when absolutely necessary, this line should have a big warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are some of the few words I wanted to add...
The following combinations of attributes might be useful for various scenarios: | ||
|
||
#. Plain frontend route: No special attribute related to CSRF or CORS | ||
#. Plain Frontend with CRSF checks disabled: Add the ``#[NoCSRFRequired]`` attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#. Plain Frontend with CRSF checks disabled: Add the ``#[NoCSRFRequired]`` attribute | |
#. Plain Frontend with CRSF checks disabled: ``Controller`` class and ``#[NoCSRFRequired]`` attribute on the method |
#. Plain frontend route: No special attribute related to CSRF or CORS | ||
#. Plain Frontend with CRSF checks disabled: Add the ``#[NoCSRFRequired]`` attribute | ||
#. REST route with CORS enabled: Add the ``#[CORS]`` attribute | ||
#. OCS-based route |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is another case with OCS+CSRF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good now!
#. Plain frontend route: ``Controller`` class | ||
#. Plain Frontend with CRSF checks disabled: ``Controller`` class and ``#[NoCSRFRequired]`` attribute on the method | ||
#. OCS-based route: ``OCSController`` class | ||
#. OCS-based route with CSRF disabled: ``OCSController`` class and ``#[NoCSRFRequired]`` attribute on the method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this case doesn't exist, NoCSRFRequired should not have an effect on OCSController, but I will have to check that in the code to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking back and forth on this as well. In the end, I decided to keep the case as there is a certain difference (at least from the implementation):
For methods with NoCSRFRequired
, this section is doing nothing.
For the case where the attribute is not given, the strict cookie check boils down to a check on the OCS-APIREQUEST
header or no valid session token.
With the header in place (as the OCS controller generally expects), the second part is also skipped: The CSRF check is hardcoded to true, thus isInvalidCSRFRequired
returns false.
When the header is not set, you enter the second if block and will check the inner if. The check isinstance OCSController
is obviously true. The isValidOCSRequest
is somewhat redundant as it can only be reached with the header unset. Alternatively, a bearer authorization header is sufficient to make the CSRF checker happy (even without OCS header and without NoCSRFRequired
attribute).
I was adding this case as a reaction on your statement
There is another case with OCS+CSRF
In my opinion the last case could be dropped (and was never planned in the first round) but I do not see clearly the motivation of it, to be honest. Did I misunderstand your statement above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion the last case could be dropped (and was never planned in the first round) but I do not see clearly the motivation of it, to be honest. Did I misunderstand your statement above?
I messed up, I meant OCS+CORS...
Currently the CORS cases are completely missing in the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, okay, then that makes more sense. I can read the cors posts and extend by the OCS+cors as well.
I will add a warning about cors in general similar to csrf of you do not mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have any idea how to describe the case Access from external website more precisely or otherwise better, feel free to comment 😉.
08e4aab
to
ca6794c
Compare
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
@provokateurin are you still looking some things up, is there any action from my side required at the moment, or was there simply no time to review/prepare merging? |
No sorry, I was just busy with other things. I'll take another look in the coming days 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some links to general resources like these would be nice:
https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS
https://developer.mozilla.org/en-US/docs/Glossary/CSRF
- *Access from an external app* indicates that the user is not using the normal browser (as logged in) but directly navigates a certain URL. | ||
This can be in a new browser tab or an external program (like an Android app or simply a curl command line). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what these two cases have in common.
Security wise they are also a bit different, since you need CSRF protection to prevent redirect attacks via GET while this is completely irrelevant for non-browser use cases like curl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have in common that the CSRF token is not (guaranteed to be) known as part of the Cookie header and that the user is doing a dedicated action to navigate there (not by using a JS/AJAX request).
You are right these might be considered as separate cases. From user perspective (what can be done? not what is secure?), both navigate to an arbitrary URL and fetch the data. Examples:
- The user navigates to http://cloud.example.com/apps/cookbook/recipe/1234 the basic HTML template should be rendered in the browser and load the recipe 1234. If the actual call to the HTML template would be CSRF-protected any bookmarked link would break.
- An app wants to fetch data from the cookbook using a REST-based API http://cloud.example.com/apps/cookbook/api/v1/recipes/1234. Here, the CSRF checks might cause trouble as well prohibiting legitimate access to the endpoint.
I have not worked out the security side of this. What do you mean by redirect attacks via GET?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"requests from non-browser"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reread this and we are focusing on the data responses. Thus, the browser bookmark issue is no longer an issue. Instead, a reference to the restructured controller section was added. Thus it boils down to non-browser access as far as I see.
adef92f
to
b7ee67f
Compare
b7ee67f
to
857ad30
Compare
OK, unfortunately, I had the impression that I needed to restructure the controller chapter. This was a bit back and forth. Many of the changes are just rearrangements. Sorry for the mess. I tried to separate the data from the template requests. As the changes are lengthy, I would have to copy&paste the complete section here. Thus, instead, I post the built documentation for download: Developer manual.zip If it helps, I could postpone this PR, rebase the restructuring of thee controller class into a new PR, get that merged, and then come back to this PR here. |
Hm yes this is indeed getting a bit hard to review. A separate PR with only the restructuring would be very appreciated. |
Needs a rebase :) |
26a5d3f
to
b0084a7
Compare
Just a short note from someone who just started to migrate their app to OCS API with OpenAPI support: getting this PR merged would be a great improvement already. Maybe further improvements can be done in follow-up PRs later on to already profit from the clarifications done so far 😊 |
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Co-authored-by: Kate <[email protected]> Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Fix typos and other errors in the code as suggested by review process Co-authored-by: Kate <[email protected]> Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
b0084a7
to
5203eaa
Compare
Signed-off-by: Christian Wolf <[email protected]>
I'm a bit busy at the moment, but will give this a proper review in the week after next. I expect that everything will be fine, since we already discussed most changes intensively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only found some minor things that need to be changed, otherwise this looks really good!
Feel free to squash everything in the end (or leave it as-is).
.. warning:: | ||
Adding the ``#[NoCRSFRequired]`` attribute imposes a security risk. | ||
You should not add this to your controller methods unless you understand the implications and be sure that you absolutely need the attribute. | ||
Typically, you can instead use the ``OCS-APIRequest`` header for data requests, instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically, you can instead use the ``OCS-APIRequest`` header for data requests, instead. | |
Typically, you can use the ``OCS-APIRequest`` header for data requests instead, in order to satisfy the CSRF checks in your API requests. |
Without further measures, the CSRF checker would fail. | ||
So, enabling CORS for plain controllers is generally and highly discouraged. | ||
|
||
You would have to disable the CSRF checker (one more security risk) or use the ``OCP-APIRequest`` header to successfully pass the checker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would have to disable the CSRF checker (one more security risk) or use the ``OCP-APIRequest`` header to successfully pass the checker. | |
You would have to disable the CSRF checks (one more security risk) or use the ``OCP-APIRequest`` header or send a CSRF token to successfully pass the checks. |
There are different ways a clients might interact with your APIs. | ||
These ways depend on your API configuration (what you allow) and on which route the request is finally made. | ||
|
||
- *Access from web frontend* means the user is browses the Nextcloud web frontend with a browser. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- *Access from web frontend* means the user is browses the Nextcloud web frontend with a browser. | |
- *Access from web frontend* means the user is accessing the Nextcloud web frontend with a web browser. |
- *Access from non-browser* is if the user accesses the resource or page not using the default browser. | ||
Thus can be e.g. a script using CURL or an Android app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- *Access from non-browser* is if the user accesses the resource or page not using the default browser. | |
Thus can be e.g. a script using CURL or an Android app. | |
- *Access from non-browser* is if the user accesses the resource or page using something that is not a web browser, like an Android app or a curl command.``` |
- --- | ||
- --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- --- | |
- --- | |
- no | |
- no |
Methods from ``Controller`` classes can return ``DataResponse`` objects similar to ``OCSController`` class methods. | ||
For methods of a ``Controller`` class, the data of this response is sent e.g. as JSON as you provide it. | ||
Basically, the output is very similar to what ``json_encode`` would do. | ||
In contrast, the OCS controller will encapsulate the data in an outer shell that provides some more (meta) information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In contrast, the OCS controller will encapsulate the data in an outer shell that provides some more (meta) information. | |
In contrast, the ``OCSController`` will encapsulate the data in an outer shell that provides some more (meta) information. |
.. deprecated:: 30 | ||
The information in this section are mainly for reference purposes. Do not use the approaches in new code. | ||
|
||
Before NC server 30 the plain ``Controller`` classes' methods did not respect the ``OCS-APIRequest`` header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before NC server 30 the plain ``Controller`` classes' methods did not respect the ``OCS-APIRequest`` header. | |
Before Nextcloud 30 the plain ``Controller`` classes' methods did not respect the ``OCS-APIRequest`` header. |
- --- | ||
- yes | ||
- yes | ||
* - Access from external website | ||
- --- | ||
- --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- --- | |
- yes | |
- yes | |
* - Access from external website | |
- --- | |
- --- | |
- no | |
- yes | |
- yes | |
* - Access from external website | |
- no | |
- no |
|
||
Additionally, it is advised to carefully select the HTTP method used for requests. | ||
Requests of type ``GET`` should not alter data but just read existing data. | ||
As long as no other attack is involved, any non-``GET`` request requires at least user interaction (transmitting a form). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how relevant this is, since it's easy to submit a form via JS, so it does not really need any user interaction. The two sentences above are good though, maybe they should also mention HTML templates again?
Only if the destination server B confirms cross site resource sharing is allowed, the browser access the resource. | ||
|
||
Basically, accessing foreign resources is not limited to embedding images. | ||
Using JavaScript, arbitrary XHR/Ajax requests can be directed at arbitrary other hosts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using JavaScript, arbitrary XHR/Ajax requests can be directed at arbitrary other hosts. | |
Using JavaScript, arbitrary XHR/Ajax requests can be directed at arbitrary other hosts, which might be used to call APIs that leak your data. |
@@ -547,6 +562,11 @@ Because returning JSON is such a common task, there's even a shorter way to do t | |||
|
|||
Why does this work? Because the dispatcher sees that the controller did not return a subclass of a Response and asks the controller to turn the value into a Response. That's where responders come in. | |||
|
|||
.. deprecated:: 30 | |||
|
|||
Usage of classical controllers for data transmission should be avoided. Use OCS instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of classical controllers for data transmission should be avoided. Use OCS instead. | |
Usage of "index.php"-controllers for data transmission should be avoided. Use OCS instead. |
@@ -6,7 +6,8 @@ REST APIs | |||
|
|||
.. sectionauthor:: Bernhard Posselt <[email protected]> | |||
|
|||
Offering a RESTful API is not different from creating a :doc:`route <../basics/routing>` and :doc:`controllers <../basics/controllers>` for the web interface. It is recommended though to inherit from ApiController and add ``#[CORS]`` attribute to the methods so that `web applications will also be able to access the API <https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS>`_. | |||
Offering a RESTful API is not different from creating a :doc:`route <../basics/routing>` and :doc:`controllers <../basics/controllers>` for the web interface. | |||
It is recommended though to inherit from ApiController and add **@CORS** annotations to the methods so that `web applications will also be able to access the API <https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is recommended though to inherit from ApiController and add **@CORS** annotations to the methods so that `web applications will also be able to access the API <https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS>`_. | |
It is recommended though to inherit from ApiController and add ``#[CORS]`` attribute to the methods so that `web applications will also be able to access the API <https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS>`_. |
CORS | ||
---- | ||
|
||
`Cross-origin resource sharing (CORS) <https://en.wikipedia.org/wiki/Cross-origin_resource_sharing>`_ (see also on `MDN <https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS>`__) is a method implemented by browser to access resources from different domains at the same time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`Cross-origin resource sharing (CORS) <https://en.wikipedia.org/wiki/Cross-origin_resource_sharing>`_ (see also on `MDN <https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS>`__) is a method implemented by browser to access resources from different domains at the same time. | |
`Cross-origin resource sharing (CORS) <https://en.wikipedia.org/wiki/Cross-origin_resource_sharing>`_ (see also on `MDN <https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS>`_) is a method implemented by browser to access resources from different domains at the same time. |
Please add a screenshot if the URL renders with 2 underscores or one
|
||
`Cross-origin resource sharing (CORS) <https://en.wikipedia.org/wiki/Cross-origin_resource_sharing>`_ (see also on `MDN <https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS>`__) is a method implemented by browser to access resources from different domains at the same time. | ||
Assume, there is a website published on host A. | ||
The URL would for example be https://A/path/to/index.html. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL would for example be https://A/path/to/index.html. | |
The URL would for example be ``https://A/path/to/index.html``. |
`Cross-origin resource sharing (CORS) <https://en.wikipedia.org/wiki/Cross-origin_resource_sharing>`_ (see also on `MDN <https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS>`__) is a method implemented by browser to access resources from different domains at the same time. | ||
Assume, there is a website published on host A. | ||
The URL would for example be https://A/path/to/index.html. | ||
If there is a _different_ host B that serves a resource (e.g. an image file) as https://B/assets/image.jpg, the index file on host A could simply link to the image on B. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a _different_ host B that serves a resource (e.g. an image file) as https://B/assets/image.jpg, the index file on host A could simply link to the image on B. | |
If there is a _different_ host B that serves a resource (e.g. an image file) as ``https://B/assets/image.jpg``, the index file on host A could simply link to the image on B. |
@@ -434,6 +438,9 @@ A ``OCP\\AppFramework\\Http\\Template\\SimpleMenuAction`` will be a link with an | |||
developers can implement their own types of menu renderings by adding a custom | |||
class implementing the ``OCP\\AppFramework\\Http\\Template\\IMenuAction`` interface. | |||
|
|||
As the public template is also some HTML template, the same argumentation as for :ref:`regular templates<controller_template>` regarding the CSRF checks hold true: | |||
The usage of ``#[NoCSRFRequired]`` for public pages is considered acceptable and is actually needed to visit the page without an active account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true as an isolated statement!?
Not every page for guests should have NoCSRFRequired.
It should at least be limited to such that return templates again, but I'm not sure that is correct. I think it is only necessary (and therefore should be limited to) pages that are entry points.
You can have multiple pages that return HTML templates but you reach the next page by submitting a form or something, then it should only be the first page that has it?
Basically it's:
Only GET requests returning HTML that do not perform any data manipulation.
Or did I misunderstand something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can have multiple pages that return HTML templates but you reach the next page by submitting a form or something, then it should only be the first page that has it?
But the user could reload the page manually, so I think it's necessary to disable csrf for all templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the user could reload the page manually, so I think it's necessary to disable csrf for all templates.
In which case they would see a "Form expired …" page from the browser. Otherwise this is exactly what CSRF is about. It should prevent being tricked into a page that is "deep linking" into a flow or to a page that is not a non-interactive frontpage
☑️ Resolves
🖼️ Screenshots
T.B.D.