implement facebook app secret proof

[mifi]

closes #5245

note that I couldn't get appsecret_time working, but it seems to be working without

---

[From Evgenia] In this PR:

• switching from singular requests to Facebook's batch requests
• implementation of appsecret_proof support for Facebook
• decorative: renaming from StreamHttpJsonError to HttpError

[github-actions]

Diff output files

diff --git a/packages/@uppy/companion/lib/server/helpers/utils.d.ts b/packages/@uppy/companion/lib/server/helpers/utils.d.ts
index e8462ea..cca8131 100644
--- a/packages/@uppy/companion/lib/server/helpers/utils.d.ts
+++ b/packages/@uppy/companion/lib/server/helpers/utils.d.ts
@@ -19,7 +19,7 @@ export function getBucket({ bucketOrFn, req, metadata, filename }: {
     metadata?: Record<string, string>;
     filename?: string;
 }): string;
-export class StreamHttpJsonError extends Error {
+export class HttpError extends Error {
     constructor({ statusCode, responseJson }: {
         statusCode: any;
         responseJson: any;
diff --git a/packages/@uppy/companion/lib/server/helpers/utils.js b/packages/@uppy/companion/lib/server/helpers/utils.js
index b998ab3..80f68a2 100644
--- a/packages/@uppy/companion/lib/server/helpers/utils.js
+++ b/packages/@uppy/companion/lib/server/helpers/utils.js
@@ -128,17 +128,17 @@ module.exports.decrypt = (encrypted, secret) => {
   return decrypted;
 };
 module.exports.defaultGetKey = ({ filename }) => `${crypto.randomUUID()}-${filename}`;
-class StreamHttpJsonError extends Error {
+class HttpError extends Error {
   statusCode;
   responseJson;
   constructor({ statusCode, responseJson }) {
     super(`Request failed with status ${statusCode}`);
     this.statusCode = statusCode;
     this.responseJson = responseJson;
-    this.name = "StreamHttpJsonError";
+    this.name = "HttpError";
   }
 }
-module.exports.StreamHttpJsonError = StreamHttpJsonError;
+module.exports.HttpError = HttpError;
 module.exports.prepareStream = async (stream) =>
   new Promise((resolve, reject) => {
     stream
@@ -150,7 +150,7 @@ module.exports.prepareStream = async (stream) =>
       })
       .on("error", (err) => {
         // In this case the error object is not a normal GOT HTTPError where json is already parsed,
-        // we create our own StreamHttpJsonError error for this case
+        // we create our own HttpError error for this case
         if (typeof err.response?.body === "string" && typeof err.response?.statusCode === "number") {
           let responseJson;
           try {
@@ -159,7 +159,7 @@ module.exports.prepareStream = async (stream) =>
             reject(err);
             return;
           }
-          reject(new StreamHttpJsonError({ statusCode: err.response.statusCode, responseJson }));
+          reject(new HttpError({ statusCode: err.response.statusCode, responseJson }));
           return;
         }
         reject(err);
diff --git a/packages/@uppy/companion/lib/server/provider/Provider.d.ts b/packages/@uppy/companion/lib/server/provider/Provider.d.ts
index 8977358..f58209f 100644
--- a/packages/@uppy/companion/lib/server/provider/Provider.d.ts
+++ b/packages/@uppy/companion/lib/server/provider/Provider.d.ts
@@ -20,16 +20,18 @@ declare class Provider {
     static get authStateExpiry(): number;
     /**
      *
-     * @param {{providerName: string, allowLocalUrls: boolean, providerGrantConfig?: object}} options
+     * @param {{providerName: string, allowLocalUrls: boolean, providerGrantConfig?: object, secret: string}} options
      */
-    constructor({ allowLocalUrls, providerGrantConfig }: {
+    constructor({ allowLocalUrls, providerGrantConfig, secret }: {
         providerName: string;
         allowLocalUrls: boolean;
         providerGrantConfig?: object;
+        secret: string;
     });
     needsCookieAuth: boolean;
     allowLocalUrls: boolean;
     providerGrantConfig: any;
+    secret: string;
     /**
      * list the files and folders in the provider account
      *
diff --git a/packages/@uppy/companion/lib/server/provider/Provider.js b/packages/@uppy/companion/lib/server/provider/Provider.js
index 1ec5d0d..a428b02 100644
--- a/packages/@uppy/companion/lib/server/provider/Provider.js
+++ b/packages/@uppy/companion/lib/server/provider/Provider.js
@@ -6,13 +6,14 @@ const { MAX_AGE_24H } = require("../helpers/jwt");
  */
 class Provider {
   /**
-   * @param {{providerName: string, allowLocalUrls: boolean, providerGrantConfig?: object}} options
+   * @param {{providerName: string, allowLocalUrls: boolean, providerGrantConfig?: object, secret: string}} options
    */
-  constructor({ allowLocalUrls, providerGrantConfig }) {
+  constructor({ allowLocalUrls, providerGrantConfig, secret }) {
     // Some providers might need cookie auth for the thumbnails fetched via companion
     this.needsCookieAuth = false;
     this.allowLocalUrls = allowLocalUrls;
     this.providerGrantConfig = providerGrantConfig;
+    this.secret = secret;
     return this;
   }
   /**
diff --git a/packages/@uppy/companion/lib/server/provider/facebook/index.js b/packages/@uppy/companion/lib/server/provider/facebook/index.js
index d760375..c394c8e 100644
--- a/packages/@uppy/companion/lib/server/provider/facebook/index.js
+++ b/packages/@uppy/companion/lib/server/provider/facebook/index.js
@@ -1,25 +1,45 @@
 "use strict";
 var _a;
 Object.defineProperty(exports, "__esModule", { value: true });
+const crypto = require("node:crypto");
 const Provider = require("../Provider");
 const { getURLMeta } = require("../../helpers/request");
 const logger = require("../../logger");
 const { adaptData, sortImages } = require("./adapter");
 const { withProviderErrorHandling } = require("../providerErrors");
 const { prepareStream } = require("../../helpers/utils");
+const { HttpError } = require("../../helpers/utils");
 const got = require("../../got");
-const getClient = async ({ token }) =>
-  (await got).extend({
-    prefixUrl: "https://graph.facebook.com",
-    headers: {
-      authorization: `Bearer ${token}`,
-    },
+async function runRequestBatch({ secret, token, requests }) {
+  // https://developers.facebook.com/docs/facebook-login/security/#appsecret
+  // couldn't get `appsecret_time` working, but it seems to be working without it
+  // const time = Math.floor(Date.now() / 1000)
+  const appSecretProof = crypto.createHmac("sha256", secret)
+    // .update(`${token}|${time}`)
+    .update(token)
+    .digest("hex");
+  const form = {
+    access_token: token,
+    appsecret_proof: appSecretProof,
+    // appsecret_time: String(time),
+    batch: JSON.stringify(requests),
+  };
+  const responsesRaw = await (await got).post("https://graph.facebook.com", { form }).json();
+  const responses = responsesRaw.map((response) => ({ ...response, body: JSON.parse(response.body) }));
+  const errorResponse = responses.find((response) => response.code !== 200);
+  if (errorResponse) {
+    throw new HttpError({ statusCode: errorResponse.code, responseJson: errorResponse.body });
+  }
+  return responses;
+}
+async function getMediaUrl({ secret, token, id }) {
+  const [{ body }] = await runRequestBatch({
+    secret,
+    token,
+    requests: [
+      { method: "GET", relative_url: `${id}?${new URLSearchParams({ fields: "images" }).toString()}` },
+    ],
   });
-async function getMediaUrl({ token, id }) {
-  const body = await (await getClient({ token })).get(String(id), {
-    searchParams: { fields: "images" },
-    responseType: "json",
-  }).json();
   const sortedImages = sortImages(body.images);
   return sortedImages[sortedImages.length - 1].source;
 }
@@ -41,17 +61,22 @@ class Facebook extends Provider {
         path = `${directory}/photos`;
         qs.fields = "icon,images,name,width,height,created_time";
       }
-      const client = await getClient({ token });
-      const [{ email }, list] = await Promise.all([
-        client.get("me", { searchParams: { fields: "email" }, responseType: "json" }).json(),
-        client.get(path, { searchParams: qs, responseType: "json" }).json(),
-      ]);
+      const [response1, response2] = await runRequestBatch({
+        secret: this.secret,
+        token,
+        requests: [
+          { method: "GET", relative_url: `me?${new URLSearchParams({ fields: "email" }).toString()}` },
+          { method: "GET", relative_url: `${path}?${new URLSearchParams(qs)}` },
+        ],
+      });
+      const { email } = response1.body;
+      const list = response2.body;
       return adaptData(list, email, directory, query);
     });
   }
   async download({ id, token }) {
     return this.#withErrorHandling("provider.facebook.download.error", async () => {
-      const url = await getMediaUrl({ token, id });
+      const url = await getMediaUrl({ secret: this.secret, token, id });
       const stream = (await got).stream.get(url, { responseType: "json" });
       await prepareStream(stream);
       return { stream };
@@ -65,14 +90,20 @@ class Facebook extends Provider {
   }
   async size({ id, token }) {
     return this.#withErrorHandling("provider.facebook.size.error", async () => {
-      const url = await getMediaUrl({ token, id });
+      const url = await getMediaUrl({ secret: this.secret, token, id });
       const { size } = await getURLMeta(url);
       return size;
     });
   }
   async logout({ token }) {
     return this.#withErrorHandling("provider.facebook.logout.error", async () => {
-      await (await getClient({ token })).delete("me/permissions", { responseType: "json" }).json();
+      await runRequestBatch({
+        secret: this.secret,
+        token,
+        requests: [
+          { method: "DELETE", relative_url: "me/permissions" },
+        ],
+      });
       return { revoked: true };
     });
   }
diff --git a/packages/@uppy/companion/lib/server/provider/index.js b/packages/@uppy/companion/lib/server/provider/index.js
index 3be93fc..d33dc20 100644
--- a/packages/@uppy/companion/lib/server/provider/index.js
+++ b/packages/@uppy/companion/lib/server/provider/index.js
@@ -38,7 +38,7 @@ module.exports.getProviderMiddleware = (providers, grantConfig) => {
   const middleware = (req, res, next, providerName) => {
     const ProviderClass = providers[providerName];
     if (ProviderClass && validOptions(req.companion.options)) {
-      const { allowLocalUrls } = req.companion.options;
+      const { allowLocalUrls, providerOptions } = req.companion.options;
       const { oauthProvider } = ProviderClass;
       let providerGrantConfig;
       if (isOAuthProvider(oauthProvider)) {
@@ -46,7 +46,8 @@ module.exports.getProviderMiddleware = (providers, grantConfig) => {
         providerGrantConfig = grantConfig[oauthProvider];
         req.companion.providerGrantConfig = providerGrantConfig;
       }
-      req.companion.provider = new ProviderClass({ providerName, providerGrantConfig, allowLocalUrls });
+      const { secret } = providerOptions[providerName];
+      req.companion.provider = new ProviderClass({ secret, providerName, providerGrantConfig, allowLocalUrls });
       req.companion.providerClass = ProviderClass;
     } else {
       logger.warn(
diff --git a/packages/@uppy/companion/lib/server/provider/providerErrors.js b/packages/@uppy/companion/lib/server/provider/providerErrors.js
index 0b6dbc6..c0b9080 100644
--- a/packages/@uppy/companion/lib/server/provider/providerErrors.js
+++ b/packages/@uppy/companion/lib/server/provider/providerErrors.js
@@ -36,7 +36,7 @@ async function withProviderErrorHandling(
     if (err?.name === "HTTPError") {
       statusCode = err.response?.statusCode;
       body = err.response?.body;
-    } else if (err?.name === "StreamHttpJsonError") {
+    } else if (err?.name === "HttpError") {
       statusCode = err.statusCode;
       body = err.responseJson;
     }

[kvz]

Are these related?

[mifi]

yes i renamed the error to a more generic name so it can be reused for non "stream" related http errors without the reader thinking "Wtf does StreamHttpJsonError mean?"

[lakesare]

Looks good to me, thanks for a quick turnaround with this!

No concerns with respect to code, added a few decorative suggestions.

Tested with:

• Facebook with "require app secret proof" setting off - works ✅︎
  This means that
  1. this is a backwards-compatible improvement - existing oauth apps that managed to get facebook-validated with this setting off will keep working.
  2. if the uppy user decides to reuse their Uppy Facebook oauth credentials client-side, they can easily do it by leaving this setting as off, uppy will work fine for them.
• Facebook with "require app secret proof" setting on - works ✅︎
• Dropbox - works ✅︎

[lakesare]

Found one issue - we're not detecting auth errors as such anymore

Before

After

Steps to reproduce:

1. Log out from Facebook
2. Refresh the http://localhost:5173 tab
3. Click on Facebook icon

[mifi]

Steps to reproduce:

1. Log out from Facebook
2. Refresh the http://localhost:5173 tab
3. Click on Facebook icon

i'm not able to reproduce this. do you have any local code changes?

when I try to logout, then refresh, then click facebook, then in the facebook oauth2 dialog i cancel the operation, i'm getting a proper error notification in uppy so it seems to me to be working.

[mifi]

implemented the feedback

[lakesare]

i'm not able to reproduce this. do you have any local code changes?

I cannot reproduce it now myself either, all good. Sorry for a false alarm!
Didn't have any local changes I think, not sure what was causing it.

[kvz]

Cool! And thank you for the thorough review @lakesare

Let's get this on the road @Murderlon