From 1918d2ea14127688d0048fc2804b913cd0a4a26e Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Tue, 9 Feb 2021 14:08:24 -0800 Subject: [PATCH] Inline CSRF token in HTML (#17748) * Inline CSRF token * Fix tests --- includes/head.html | 27 ++++++++++++++------------- javascripts/get-csrf.js | 14 -------------- javascripts/index.js | 8 +++----- middleware/csrf-route.js | 14 -------------- middleware/index.js | 1 - middleware/render-page.js | 15 ++++++++++++--- tests/rendering/csrf-route.js | 16 ---------------- tests/rendering/events.js | 6 ++++-- 8 files changed, 33 insertions(+), 68 deletions(-) delete mode 100644 middleware/csrf-route.js delete mode 100644 tests/rendering/csrf-route.js diff --git a/includes/head.html b/includes/head.html index 480c9cbc7dfc..5d931a9048b4 100644 --- a/includes/head.html +++ b/includes/head.html @@ -1,20 +1,19 @@ + {% comment %} For human readers {% endcomment %} {% if error == '404' %}{% data ui.errors.oops %}{% elsif currentVersion == 'homepage' %}GitHub Documentation{% elsif page.fullTitle %}{{ page.fullTitle }}{% else %}GitHub Documentation{% endif %} - {% if page.hidden %} - {% endif %} - - - - - - + + + + + {% comment %} For Google and Bots {% endcomment %} {% if page.intro %} {% endif %} - - + {% if page.hidden %} + + {% endif %} {% for languageVariant in page.languageVariants %} {% endfor %} + + - - - + {% comment %} For our JS {% endcomment %} + + diff --git a/javascripts/get-csrf.js b/javascripts/get-csrf.js index a2ea86db4666..825dce25cb74 100644 --- a/javascripts/get-csrf.js +++ b/javascripts/get-csrf.js @@ -1,17 +1,3 @@ -export async function fillCsrf () { - const response = await fetch('/csrf', { - method: 'GET', - headers: { - 'Content-Type': 'application/json' - } - }) - const json = response.ok ? await response.json() : {} - const meta = document.createElement('meta') - meta.setAttribute('name', 'csrf-token') - meta.setAttribute('content', json.token) - document.querySelector('head').append(meta) -} - export default function getCsrf () { const csrfEl = document .querySelector('meta[name="csrf-token"]') diff --git a/javascripts/index.js b/javascripts/index.js index 4c4028c8cdb1..ea96ef7d1d84 100644 --- a/javascripts/index.js +++ b/javascripts/index.js @@ -12,7 +12,6 @@ import localization from './localization' import helpfulness from './helpfulness' import experiment from './experiment' import copyCode from './copy-code' -import { fillCsrf } from './get-csrf' import initializeEvents from './events' import filterCards from './filter-cards' import allArticles from './all-articles' @@ -35,9 +34,8 @@ document.addEventListener('DOMContentLoaded', async () => { allArticles() devToc() showMore() - await fillCsrf() // this must complete before any POST calls - initializeEvents() // requires fillCsrf to complete - experiment() // requires fillCsrf to complete - helpfulness() // requires fillCsrf to complete releaseNotes() + initializeEvents() + experiment() + helpfulness() }) diff --git a/middleware/csrf-route.js b/middleware/csrf-route.js deleted file mode 100644 index c884f294d83e..000000000000 --- a/middleware/csrf-route.js +++ /dev/null @@ -1,14 +0,0 @@ -const express = require('express') - -const router = express.Router() - -router.get('/', (req, res) => { - res.status(200) - res.set({ - 'surrogate-control': 'private, no-store', - 'cache-control': 'private, no-store' - }) - res.json({ token: req.csrfToken() }) -}) - -module.exports = router diff --git a/middleware/index.js b/middleware/index.js index 866047c346aa..6d3961643286 100644 --- a/middleware/index.js +++ b/middleware/index.js @@ -83,7 +83,6 @@ module.exports = function (app) { maxAge: '7 days' // A bit longer since releases are more sparse })) app.use('/events', instrument('./events')) - app.use('/csrf', instrument('./csrf-route')) app.use('/search', instrument('./search')) app.use(instrument('./archived-enterprise-versions')) app.use(instrument('./robots')) diff --git a/middleware/render-page.js b/middleware/render-page.js index 1c887bb3d497..1ee5b3d161ae 100644 --- a/middleware/render-page.js +++ b/middleware/render-page.js @@ -21,6 +21,10 @@ const pageCache = new RedisAccessor({ // a list of query params that *do* alter the rendered page, and therefore should be cached separately const cacheableQueries = ['learn'] +function addCsrf (req, text) { + return text.replace('$CSRFTOKEN$', req.csrfToken()) +} + module.exports = async function renderPage (req, res, next) { const page = req.context.page @@ -45,7 +49,7 @@ module.exports = async function renderPage (req, res, next) { if (cachedHtml) { console.log(`Serving from cached version of ${originalUrl}`) statsd.increment('page.sent_from_cache') - return res.send(cachedHtml) + return res.send(addCsrf(req, cachedHtml)) } } @@ -54,7 +58,12 @@ module.exports = async function renderPage (req, res, next) { if (process.env.NODE_ENV !== 'test' && req.context.redirectNotFound) { console.error(`\nTried to redirect to ${req.context.redirectNotFound}, but that page was not found.\n`) } - return res.status(404).send(await liquid.parseAndRender(layouts['error-404'], req.context)) + return res.status(404).send( + addCsrf( + req, + await liquid.parseAndRender(layouts['error-404'], req.context) + ) + ) } if (req.method === 'HEAD') { @@ -116,7 +125,7 @@ module.exports = async function renderPage (req, res, next) { // First, send the response so the user isn't waiting // NOTE: Do NOT `return` here as we still need to cache the response afterward! - res.send(output) + res.send(addCsrf(req, output)) // Finally, save output to cache for the next time around if (isCacheable) { diff --git a/tests/rendering/csrf-route.js b/tests/rendering/csrf-route.js deleted file mode 100644 index 62609e7a1dbe..000000000000 --- a/tests/rendering/csrf-route.js +++ /dev/null @@ -1,16 +0,0 @@ -const request = require('supertest') -const app = require('../../server') - -describe('GET /csrf', () => { - jest.setTimeout(60 * 1000) - // There's a "warmServer" in middleware/context.js - // that takes about 6-10 seconds to process first time - - it('should render a non-cache include for CSRF token', async () => { - const res = await request(app).get('/csrf') - expect(res.status).toBe(200) - expect(res.body).toHaveProperty('token') - expect(res.headers['surrogate-control']).toBe('private, no-store') - expect(res.headers['cache-control']).toBe('private, no-store') - }) -}) diff --git a/tests/rendering/events.js b/tests/rendering/events.js index 7447722c300c..15daa36e7ba0 100644 --- a/tests/rendering/events.js +++ b/tests/rendering/events.js @@ -1,5 +1,6 @@ const request = require('supertest') const nock = require('nock') +const cheerio = require('cheerio') const app = require('../../server') describe('POST /events', () => { @@ -14,8 +15,9 @@ describe('POST /events', () => { process.env.HYDRO_SECRET = '$HYDRO_SECRET$' process.env.HYDRO_ENDPOINT = 'http://example.com/hydro' agent = request.agent(app) - const csrfRes = await agent.get('/csrf') - csrfToken = csrfRes.body.token + const csrfRes = await agent.get('/en') + const $ = cheerio.load(csrfRes.text || '', { xmlMode: true }) + csrfToken = $('meta[name="csrf-token"]').attr('content') nock('http://example.com') .post('/hydro') .reply(200, {})