Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/app/lib/config/services/archive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ export const service: DefaultServiceConfig = {
publishingPrinciples: null,
isTrustProjectParticipant: false,
script: latin,
swPath: '/articles/sw.js',
homePageTitle: 'Home',
showAdPlaceholder: false,
showRelatedTopics: true,
Expand Down
1 change: 0 additions & 1 deletion src/app/lib/config/services/scotland.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ export const service: DefaultServiceConfig = {
publishingPrinciples: null,
isTrustProjectParticipant: false,
script: latin,
swPath: '/articles/sw.js',
homePageTitle: 'Home',
passportHomes: ['BBCScotland'],
showAdPlaceholder: false,
Expand Down
5 changes: 0 additions & 5 deletions src/app/routes/utils/regex/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import services from '../../../lib/config/services/loadableConfig';
import {
getArticleRegex,
getArticleSwRegex,
getArticleManifestRegex,
getHomePageRegex,
getSwRegex,
getManifestRegex,
Expand All @@ -27,9 +25,6 @@ const allServices = Object.keys(services);
export const articlePath = getArticleRegex(allServices);
export const articleDataPath = `${articlePath}.json`;

export const articleSwPath = getArticleSwRegex(allServices);
export const articleManifestPath = getArticleManifestRegex(allServices);

export const homePageSwPath = getSwRegex(allServices);
export const homePageManifestPath = getManifestRegex(allServices);
export const homePagePath = getHomePageRegex(allServices);
Expand Down
49 changes: 5 additions & 44 deletions src/app/routes/utils/regex/index.test.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import { matchPath } from 'react-router-dom';
import {
articleDataPath,
articleManifestPath,
articlePath,
articleSwPath,
cpsAssetPageDataPath,
cpsAssetPagePath,
homePagePath,
Expand Down Expand Up @@ -124,55 +122,16 @@ describe('homePageDataPath', () => {
shouldNotMatchInvalidRoutes(invalidRoutes, homePageDataPath);
});

describe('articleSwPath', () => {
const validRoutes = [
'/news/articles/sw.js',
'/persian/articles/sw.js',
'/cymrufyw/erthyglau/sw.js',
];
shouldMatchValidRoutes(validRoutes, articleSwPath);

const invalidRoutes = [
'/news/sw.js',
'/persian/articles/sw',
'/news/trad/sw.js',
'/cymrufyw/sw.js',
];
shouldNotMatchInvalidRoutes(invalidRoutes, articleSwPath);
});

describe('articleManifestPath', () => {
const validRoutes = [
'/persian/articles/manifest.json',
'/serbian/articles/manifest.json',
];
shouldMatchValidRoutes(validRoutes, articleManifestPath);

const invalidRoutes = [
'/news/articles/manifest.json',
'/sport/articles/manifest.json',
'/naidheachdan/sgeulachdan/manifest.json',
'/cymrufyw/erthyglau/manifest.json',
'/newsround/articles/manifest.json',
'/news/manifest.json',
'/sport/manifest.json',
'/naidheachdan/manifest.json',
'/cymrufyw/manifest.json',
'/newsround/manifest.json',
'/persian/articles/manifest',
'/news/simp/sw.js',
];
shouldNotMatchInvalidRoutes(invalidRoutes, articleManifestPath);
});

describe('homePageSwPath', () => {
const validRoutes = ['/news/sw.js', '/persian/sw.js'];
const validRoutes = ['/gahuza/sw.js', '/persian/sw.js'];
shouldMatchValidRoutes(validRoutes, homePageSwPath);

const invalidRoutes = [
'/news/sw.js',
'/news/articles/sw.js',
'/persian/sw',
'/persian/simp/sw.js',
'/gahuza/articles/sw.js',
];
shouldNotMatchInvalidRoutes(invalidRoutes, homePageSwPath);
});
Expand All @@ -190,6 +149,8 @@ describe('homePageManifestPath', () => {
'/foobar/manifest.json',
'/foobar/manifest',
'/news/trad/sw.js',
'/persian/articles/manifest.json',
'/serbian/articles/manifest.json',
];
shouldNotMatchInvalidRoutes(invalidRoutes, homePageManifestPath);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,8 @@

exports[`regex utils snapshots should create expected regex from getAfricaEyeTVPageRegex 1`] = `"/worldservice/tv/africa_eye/:episodeId([a-z0-9]+)?:lite(.lite)?"`;

exports[`regex utils snapshots should create expected regex from getArticleManifestRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|azeri|bengali|burmese|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|nepali|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/:local(articles|erthyglau|sgeulachdan)/manifest.json"`;

exports[`regex utils snapshots should create expected regex from getArticleRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|archive|azeri|bengali|burmese|cymrufyw|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|naidheachdan|nepali|news|newsround|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|sport|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)?:discipline(/[a-z0-9-_]{1,})?/:local(articles|erthyglau|sgeulachdan)/:id(c[a-zA-Z0-9]{10}o):variant(/simp|/trad|/cyr|/lat)?:nonCanonicalArticleRenderPlatform(.amp|.app|.lite)?"`;

exports[`regex utils snapshots should create expected regex from getArticleSwRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|archive|azeri|bengali|burmese|cymrufyw|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|naidheachdan|nepali|news|newsround|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|sport|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/:local(articles|erthyglau|sgeulachdan)/sw.js"`;

exports[`regex utils snapshots should create expected regex from getCpsAssetRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|archive|azeri|bengali|burmese|cymrufyw|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|naidheachdan|nepali|news|newsround|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|sport|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen):variant(/simp|/trad|/cyr|/lat)?/:assetUri([a-z0-9-_+]{0,}[0-9]{8,}):amp(.amp)?:lite(.lite)?"`;

exports[`regex utils snapshots should create expected regex from getErrorPageRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|archive|azeri|bengali|burmese|cymrufyw|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|naidheachdan|nepali|news|newsround|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|sport|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/:errorCode(404|500):variant(/simp|/trad|/cyr|/lat)?:lite(.lite)?"`;
Expand All @@ -18,7 +14,7 @@ exports[`regex utils snapshots should create expected regex from getLegacyAssetR

exports[`regex utils snapshots should create expected regex from getLiveRadioRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|archive|azeri|bengali|burmese|cymrufyw|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|naidheachdan|nepali|news|newsround|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|sport|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/:masterBrand(bbc_[a-z]+_radio)/:mediaId(liveRadio):lite(.lite)?"`;

exports[`regex utils snapshots should create expected regex from getManifestRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|azeri|bengali|burmese|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|nepali|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/manifest.json"`;
exports[`regex utils snapshots should create expected regex from getManifestRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|azeri|bengali|burmese|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|nepali|pashto|persian|pidgin|portuguese|punjabi|russian|serbian|sinhala|somali|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/manifest.json"`;

exports[`regex utils snapshots should create expected regex from getMostReadDataRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|archive|azeri|bengali|burmese|cymrufyw|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|naidheachdan|nepali|news|newsround|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|sport|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/mostread:variant(/simp|/trad|/cyr|/lat)?.json"`;

Expand All @@ -36,6 +32,6 @@ exports[`regex utils snapshots should create expected regex from getRecommendati

exports[`regex utils snapshots should create expected regex from getSecondaryColumnDataRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|archive|azeri|bengali|burmese|cymrufyw|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|naidheachdan|nepali|news|newsround|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|sport|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/sty-secondary-column:variant(/simp|/trad|/cyr|/lat)?.json"`;

exports[`regex utils snapshots should create expected regex from getSwRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|archive|azeri|bengali|burmese|cymrufyw|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|naidheachdan|nepali|news|newsround|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|sport|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/sw.js"`;
exports[`regex utils snapshots should create expected regex from getSwRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|azeri|bengali|burmese|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|nepali|pashto|persian|pidgin|portuguese|punjabi|russian|serbian|sinhala|somali|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/sw.js"`;

exports[`regex utils snapshots should create expected regex from getTopicPageRegex 1`] = `"/:service(afaanoromoo|afrique|amharic|arabic|archive|azeri|bengali|burmese|cymrufyw|gahuza|gujarati|hausa|hindi|igbo|indonesia|japanese|korean|kyrgyz|marathi|mundo|naidheachdan|nepali|news|newsround|pashto|persian|pidgin|portuguese|punjabi|russian|scotland|serbian|sinhala|somali|sport|swahili|tamil|telugu|thai|tigrinya|turkce|ukchina|ukrainian|urdu|uzbek|vietnamese|ws|yoruba|zhongwen)/topics/:id([a-z0-9]+)?:variant(/simp|/trad|/cyr|/lat)?:lite(.lite)?"`;
13 changes: 2 additions & 11 deletions src/app/routes/utils/regex/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ export const getArticleRegex = services => {
return `/:service(${serviceRegex})?:discipline(${sportDisciplineRegex})?/:local(${articleLocalRegex})/:id(${idRegex}):variant(${variantRegex})?:nonCanonicalArticleRenderPlatform(${nonCanonicalArticleRenderPlatform})?`;
};

export const getArticleSwRegex = services => {
const serviceRegex = getServiceRegex(services);
return `/:service(${serviceRegex})/:local(${articleLocalRegex})/sw.js`;
};

const getWorldServices = services => {
const publicServices = [
'news',
Expand All @@ -35,23 +30,19 @@ const getWorldServices = services => {
'cymrufyw',
'naidheachdan',
'archive',
'scotland',
];

return services.filter(service => !publicServices.includes(service));
};

export const getArticleManifestRegex = services => {
const serviceRegex = getServiceRegex(getWorldServices(services));
return `/:service(${serviceRegex})/:local(${articleLocalRegex})/manifest.json`;
};

export const getHomePageRegex = services => {
const homePageServiceRegex = getServiceRegex(services);
return `/:service(${homePageServiceRegex}):variant(${variantRegex})?:lite(${liteRegex})?`;
};

export const getSwRegex = services => {
const serviceRegex = getServiceRegex(services);
const serviceRegex = getServiceRegex(getWorldServices(services));
return `/:service(${serviceRegex})/sw.js`;
};

Expand Down
6 changes: 2 additions & 4 deletions src/server/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
import logResponseTime from './utilities/logResponseTime';
import renderDocument from './Document';
import {
articleManifestPath,
articleSwPath,
homePageManifestPath,
homePageSwPath,
} from '../app/routes/utils/regex';
Expand Down Expand Up @@ -112,34 +110,34 @@
* Application env routes
*/
server
.get([articleSwPath, homePageSwPath], (req, res) => {
.get([homePageSwPath], (req, res) => {
const swPath = `${__dirname}/public/sw.js`;
res.set(
`Cache-Control`,
`public, stale-if-error=6000, stale-while-revalidate=600, max-age=300`,
);
res.sendFile(swPath, {}, error => {
if (error) {
logger.error(SERVICE_WORKER_SENDFILE_ERROR, { error });
res.status(500).send(`Unable to find service worker in ${swPath}`);
}
});
})

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
, but is not rate-limited.
.get([articleManifestPath, homePageManifestPath], async ({ params }, res) => {
.get([homePageManifestPath], async ({ params }, res) => {
const { service } = params;
const variant = defaultServiceVariants[service] || 'default';
const manifestPath = `${__dirname}/public${serviceConfigs[service][variant].manifestPath}`;
res.set(
'Cache-Control',
'public, stale-if-error=172800, stale-while-revalidate=172800, max-age=86400',
);
res.sendFile(manifestPath, {}, error => {
if (error) {
logger.error(MANIFEST_SENDFILE_ERROR, { error });
res.status(500).send('Unable to find manifest.');
}
});
});

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
, but is not rate-limited.

// Set Up Local Server
if (process.env.SIMORGH_APP_ENV === 'local') {
Expand Down
17 changes: 4 additions & 13 deletions src/server/index.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -952,13 +952,6 @@ describe('Server', () => {
});

describe('Service workers', () => {
it('should serve a file for existing service workers', async () => {
Copy link
Contributor

@karinathomasbbc karinathomasbbc Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make sure there's still a test for existence of /world-service/sw.js? (choose one of the world services as an example)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, we should def keep it. Updated

await makeRequest('/news/articles/sw.js');
expect(sendFileSpy.mock.calls[0][0]).toEqual(
path.join(__dirname, '/public/sw.js'),
);
});

it('should not serve a file for non-existing service workers', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we update this test to use one of the public services as the test (instead of some-service)

const { statusCode } = await makeRequest('/some-service/articles/sw.js');
expect(sendFileSpy.mock.calls.length).toEqual(0);
Expand All @@ -975,11 +968,9 @@ describe('Server', () => {

describe('Manifest json', () => {
it.each`
manifestPath | expectedManifestFile
${'/pidgin/articles/manifest.json'} | ${'/pidgin/manifest.json'}
${'/pidgin/manifest.json'} | ${'/pidgin/manifest.json'}
${'/serbian/articles/manifest.json'} | ${'/serbian/manifest.json'}
${'/serbian/manifest.json'} | ${'/serbian/manifest.json'}
manifestPath | expectedManifestFile
${'/pidgin/manifest.json'} | ${'/pidgin/manifest.json'}
${'/serbian/manifest.json'} | ${'/serbian/manifest.json'}
`(
'should serve a file for $manifestPath',
async ({ manifestPath, expectedManifestFile }) => {
Expand All @@ -997,7 +988,7 @@ describe('Server', () => {
});

it('should serve a response cache control of 1 day', async () => {
const { header } = await makeRequest('/pidgin/articles/manifest.json');
const { header } = await makeRequest('/pidgin/manifest.json');
expect(header['cache-control']).toBe(
'public, stale-if-error=172800, stale-while-revalidate=172800, max-age=86400',
);
Expand Down
Loading