Refactor 'uploadFromUrl' to base storage implementation

Add safety around using fetch implementation

plugins/iframely/server/iframely.ts

@@ -1,8 +1,8 @@
-import fetch from "fetch-with-proxy";
 import env from "@server/env";
 import { InternalError } from "@server/errors";
 import Logger from "@server/logging/Logger";
 import Redis from "@server/storage/redis";
+import fetch from "@server/utils/fetch";
 
 class Iframely {
   private static apiUrl = `${env.IFRAMELY_URL}/api`;

plugins/slack/server/processors/SlackProcessor.ts

@@ -1,5 +1,4 @@
 import { differenceInMilliseconds } from "date-fns";
-import fetch from "fetch-with-proxy";
 import { Op } from "sequelize";
 import { IntegrationService, IntegrationType } from "@shared/types";
 import { Minute } from "@shared/utils/time";
@@ -12,6 +11,7 @@ import {
   RevisionEvent,
   Event,
 } from "@server/types";
+import fetch from "@server/utils/fetch";
 import presentMessageAttachment from "../presenters/messageAttachment";
 
 export default class SlackProcessor extends BaseProcessor {

plugins/slack/server/slack.ts

@@ -1,7 +1,7 @@
 import querystring from "querystring";
-import fetch from "fetch-with-proxy";
 import env from "@server/env";
 import { InvalidRequestError } from "@server/errors";
+import fetch from "@server/utils/fetch";
 
 const SLACK_API_URL = "https://slack.com/api";
 

plugins/webhooks/server/tasks/DeliverWebhookTask.ts

@@ -1,6 +1,4 @@
-import fetchWithProxy from "fetch-with-proxy";
+import { FetchError } from "node-fetch";
-import fetch, { FetchError } from "node-fetch";
-import { useAgent } from "request-filtering-agent";
 import { Op } from "sequelize";
 import WebhookDisabledEmail from "@server/emails/templates/WebhookDisabledEmail";
 import env from "@server/env";
@@ -64,6 +62,7 @@ import {
   ViewEvent,
   WebhookSubscriptionEvent,
 } from "@server/types";
+import fetch from "@server/utils/fetch";
 import presentWebhook, { WebhookPayload } from "../presenters/webhook";
 import presentWebhookSubscription from "../presenters/webhookSubscription";
 
@@ -591,21 +590,12 @@ export default class DeliverWebhookTask extends BaseTask<Props> {
         requestHeaders["Outline-Signature"] = signature;
       }
 
-      // In cloud-hosted environment we don't use fetchWithProxy as it prevents
+      response = await fetch(subscription.url, {
-      // the use of the request agent parameter, and is not required.
-      //
-      // In self-hosted, webhooks support proxying and are also allowed to
-      // connect to internal services, so use fetchWithProxy without the filtering
-      // agent.
-      const fetchMethod = env.isCloudHosted() ? fetch : fetchWithProxy;
-
-      response = await fetchMethod(subscription.url, {
         method: "POST",
         headers: requestHeaders,
         body: JSON.stringify(requestBody),
         redirect: "error",
         timeout: 5000,
-        agent: env.isCloudHosted() ? useAgent(subscription.url) : undefined,
       });
       status = response.ok ? "success" : "failed";
     } catch (err) {

server/.eslintrc

@@ -11,7 +11,10 @@
       {
         "checksVoidReturn": true
       }
-    ]
+    ],
+    "no-restricted-imports": ["error", {
+      "paths": ["fetch-with-proxy", "node-fetch"]
+    }]
   },
   "overrides": [
     {

server/storage/files/BaseStorage.ts

@@ -1,5 +1,7 @@
 import { Readable } from "stream";
 import { PresignedPost } from "aws-sdk/clients/s3";
+import Logger from "@server/logging/Logger";
+import fetch from "@server/utils/fetch";
 
 export default abstract class BaseStorage {
   /**
@@ -83,11 +85,31 @@ export default abstract class BaseStorage {
    * @param acl The ACL to use
    * @returns The URL of the file
    */
-  public abstract uploadFromUrl(
+  public async uploadFromUrl(url: string, key: string, acl: string) {
-    url: string,
+    const endpoint = this.getPublicEndpoint(true);
-    key: string,
+    if (url.startsWith("/api") || url.startsWith(endpoint)) {
-    acl: string
+      return;
-  ): Promise<string | undefined>;
+    }
+
+    try {
+      const res = await fetch(url);
+      const buffer = await res.buffer();
+      return this.upload({
+        body: buffer,
+        contentLength: res.headers["content-length"],
+        contentType: res.headers["content-type"],
+        key,
+        acl,
+      });
+    } catch (err) {
+      Logger.error("Error uploading to S3 from URL", err, {
+        url,
+        key,
+        acl,
+      });
+      return;
+    }
+  }
 
   /**
    * Delete a file from the storage provider.

server/storage/files/S3Storage.ts

@@ -1,9 +1,7 @@
 import util from "util";
 import AWS, { S3 } from "aws-sdk";
-import fetch from "fetch-with-proxy";
 import invariant from "invariant";
 import compact from "lodash/compact";
-import { useAgent } from "request-filtering-agent";
 import env from "@server/env";
 import Logger from "@server/logging/Logger";
 import BaseStorage from "./BaseStorage";
@@ -113,44 +111,6 @@ export default class S3Storage extends BaseStorage {
     return `${endpoint}/${key}`;
   };
 
-  public async uploadFromUrl(url: string, key: string, acl: string) {
-    invariant(
-      env.AWS_S3_UPLOAD_BUCKET_NAME,
-      "AWS_S3_UPLOAD_BUCKET_NAME is required"
-    );
-
-    const endpoint = this.getPublicEndpoint(true);
-    if (url.startsWith("/api") || url.startsWith(endpoint)) {
-      return;
-    }
-
-    try {
-      const res = await fetch(url, {
-        agent: useAgent(url),
-      });
-      const buffer = await res.buffer();
-      await this.client
-        .putObject({
-          ACL: acl,
-          Bucket: env.AWS_S3_UPLOAD_BUCKET_NAME,
-          Key: key,
-          ContentType: res.headers["content-type"],
-          ContentLength: res.headers["content-length"],
-          ContentDisposition: "attachment",
-          Body: buffer,
-        })
-        .promise();
-      return `${endpoint}/${key}`;
-    } catch (err) {
-      Logger.error("Error uploading to S3 from URL", err, {
-        url,
-        key,
-        acl,
-      });
-      return;
-    }
-  }
-
   public async deleteFile(key: string) {
     invariant(
       env.AWS_S3_UPLOAD_BUCKET_NAME,

server/typings/fetch-with-proxy.d.ts

@@ -1,4 +1,5 @@
 declare module "fetch-with-proxy" {
+  // eslint-disable-next-line no-restricted-imports
   import nodeFetch from "node-fetch";
 
   export = nodeFetch;

server/utils/avatars.ts

@@ -1,5 +1,5 @@
 import crypto from "crypto";
-import fetch from "fetch-with-proxy";
+import fetch from "./fetch";
 
 export async function generateAvatarUrl({
   id,

server/utils/fetch.ts

@@ -0,0 +1,28 @@
+/* eslint-disable no-restricted-imports */
+import fetchWithProxy from "fetch-with-proxy";
+import nodeFetch, { RequestInit, Response } from "node-fetch";
+import { useAgent } from "request-filtering-agent";
+import env from "@server/env";
+
+/**
+ * Wrapper around fetch that uses the request-filtering-agent in cloud hosted
+ * environments to filter malicious requests, and the fetch-with-proxy library
+ * in self-hosted environments to allow for request from behind a proxy.
+ *
+ * @param url The url to fetch
+ * @param init The fetch init object
+ * @returns The response
+ */
+export default function fetch(
+  url: string,
+  init?: RequestInit
+): Promise<Response> {
+  // In self-hosted, webhooks support proxying and are also allowed to connect
+  // to internal services, so use fetchWithProxy without the filtering agent.
+  const fetch = env.isCloudHosted() ? nodeFetch : fetchWithProxy;
+
+  return fetch(url, {
+    ...init,
+    agent: env.isCloudHosted() ? useAgent(url) : undefined,
+  });
+}

server/utils/oauth.ts

@@ -1,6 +1,6 @@
-import fetch from "fetch-with-proxy";
 import Logger from "@server/logging/Logger";
 import { AuthenticationError, InvalidRequestError } from "../errors";
+import fetch from "./fetch";
 
 export default abstract class OAuthClient {
   private clientId: string;

server/utils/passport.ts

@@ -1,6 +1,5 @@
 import crypto from "crypto";
 import { addMinutes, subMinutes } from "date-fns";
-import fetch from "fetch-with-proxy";
 import type { Context } from "koa";
 import {
   StateStoreStoreCallback,
@@ -11,6 +10,7 @@ import { getCookieDomain, parseDomain } from "@shared/utils/domains";
 import env from "@server/env";
 import { Team } from "@server/models";
 import { OAuthStateMismatchError } from "../errors";
+import fetch from "./fetch";
 
 export class StateStore {
   key = "state";

server/utils/updates.ts

@@ -1,5 +1,4 @@
 import crypto from "crypto";
-import fetch from "fetch-with-proxy";
 import env from "@server/env";
 import Collection from "@server/models/Collection";
 import Document from "@server/models/Document";
@@ -7,6 +6,7 @@ import Team from "@server/models/Team";
 import User from "@server/models/User";
 import Redis from "@server/storage/redis";
 import packageInfo from "../../package.json";
+import fetch from "./fetch";
 
 const UPDATES_URL = "https://updates.getoutline.com";
 const UPDATES_KEY = "UPDATES_KEY";