From 58e82c00b43067e97360af12e73a9990676d32e4 Mon Sep 17 00:00:00 2001 From: Chris Banks Date: Fri, 7 Jun 2024 14:00:29 +0100 Subject: [PATCH 1/2] Fix regex misuse. - Overbroad matching of hostnames: `.` vs `\.` - Overuse of `RegExp.prototype.match` where `String.prototype.includes` or `RegExp.prototype.test` suffice and better communicate the intent by returning a boolean. - Use `Location.hostname` and not `Location.host`, because we're already assuming there isn't a port on the end (e.g. `:8080`). This avoids further complicating the (corrected) regexes. Should resolve: - https://github.com/alphagov/govuk-browser-extension/security/code-scanning/1 - https://github.com/alphagov/govuk-browser-extension/security/code-scanning/2 - https://github.com/alphagov/govuk-browser-extension/security/code-scanning/3 - https://github.com/alphagov/govuk-browser-extension/security/code-scanning/4 I don't believe these are exploitable, hence raising a regular PR. --- spec/javascripts/environment.spec.js | 2 +- src/fetch-page-data.js | 2 +- src/popup/content_links.js | 7 ++++--- src/popup/environment.js | 11 ++++------- src/popup/extract_path.js | 18 +++++++++--------- src/popup/popup.js | 12 ++++++------ 6 files changed, 25 insertions(+), 27 deletions(-) diff --git a/spec/javascripts/environment.spec.js b/spec/javascripts/environment.spec.js index 66f7bb0..5d34e2f 100644 --- a/spec/javascripts/environment.spec.js +++ b/spec/javascripts/environment.spec.js @@ -2,7 +2,7 @@ describe('Popup.environment', function () { function createEnvironmentForUrl (location) { var a = document.createElement('a') a.href = location - return Popup.environment(a.href, a.host, a.origin).allEnvironments + return Popup.environment(a.href, a.hostname, a.origin).allEnvironments } it('returns the correct environment links when the user is on production', function () { diff --git a/src/fetch-page-data.js b/src/fetch-page-data.js index 396765b..2e82b2c 100644 --- a/src/fetch-page-data.js +++ b/src/fetch-page-data.js @@ -5,7 +5,7 @@ chrome.runtime.sendMessage({ action: 'populatePopup', currentLocation: window.location.href, - currentHost: window.location.host, + currentHost: window.location.hostname, currentOrigin: window.location.origin, currentPathname: window.location.pathname, renderingApplication: getMetatag('govuk:rendering-application'), diff --git a/src/popup/content_links.js b/src/popup/content_links.js index 818f7e1..9e2e0f5 100644 --- a/src/popup/content_links.js +++ b/src/popup/content_links.js @@ -6,12 +6,13 @@ Popup.generateContentLinks = function (location, origin, pathname, currentEnviro // If no path can be found (which means we're probably in a publishing app) // Similarly if we're on GOVUK account, not many of the links are relevant - if (!path || origin.match(/www.account/)) { + if (!path || origin.includes('www.account')) { return [] } - // This is 'https://www.gov.uk' or 'https://www-origin.integration.publishing.service.gov.uk/', etc. - if (origin === 'http://webarchive.nationalarchives.gov.uk' || origin.match(/draft-origin/) || origin.match(/content-data/) || origin.match(/support/)) { + // Origin looks like 'https://www.gov.uk' or 'https://www-origin.integration.publishing.service.gov.uk/' or similar. + if (origin === 'http://webarchive.nationalarchives.gov.uk' || + /draft-origin|content-data|support/.test(origin)) { origin = 'https://www.gov.uk' } diff --git a/src/popup/environment.js b/src/popup/environment.js index d755321..2b21297 100644 --- a/src/popup/environment.js +++ b/src/popup/environment.js @@ -1,18 +1,16 @@ var Popup = Popup || {} -// Given a location, host and origin, generate URLs for all GOV.UK environments. +// Given a location, hostname and origin, generate URLs for all GOV.UK environments. // // Returns a hash with envs, including one with `class: "current"` to show // the current environment. Popup.environment = function (location, host, origin) { function isPartOfGOVUK () { - return host.match(/www.gov.uk/) || - host.match(/publishing.service.gov.uk/) || - host.match(/dev.gov.uk/) + return /^(www|.*\.publishing\.service|(www\.)?dev)\.gov\.uk$/.test(host) } function isGOVUKAccount () { - return host.match(/www.account/) || host.match(/login.service.dev/) + return /^(www\.account.*\.service\.gov\.uk|login\.service\.dev)$/.test(host) } if (!isPartOfGOVUK()) { @@ -28,7 +26,6 @@ Popup.environment = function (location, host, origin) { host: 'https://www.gov.uk', origin: origin } - } } @@ -64,7 +61,7 @@ Popup.environment = function (location, host, origin) { ] var application = isGOVUKAccount() ? host.split('.')[1] : host.split('.')[0] - var inFrontend = application.match(/www/) && !isGOVUKAccount() + var inFrontend = application.includes('www') && !isGOVUKAccount() var environments = ENVIRONMENTS var currentEnvironment diff --git a/src/popup/extract_path.js b/src/popup/extract_path.js index c0ae5ad..9e15f2e 100644 --- a/src/popup/extract_path.js +++ b/src/popup/extract_path.js @@ -5,23 +5,23 @@ var Popup = Popup || {} Popup.extractPath = function (location, pathname, renderingApplication) { var extractedPath - if (location.match(/api\/content/)) { + if (location.includes('api/content')) { extractedPath = pathname.replace('api/content/', '') - } else if (location.match(/anonymous_feedback/)) { + } else if (location.includes('anonymous_feedback')) { extractedPath = extractQueryParameter(location, 'path') - } else if (location.match(/content-data/)) { + } else if (location.includes('content-data')) { extractedPath = pathname.replace('metrics/', '') - } else if (location.match(/nationalarchives.gov.uk/)) { + } else if (location.includes('nationalarchives.gov.uk')) { extractedPath = pathname.split('https://www.gov.uk')[1] - } else if (location.match(/api\/search.json/)) { + } else if (location.includes('api/search.json')) { extractedPath = extractQueryParameter(location, 'filter_link') - } else if (location.match(/info/)) { + } else if (location.includes('info')) { extractedPath = pathname.replace('info/', '') - } else if (location.match(/api.*\.json/)) { + } else if (/api.*\.json/.test(location)) { extractedPath = pathname.replace('api/', '').replace('.json', '') - } else if (location.match(/visualise/)) { + } else if (location.includes('visualise')) { extractedPath = pathname.replace('/y/visualise', '') - } else if (location.match(/www/) || location.match(/draft-origin/)) { + } else if (/www|draft-origin/.test(location)) { extractedPath = pathname } diff --git a/src/popup/popup.js b/src/popup/popup.js index 829b143..bfeea6f 100644 --- a/src/popup/popup.js +++ b/src/popup/popup.js @@ -71,9 +71,9 @@ var Popup = Popup || {}; }) // Render the popup. - function renderPopup (location, host, origin, pathname, renderingApplication, windowHeight, abTestBuckets) { + function renderPopup (location, hostname, origin, pathname, renderingApplication, windowHeight, abTestBuckets) { // Creates a view object with the data and render a template with it. - var view = createView(location, host, origin, pathname, renderingApplication, abTestBuckets) + var view = createView(location, hostname, origin, pathname, renderingApplication, abTestBuckets) var contentStore = view.contentLinks.find(function (el) { return el.name === 'Content item (JSON)' }) @@ -162,7 +162,7 @@ var Popup = Popup || {}; // TODO: we're not actually re-rendering the popup correctly here, because // we don't have access to the DOM here. This is a temporary solution to // make most functionality work after the user clicks a button in the popup. - renderPopup(location.href, location.host, location.origin, location.pathname, {}) + renderPopup(location.href, location.hostname, location.origin, location.pathname, {}) }) }) @@ -212,11 +212,11 @@ var Popup = Popup || {}; }) } - // This is the view object. It takes a location, host, origin, the name of the + // This is the view object. It takes a location, hostname, origin, the name of the // rendering app and a list of A/B test buckets and creates an object with all // URLs and other view data to render the popup. - function createView (location, host, origin, pathname, renderingApplication, abTestBuckets) { - var environment = Popup.environment(location, host, origin) + function createView (location, hostname, origin, pathname, renderingApplication, abTestBuckets) { + var environment = Popup.environment(location, hostname, origin) var contentLinks = Popup.generateContentLinks(location, origin, pathname, environment.currentEnvironment, renderingApplication) // var abTests = Popup.findActiveAbTests(abTestBuckets) From 29e4db589251245db5e11dfdc7bf43f06984e324 Mon Sep 17 00:00:00 2001 From: Chris Banks Date: Fri, 7 Jun 2024 14:35:05 +0100 Subject: [PATCH 2/2] Parse URL properly when matching nationalarchives. Otherwise we just substitute one CodeQL warning for another :/ --- src/popup/extract_path.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/popup/extract_path.js b/src/popup/extract_path.js index 9e15f2e..7f41300 100644 --- a/src/popup/extract_path.js +++ b/src/popup/extract_path.js @@ -3,6 +3,7 @@ var Popup = Popup || {} // Extract the relevant path from a location, such as `/foo` from URLs like // `www.gov.uk/foo` and `www.gov.uk/api/content/foo`. Popup.extractPath = function (location, pathname, renderingApplication) { + const url = new URL(location) var extractedPath if (location.includes('api/content')) { @@ -11,7 +12,7 @@ Popup.extractPath = function (location, pathname, renderingApplication) { extractedPath = extractQueryParameter(location, 'path') } else if (location.includes('content-data')) { extractedPath = pathname.replace('metrics/', '') - } else if (location.includes('nationalarchives.gov.uk')) { + } else if (/\.?nationalarchives\.gov\.uk$/.test(url.hostname)) { extractedPath = pathname.split('https://www.gov.uk')[1] } else if (location.includes('api/search.json')) { extractedPath = extractQueryParameter(location, 'filter_link')