diff --git a/package-lock.json b/package-lock.json index 5ef2a4708..a912a81e9 100644 --- a/package-lock.json +++ b/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", diff --git a/package.json b/package.json index 00851d35b..e6c538942 100644 --- a/package.json +++ b/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", diff --git a/src/core/common/errors.ts b/src/core/common/errors.ts index f630cfa7c..8046fb85c 100644 --- a/src/core/common/errors.ts +++ b/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. */ diff --git a/src/core/server/config.ts b/src/core/server/config.ts index 9a5dee7bc..7e32c65ac 100644 --- a/src/core/server/config.ts +++ b/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; diff --git a/src/core/server/errors/index.ts b/src/core/server/errors/index.ts index 22fb917de..867405822 100644 --- a/src/core/server/errors/index.ts +++ b/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 } }, + }); + } + } +} diff --git a/src/core/server/errors/translations.ts b/src/core/server/errors/translations.ts index 08a322e4e..19bbbff19 100644 --- a/src/core/server/errors/translations.ts +++ b/src/core/server/errors/translations.ts @@ -58,4 +58,5 @@ export const ERROR_TRANSLATIONS: Record = { INVITE_INCLUDES_EXISTING_USER: "error-inviteIncludesExistingUser", REPEAT_POST: "error-repeatPost", INSTALLATION_FORBIDDEN: "error-installationForbidden", + SCRAPE_FAILED: "error-scrapeFailed", }; diff --git a/src/core/server/graph/tenant/loaders/Stories.ts b/src/core/server/graph/tenant/loaders/Stories.ts index 7cd5b1da7..15bd58eb0 100644 --- a/src/core/server/graph/tenant/loaders/Stories.ts +++ b/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. diff --git a/src/core/server/graph/tenant/mutators/Stories.ts b/src/core/server/graph/tenant/mutators/Stories.ts index 5c59a8a42..6d7263834 100644 --- a/src/core/server/graph/tenant/mutators/Stories.ts +++ b/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 | null> => remove(ctx.mongo, ctx.tenant, input.id, input.includeComments), scrape: async (input: GQLScrapeStoryInput): Promise | null> => - scrape(ctx.mongo, ctx.tenant.id, input.id), + scrape(ctx.mongo, ctx.config, ctx.tenant.id, input.id), }); diff --git a/src/core/server/graph/tenant/subscriptions/server.ts b/src/core/server/graph/tenant/subscriptions/server.ts index 1c67401db..2320009dd 100644 --- a/src/core/server/graph/tenant/subscriptions/server.ts +++ b/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( { diff --git a/src/core/server/index.ts b/src/core/server/index.ts index 4efa8f4ef..756ab3516 100644 --- a/src/core/server/index.ts +++ b/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(); diff --git a/src/core/server/locales/en-US/errors.ftl b/src/core/server/locales/en-US/errors.ftl index 20e04210e..72e8a766a 100644 --- a/src/core/server/locales/en-US/errors.ftl +++ b/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. diff --git a/src/core/server/queue/tasks/scraper.ts b/src/core/server/queue/tasks/scraper.ts index b2cd32e6b..5e4f95560 100644 --- a/src/core/server/queue/tasks/scraper.ts +++ b/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 -) => { +const createJobProcessor = ({ + mongo, + config, +}: ScrapeProcessorOptions) => async (job: Job) => { // 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"); diff --git a/src/core/server/services/stories/index.ts b/src/core/server/services/stories/index.ts index 3cb3d0931..76ebea9c7 100644 --- a/src/core/server/services/stories/index.ts +++ b/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; diff --git a/src/core/server/services/stories/scraper/abortAfter.ts b/src/core/server/services/stories/scraper/abortAfter.ts new file mode 100644 index 000000000..180706a58 --- /dev/null +++ b/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; diff --git a/src/core/server/services/stories/scraper/scraper.ts b/src/core/server/services/stories/scraper/scraper.ts index 809503054..56eb93ef4 100644 --- a/src/core/server/services/stories/scraper/scraper.ts +++ b/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 { - 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) {