Skip to content
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

Misleading authorisation token error messages #132

Open
bootsa opened this issue Mar 16, 2022 · 3 comments
Open

Misleading authorisation token error messages #132

bootsa opened this issue Mar 16, 2022 · 3 comments

Comments

@bootsa
Copy link

bootsa commented Mar 16, 2022

Package version (if known): v1.2.7 & current (2022-02-16) InvenioRDM demo install

Describe the bug

Posting to the REST api (e.g. https://inveniordm.web.cern.ch/api/records) with an invalid or missing token returns Referer errors and is a generic 400 response rather than a more specific error - probably 401 rather than 403 as the user is not known without a valid token.

Steps to Reproduce

  1. Post to REST api with missing or invalid token
  2. Response:
{
  "message": "Referer checking failed - no Referer.",
  "status": 400
}

Expected behavior

Error message related to actual issue and a more specific error code so it can be better handled by client applications.

Additional context

A small issue but it caused some confusion in figuring out the actual cause.

@bootsa bootsa changed the title Misleading autherisation token error messages Misleading authorisation token error messages Mar 16, 2022
@lnielsen
Copy link
Member

Can you provide the REST API calls that you're making? This looks like a CSRF token validation error, so I assume you're using session based authentication rather than access token?

@slint
Copy link
Member

slint commented Sep 21, 2023

TLDR: This is indeed a bug, that affects non-browser REST API clients (found also by a ZenodoRDM testing partner). I think the solution is to ignore CSRF if there was no session cookie passed in the request, which boils down to simply checking if session.new.


The issue is about the fact that our REST API endpoints accept authentication using both cookie-based sessions and "stateless" private access tokens (passed via ?access_token={TOKEN} or Authorization: Bearer {TOKEN} header).

To replicate the issue:

# POST without headers or querystring param
$ curl -X POST "https://inveniordm.web.cern.ch/api"
{"message": "Referer checking failed - no Referer.", "status": 400}

# POST without an invalid access token
$ curl -X POST "https://inveniordm.web.cern.ch/api?access_token=invalid"
{"message": "Referer checking failed - no Referer.", "status": 400}

What I would expect is:

# 405, since no need for CSRF, and we don't support POST on `/api`
$ curl -X POST "https://inveniordm.web.cern.ch/api"
405 Method Not Allowed

# Not sure here... 
$ curl -X POST "https://inveniordm.web.cern.ch/api?access_token=invalid"
400 Bad Request

# Endpoint requires auth, and token is invalid, so it's a 401
$ curl -X POST "https://inveniordm.web.cern.ch/api/records?access_token=invalid"
401 Unauthorized

For the 2nd request, I'm not sure since there are many ways to approach this:

  • Ignore the access_token and go with the usual 405 Method Not Allowed
  • Acknowledge the access_token and report either:
    • 401 Unauthorized, because of "bad credentials"... On the other hand, if we didn't need credentials in the first place for this endpoint, why even bother saying that they're bad?
    • 400 Bad Request, to communicate that the access token is invalid.

Solution

The case here is that if the request is not coming from the browser, there's no point in checking for CSRF... The real question now is: "How do we know the request is not coming from the browser?". I feel that this SO answer gets it right:

While cookies are the primary attack vector for CSRF attacks, you are also vulnerable if you use HTTP/Basic authentication. More generally, if the browser is able automatically pass along login credentials for your app, then CSRF matters.
...
Either way, the overall answer is simple: if you are using cookies (or other authentication methods that the browser can do automatically) then you need CSRF protection. If you aren't using cookies then you don't.

In our case, the only authentication methods we're using in Invenio(RDM) which the browser performs automatically are cookie/session-based. So the solution is to disable CSRF in csrf_validate if there is a session cookie passed, which boils down to simply checking if session.new.

I would also look for a 2nd opinion, possibly from the security team at CERN.

@phette23
Copy link

phette23 commented Feb 9, 2024

FYI for others running into this bug, you can set REST_CSRF_ENABLED = False in invenio.cfg to disable CSRF checking on API requests. There's a warning in ext.py that "CSRF validation will be enabled by default in the version 1.3.x" though.

I am testing locally so security is not a concern, but this obviously has security implications if used in production.

@jrcastro2 jrcastro2 self-assigned this May 31, 2024
jrcastro2 added a commit to jrcastro2/invenio-rest that referenced this issue May 31, 2024
@jrcastro2 jrcastro2 removed their assignment May 31, 2024
kpsherva pushed a commit that referenced this issue Jul 4, 2024
carlinmack added a commit to carlinmack/zenodo-rdm that referenced this issue Jul 17, 2024
📁 invenio-administration (2.4.0 -> 2.4.1 ��)

    📦 release: v2.4.1
    global: remove dependency on invenio-vocabularies

    * Removes the dependency from invenio-vocabularies.
    * Delegates administration UI schema type definition to the mashmallow
      class.

📁 invenio-app-rdm (13.0.0b0.dev6 -> 13.0.0b0.dev8 )

    📦 release: v13.0.0b0.dev8
    previewer: gracefuly handle default preference
    📦 release: v13.0.0b0.dev7
    datastreams: add affiliations and update funders

📁 invenio-banners (3.0.1 -> 3.0.2 🐛)

    release: v3.0.2
    errors: fix validation errors not propagated to resource
    administration: set default category to info

    * Fixes inveniosoftware/invenio-banners#26 by setting default category value,
      which is a required field.

📁 invenio-rdm-records (11.3.1 -> 11.4.0 🌈)

    📦 release: v11.4.0
    affiliations: update defaults to ror v2

📁 invenio-rest (1.3.0 -> 1.3.1 🐛)

    release: v1.3.1
    tests; fix csrf tests
    csrf: improve validation

    * inveniosoftware/invenio-rest#132

📁 invenio-vocabularies (4.0.0 -> 4.1.1 🌈)

    📦 release: v4.1.1
    installation: use invenio-oaipmh-scythe from PyPI
    📦 release: v4.1.0
    readers: make OAI-PMH an optional extra
    schema: add administration UI attributes
    ror: fix duplicate acronymns and aliases
    affiliations: fix title search
    datastreams: have yaml writer output utf8
    datastreams: add configs for funders and affiliations
    affiliations: add datastreams
    datastreams: move ror transformer to common
    affiliations: add new fields
    vocabulary-types: services, resources, and administration UI (inveniosoftware/invenio-vocabularies#310)

    config: add OpenAIRE mapping for "Latvian Council of Science"
    tasks: fixed exception logging
    funding: update award label
    funders: fix country name display (inveniosoftware/invenio-vocabularies#343)
    first implementation of OAIPMHReader (inveniosoftware/invenio-vocabularies#329)

    * first implementation of OAIPMHReader

    * introduce a simple way to map OAI records to a dict without expecting a special metadata format.

    * fix installation requirements

    * fix tests

    * small fixes to make the tests run

    * add error handling

    * renamed oaipmh_scythe package

    * handle remarks/questions from review.

    * replaced access to a real OAI server with a mocking implementation.

    * Update invenio_vocabularies/datastreams/readers.py

    * Update tests/datastreams/test_datastreams.py

    * Moved reader tests to testreaders.py

    * add missing copyright in header

    ---------

    global: add "tags" field to all vocabularies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants