diff --git a/lib/languages.js b/lib/languages.js index 63f2ed2abb..c3de7d6d40 100644 --- a/lib/languages.js +++ b/lib/languages.js @@ -1,4 +1,7 @@ -// see also languages-schema.js +// See also languages-schema.js +// Nota bene: If you are adding a new language, +// change accept-language handling in CDN config as well. + import dotenv from 'dotenv' import { TRANSLATIONS_ROOT } from './constants.js' diff --git a/middleware/archived-enterprise-versions.js b/middleware/archived-enterprise-versions.js index 745dbf906a..74594d966a 100644 --- a/middleware/archived-enterprise-versions.js +++ b/middleware/archived-enterprise-versions.js @@ -13,7 +13,7 @@ import isArchivedVersion from '../lib/is-archived-version.js' import { setFastlySurrogateKey, SURROGATE_ENUMS } from './set-fastly-surrogate-key.js' import got from 'got' import { readCompressedJsonFileFallbackLazily } from '../lib/read-json-file.js' -import { archivedCacheControl, noCacheControl } from './cache-control.js' +import { archivedCacheControl, languageCacheControl } from './cache-control.js' import { pathLanguagePrefixed, languagePrefixPathRegex } from '../lib/languages.js' import getRedirect, { splitPathByLanguage } from '../lib/get-redirect.js' @@ -100,11 +100,10 @@ export default async function archivedEnterpriseVersions(req, res, next) { if (deprecatedWithFunctionalRedirects.includes(requestedVersion)) { const redirectTo = getRedirect(req.path, req.context) if (redirectTo) { - if (redirectCode === 301) { - archivedCacheControl(res) - } else { - noCacheControl(res) + if (redirectCode === 302) { + languageCacheControl(res) // call first to get `vary` } + archivedCacheControl(res) // call second to extend duration return res.redirect(redirectCode, redirectTo) } @@ -120,11 +119,10 @@ export default async function archivedEnterpriseVersions(req, res, next) { const [language, withoutLanguage] = splitPathByLanguage(req.path, req.context.userLanguage) const newRedirectTo = redirectJson[withoutLanguage] if (newRedirectTo) { - if (redirectCode === 301) { - archivedCacheControl(res) - } else { - noCacheControl(res) + if (redirectCode === 302) { + languageCacheControl(res) // call first to get `vary` } + archivedCacheControl(res) // call second to extend duration return res.redirect(redirectCode, `/${language}${newRedirectTo}`) } } diff --git a/middleware/cache-control.js b/middleware/cache-control.js index ba64dc0352..8cd603e117 100644 --- a/middleware/cache-control.js +++ b/middleware/cache-control.js @@ -55,3 +55,12 @@ export const noCacheControl = cacheControlFactory(0) // Long caching for archived pages and assets export const archivedCacheControl = cacheControlFactory(60 * 60 * 24 * 365) + +// Vary on language when needed +// x-user-language is a custom request header derived from req.cookie:user_language +// accept-language is truncated to one of our available languages +// https://bit.ly/3u5UeRN +export function languageCacheControl(res) { + defaultCacheControl(res) + res.set('vary', 'accept-language, x-user-language') +} diff --git a/middleware/fast-root-redirect.js b/middleware/fast-root-redirect.js deleted file mode 100644 index d44ac85e49..0000000000 --- a/middleware/fast-root-redirect.js +++ /dev/null @@ -1,21 +0,0 @@ -import { cacheControlFactory } from './cache-control.js' -import { getLanguageCodeFromHeader } from './detect-language.js' -import { USER_LANGUAGE_COOKIE_NAME } from '../lib/constants.js' - -const cacheControl = cacheControlFactory(0) -const noCacheSurrogateControl = cacheControlFactory(0, { - key: 'surrogate-control', - maxAgeZero: true, -}) - -export default function fastRootRedirect(req, res, next) { - if (!req.headers.cookie || !req.headers.cookie.includes(USER_LANGUAGE_COOKIE_NAME)) { - // No preferred language cookie header! - const language = getLanguageCodeFromHeader(req) || 'en' - cacheControl(res) - // See #2287 - noCacheSurrogateControl(res) - return res.redirect(`/${language}`) - } - next() -} diff --git a/middleware/index.js b/middleware/index.js index d642bc62d3..d47d0ce5c8 100644 --- a/middleware/index.js +++ b/middleware/index.js @@ -56,7 +56,6 @@ import favicons from './favicons.js' import setStaticAssetCaching from './static-asset-caching.js' import fastHead from './fast-head.js' import fastlyCacheTest from './fastly-cache-test.js' -import fastRootRedirect from './fast-root-redirect.js' import trailingSlashes from './trailing-slashes.js' import fastlyBehavior from './fastly-behavior.js' @@ -189,7 +188,6 @@ export default function (app) { } // *** Early exits *** - app.get('/', fastRootRedirect) app.use(instrument(handleInvalidPaths, './handle-invalid-paths')) app.use(instrument(handleNextDataPath, './handle-next-data-path')) diff --git a/middleware/redirects/handle-redirects.js b/middleware/redirects/handle-redirects.js index ea04f02e80..819118d790 100644 --- a/middleware/redirects/handle-redirects.js +++ b/middleware/redirects/handle-redirects.js @@ -3,10 +3,7 @@ import { URL } from 'url' import { pathLanguagePrefixed } from '../../lib/languages.js' import { deprecatedWithFunctionalRedirects } from '../../lib/enterprise-server-releases.js' import getRedirect from '../../lib/get-redirect.js' -import { cacheControlFactory } from '../cache-control.js' - -const cacheControl = cacheControlFactory(60 * 60 * 24) // one day -const noCacheControl = cacheControlFactory(0) +import { defaultCacheControl, languageCacheControl } from '../cache-control.js' export default function handleRedirects(req, res, next) { // never redirect assets @@ -20,7 +17,7 @@ export default function handleRedirects(req, res, next) { // blanket redirects for languageless homepage if (req.path === '/') { const language = getLanguage(req) - noCacheControl(res) + languageCacheControl(res) return res.redirect(302, `/${language}`) } @@ -154,9 +151,9 @@ export default function handleRedirects(req, res, next) { // do the redirect if the from-URL already had a language in it if (pathLanguagePrefixed(req.path) || redirect.includes('://')) { - cacheControl(res) + defaultCacheControl(res) } else { - noCacheControl(res) + languageCacheControl(res) } const permanent = redirect.includes('://') || usePermanentRedirect(req) diff --git a/tests/rendering/server.js b/tests/rendering/server.js index a74d35f2e5..37058f64db 100644 --- a/tests/rendering/server.js +++ b/tests/rendering/server.js @@ -609,8 +609,11 @@ describe('server', () => { const res = await get('/articles/deleting-a-team', { followRedirects: false }) expect(res.statusCode).toBe(302) expect(res.headers['set-cookie']).toBeUndefined() - // no cache control because a language prefix had to be injected - expect(res.headers['cache-control']).toBe('private, no-store') + // language specific caching + expect(res.headers['cache-control']).toContain('public') + expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) + expect(res.headers.vary).toContain('accept-language') + expect(res.headers.vary).toContain('x-user-language') }) test('redirects old articles to their slugified URL', async () => { @@ -625,8 +628,12 @@ describe('server', () => { const res = await get('/') expect(res.statusCode).toBe(302) expect(res.headers.location).toBe('/en') - expect(res.headers['cache-control']).toBe('private, no-store') expect(res.headers['set-cookie']).toBeUndefined() + // language specific caching + expect(res.headers['cache-control']).toContain('public') + expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) + expect(res.headers.vary).toContain('accept-language') + expect(res.headers.vary).toContain('x-user-language') }) // This test exists because in a previous life, our NextJS used to @@ -644,8 +651,12 @@ describe('server', () => { expect(res.statusCode).toBe(302) expect(res.headers.location).toBe('/en') - expect(res.headers['cache-control']).toBe('private, no-store') expect(res.headers['set-cookie']).toBeUndefined() + // language specific caching + expect(res.headers['cache-control']).toContain('public') + expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) + expect(res.headers.vary).toContain('accept-language') + expect(res.headers.vary).toContain('x-user-language') }) test('redirects / to /en when unsupported language preference is specified', async () => { @@ -658,8 +669,12 @@ describe('server', () => { }) expect(res.statusCode).toBe(302) expect(res.headers.location).toBe('/en') - expect(res.headers['cache-control']).toBe('private, no-store') expect(res.headers['set-cookie']).toBeUndefined() + // language specific caching + expect(res.headers['cache-control']).toContain('public') + expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) + expect(res.headers.vary).toContain('accept-language') + expect(res.headers.vary).toContain('x-user-language') }) test('adds English prefix to old article URLs', async () => { @@ -667,8 +682,11 @@ describe('server', () => { expect(res.statusCode).toBe(302) expect(res.headers.location.startsWith('/en/')).toBe(true) expect(res.headers['set-cookie']).toBeUndefined() - // no cache control because a language prefix had to be injected - expect(res.headers['cache-control']).toBe('private, no-store') + // language specific caching + expect(res.headers['cache-control']).toContain('public') + expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) + expect(res.headers.vary).toContain('accept-language') + expect(res.headers.vary).toContain('x-user-language') }) test('redirects that not only injects /en/ should have cache-control', async () => { diff --git a/tests/routing/deprecated-enterprise-versions.js b/tests/routing/deprecated-enterprise-versions.js index b02820863e..1a94d532f6 100644 --- a/tests/routing/deprecated-enterprise-versions.js +++ b/tests/routing/deprecated-enterprise-versions.js @@ -95,8 +95,11 @@ describe('recently deprecated redirects', () => { expect(res.statusCode).toBe(302) expect(res.headers.location).toBe('/en/enterprise-server@3.0') expect(res.headers['set-cookie']).toBeUndefined() - // Deliberately no cache control because it is user-dependent - expect(res.headers['cache-control']).toBe('private, no-store') + // language specific caching + expect(res.headers['cache-control']).toContain('public') + expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) + expect(res.headers.vary).toContain('accept-language') + expect(res.headers.vary).toContain('x-user-language') }) test('already languaged enterprise 3.0 redirects', async () => { @@ -108,14 +111,18 @@ describe('recently deprecated redirects', () => { expect(res.headers['cache-control']).toContain('public') expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) }) + test('redirects enterprise-server 3.0 with actual redirect without language', async () => { const res = await get( '/enterprise-server@3.0/github/getting-started-with-github/githubs-products' ) expect(res.statusCode).toBe(302) expect(res.headers['set-cookie']).toBeUndefined() - // Deliberately no cache control because it is user-dependent - expect(res.headers['cache-control']).toBe('private, no-store') + // language specific caching + expect(res.headers['cache-control']).toContain('public') + expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) + expect(res.headers.vary).toContain('accept-language') + expect(res.headers.vary).toContain('x-user-language') // This is based on // https://github.com/github/help-docs-archived-enterprise-versions/blob/master/3.0/redirects.json expect(res.headers.location).toBe( diff --git a/tests/routing/redirects.js b/tests/routing/redirects.js index 743353bb29..96e6e18947 100644 --- a/tests/routing/redirects.js +++ b/tests/routing/redirects.js @@ -126,10 +126,14 @@ describe('redirects', () => { const res = await get('/') expect(res.statusCode).toBe(302) expect(res.headers.location).toBe('/en') - expect(res.headers['cache-control']).toBe('private, no-store') + // language specific caching + expect(res.headers['cache-control']).toContain('public') + expect(res.headers['cache-control']).toMatch(/max-age=\d+/) + expect(res.headers.vary).toContain('accept-language') + expect(res.headers.vary).toContain('x-user-language') }) - test('trailing slash on languaged homepage should permantently redirect', async () => { + test('trailing slash on languaged homepage should permanently redirect', async () => { const res = await get('/en/') expect(res.statusCode).toBe(301) expect(res.headers.location).toBe('/en')