From fae4b96762d447cf662ef6bc50aa51409f4f32b5 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 20 Oct 2021 12:17:59 +0200 Subject: [PATCH] Merge pull request #5349 from overleaf/jpa-no-depreacted-api [misc] fix eslint violations for node/no-depreacted-api GitOrigin-RevId: 0f7d64984da9e789c4ab95381db34afb89fa1a94 --- .../clsi/app/js/StaticServerForbidSymlinks.js | 3 +- services/clsi/app/js/UrlFetcher.js | 6 +-- services/web/.eslintrc | 4 +- .../src/Features/Analytics/AnalyticsProxy.js | 5 ++- .../BrandVariations/BrandVariationsHandler.js | 4 +- .../app/src/Features/Compile/ClsiManager.js | 4 +- .../app/src/infrastructure/ExpressLocals.js | 7 +-- .../web/app/src/infrastructure/GeoIpLookup.js | 4 +- .../app/src/infrastructure/ProxyManager.js | 9 ++-- .../app/src/infrastructure/RedirectManager.js | 6 +-- .../UnsupportedBrowserMiddleware.js | 2 +- .../test/acceptance/src/mocks/MockClsiApi.js | 4 +- .../PasswordResetHandlerTests.js | 2 +- .../src/Project/ProjectControllerTests.js | 2 +- .../UserEmailsConfirmationHandlerTests.js | 4 +- .../src/infrastructure/ProxyManagerTests.js | 43 ++++++++----------- 16 files changed, 50 insertions(+), 59 deletions(-) diff --git a/services/clsi/app/js/StaticServerForbidSymlinks.js b/services/clsi/app/js/StaticServerForbidSymlinks.js index f810d2b8ab..774178ee46 100644 --- a/services/clsi/app/js/StaticServerForbidSymlinks.js +++ b/services/clsi/app/js/StaticServerForbidSymlinks.js @@ -19,14 +19,13 @@ const Path = require('path') const fs = require('fs') const Settings = require('@overleaf/settings') const logger = require('logger-sharelatex') -const url = require('url') module.exports = ForbidSymlinks = function (staticFn, root, options) { const expressStatic = staticFn(root, options) const basePath = Path.resolve(root) return function (req, res, next) { let file, project_id, result - const path = __guard__(url.parse(req.url), x => x.pathname) + const path = req.url // check that the path is of the form /project_id_or_name/path/to/file.log if ((result = path.match(/^\/?([a-zA-Z0-9_-]+)\/(.*)/))) { project_id = result[1] diff --git a/services/clsi/app/js/UrlFetcher.js b/services/clsi/app/js/UrlFetcher.js index 2f20ad8956..a04f1f682b 100644 --- a/services/clsi/app/js/UrlFetcher.js +++ b/services/clsi/app/js/UrlFetcher.js @@ -17,8 +17,8 @@ const request = require('request').defaults({ jar: false }) const fs = require('fs') const logger = require('logger-sharelatex') const settings = require('@overleaf/settings') -const URL = require('url') const async = require('async') +const { URL } = require('url') const { promisify } = require('util') const oneMinute = 60 * 1000 @@ -43,12 +43,12 @@ module.exports = UrlFetcher = { return (_callback = function () {}) } - const u = URL.parse(url) + const u = new URL(url) if ( settings.filestoreDomainOveride && u.host !== settings.apis.clsiPerf.host ) { - url = `${settings.filestoreDomainOveride}${u.path}` + url = `${settings.filestoreDomainOveride}${u.pathname}${u.search}` } var timeoutHandler = setTimeout( function () { diff --git a/services/web/.eslintrc b/services/web/.eslintrc index 4f8010324a..c8b816d242 100644 --- a/services/web/.eslintrc +++ b/services/web/.eslintrc @@ -20,9 +20,7 @@ "no-var": "off", // do not allow importing of implicit dependencies. - "import/no-extraneous-dependencies": "error", - - "node/no-deprecated-api": "off" + "import/no-extraneous-dependencies": "error" }, "overrides": [ // NOTE: changing paths may require updating them in the Makefile too. diff --git a/services/web/app/src/Features/Analytics/AnalyticsProxy.js b/services/web/app/src/Features/Analytics/AnalyticsProxy.js index b1c19008a4..809d0d0684 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsProxy.js +++ b/services/web/app/src/Features/Analytics/AnalyticsProxy.js @@ -1,7 +1,7 @@ const settings = require('@overleaf/settings') const Errors = require('../Errors/Errors') const httpProxy = require('express-http-proxy') -const URL = require('url') +const { URL } = require('url') module.exports = { call(basePath) { @@ -16,7 +16,8 @@ module.exports = { return httpProxy(settings.apis.analytics.url, { proxyReqPathResolver(req) { - const requestPath = URL.parse(req.url).path + const u = new URL(req.originalUrl, settings.siteUrl) + const requestPath = u.pathname + u.search return `${basePath}${requestPath}` }, proxyReqOptDecorator(proxyReqOpts, srcReq) { diff --git a/services/web/app/src/Features/BrandVariations/BrandVariationsHandler.js b/services/web/app/src/Features/BrandVariations/BrandVariationsHandler.js index 350c822488..a135b27771 100644 --- a/services/web/app/src/Features/BrandVariations/BrandVariationsHandler.js +++ b/services/web/app/src/Features/BrandVariations/BrandVariationsHandler.js @@ -1,5 +1,5 @@ const OError = require('@overleaf/o-error') -const url = require('url') +const { URL } = require('url') const settings = require('@overleaf/settings') const logger = require('logger-sharelatex') const V1Api = require('../V1/V1Api') @@ -78,5 +78,5 @@ function setV1AsHostIfRelativeURL(urlString) { // As it only applies if the second argument is not absolute, we can use it to transform relative URLs into // absolute ones using v1 as the host. If the URL is absolute (e.g. a filepicker one), then the base // argument is just ignored - return url.resolve(settings.apis.v1.url, urlString) + return new URL(urlString, settings.apis.v1.url).href } diff --git a/services/web/app/src/Features/Compile/ClsiManager.js b/services/web/app/src/Features/Compile/ClsiManager.js index 0998250e3f..62aa8d7897 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.js +++ b/services/web/app/src/Features/Compile/ClsiManager.js @@ -4,7 +4,7 @@ const request = require('request') const ProjectGetter = require('../Project/ProjectGetter') const ProjectEntityHandler = require('../Project/ProjectEntityHandler') const logger = require('logger-sharelatex') -const Url = require('url') +const { URL } = require('url') const OError = require('@overleaf/o-error') const ClsiCookieManager = require('./ClsiCookieManager')( @@ -535,7 +535,7 @@ const ClsiManager = { for (const file of rawOutputFiles) { outputFiles.push({ path: file.path, // the clsi is now sending this to web - url: Url.parse(file.url).path, // the location of the file on the clsi, excluding the host part + url: new URL(file.url).pathname, // the location of the file on the clsi, excluding the host part type: file.type, build: file.build, contentId: file.contentId, diff --git a/services/web/app/src/infrastructure/ExpressLocals.js b/services/web/app/src/infrastructure/ExpressLocals.js index 33b57f699e..72c1380b3c 100644 --- a/services/web/app/src/infrastructure/ExpressLocals.js +++ b/services/web/app/src/infrastructure/ExpressLocals.js @@ -2,7 +2,7 @@ const logger = require('logger-sharelatex') const Settings = require('@overleaf/settings') const querystring = require('querystring') const _ = require('lodash') -const Url = require('url') +const { URL } = require('url') const Path = require('path') const moment = require('moment') const pug = require('pug-runtime') @@ -221,9 +221,10 @@ module.exports = function (webRouter, privateApiRouter, publicApiRouter) { } // Don't include the query string parameters, otherwise Google // treats ?nocdn=true as the canonical version - const parsedOriginalUrl = Url.parse(req.originalUrl) + const parsedOriginalUrl = new URL(req.originalUrl, Settings.siteUrl) res.locals.currentUrl = parsedOriginalUrl.pathname - res.locals.currentUrlWithQueryParams = parsedOriginalUrl.path + res.locals.currentUrlWithQueryParams = + parsedOriginalUrl.pathname + parsedOriginalUrl.search res.locals.capitalize = function (string) { if (string.length === 0) { return '' diff --git a/services/web/app/src/infrastructure/GeoIpLookup.js b/services/web/app/src/infrastructure/GeoIpLookup.js index 4c9c56c3d8..59ab8f5c23 100644 --- a/services/web/app/src/infrastructure/GeoIpLookup.js +++ b/services/web/app/src/infrastructure/GeoIpLookup.js @@ -2,7 +2,7 @@ const request = require('request') const settings = require('@overleaf/settings') const _ = require('underscore') const logger = require('logger-sharelatex') -const URL = require('url') +const { URL } = require('url') const { promisify, promisifyMultiResult } = require('../util/promises') const currencyMappings = { @@ -63,7 +63,7 @@ function getDetails(ip, callback) { } ip = ip.trim().split(' ')[0] const opts = { - url: URL.resolve(settings.apis.geoIpLookup.url, ip), + url: new URL(ip, settings.apis.geoIpLookup.url).href, timeout: 1000, json: true, } diff --git a/services/web/app/src/infrastructure/ProxyManager.js b/services/web/app/src/infrastructure/ProxyManager.js index a5afff292b..96edb415bb 100644 --- a/services/web/app/src/infrastructure/ProxyManager.js +++ b/services/web/app/src/infrastructure/ProxyManager.js @@ -14,7 +14,7 @@ let ProxyManager const settings = require('@overleaf/settings') const logger = require('logger-sharelatex') const request = require('request') -const URL = require('url') +const { URL, URLSearchParams } = require('url') module.exports = ProxyManager = { apply(publicApiRouter) { @@ -68,12 +68,11 @@ module.exports = ProxyManager = { // make a URL from a proxy target. // if the query is specified, set/replace the target's query with the given query var makeTargetUrl = function (target, req) { - const targetUrl = URL.parse(parseSettingUrl(target, req)) + const targetUrl = new URL(parseSettingUrl(target, req)) if (req.query != null && Object.keys(req.query).length > 0) { - targetUrl.query = req.query - targetUrl.search = null // clear `search` as it takes precedence over `query` + targetUrl.search = new URLSearchParams(req.query).toString() } - return targetUrl.format() + return targetUrl.href } var parseSettingUrl = function (target, { params }) { diff --git a/services/web/app/src/infrastructure/RedirectManager.js b/services/web/app/src/infrastructure/RedirectManager.js index 6f0d3c3ec2..638aa3f9dd 100644 --- a/services/web/app/src/infrastructure/RedirectManager.js +++ b/services/web/app/src/infrastructure/RedirectManager.js @@ -14,9 +14,7 @@ */ let RedirectManager const settings = require('@overleaf/settings') -const logger = require('logger-sharelatex') -const URL = require('url') -const querystring = require('querystring') +const { URL } = require('url') module.exports = RedirectManager = { apply(webRouter) { @@ -76,7 +74,7 @@ module.exports = RedirectManager = { // have differences between Express and Rails, so safer to just pass the raw // string var getQueryString = function (req) { - const { search } = URL.parse(req.url) + const { search } = new URL(req.originalUrl, settings.siteUrl) if (search) { return search } else { diff --git a/services/web/app/src/infrastructure/UnsupportedBrowserMiddleware.js b/services/web/app/src/infrastructure/UnsupportedBrowserMiddleware.js index 690648f26d..c145c9f468 100644 --- a/services/web/app/src/infrastructure/UnsupportedBrowserMiddleware.js +++ b/services/web/app/src/infrastructure/UnsupportedBrowserMiddleware.js @@ -7,7 +7,7 @@ function unsupportedBrowserMiddleware(req, res, next) { if (!Settings.unsupportedBrowsers) return next() // Prevent redirect loop - const path = Url.parse(req.url).pathname + const path = req.path if (path === '/unsupported-browser') return next() const userAgent = req.headers['user-agent'] diff --git a/services/web/test/acceptance/src/mocks/MockClsiApi.js b/services/web/test/acceptance/src/mocks/MockClsiApi.js index 6bd90f5e9b..7ca60ad0d1 100644 --- a/services/web/test/acceptance/src/mocks/MockClsiApi.js +++ b/services/web/test/acceptance/src/mocks/MockClsiApi.js @@ -8,13 +8,13 @@ class MockClsiApi extends AbstractMockApi { error: null, outputFiles: [ { - url: `/project/${req.params.project_id}/build/1234/output/project.pdf`, + url: `http://clsi:3013/project/${req.params.project_id}/build/1234/output/project.pdf`, path: 'project.pdf', type: 'pdf', build: 1234, }, { - url: `/project/${req.params.project_id}/build/1234/output/project.log`, + url: `http://clsi:3013/project/${req.params.project_id}/build/1234/output/project.log`, path: 'project.log', type: 'log', build: 1234, diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js index 855ca30318..b38708744d 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js @@ -23,7 +23,7 @@ const modulePath = path.join( describe('PasswordResetHandler', function () { beforeEach(function () { - this.settings = { siteUrl: 'www.sharelatex.com' } + this.settings = { siteUrl: 'https://www.overleaf.com' } this.OneTimeTokenHandler = { getNewToken: sinon.stub(), getValueFromTokenAndExpire: sinon.stub(), diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index fa0fdc158b..a747c72ee5 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -27,7 +27,7 @@ describe('ProjectController', function () { url: 'chat.com', }, }, - siteUrl: 'mysite.com', + siteUrl: 'https://overleaf.com', algolia: {}, } this.brandVariationDetails = { diff --git a/services/web/test/unit/src/User/UserEmailsConfirmationHandlerTests.js b/services/web/test/unit/src/User/UserEmailsConfirmationHandlerTests.js index cb9e31bbad..585a3cb149 100644 --- a/services/web/test/unit/src/User/UserEmailsConfirmationHandlerTests.js +++ b/services/web/test/unit/src/User/UserEmailsConfirmationHandlerTests.js @@ -27,7 +27,7 @@ describe('UserEmailsConfirmationHandler', function () { this.UserEmailsConfirmationHandler = SandboxedModule.require(modulePath, { requires: { '@overleaf/settings': (this.settings = { - siteUrl: 'emails.example.com', + siteUrl: 'https://emails.example.com', }), '../Security/OneTimeTokenHandler': (this.OneTimeTokenHandler = {}), './UserUpdater': (this.UserUpdater = {}), @@ -80,7 +80,7 @@ describe('UserEmailsConfirmationHandler', function () { .calledWith('confirmEmail', { to: this.email, confirmEmailUrl: - 'emails.example.com/user/emails/confirm?token=new-token', + 'https://emails.example.com/user/emails/confirm?token=new-token', sendingUser_id: this.user_id, }) .should.equal(true) diff --git a/services/web/test/unit/src/infrastructure/ProxyManagerTests.js b/services/web/test/unit/src/infrastructure/ProxyManagerTests.js index fa31285a42..b147a16428 100644 --- a/services/web/test/unit/src/infrastructure/ProxyManagerTests.js +++ b/services/web/test/unit/src/infrastructure/ProxyManagerTests.js @@ -82,14 +82,6 @@ describe('ProxyManager', function () { return this.request.reset() }) - it('does not calls next when match', function () { - const target = '/' - this.settings.proxyUrls[this.proxyPath] = target - this.proxyManager.createProxy(target)(this.req, this.res, this.next) - sinon.assert.notCalled(this.next) - return sinon.assert.called(this.request) - }) - it('proxy full URL', function () { const targetUrl = 'https://user:pass@foo.bar:123/pa/th.ext?query#hash' this.settings.proxyUrls[this.proxyPath] = targetUrl @@ -98,31 +90,32 @@ describe('ProxyManager', function () { }) it('overwrite query', function () { - const targetUrl = 'foo.bar/baz?query' + const targetUrl = 'https://foo.bar/baz?query' this.req.query = { requestQuery: 'important' } this.settings.proxyUrls[this.proxyPath] = targetUrl this.proxyManager.createProxy(targetUrl)(this.req) - const newTargetUrl = 'foo.bar/baz?requestQuery=important' + const newTargetUrl = 'https://foo.bar/baz?requestQuery=important' return assertCalledWith(this.request, { url: newTargetUrl }) }) it('handles target objects', function () { - const target = { baseUrl: 'api.v1', path: '/pa/th' } + const target = { baseUrl: 'https://api.v1', path: '/pa/th' } this.settings.proxyUrls[this.proxyPath] = target this.proxyManager.createProxy(target)(this.req, this.res, this.next) - return assertCalledWith(this.request, { url: 'api.v1/pa/th' }) + return assertCalledWith(this.request, { url: 'https://api.v1/pa/th' }) }) it('handles missing baseUrl', function () { const target = { path: '/pa/th' } this.settings.proxyUrls[this.proxyPath] = target - this.proxyManager.createProxy(target)(this.req, this.res, this.next) - return assertCalledWith(this.request, { url: 'undefined/pa/th' }) + expect(() => + this.proxyManager.createProxy(target)(this.req, this.res, this.next) + ).to.throw }) it('handles dynamic path', function () { const target = { - baseUrl: 'api.v1', + baseUrl: 'https://api.v1', path(params) { return `/resource/${params.id}` }, @@ -132,12 +125,14 @@ describe('ProxyManager', function () { this.req.route.path = '/res/:id' this.req.params = { id: 123 } this.proxyManager.createProxy(target)(this.req, this.res, this.next) - return assertCalledWith(this.request, { url: 'api.v1/resource/123' }) + return assertCalledWith(this.request, { + url: 'https://api.v1/resource/123', + }) }) it('set arbitrary options on request', function () { const target = { - baseUrl: 'api.v1', + baseUrl: 'https://api.v1', path: '/foo', options: { foo: 'bar' }, } @@ -146,12 +141,12 @@ describe('ProxyManager', function () { this.proxyManager.createProxy(target)(this.req, this.res, this.next) return assertCalledWith(this.request, { foo: 'bar', - url: 'api.v1/foo', + url: 'https://api.v1/foo', }) }) it('passes cookies', function () { - const target = { baseUrl: 'api.v1', path: '/foo' } + const target = { baseUrl: 'https://api.v1', path: '/foo' } this.req.url = '/foo' this.req.route.path = '/foo' this.req.headers = { cookie: 'cookie' } @@ -160,13 +155,13 @@ describe('ProxyManager', function () { headers: { Cookie: 'cookie', }, - url: 'api.v1/foo', + url: 'https://api.v1/foo', }) }) it('passes body for post', function () { const target = { - baseUrl: 'api.v1', + baseUrl: 'https://api.v1', path: '/foo', options: { method: 'post' }, } @@ -179,13 +174,13 @@ describe('ProxyManager', function () { foo: 'bar', }, method: 'post', - url: 'api.v1/foo', + url: 'https://api.v1/foo', }) }) it('passes body for put', function () { const target = { - baseUrl: 'api.v1', + baseUrl: 'https://api.v1', path: '/foo', options: { method: 'put' }, } @@ -198,7 +193,7 @@ describe('ProxyManager', function () { foo: 'bar', }, method: 'put', - url: 'api.v1/foo', + url: 'https://api.v1/foo', }) }) })