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

Enable CSRF protection in grant (OAuth2) #5504

Merged
merged 2 commits into from
Nov 11, 2024
Merged

Enable CSRF protection in grant (OAuth2) #5504

merged 2 commits into from
Nov 11, 2024

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Nov 8, 2024

I've been doing some testing and from what I can see, this is already supported in https://github.com/simov/grant (which companion uses for OAuth2), when enabling the state parameter. It seems to be working and it is checking the parameter when redirected back from the provider: https://github.com/simov/grant/blob/61fe48a8dac6aa4ec5764fadff0898b743b85588/lib/flow/oauth2.js#L72So

For future reference:
The whole state parameter is very confusing and it's used for multiple purposes in different contexts. It's mentioned everywhere in grant. There's:

  • Dynamic state
  • transport: 'state'
  • state: true

(when state: true), the state parameter that gets passed in the URL is also different from request to request. it can be

  • JWT (e.g. /drive/connect?state=eyJ...) containing internally { origin: '...' }
  • A long base64 encoded string /connect/googledrive?state=82efcf9896e...
  • The randomly generated CSRF token
    • /drive/redirect?state=278d04ad784a960a0b1eb4bd0e4656c2947c8e26&code=...
    • /connect/googledrive/callback?state=278d04ad784a960a0b1eb4bd0e4656c2947c8e26&code=...

When deactivating state: true (before this PR) there is no state query string being passed back from the google drive window in /drive/redirect.

This means that the CSRF token must be somewhere inside the long base64 encoded string, because how else can the OAuth2 provider (google) know about it. Note however when I decode it, it's just binary data. I also tried to decode it with cookie-signature and the secret from .env, but it just returns false.

I've been doing some testing and from what I can see, this is already supported in https://github.com/simov/grant (which companion uses for OAuth2), when enabling the `state` parameter. It seems to be working and it is checking the parameter when redirected back from the provider: https://github.com/simov/grant/blob/61fe48a8dac6aa4ec5764fadff0898b743b85588/lib/flow/oauth2.js#L72So
Copy link
Contributor

github-actions bot commented Nov 8, 2024

Diff output files
diff --git a/packages/@uppy/companion/lib/config/grant.d.ts b/packages/@uppy/companion/lib/config/grant.d.ts
index 97c46b1..183e854 100644
--- a/packages/@uppy/companion/lib/config/grant.d.ts
+++ b/packages/@uppy/companion/lib/config/grant.d.ts
@@ -1,8 +1,8 @@
 declare function _exports(): {
     googledrive: {
+        state: boolean;
         callback: string;
         scope: string[];
-        transport: string;
         custom_params: {
             access_type: string;
             prompt: string;
@@ -11,11 +11,11 @@ declare function _exports(): {
         access_url: string;
         oauth: number;
         scope_delimiter: string;
+        transport: string;
     };
     googlephotos: {
         callback: string;
         scope: string[];
-        transport: string;
         custom_params: {
             access_type: string;
             prompt: string;
@@ -24,41 +24,49 @@ declare function _exports(): {
         access_url: string;
         oauth: number;
         scope_delimiter: string;
+        transport: string;
+        state: boolean;
     };
     dropbox: {
-        transport: string;
         authorize_url: string;
         access_url: string;
         callback: string;
         custom_params: {
             token_access_type: string;
         };
+        transport: string;
+        state: boolean;
     };
     box: {
-        transport: string;
         authorize_url: string;
         access_url: string;
         callback: string;
+        transport: string;
+        state: boolean;
     };
     instagram: {
-        transport: string;
         callback: string;
+        transport: string;
+        state: boolean;
     };
     facebook: {
-        transport: string;
         scope: string[];
         callback: string;
+        transport: string;
+        state: boolean;
     };
     microsoft: {
-        transport: string;
         scope: string[];
         callback: string;
+        transport: string;
+        state: boolean;
     };
     zoom: {
-        transport: string;
         authorize_url: string;
         access_url: string;
         callback: string;
+        transport: string;
+        state: boolean;
     };
 };
 export = _exports;
diff --git a/packages/@uppy/companion/lib/config/grant.js b/packages/@uppy/companion/lib/config/grant.js
index 6f113e3..f475ecd 100644
--- a/packages/@uppy/companion/lib/config/grant.js
+++ b/packages/@uppy/companion/lib/config/grant.js
@@ -1,7 +1,6 @@
 "use strict";
 Object.defineProperty(exports, "__esModule", { value: true });
 const google = {
-  transport: "session",
   // access_type: offline is needed in order to get refresh tokens.
   // prompt: 'consent' is needed because sometimes a user will get stuck in an authenticated state where we will
   // receive no refresh tokens from them. This seems to be happen when running on different subdomains.
@@ -14,17 +13,24 @@ const google = {
   "oauth": 2,
   "scope_delimiter": " ",
 };
+const defaults = {
+  transport: "session",
+  state: true, // Enable CSRF check
+};
 // oauth configuration for provider services that are used.
 module.exports = () => {
   return {
     // we need separate auth providers because scopes are different,
     // and because it would be a too big rewrite to allow reuse of the same provider.
     googledrive: {
+      ...defaults,
       ...google,
+      state: true,
       callback: "/drive/callback",
       scope: ["https://www.googleapis.com/auth/drive.readonly"],
     },
     googlephotos: {
+      ...defaults,
       ...google,
       callback: "/googlephotos/callback",
       scope: [
@@ -33,35 +39,35 @@ module.exports = () => {
       ], // if name is needed, then add https://www.googleapis.com/auth/userinfo.profile too
     },
     dropbox: {
-      transport: "session",
+      ...defaults,
       authorize_url: "https://www.dropbox.com/oauth2/authorize",
       access_url: "https://api.dropbox.com/oauth2/token",
       callback: "/dropbox/callback",
       custom_params: { token_access_type: "offline" },
     },
     box: {
-      transport: "session",
+      ...defaults,
       authorize_url: "https://account.box.com/api/oauth2/authorize",
       access_url: "https://api.box.com/oauth2/token",
       callback: "/box/callback",
     },
     instagram: {
-      transport: "session",
+      ...defaults,
       callback: "/instagram/callback",
     },
     facebook: {
-      transport: "session",
+      ...defaults,
       scope: ["email", "user_photos"],
       callback: "/facebook/callback",
     },
     // for onedrive
     microsoft: {
-      transport: "session",
+      ...defaults,
       scope: ["files.read.all", "offline_access", "User.Read"],
       callback: "/onedrive/callback",
     },
     zoom: {
-      transport: "session",
+      ...defaults,
       authorize_url: "https://zoom.us/oauth/authorize",
       access_url: "https://zoom.us/oauth/token",
       callback: "/zoom/callback",

@Murderlon Murderlon merged commit d6d940c into main Nov 11, 2024
20 checks passed
@Murderlon Murderlon deleted the oauth2-csrf branch November 11, 2024 09:03
github-actions bot added a commit that referenced this pull request Nov 11, 2024
| Package         | Version | Package         | Version |
| --------------- | ------- | --------------- | ------- |
| @uppy/aws-s3    |   4.1.2 | @uppy/tus       |   4.1.4 |
| @uppy/companion |   5.1.4 | uppy            |   4.7.0 |
| @uppy/locales   |   4.3.0 |                 |         |

- @uppy/aws-s3: clarify and warn when incorrect buckets settings are used (Mikael Finstad / #5505)
- @uppy/tus: fix event upload-success response.body.xhr (ItsOnlyBinary / #5503)
- @uppy/companion: Enable CSRF protection in grant (OAuth2) (Mikael Finstad / #5504)
- @uppy/locales: Add ms_MY (Malay) locale (Salimi / #5488)
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

Successfully merging this pull request may close these issues.

2 participants