fix: introduced scraper timeout (#2724)

- fixes #2716

package-lock.json

@@ -4052,9 +4052,9 @@
       "integrity": "sha512-m9zXmifkZsMHZBOyxZWilMwmTlpC8x5Ty360JKTiXvlXZfBWYpsg9ZZvP/Ye+iZUh+Q+MxDLjItVTWIsfwz+8Q=="
     },
     "@types/node-fetch": {
-      "version": "2.3.3",
-      "resolved": "https://registry.npmjs.org/@types/node-fetch/-/node-fetch-2.3.3.tgz",
-      "integrity": "sha512-MIplfRxrDTsIbOLGyFqNWTmxho5Fs710Kul35tEcaqkx9He86mGbSCDvILL0LCMfmm+oJ8tDg51crE9+pJGgiQ==",
+      "version": "2.5.3",
+      "resolved": "https://registry.npmjs.org/@types/node-fetch/-/node-fetch-2.5.3.tgz",
+      "integrity": "sha512-X3TNlzZ7SuSwZsMkb5fV7GrPbVKvHc2iwHmslb8bIxRKWg2iqkfm3F/Wd79RhDpOXR7wCtKAwc5Y2JE6n/ibyw==",
       "dev": true,
       "requires": {
         "@types/node": "*"
@@ -4844,6 +4844,14 @@
       "resolved": "https://registry.npmjs.org/abab/-/abab-2.0.0.tgz",
       "integrity": "sha512-sY5AXXVZv4Y1VACTtR11UJCPHHudgY5i26Qj5TypE6DKlIApbwb5uqhXcJ5UUGbvZNRh7EeIoW+LrJumBsKp7w=="
     },
+    "abort-controller": {
+      "version": "3.0.0",
+      "resolved": "https://registry.npmjs.org/abort-controller/-/abort-controller-3.0.0.tgz",
+      "integrity": "sha512-h8lQ8tacZYnR3vNQTgibj+tODHI5/+l06Au2Pcriv/Gmet0eaj4TwWH41sO9wnHDiQsEj19q0drzdWdeAHtweg==",
+      "requires": {
+        "event-target-shim": "^5.0.0"
+      }
+    },
     "accepts": {
       "version": "1.3.5",
       "resolved": "https://registry.npmjs.org/accepts/-/accepts-1.3.5.tgz",
@@ -13979,6 +13987,11 @@
         "es5-ext": "~0.10.14"
       }
     },
+    "event-target-shim": {
+      "version": "5.0.1",
+      "resolved": "https://registry.npmjs.org/event-target-shim/-/event-target-shim-5.0.1.tgz",
+      "integrity": "sha512-i/2XbnSz/uxRCU6+NdVJgKWDTM427+MqYbkQzD321DuCQJUqOuJKIA0IM2+W2xtYHdKOmZ4dR6fExsd4SXL+WQ=="
+    },
     "eventemitter2": {
       "version": "5.0.1",
       "resolved": "https://registry.npmjs.org/eventemitter2/-/eventemitter2-5.0.1.tgz",
@@ -23023,9 +23036,9 @@
       }
     },
     "node-fetch": {
-      "version": "2.2.0",
-      "resolved": "https://registry.npmjs.org/node-fetch/-/node-fetch-2.2.0.tgz",
-      "integrity": "sha512-OayFWziIxiHY8bCUyLX6sTpDH8Jsbp4FfYd1j1f7vZyfgkcOnAyM4oQR16f8a0s7Gl/viMGRey8eScYk4V4EZA=="
+      "version": "2.6.0",
+      "resolved": "https://registry.npmjs.org/node-fetch/-/node-fetch-2.6.0.tgz",
+      "integrity": "sha512-8dG4H5ujfvFiqDmVu9fQ5bOHUC15JMjMY/Zumv26oOvvVJjM67KF8koCWIabKQ1GJIa9r2mMZscBq/TbdOcmNA=="
     },
     "node-forge": {
       "version": "0.7.5",

package.json

@@ -59,6 +59,7 @@
   "dependencies": {
     "@coralproject/bunyan-prettystream": "^0.1.4",
     "@metascraper/helpers": "^5.7.21",
+    "abort-controller": "^3.0.0",
     "akismet-api": "^4.2.0",
     "apollo-server-express": "^2.8.1",
     "archiver": "^3.0.3",
@@ -113,7 +114,7 @@
     "mongodb": "^3.2.7",
     "mongodb-core": "^3.2.7",
     "ms": "^2.1.1",
-    "node-fetch": "^2.2.0",
+    "node-fetch": "^2.6.0",
     "nodemailer": "^4.6.7",
     "nunjucks": "^3.1.3",
     "on-finished": "^2.3.0",
@@ -194,7 +195,7 @@
     "@types/mongodb": "^3.1.22",
     "@types/ms": "^0.7.30",
     "@types/node": "^10.5.2",
-    "@types/node-fetch": "^2.3.3",
+    "@types/node-fetch": "^2.5.3",
     "@types/nodemailer": "^4.6.2",
     "@types/nunjucks": "^3.1.1",
     "@types/object-diff": "0.0.0",

src/core/common/errors.ts

@@ -285,6 +285,11 @@ export enum ERROR_CODES {
    */
   RATE_LIMIT_EXCEEDED = "RATE_LIMIT_EXCEEDED",
 
+  /**
+   * SCRAPE_FAILED is returned when a scrape operation has failed.
+   */
+  SCRAPE_FAILED = "SCRAPE_FAILED",
+
   /**
    * JWT_REVOKED is returned when the token referenced has been revoked.
    */

src/core/server/config.ts

@@ -1,6 +1,7 @@
 import convict from "convict";
 import Joi from "joi";
 import { parseConnectionString } from "mongodb-core";
+import ms from "ms";
 import os from "os";
 
 import { LOCALES } from "coral-common/helpers/i18n/locales";
@@ -45,6 +46,16 @@ convict.addFormat({
   coerce: (url: string) => (url ? ensureEndSlash(url) : url),
 });
 
+// Add a custom format that is a duration parsed with `ms` to used instead of
+// the default format which will use `moment`.
+convict.addFormat({
+  name: "ms",
+  validate: (val: number) => {
+    Joi.assert(val, Joi.number().min(0));
+  },
+  coerce: (val: string) => ms(val),
+});
+
 const algorithms = [
   "HS256",
   "HS384",
@@ -200,7 +211,7 @@ const config = convict({
   websocket_keep_alive_timeout: {
     doc:
       "The keepalive timeout (in ms) that should be used to send keep alive messages through the websocket to keep the socket alive",
-    format: "duration",
+    format: "ms",
     default: "30 seconds",
     env: "WEBSOCKET_KEEP_ALIVE_TIMEOUT",
     arg: "websocketKeepAliveTimeout",
@@ -237,6 +248,13 @@ const config = convict({
     env: "DISABLE_RATE_LIMITERS",
     arg: "disableRateLimiters",
   },
+  scrape_timeout: {
+    doc: "The request timeout (in ms) for scraping operations.",
+    format: "ms",
+    default: "10 seconds",
+    env: "SCRAPE_TIMEOUT",
+    arg: "scrapeTimeout",
+  },
 });
 
 export type Config = typeof config;

src/core/server/errors/index.ts

@@ -757,3 +757,20 @@ export class RawQueryNotAuthorized extends CoralError {
     });
   }
 }
+
+export class ScrapeFailed extends CoralError {
+  constructor(url: string, cause?: Error | string) {
+    if (cause instanceof Error) {
+      super({
+        code: ERROR_CODES.SCRAPE_FAILED,
+        cause,
+        context: { pub: { url } },
+      });
+    } else {
+      super({
+        code: ERROR_CODES.SCRAPE_FAILED,
+        context: { pub: { url }, pvt: { cause } },
+      });
+    }
+  }
+}

src/core/server/errors/translations.ts

@@ -58,4 +58,5 @@ export const ERROR_TRANSLATIONS: Record<ERROR_CODES, string> = {
   INVITE_INCLUDES_EXISTING_USER: "error-inviteIncludesExistingUser",
   REPEAT_POST: "error-repeatPost",
   INSTALLATION_FORBIDDEN: "error-installationForbidden",
+  SCRAPE_FAILED: "error-scrapeFailed",
 };

src/core/server/graph/tenant/loaders/Stories.ts

@@ -113,7 +113,14 @@ export default (ctx: TenantContext) => ({
       },
     }).then(primeStoriesFromConnection(ctx)),
   debugScrapeMetadata: new DataLoader(
-    createManyBatchLoadFn((url: string) => scraper.scrape(url)),
+    createManyBatchLoadFn((url: string) =>
+      // This typecast is needed because the custom `ms` format does not return
+      // the desired `number` type even though that's the only type it can
+      // output.
+      scraper.scrape(url, (ctx.config.get(
+        "scrape_timeout"
+      ) as unknown) as number)
+    ),
     {
       // Disable caching for the DataLoader if the Context is designed to be
       // long lived.

src/core/server/graph/tenant/mutators/Stories.ts

@@ -31,6 +31,7 @@ export const Stories = (ctx: TenantContext) => ({
       create(
         ctx.mongo,
         ctx.tenant,
+        ctx.config,
         input.story.id,
         input.story.url,
         omitBy(input.story, isNull),
@@ -72,5 +73,5 @@ export const Stories = (ctx: TenantContext) => ({
   remove: async (input: GQLRemoveStoryInput): Promise<Readonly<Story> | null> =>
     remove(ctx.mongo, ctx.tenant, input.id, input.includeComments),
   scrape: async (input: GQLScrapeStoryInput): Promise<Readonly<Story> | null> =>
-    scrape(ctx.mongo, ctx.tenant.id, input.id),
+    scrape(ctx.mongo, ctx.config, ctx.tenant.id, input.id),
 });

src/core/server/graph/tenant/subscriptions/server.ts

@@ -263,12 +263,11 @@ export function createSubscriptionServer(
   schema: GraphQLSchema,
   options: Options
 ) {
-  const keepAlive = options.config.get("websocket_keep_alive_timeout");
-  if (typeof keepAlive !== "number" || keepAlive <= 0) {
-    throw new Error(
-      "expected the websocket_keep_alive_timeout configuration to be a positive number"
-    );
-  }
+  // This typecast is needed because the custom `ms` format does not return the
+  // desired `number` type even though that's the only type it can output.
+  const keepAlive = (options.config.get(
+    "websocket_keep_alive_timeout"
+  ) as unknown) as number;
 
   return SubscriptionServer.create(
     {

src/core/server/index.ts

@@ -116,6 +116,19 @@ class Server {
       .validate({ allowed: "strict" });
     logger.debug({ config: this.config.toString() }, "loaded configuration");
 
+    // Do some extra validation for production.
+    if (this.config.get("env") === "production") {
+      // Ensure that the signing secret has been specified.
+      if (
+        this.config.get("signing_secret") ===
+        this.config.default("signing_secret")
+      ) {
+        throw new Error(
+          "SIGNING_SECRET is required in production environments"
+        );
+      }
+    }
+
     // Load the graph schemas.
     this.schema = getTenantSchema();
 

src/core/server/locales/en-US/errors.ftl

@@ -11,7 +11,6 @@ error-tenantNotFound = Tenant hostname ({$hostname}) not found.
 error-userNotFound = User ({$userID}) not found.
 error-notFound = Unrecognized request URL ({$method} {$path}).
 error-tokenInvalid = Invalid API Token provided.
-
 error-tokenNotFound = Specified token does not exist.
 error-emailAlreadySet = Email address has already been set.
 error-emailNotSet = Email address has not been set yet.
@@ -35,6 +34,7 @@ error-emailInvalidFormat =
   Provided email address does not appear to be a valid email.
 error-emailExceedsMaxLength =
   Email address exceeds maximum length of {$max} characters.
+error-scrapeFailed = Could not scrape the URL { $url }.
 error-internalError = Internal Error
 error-tenantInstalledAlready = Tenant has already been installed.
 error-userNotEntitled = You are not authorized to access that resource.

src/core/server/queue/tasks/scraper.ts

@@ -2,6 +2,7 @@ import Queue, { Job } from "bull";
 import { Db } from "mongodb";
 import now from "performance-now";
 
+import { Config } from "coral-server/config";
 import logger from "coral-server/logger";
 import Task from "coral-server/queue/Task";
 import { scrape } from "coral-server/services/stories/scraper";
@@ -10,6 +11,7 @@ const JOB_NAME = "scraper";
 
 export interface ScrapeProcessorOptions {
   mongo: Db;
+  config: Config;
 }
 
 export interface ScraperData {
@@ -18,9 +20,10 @@ export interface ScraperData {
   tenantID: string;
 }
 
-const createJobProcessor = ({ mongo }: ScrapeProcessorOptions) => async (
-  job: Job<ScraperData>
-) => {
+const createJobProcessor = ({
+  mongo,
+  config,
+}: ScrapeProcessorOptions) => async (job: Job<ScraperData>) => {
   // Pull out the job data.
   const { storyID, storyURL, tenantID } = job.data;
 
@@ -41,7 +44,7 @@ const createJobProcessor = ({ mongo }: ScrapeProcessorOptions) => async (
   log.debug("starting to scrape the story");
 
   try {
-    await scrape(mongo, tenantID, storyID, storyURL);
+    await scrape(mongo, config, tenantID, storyID, storyURL);
   } catch (err) {
     log.error({ err }, "could not scrape the story");
 

src/core/server/services/stories/index.ts

@@ -1,6 +1,7 @@
 import { uniq, zip } from "lodash";
 import { Db } from "mongodb";
 
+import { Config } from "coral-server/config";
 import { StoryURLInvalidError } from "coral-server/errors";
 import logger from "coral-server/logger";
 import {
@@ -163,6 +164,7 @@ export type CreateStory = CreateStoryInput;
 export async function create(
   mongo: Db,
   tenant: Tenant,
+  config: Config,
   storyID: string,
   storyURL: string,
   { metadata, closedAt }: CreateStory,
@@ -194,7 +196,7 @@ export async function create(
   if (!metadata && tenant.stories.scraping.enabled) {
     // If the scraper has not scraped this story and story metadata was not
     // provided, we need to scrape it now!
-    newStory = await scrape(mongo, tenant.id, newStory.id, storyURL);
+    newStory = await scrape(mongo, config, tenant.id, newStory.id, storyURL);
   }
 
   return newStory;

src/core/server/services/stories/scraper/abortAfter.ts

@@ -0,0 +1,12 @@
+import AbortController from "abort-controller";
+
+function abortAfter(ms: number) {
+  const controller = new AbortController();
+  const timeout = setTimeout(() => {
+    controller.abort();
+  }, ms);
+
+  return { controller, timeout };
+}
+
+export default abortAfter;

src/core/server/services/stories/scraper/scraper.ts

@@ -9,12 +9,15 @@ import fetch, { RequestInit } from "node-fetch";
 import ProxyAgent from "proxy-agent";
 
 import { version } from "coral-common/version";
+import { Config } from "coral-server/config";
+import { ScrapeFailed } from "coral-server/errors";
 import logger from "coral-server/logger";
 import { retrieveStory, updateStory } from "coral-server/models/story";
 import { retrieveTenant } from "coral-server/models/tenant";
 
 import { GQLStoryMetadata } from "coral-server/graph/tenant/schema/__generated__/types";
 
+import abortAfter from "./abortAfter";
 import { modifiedScraper } from "./rules/modified";
 import { publishedScraper } from "./rules/published";
 import { sectionScraper } from "./rules/section";
@@ -27,8 +30,8 @@ export type Rule = Record<
 >;
 
 class Scraper {
-  private rules: Rule[];
-  private log: Logger;
+  private readonly rules: Rule[];
+  private readonly log: Logger;
 
   constructor(rules: Rule[]) {
     this.rules = rules;
@@ -80,14 +83,23 @@ class Scraper {
     };
   }
 
-  public async download(url: string, proxyURL?: string) {
+  public async download(
+    url: string,
+    abortAfterMilliseconds: number,
+    proxyURL?: string
+  ) {
     const log = this.log.child({ storyURL: url }, true);
 
+    // Abort the scrape request after the timeout is reached.
+    const { controller, timeout } = abortAfter(abortAfterMilliseconds);
+
     const options: RequestInit = {
       headers: {
         "User-Agent": `Talk Scraper/${version}`,
       },
+      signal: controller.signal,
     };
+
     if (proxyURL) {
       // Force the type here because there's a slight mismatch.
       options.agent = (new ProxyAgent(
@@ -99,27 +111,34 @@ class Scraper {
     const start = Date.now();
     log.debug("starting scrape of Story");
 
-    const res = await fetch(url, options);
-    if (!res.ok || res.status !== 200) {
-      log.warn(
-        { statusCode: res.status, statusText: res.statusText },
-        "scrape failed with non-200 status code"
-      );
-      return null;
+    try {
+      const res = await fetch(url, options);
+      if (!res.ok || res.status !== 200) {
+        log.warn(
+          { statusCode: res.status, statusText: res.statusText },
+          "scrape failed with non-200 status code"
+        );
+        return null;
+      }
+
+      const html = await res.text();
+
+      log.debug({ timeElapsed: Date.now() - start }, "scrape complete");
+
+      return html;
+    } catch (err) {
+      throw new ScrapeFailed(url, err);
+    } finally {
+      clearTimeout(timeout);
     }
-
-    const html = await res.text();
-
-    log.debug({ timeElapsed: Date.now() - start }, "scrape complete");
-
-    return html;
   }
 
   public async scrape(
     url: string,
+    abortAfterMilliseconds: number,
     proxyURL?: string
   ): Promise<GQLStoryMetadata | null> {
-    const html = await this.download(url, proxyURL);
+    const html = await this.download(url, abortAfterMilliseconds, proxyURL);
     if (!html) {
       return null;
     }
@@ -148,6 +167,7 @@ export const scraper = createScraper();
 
 export async function scrape(
   mongo: Db,
+  config: Config,
   tenantID: string,
   storyID: string,
   storyURL?: string
@@ -169,9 +189,16 @@ export async function scrape(
     storyURL = retrievedStory.url;
   }
 
+  // This typecast is needed because the custom `ms` format does not return the
+  // desired `number` type even though that's the only type it can output.
+  const abortAfterMilliseconds = (config.get(
+    "scrape_timeout"
+  ) as unknown) as number;
+
   // Get the metadata from the scraped html.
   const metadata = await scraper.scrape(
     storyURL,
+    abortAfterMilliseconds,
     tenant.stories.scraping.proxyURL
   );
   if (!metadata) {