-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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?
Changes from all commits
1e63339
6a9fb9d
9b817ad
5fd856f
64453ed
5e0e926
2fdeb0b
86d6bae
08a8920
845e062
146e624
bda070d
5203eaa
3071dbf
34756e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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]`` 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>`_. | ||||||
|
||||||
.. code-block:: php | ||||||
|
||||||
|
@@ -44,7 +45,8 @@ CORS also needs a separate URL for the preflighted **OPTIONS** request that can | |||||
) | ||||||
|
||||||
|
||||||
Keep in mind that multiple apps will likely depend on the API interface once it is published and they will move at different speeds to react to changes implemented in the API. Therefore it is recommended to version the API in the URL to not break existing apps when backwards incompatible changes are introduced:: | ||||||
Keep in mind that multiple apps will likely depend on the API interface once it is published and they will move at different speeds to react to changes implemented in the API. | ||||||
Therefore it is recommended to version the API in the URL to not break existing apps when backwards incompatible changes are introduced:: | ||||||
provokateurin marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
/index.php/apps/myapp/api/1.0/resource | ||||||
|
||||||
|
@@ -79,3 +81,146 @@ To add an additional method or header or allow less headers, simply pass additio | |||||
} | ||||||
|
||||||
} | ||||||
|
||||||
.. _ocs-vs-rest: | ||||||
|
||||||
Relation of REST and OCS | ||||||
------------------------ | ||||||
|
||||||
There is a close relationship between REST APIs and :ref:`OCS <ocscontroller>`. | ||||||
Both provide a way to transmit data between the backend of the app in the Nextcloud server and some frontend. | ||||||
This is explicitly not about :ref:`HTML template responses <controller_html_responses>`. | ||||||
|
||||||
State-of-the-Art methods and comparison | ||||||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||||||
|
||||||
The following combinations of attributes might be relevant for various scenarios: | ||||||
|
||||||
#. Plain frontend route: ``Controller`` class | ||||||
#. OCS route: ``OCSController`` class | ||||||
#. OCS route with CORS enabled: ``OCSController`` class and ``#[CORS]`` attribute on the method | ||||||
|
||||||
.. 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 use the ``OCS-APIRequest`` header for data requests instead, in order to satisfy the CSRF checks in your API requests. | ||||||
|
||||||
.. warning:: | ||||||
Adding the attribute ``#[CORS]`` alone is not sufficient to allow access using CORS with plain frontend routes. | ||||||
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 checks (one more security risk) or use the ``OCP-APIRequest`` header or send a CSRF token to successfully pass the checks. | ||||||
The latter requires dedicated JS code on the importing page. | ||||||
|
||||||
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 accessing the Nextcloud web frontend with a web browser. | ||||||
- *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.``` | ||||||
- *Access from external website* means that the user browses some third party web site and data from your Nextcloud server appears. | ||||||
The other website has to embed/load/use images, JSON data, or other resources from a URL pointing to the Nextcloud server, to be able to do this. | ||||||
|
||||||
.. hint:: | ||||||
The discussion here is for data requests only. | ||||||
If you think of controller :ref:`methods serving (HTML) templates <controller_html_responses>`, disabling CSRF is considered fine. | ||||||
|
||||||
.. list-table:: Comparison of different API types | ||||||
:header-rows: 1 | ||||||
:align: center | ||||||
|
||||||
* - Description | ||||||
- ``Controller`` class | ||||||
- ``OCSController`` class | ||||||
- ``OCSController`` class & ``CORS`` on method | ||||||
* - URL prefix (relative to server) | ||||||
- ``/apps/<appid>/`` | ||||||
- ``/ocs/v2.php/apps/<appid>/`` | ||||||
- ``/ocs/v2.php/apps/<appid>/`` | ||||||
* - Access from web frontend | ||||||
- yes | ||||||
- yes | ||||||
- yes | ||||||
* - Access from non-browser | ||||||
- partial [#]_ | ||||||
- yes | ||||||
- yes | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
OCS is still protected against CSRF attacks using the OCS-APIRequest header or a CSRF token. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this line disabling the csrf checks for OCS when done with a bearer token? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, it also checks that the custom header is set (https://github.com/nextcloud/server/blob/94c529409813e03d632662283a5cd302ef8e9781/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php#L235) which is a CSRF check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think, we are arguing from different viewing angles here. Yes, the custom header is some way to enforce CSRF protection. Either a token or the custom header is a way to prevent access to the endpoint. However, when thinking of external (native) apps, this would allow to access the endpoint just by sending the header without knowing the actual token. PS: Having reread the last comment, I see the point in splitting direct access in browser (which you would not do on OCS directly) from the native app approach. |
||||||
* - Access from external website | ||||||
- no | ||||||
- no | ||||||
- yes | ||||||
* - Encapsulated data | ||||||
- no | ||||||
- yes (JSON or XML) | ||||||
- yes (JSON or XML) | ||||||
|
||||||
.. [#] The external app has to satisfy the CSRF checks. | ||||||
That is, you need to have the ``OCS-APIRequest`` HTTP request header set to ``true``. | ||||||
This is only possible for Nextcloud 30 onwards, older versions do not respect the header. | ||||||
|
||||||
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 ``OCSController`` will encapsulate the data in an outer shell that provides some more (meta) information. | ||||||
For example a status code (similar to the HTTP status code) is transmitted at top level. | ||||||
The actual data is transmitted in the ``data`` property. | ||||||
|
||||||
As a rule of thumb one can conclude that OCS provides a good way to handle most use cases including sufficient security checks. | ||||||
The only exception to this is if you want to provide an API for external usage where you have to comply with an externally defined API scheme. | ||||||
Here, the encapsulation introduced in OCS and CSRF checks might be in your way. | ||||||
|
||||||
|
||||||
Historical options | ||||||
~~~~~~~~~~~~~~~~~~ | ||||||
|
||||||
.. deprecated:: 30 | ||||||
The information in this section are mainly for reference purposes. Do not use the approaches in new code. | ||||||
|
||||||
Before Nextcloud 30 the plain ``Controller`` classes' methods did not respect the ``OCS-APIRequest`` header. | ||||||
Thus, to provide access to this type of controller methods for external apps, it was necessary to use the ``#[NoCSRFRequired]`` attribute (or the corresponding ``@NoCSRFRequired`` annotation). | ||||||
|
||||||
The following combinations of attributes were relevant for various scenarios: | ||||||
|
||||||
#. Plain frontend route: ``Controller`` class | ||||||
#. Plain frontend with CRSF checks disabled: ``Controller`` class and ``#[NoCSRFRequired]`` attribute on the method | ||||||
#. Plain frontend route with CORS enabled: ``Controller`` class and ``#[CORS]`` and ``#[NoCSRFRequired]`` attributes on the route | ||||||
#. OCS route: ``OCSController`` class | ||||||
#. OCS route with CORS enabled: ``OCSController`` class and ``#[CORS]`` attribute on the method | ||||||
|
||||||
.. hint:: | ||||||
The two scenarios involving the ``OCSController`` have not changed and, thus, the state-of-the-art documentation as noted above still holds true. | ||||||
Thus, these options are not reconsidered here again for simplicity reasons and to get the overall view more crisp. | ||||||
|
||||||
The warnings about not using ``NoCSRFRequired`` and ``CORS`` as mentioned in the state-of-the-art section holds true here as well. | ||||||
|
||||||
.. list-table:: Comparison of different API types | ||||||
:header-rows: 1 | ||||||
:align: center | ||||||
|
||||||
* - | Description | ||||||
- | ``Controller`` class | ||||||
- | ``Controller`` class with | ||||||
| ``NoCSRFRequired`` on method | ||||||
- | ``Controller`` class with | ||||||
| ``NoCSRFRequired`` and ``CORS`` | ||||||
| on method | ||||||
* - URL prefix (relative to server) | ||||||
- ``/apps/<appid>/`` | ||||||
- ``/apps/<appid>/`` | ||||||
- ``/apps/<appid>/`` | ||||||
* - Access from web frontend | ||||||
- yes | ||||||
- yes (CSRF risk) | ||||||
- yes (CSRF risk) | ||||||
* - Access from non-browser | ||||||
- no | ||||||
- yes | ||||||
- yes | ||||||
* - Access from external website | ||||||
- no | ||||||
- no | ||||||
- yes | ||||||
* - Encapsulated data | ||||||
- no | ||||||
- no | ||||||
- no |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -213,10 +213,12 @@ Sensitive data exposure | |||||
|
||||||
Always store user data or configuration files in safe locations, e.g. **nextcloud/data/** and not in the webroot where they can be accessed by anyone using a web browser. | ||||||
|
||||||
.. _csrf_introduction: | ||||||
|
||||||
Cross site request forgery | ||||||
-------------------------- | ||||||
|
||||||
Using `CSRF <https://en.wikipedia.org/wiki/Cross-site_request_forgery>`_ one can trick a user into executing a request that they did not want to make. Thus every POST and GET request needs to be protected against it. The only places where no CSRF checks are needed are in the main template, which is rendering the application, or in externally callable interfaces. | ||||||
Using `CSRF <https://en.wikipedia.org/wiki/Cross-site_request_forgery>`_ (see also on `MDN <https://developer.mozilla.org/en-US/docs/Glossary/CSRF>`__) one can trick a user into executing a request that they did not want to make. Thus every POST and GET request needs to be protected against it. The only places where no CSRF checks are needed are in the main template, which is rendering the application, or in externally callable interfaces. | ||||||
|
||||||
.. note:: Submitting a form is also a POST/GET request! | ||||||
|
||||||
|
@@ -227,7 +229,11 @@ To prevent CSRF in an app, be sure to call the following method at the top of al | |||||
<?php | ||||||
OCP\JSON::callCheck(); | ||||||
|
||||||
If you are using the App Framework, every controller method is automatically checked for CSRF unless you explicitly exclude it by setting the ``#[NoCSRFRequired]`` attribute or ``@NoCSRFRequired`` annotation before the controller method, see :doc:`../basics/controllers` | ||||||
If you are using the App Framework, every controller method is automatically checked for CSRF unless you explicitly exclude it by setting the ``#[NoCSRFRequired]`` attribute or ``@NoCSRFRequired`` annotation before the controller method, see :doc:`../basics/controllers`. | ||||||
|
||||||
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 commentThe 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? |
||||||
|
||||||
Unvalidated redirects | ||||||
--------------------- | ||||||
|
@@ -250,6 +256,28 @@ Always validate the URL before redirecting if the requested URL is on the same d | |||||
<?php | ||||||
header('Location: https://example.com'. $_GET['redirectURL']); | ||||||
|
||||||
|
||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Please add a screenshot if the URL renders with 2 underscores or one |
||||||
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. | ||||||
However, to protect B and its property (the image), the browsers do not silently embed the image of B into the page of A. | ||||||
Instead, B is kindly asked by the browser if embedding is allowed (the so-called `preflight <https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request>`_). | ||||||
|
||||||
To do so, there is a first request made to the resource on B with the ``OPTIONS`` HTTP command/verb. | ||||||
The server only answers with the headers as specified and adds ``Access-Control-*`` headers. | ||||||
These define, what the browser is to be allowed to do. | ||||||
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, which might be used to call APIs that leak your data. | ||||||
There are some safety measurements in place (especially about cookie handling), but one has still to be careful not to leak information unwillingly. | ||||||
Especially, if the destination server B allows to sent credentials using ``Access-Control-Allow-Credentials: true``, cross site scripting is very critical. | ||||||
You need :ref:`CSRF protection <csrf_introduction>` in place or your users are at relatively high risk. | ||||||
|
||||||
Getting help | ||||||
------------ | ||||||
|
||||||
|
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:
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.
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.
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