fix external redirects (#28063)

* fix external redirects

* oops

* feedbacked

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: '*'

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: '*'

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: '*'

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]
   }
 

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://www.githubstatus.com/",
-  "/github-status": "https://status.github.com/messages",
   "/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/"
-  "/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/"
 }

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

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
 

middleware/redirects/external.js

@@ -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()
-  }
-}

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)
 }
 

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', () => {

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 () => {
+    test('work for top-level request paths with /en/ prefix', async () => {
-      const res = await get('/articles/testing-webhooks')
+      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/')
     })
   })