From 8a5f8828f6d11099558a655b0277f859c0543bdf Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 24 Jan 2025 14:20:59 +0100 Subject: [PATCH 1/2] fix: Everything - Don't double encode urls for resources - Name default export so that the codebase is less confusing - Actually end response on unknown errors - Transmit to client if an unknown error occurs --- client/src/views/Home.tsx | 28 +++---- server/src/index.ts | 73 +++++++++---------- .../__tests__/validateGeneratedFile.spec.ts | 40 +++++----- server/src/lib/errors.ts | 14 +++- server/src/lib/validateGeneratedFile.ts | 15 ++-- server/src/lib/validateSourceMap.ts | 9 ++- 6 files changed, 98 insertions(+), 81 deletions(-) diff --git a/client/src/views/Home.tsx b/client/src/views/Home.tsx index 870859e4..4699b5e7 100644 --- a/client/src/views/Home.tsx +++ b/client/src/views/Home.tsx @@ -7,13 +7,13 @@ const VALIDATE_URL = import.meta.env.REACT_APP_VALIDATE_URL; const TARGET_URL_PLACEHOLDER = 'http://code.jquery.com/jquery-1.9.1.min.js'; interface ExampleProps { - name: string, - url: string, - version: string, - onClick: React.MouseEventHandler, + name: string; + url: string; + version: string; + onClick: React.MouseEventHandler; } -function Example({name, url, version, onClick}: ExampleProps) { +function Example({ name, url, version, onClick }: ExampleProps) { return (
  • @@ -51,13 +51,12 @@ export default function Home() { const [isLoading, setIsLoading] = useState(false); const navigate = useNavigate(); - + const handleSubmit = useCallback(() => { - const url = encodeURIComponent(targetUrl || TARGET_URL_PLACEHOLDER); + const url = targetUrl || TARGET_URL_PLACEHOLDER; setIsLoading(true); - // Encode the url again to match whats saved on the server fetch(`${VALIDATE_URL}?url=${encodeURIComponent(url)}`, { method: 'POST' }).then(response => { @@ -74,10 +73,13 @@ export default function Home() { return (
    -
    { - event.preventDefault(); - handleSubmit(); - }}> + { + event.preventDefault(); + handleSubmit(); + }} + >
    { + onClick={event => { event.preventDefault(); setTargetUrl(example.url); diff --git a/server/src/index.ts b/server/src/index.ts index 0d5aaf8c..c3de713d 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -10,7 +10,7 @@ Sentry.init({ import path from 'path'; import { Request, Response } from 'express'; import { Storage } from '@google-cloud/storage'; -import _validateGeneratedFile from './lib/validateGeneratedFile'; +import { validateMinifiedFileAtUrl } from './lib/validateGeneratedFile'; let config: { [key: string]: string } = {}; try { @@ -33,49 +33,44 @@ const storage = new Storage({ * @param {function} The callback function. */ export function validateGeneratedFile(req: Request, res: Response) { - return Sentry.startSpan( - { - name: 'validateGeneratedFile' - }, - () => { - res.set('Access-Control-Allow-Origin', '*'); - res.set('Access-Control-Allow-Methods', 'POST'); + res.set('Access-Control-Allow-Origin', '*'); + res.set('Access-Control-Allow-Methods', 'POST'); - const url = req.query.url; - if (!url) { - res.status(500).send('URL not specified'); - return; - } + const url = req.query.url; + if (!url) { + res.status(500).send('URL not specified'); + return; + } - if (typeof url !== 'string') { - throw new TypeError('url is not a string'); - } + if (typeof url !== 'string') { + throw new TypeError('url is not a string'); + } + + Sentry.setTag('sourcemap_url', url); - Sentry.setTag('sourcemap_url', url); + validateMinifiedFileAtUrl(url, report => { + const bucket = storage.bucket(config.STORAGE_BUCKET); - _validateGeneratedFile(url, report => { - const bucket = storage.bucket(config.STORAGE_BUCKET); + // object names can't contain most symbols, so encode as a URI component + const objectName = `${Date.now()}_${encodeURIComponent(url)}`; + const file = bucket.file(objectName); + + const stream = file.createWriteStream({ + gzip: true, + metadata: { + contentType: 'text/plain; charset=utf-8' + } + }); - // object names can't contain most symbols, so encode as a URI component - const objectName = `${Date.now()}_${encodeURIComponent(url)}`; - const file = bucket.file(objectName); + stream.on('error', async err => { + res.status(500).send(err.message); + Sentry.captureException(err); + }); - const stream = file.createWriteStream({ - gzip: true, - metadata: { - contentType: 'text/plain; charset=utf-8' - } - }); - stream.on('error', async err => { - res.status(500).send(err.message); - Sentry.captureException(err); - }); - stream.on('finish', async () => { - res.status(200).send(encodeURIComponent(objectName)); - }); + stream.on('finish', async () => { + res.status(200).send(encodeURIComponent(objectName)); + }); - stream.end(JSON.stringify(report)); - }); - } - ); + stream.end(JSON.stringify(report)); + }); } diff --git a/server/src/lib/__tests__/validateGeneratedFile.spec.ts b/server/src/lib/__tests__/validateGeneratedFile.spec.ts index f41e8898..f05af809 100644 --- a/server/src/lib/__tests__/validateGeneratedFile.spec.ts +++ b/server/src/lib/__tests__/validateGeneratedFile.spec.ts @@ -3,7 +3,7 @@ import fs from 'fs'; import path from 'path'; import nock from 'nock'; -import validateGeneratedFile from '../validateGeneratedFile'; +import { validateMinifiedFileAtUrl } from '../validateGeneratedFile'; const { HOST, @@ -27,7 +27,7 @@ it('should download the target minified file, source maps, and external source f .get('/static/two.js') .reply(200, TWO_JS); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { // verify all mocked requests satisfied scope.done(); @@ -53,7 +53,7 @@ describe('source map location', () => { .get('/static/app.js.map') .reply(200, RAW_INLINE_SOURCE_MAP); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(0); done(); }); @@ -71,7 +71,7 @@ describe('source map location', () => { .get(appPath) .reply(200, fs.readFileSync(minFilePath, 'utf-8')); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(0); done(); }); @@ -85,7 +85,7 @@ describe('source map location', () => { nock(HOST) .get('/static/app.js.map') .reply(200, RAW_INLINE_SOURCE_MAP); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(0); done(); }); @@ -101,7 +101,7 @@ describe('source map location', () => { .get('/static/app.js.map') .reply(200, RAW_INLINE_SOURCE_MAP); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(0); done(); }); @@ -118,7 +118,7 @@ describe('source map location', () => { .get('/static/app.js.map') .reply(200, RAW_INLINE_SOURCE_MAP); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(0); done(); }); @@ -129,7 +129,7 @@ describe('source map location', () => { .get(appPath) .reply(200, 'function(){}();'); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(1); expect(report.errors[0].name).toBe('SourceMapNotFoundError'); done(); @@ -144,7 +144,7 @@ describe('http failures', () => { .socketDelay(5001) .reply(200, ''); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(1); expect(report.errors[0].name).toBe('ResourceTimeoutError'); expect(report.errors[0]).toHaveProperty( @@ -164,7 +164,7 @@ describe('http failures', () => { .get('/static/app.js.map') .socketDelay(5001) .reply(200, RAW_DEFAULT_SOURCE_MAP); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(1); expect(report.errors[0].name).toBe('ResourceTimeoutError'); expect(report.errors[0]).toHaveProperty( @@ -180,7 +180,7 @@ describe('http failures', () => { .get(appPath) .reply(401, 'Not Authenticated'); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(1); expect(report.errors[0].name).toBe('UnableToFetchMinifiedError'); done(); @@ -198,7 +198,7 @@ describe('http failures', () => { port: 1337 }); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(1); expect(report.errors[0].name).toBe('ConnectionRefusedError'); done(); @@ -214,7 +214,7 @@ describe('http failures', () => { .get('/static/app.js.map') .reply(401, 'Not Authenticated'); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(1); expect(report.errors[0].name).toBe('UnableToFetchSourceMapError'); done(); @@ -232,7 +232,7 @@ describe('http failures', () => { .get('/static/two.js') .reply(401, 'Not authenticated'); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { // verify all mocked requests satisfied scope.done(); expect(report.errors).toHaveLength(1); @@ -252,7 +252,7 @@ describe('parsing failures', () => { .get('/static/app.js.map') .reply(200, '!@#(!*@#(*&@'); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(1); expect(report.errors[0].name).toBe('InvalidJSONError'); expect(report.errors[0]).toHaveProperty( @@ -272,7 +272,7 @@ describe('parsing failures', () => { .get('/static/app.js.map') .reply(200, '{"version":"3"}'); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(1); expect(report.errors[0].name).toBe('InvalidSourceMapFormatError'); expect(report.errors[0]).toHaveProperty( @@ -296,7 +296,7 @@ describe('content failures', () => { .get('/static/two.js') .reply(200, ' \n\n\nlol'); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { scope.done(); expect(report.errors).toHaveLength(1); expect(report.errors[0].name).toBe('BadContentError'); @@ -327,7 +327,7 @@ describe('mappings', () => { .get('/static/add.inlineSources.js.map') .reply(200, fs.readFileSync(mapFilePath, 'utf-8')); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).toHaveLength(0); done(); }); @@ -349,7 +349,7 @@ describe('mappings', () => { .get('/static/add.fuzzLines.js.map') .reply(200, fs.readFileSync(mapFilePath, 'utf-8')); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.errors).not.toHaveLength(0); expect(report.errors[0].name).toBe('BadTokenError'); expect(report.errors[0]).toHaveProperty( @@ -376,7 +376,7 @@ describe('mappings', () => { .get('/static/add.fuzzColumns.js.map') .reply(200, fs.readFileSync(mapFilePath, 'utf-8')); - validateGeneratedFile(url, report => { + validateMinifiedFileAtUrl(url, report => { expect(report.warnings).not.toHaveLength(0); expect(report.warnings[0].name).toBe('BadColumnError'); expect(report.warnings[0]).toHaveProperty( diff --git a/server/src/lib/errors.ts b/server/src/lib/errors.ts index e6d547d6..aa947034 100644 --- a/server/src/lib/errors.ts +++ b/server/src/lib/errors.ts @@ -169,6 +169,17 @@ class BadColumnError extends BadTokenError { } } +class UnknownError extends Error { + resolutions: Array; + + constructor(url: string) { + super(); + this.name = 'UnknownError'; + this.message = `An unknown error occurred for url: ${url}`; + this.resolutions = ['Try again.']; + } +} + export { SourceMapNotFoundError, UnableToFetchError, @@ -182,5 +193,6 @@ export { BadContentError, BadColumnError, ResourceTimeoutError, - ConnectionRefusedError as SocketRefusedError + ConnectionRefusedError as SocketRefusedError, + UnknownError }; diff --git a/server/src/lib/validateGeneratedFile.ts b/server/src/lib/validateGeneratedFile.ts index 696f9a49..faf1d00b 100644 --- a/server/src/lib/validateGeneratedFile.ts +++ b/server/src/lib/validateGeneratedFile.ts @@ -7,10 +7,12 @@ import { SourceMapNotFoundError, UnableToFetchMinifiedError, ResourceTimeoutError, - SocketRefusedError + SocketRefusedError, + UnknownError } from './errors'; import { MAX_TIMEOUT } from './constants'; import { resolveUrl, getSourceMapLocation } from './utils'; +import { setTag } from '@sentry/node'; /** * Validates a target transpiled/minified file located at a given url @@ -18,7 +20,7 @@ import { resolveUrl, getSourceMapLocation } from './utils'; * e.g. https://example.com/static/app.min.js * @param {function} callback Invoked when validation is finished, passed a Report object */ -export default function validateGeneratedFile( +export function validateMinifiedFileAtUrl( url: string, callback: (report: Report) => void ) { @@ -26,15 +28,18 @@ export default function validateGeneratedFile( request(url, { timeout: MAX_TIMEOUT }, (error, response, body) => { if (error) { + setTag('outgoing_request_had_error', true); + if (error.message === 'ESOCKETTIMEDOUT') { report.pushError(new ResourceTimeoutError(url, MAX_TIMEOUT)); - return void callback(report); } else if (error.code === 'ECONNREFUSED') { report.pushError(new SocketRefusedError(url)); - return void callback(report); + } else { + report.pushError(new UnknownError(url)); } - return void console.error(error); + callback(report); + return; } if (response && response.statusCode !== 200) { diff --git a/server/src/lib/validateSourceMap.ts b/server/src/lib/validateSourceMap.ts index 67c89fd4..eec74dc9 100644 --- a/server/src/lib/validateSourceMap.ts +++ b/server/src/lib/validateSourceMap.ts @@ -20,7 +20,8 @@ import { InvalidSourceMapFormatError, InvalidJSONError, BadContentError, - ResourceTimeoutError + ResourceTimeoutError, + UnknownError } from './errors'; import { MAX_TIMEOUT } from './constants'; @@ -70,9 +71,11 @@ export default function validateSourceMap( if (error) { if (error.message === 'ESOCKETTIMEDOUT') { report.pushError(new ResourceTimeoutError(sourceMapUrl, MAX_TIMEOUT)); - return void reportCallback(report); + } else { + report.pushError(new UnknownError(sourceMapUrl)); } - return void console.error(error); + reportCallback(report); + return; } if (response && response.statusCode !== 200) { From d49c36c288d64d9cac99578daba4c8c5db0eab0f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 29 Jan 2025 17:47:32 +0100 Subject: [PATCH 2/2] Add back start span --- server/src/index.ts | 69 +++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 31 deletions(-) diff --git a/server/src/index.ts b/server/src/index.ts index c3de713d..ad906b7c 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -33,44 +33,51 @@ const storage = new Storage({ * @param {function} The callback function. */ export function validateGeneratedFile(req: Request, res: Response) { - res.set('Access-Control-Allow-Origin', '*'); - res.set('Access-Control-Allow-Methods', 'POST'); + return Sentry.startSpan( + { + name: 'validateGeneratedFile' + }, + () => { + res.set('Access-Control-Allow-Origin', '*'); + res.set('Access-Control-Allow-Methods', 'POST'); - const url = req.query.url; - if (!url) { - res.status(500).send('URL not specified'); - return; - } + const url = req.query.url; + if (!url) { + res.status(500).send('URL not specified'); + return; + } - if (typeof url !== 'string') { - throw new TypeError('url is not a string'); - } + if (typeof url !== 'string') { + throw new TypeError('url is not a string'); + } - Sentry.setTag('sourcemap_url', url); + Sentry.setTag('sourcemap_url', url); - validateMinifiedFileAtUrl(url, report => { - const bucket = storage.bucket(config.STORAGE_BUCKET); + validateMinifiedFileAtUrl(url, report => { + const bucket = storage.bucket(config.STORAGE_BUCKET); - // object names can't contain most symbols, so encode as a URI component - const objectName = `${Date.now()}_${encodeURIComponent(url)}`; - const file = bucket.file(objectName); + // object names can't contain most symbols, so encode as a URI component + const objectName = `${Date.now()}_${encodeURIComponent(url)}`; + const file = bucket.file(objectName); - const stream = file.createWriteStream({ - gzip: true, - metadata: { - contentType: 'text/plain; charset=utf-8' - } - }); + const stream = file.createWriteStream({ + gzip: true, + metadata: { + contentType: 'text/plain; charset=utf-8' + } + }); - stream.on('error', async err => { - res.status(500).send(err.message); - Sentry.captureException(err); - }); + stream.on('error', async err => { + res.status(500).send(err.message); + Sentry.captureException(err); + }); - stream.on('finish', async () => { - res.status(200).send(encodeURIComponent(objectName)); - }); + stream.on('finish', async () => { + res.status(200).send(encodeURIComponent(objectName)); + }); - stream.end(JSON.stringify(report)); - }); + stream.end(JSON.stringify(report)); + }); + } + ); }