feat(auth): Add optional CSRF protection

[TheCatLady]

Description

Resolves LGTM alert/error for query js/missing-token-validation
More info: https://lgtm.com/rules/1506064038914/

Screenshot (if UI related)

Result when trying to log in without CSRF cookie:

[lgtm-com]

This pull request fixes 1 alert when merging 52e8d50 into e80920f - view on LGTM.com

fixed alerts:

• 1 for Missing CSRF middleware

[sct]

What will happen with external api access if we add this, though? Also, adding the middleware by itself isn't enough right? We would have to actually generate the tokens and inject them into the login page?

[TheCatLady]

@sct If you test it out, you'll see the token is automatically generated and put in the _csrf cookie. I tested logging in both with & without the cookie to verify this works. And I'm still successfully accessing the API externally from Heimdall. 😃

[sct]

@sct If you test it out, you'll see the token is automatically generated and put in the _csrf cookie. I tested logging in both with & without the cookie to verify this works. And I'm still successfully accessing the API externally from Heimdall. 😃

Are you accessing it externally with an API key or with cookies? Can you try logging out entirely and accessing it again with all your cookies cleared? Using only your API key?

[TheCatLady]

@sct Wait, found an issue. Will investigate and fix

[TheCatLady]

@sct Oops, sorry I fell asleep last night before I could fix this! But I believe everything should be working as expected now (login with & without the relevant cookies, accessing API externally using token, and setting up a new install).

[lgtm-com]

This pull request fixes 1 alert when merging 942ee2f into 2bfab5e - view on LGTM.com

fixed alerts:

• 1 for Missing CSRF middleware

[sct]

If we always generate an XSRF-Token header for every request, doesn't that defeat the purpose of having CSRF protection in the first place? What purpose does it serve then?

Maybe I am not understanding something.

[TheCatLady]

@sct So I'm totally new to this, but spent some time yesterday doing some searching/reading about the topic. This PR is supposed to implement the "double submit cookie pattern."

More info: https://stackoverflow.com/questions/34782493/difference-between-csrf-and-x-csrf-token

[lgtm-com]

This pull request fixes 1 alert when merging f87599b into 2bfab5e - view on LGTM.com

fixed alerts:

• 1 for Missing CSRF middleware

[sct]

Okay. My final concern is that some services may not support cookies or they are setting their own cookies to log in. I know the Organizr SSO is creating its own cookie separate from ours. I'm partly worried they won't have the CSRF cookie.

[TheCatLady]

@sct I personally don't use Organizr; but I'll rebase and rebuild, then do some testing with Organizr and report back. Let me know if there's anything else that needs to be tested that I didn't think of.

[TheCatLady]

Okay. My final concern is that some services may not support cookies or they are setting their own cookies to log in. I know the Organizr SSO is creating its own cookie separate from ours. I'm partly worried they won't have the CSRF cookie.

@sct Okay, so I took a look at Organizr. Their SSO unfortunately won't work if CSRF protection is added to Overseerr. The CSRF cookie is required to prevent POST requests originating from external sources from mutating state—that's the whole point. Heimdall works fine because it only makes GET requests.

I guess you'll have to make a decision here as to what is more important. I personally prefer to have CSRF protection, but I don't know if Organizer SSO is a must-have feature for other Overseerr users.

[sct]

Yes. It's a must-have. A ton of people in the community use Organizr (myself included). But it doesn't just stop with Organizr. Any 3rd party apps that want to interface with the Overseerr API will not be able to. We already have a home assistant integration, that would break from this as well. Discord bots/phone apps/and so on. CSRF doesn't really work when one of the major points of your API is to allow cross-server requests.

[TheCatLady]

@sct How about adding a setting so that users can enable/disable CSRF protection?

[sct]

Sure, an option for it is fine.

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging c626dde into 2f75c4c - view on LGTM.com

new alerts:

• 1 for Syntax error

fixed alerts:

• 1 for Missing CSRF middleware

[TheCatLady]

Oops, didn't catch that while rebasing.

@sct I've added a setting option and tested it, but when changed Overseerr needs to be reloaded. What's the best way to handle that?

[lgtm-com]

This pull request fixes 1 alert when merging 8429ac4 into 2f75c4c - view on LGTM.com

fixed alerts:

• 1 for Missing CSRF middleware

[TheCatLady]

Changed the label to mimic the "tip" for SSL in the Email Notification Agent settings area.

[lgtm-com]

This pull request fixes 1 alert when merging 568b0d9 into 2f75c4c - view on LGTM.com

fixed alerts:

• 1 for Missing CSRF middleware

[ankarhem]

Changed the label to mimic the "tip" for SSL in the Email Notification Agent settings area.

What do you think about using normal punctuation instead of semicolon here? I don't see why these need to be linked in anyway.

[TheCatLady]

@ankarhem Honestly I feel like they should be on two separate lines. But the tips in other places aren't complete sentences and don't end in punctuation, so I thought this was the best way to handle it since it's two independent clauses.

Alternatively, I could change it to:

Sets external API access to read-only (Overseerr must be reloaded for changes to take effect)

Let me know what you think!

[TheCatLady]

@sct wants parentheses, so parentheses it is!

[lgtm-com]

This pull request fixes 1 alert when merging 1cbb77a into 2f75c4c - view on LGTM.com

fixed alerts:

• 1 for Missing CSRF middleware

[lgtm-com]

This pull request fixes 1 alert when merging ef7afda into 2f75c4c - view on LGTM.com

fixed alerts:

• 1 for Missing CSRF middleware

[lgtm-com]

This pull request fixes 1 alert when merging 19818d1 into 4b0241c - view on LGTM.com

fixed alerts:

• 1 for Missing CSRF middleware

[github-actions]

🎉 This PR is included in version 1.18.0 🎉

The release is available on:

• v1.18.0
• GitHub release

Your semantic-release bot 📦🚀