diff --git a/content/developers/overview/managing-deploy-keys.md b/content/developers/overview/managing-deploy-keys.md index 3c08f03ccc..252038449e 100644 --- a/content/developers/overview/managing-deploy-keys.md +++ b/content/developers/overview/managing-deploy-keys.md @@ -4,6 +4,9 @@ intro: Learn different ways to manage SSH keys on your servers when you automate redirect_from: - /guides/managing-deploy-keys - /v3/guides/managing-deploy-keys + - /deploy-keys + - /articles/managing-deploy-keys + - /multiple-keys versions: fpt: '*' ghes: '*' diff --git a/content/developers/overview/using-ssh-agent-forwarding.md b/content/developers/overview/using-ssh-agent-forwarding.md index c1e795b375..cb7a2ce912 100644 --- a/content/developers/overview/using-ssh-agent-forwarding.md +++ b/content/developers/overview/using-ssh-agent-forwarding.md @@ -4,6 +4,7 @@ intro: 'To simplify deploying to a server, you can set up SSH agent forwarding t redirect_from: - /guides/using-ssh-agent-forwarding - /v3/guides/using-ssh-agent-forwarding + - /articles/using-ssh-agent-forwarding versions: fpt: '*' ghes: '*' diff --git a/content/developers/webhooks-and-events/webhooks/testing-webhooks.md b/content/developers/webhooks-and-events/webhooks/testing-webhooks.md index 964e623323..86e3f26ef4 100644 --- a/content/developers/webhooks-and-events/webhooks/testing-webhooks.md +++ b/content/developers/webhooks-and-events/webhooks/testing-webhooks.md @@ -4,6 +4,7 @@ intro: 'Review your webhook deliveries on {% data variables.product.prodname_dot redirect_from: - /webhooks/testing - /developers/webhooks-and-events/testing-webhooks + - /articles/testing-webhooks versions: fpt: '*' ghes: '*' diff --git a/lib/get-redirect.js b/lib/get-redirect.js index dc87b3caee..d26551b710 100644 --- a/lib/get-redirect.js +++ b/lib/get-redirect.js @@ -39,6 +39,10 @@ export default function getRedirect(uri, context) { // the old formatting of the version. So to leverage the redirects // from `developer.json` we'll look at it right away. if (withoutLanguage in redirects) { + // But only inject the language if it's NOT an external redirect + if (redirects[withoutLanguage].includes('://')) { + return redirects[withoutLanguage] + } return `/${language}` + redirects[withoutLanguage] } diff --git a/lib/redirects/external-sites.json b/lib/redirects/external-sites.json index 23e0dde8cd..00f0370fec 100644 --- a/lib/redirects/external-sites.json +++ b/lib/redirects/external-sites.json @@ -1,21 +1,14 @@ { "/articles/deploying-with-capistrano": "https://github.com/capistrano/capistrano/blob/master/README.md", - "/articles/managing-deploy-keys": "https://developer.github.com/guides/managing-deploy-keys/", - "/articles/testing-webhooks": "http://developer.github.com/webhooks/testing/", - "/articles/using-ssh-agent-forwarding": "https://developer.github.com/guides/using-ssh-agent-forwarding/", "/capistrano": "https://github.com/capistrano/capistrano/blob/master/README.md", "/contact-github": "https://support.github.com/contact", "/contact": "https://support.github.com/contact", - "/deploy-keys": "https://developer.github.com/guides/managing-deploy-keys", "/deploy-with-capistrano": "https://github.com/capistrano/capistrano/blob/master/README.md", - "/git-cheat-sheets": "http://cheat.errtheblog.com/s/git", "/git-man-pages": "https://git-scm.com/docs", - "/git-ready": "http://gitready.com/", + "/git-ready": "https://gitready.com/", "/git-scm": "https://git-scm.com/", - "/git-user-manual": "http://schacon.github.com/git/user-manual.html", - "/github-status": "https://status.github.com/messages", + "/github-status": "https://www.githubstatus.com/", "/github-support": "https://support.github.com", - "/multiple-keys": "https://developer.github.com/guides/managing-deploy-keys", "/pro-git": "https://git-scm.com/book", "/submodules": "https://git-scm.com/book/en/Git-Tools-Submodules", "/articles/github-security": "https://github.com/security", @@ -23,7 +16,5 @@ "/partnerships": "https://partner.github.com", "/desktop-classic": "https://desktop.github.com", "/github/copilot/research-recitation": "https://github.blog/2021-06-30-github-copilot-research-recitation/", - "/en/github/copilot/research-recitation": "https://github.blog/2021-06-30-github-copilot-research-recitation/", - "/early-access/github/copilot/research-recitation": "https://github.blog/2021-06-30-github-copilot-research-recitation/", - "/en/early-access/github/copilot/research-recitation": "https://github.blog/2021-06-30-github-copilot-research-recitation/" + "/early-access/github/copilot/research-recitation": "https://github.blog/2021-06-30-github-copilot-research-recitation/" } diff --git a/lib/redirects/precompile.js b/lib/redirects/precompile.js index 0ddcaef23a..9b7e7019ce 100755 --- a/lib/redirects/precompile.js +++ b/lib/redirects/precompile.js @@ -54,6 +54,9 @@ const DISK_CACHE_FILEPATH = path.join(__dirname, '.redirects-cache.json') const precompileRedirects = diskMemoize(DISK_CACHE_FILEPATH, async (pageList) => { const allRedirects = readCompressedJsonFileFallback('./lib/redirects/static/developer.json') + const externalRedirects = readCompressedJsonFileFallback('./lib/redirects/external-sites.json') + Object.assign(allRedirects, externalRedirects) + // Exception redirects are those that are essentially unicorn one-offs. // For example, we have redirects for documents that used to be on // `free-pro-team@latest` but have now been moved to diff --git a/middleware/index.js b/middleware/index.js index b2941076bb..117ca1e812 100644 --- a/middleware/index.js +++ b/middleware/index.js @@ -25,7 +25,6 @@ import handleNextDataPath from './handle-next-data-path.js' import detectLanguage from './detect-language.js' import context from './context.js' import shortVersions from './contextualizers/short-versions.js' -import redirectsExternal from './redirects/external.js' import languageCodeRedirects from './redirects/language-code-redirects.js' import handleRedirects from './redirects/handle-redirects.js' import findPage from './find-page.js' @@ -239,7 +238,6 @@ export default function (app) { // *** Redirects, 3xx responses *** // I ordered these by use frequency app.use(instrument(trailingSlashes, './redirects/trailing-slashes')) - app.use(instrument(redirectsExternal, './redirects/external')) app.use(instrument(languageCodeRedirects, './redirects/language-code-redirects')) // Must come before contextualizers app.use(instrument(handleRedirects, './redirects/handle-redirects')) // Must come before contextualizers diff --git a/middleware/redirects/external.js b/middleware/redirects/external.js deleted file mode 100644 index a234bc759a..0000000000 --- a/middleware/redirects/external.js +++ /dev/null @@ -1,13 +0,0 @@ -import readJsonFile from '../../lib/read-json-file.js' -const externalSites = readJsonFile('./lib/redirects/external-sites.json') - -// blanket redirects to external websites -export default function externalRedirects(req, res, next) { - if (req.path in externalSites) { - // Undo the cookie setting that CSRF sets. - res.removeHeader('set-cookie') - return res.redirect(301, externalSites[req.path]) - } else { - return next() - } -} diff --git a/middleware/redirects/handle-redirects.js b/middleware/redirects/handle-redirects.js index 76c2fb39ee..d84639e686 100644 --- a/middleware/redirects/handle-redirects.js +++ b/middleware/redirects/handle-redirects.js @@ -77,7 +77,7 @@ export default function handleRedirects(req, res, next) { } // do not redirect if the redirected page can't be found - if (!req.context.pages[removeQueryParams(redirect)]) { + if (!req.context.pages[removeQueryParams(redirect)] && !redirect.includes('://')) { // display error on the page in development, but not in production // include final full redirect path in the message if (process.env.NODE_ENV !== 'production' && req.context) { @@ -90,13 +90,13 @@ export default function handleRedirects(req, res, next) { res.removeHeader('set-cookie') // do the redirect if the from-URL already had a language in it - if (pathLanguagePrefixed(req.path)) { + if (pathLanguagePrefixed(req.path) || redirect.includes('://')) { cacheControl(res) } else { noCacheControl(res) } - const permanent = usePermanentRedirect(req) + const permanent = redirect.includes('://') || usePermanentRedirect(req) return res.redirect(permanent ? 301 : 302, redirect) } diff --git a/tests/rendering/server.js b/tests/rendering/server.js index 9930b88c60..3a82094bcc 100644 --- a/tests/rendering/server.js +++ b/tests/rendering/server.js @@ -704,14 +704,6 @@ describe('server', () => { expect(res.headers['cache-control']).toContain('public') expect(res.headers['cache-control']).toMatch(/max-age=\d+/) }) - - test('redirects Desktop Classic paths to desktop.github.com', async () => { - const res = await get('/desktop-classic') - expect(res.statusCode).toBe(301) - expect(res.headers.location).toBe('https://desktop.github.com') - expect(res.headers['set-cookie']).toBeUndefined() - expect(res.headers['cache-control']).toBeUndefined() - }) }) describe('categories and map topics', () => { diff --git a/tests/routing/redirects.js b/tests/routing/redirects.js index 7f8bc1be93..e034e05df7 100644 --- a/tests/routing/redirects.js +++ b/tests/routing/redirects.js @@ -1,7 +1,7 @@ import { fileURLToPath } from 'url' import path from 'path' import { isPlainObject } from 'lodash-es' -import { jest } from '@jest/globals' +import { expect, jest, test } from '@jest/globals' import enterpriseServerReleases from '../../lib/enterprise-server-releases.js' import Page from '../../lib/page.js' @@ -106,7 +106,9 @@ describe('redirects', () => { }) test('are absent from all destination URLs', async () => { - const values = Object.values(redirects) + const values = Object.entries(redirects) + .filter(([from_, to]) => !to.includes('://')) + .map(([from_]) => from_) expect(values.length).toBeGreaterThan(100) expect(values.every((value) => !value.endsWith('/'))).toBe(true) }) @@ -173,16 +175,40 @@ describe('redirects', () => { }) describe('external redirects', () => { + test('no external redirect starts with a language prefix', () => { + const values = Object.entries(redirects) + .filter(([from_, to]) => to.includes('://')) + .map(([from_]) => from_) + .filter((from_) => from_.startsWith('/en/')) + expect(values.length).toBe(0) + }) + + test('no external redirect should go to developer.github.com', () => { + const values = Object.values(redirects) + .filter((to) => to.includes('://')) + .filter((to) => new URL(to).hostname === 'developer.github.com') + expect(values.length).toBe(0) + }) + test('work for top-level request paths', async () => { const res = await get('/git-ready') expect(res.statusCode).toBe(301) - expect(res.headers.location).toBe('http://gitready.com/') + expect(res.headers.location).toBe('https://gitready.com/') + expect(res.headers['set-cookie']).toBeUndefined() + expect(res.headers['cache-control']).toContain('public') + expect(res.headers['cache-control']).toMatch(/max-age=\d+/) }) - test('work for article-level request paths', async () => { - const res = await get('/articles/testing-webhooks') + test('work for top-level request paths with /en/ prefix', async () => { + const res = await get('/en/git-ready') expect(res.statusCode).toBe(301) - expect(res.headers.location).toBe('http://developer.github.com/webhooks/testing/') + expect(res.headers.location).toBe('https://gitready.com/') + }) + + test('work for top-level request paths with /ja/ prefix', async () => { + const res = await get('/ja/git-ready') + expect(res.statusCode).toBe(301) + expect(res.headers.location).toBe('https://gitready.com/') }) })