Skip to content

Commit

Permalink
Fix and test the github admin route (#1886)
Browse files Browse the repository at this point in the history
Fix regressions in the github admin and token acceptor endpoints, introduced in #1813.
  • Loading branch information
paulmelnikow authored Aug 11, 2018
1 parent cd6c38a commit 39d3930
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 8 deletions.
5 changes: 2 additions & 3 deletions lib/github-auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const crypto = require('crypto')
const log = require('./log')
const secretIsValid = require('./sys/secret-is-valid')
const queryString = require('query-string')
const request = require('request')
const autosave = require('json-autosave')
Expand Down Expand Up @@ -121,9 +122,7 @@ function setRoutes(server) {
})

server.route(/^\/github-auth\/add-token$/, function(data, match, end, ask) {
if (
!crypto.timingSafeEqual(data.shieldsSecret, serverSecrets.shieldsSecret)
) {
if (!secretIsValid(data.shieldsSecret)) {
// An unknown entity tries to connect. Let the connection linger for 10s.
return setTimeout(function() {
end('Invalid secret.')
Expand Down
4 changes: 3 additions & 1 deletion lib/sys/secret-is-valid.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
const serverSecrets = require('../server-secrets')

function secretIsValid(secret = '') {
return constEq(secret, serverSecrets.shieldsSecret)
return (
serverSecrets.shieldsSecret && constEq(secret, serverSecrets.shieldsSecret)
)
}

function constEq(a, b) {
Expand Down
7 changes: 3 additions & 4 deletions services/github/auth/admin.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
'use strict'

const crypto = require('crypto')
const { serializeDebugInfo } = require('../../../lib/github-auth')
const serverSecrets = require('../../../lib/server-secrets')
const secretIsValid = require('../../../lib/sys/secret-is-valid')

function setRoutes(server) {
// Allow the admin to obtain the tokens for operational and debugging
Expand All @@ -16,9 +15,9 @@ function setRoutes(server) {
// password.
//
// e.g.
// curl -u ':very-very-secret' 'https://example.com/$github-auth/tokens'
// curl --insecure -u ':very-very-secret' 'https://s0.shields-server.com/$github-auth/tokens'
server.ajax.on('github-auth/tokens', (json, end, ask) => {
if (!crypto.timingSafeEqual(ask.password, serverSecrets.shieldsSecret)) {
if (!secretIsValid(ask.password)) {
// An unknown entity tries to connect. Let the connection linger for a minute.
return setTimeout(function() {
end('Invalid secret.')
Expand Down
76 changes: 76 additions & 0 deletions services/github/auth/admin.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
'use strict'

const { expect } = require('chai')
const sinon = require('sinon')
const Camp = require('camp')
const fetch = require('node-fetch')
const config = require('../../../lib/test-config')
const serverSecrets = require('../../../lib/server-secrets')
const { setRoutes } = require('./admin')

function createAuthHeader({ username, password }) {
const headers = new fetch.Headers()
const encoded = Buffer.from(`${username}:${password}`).toString('base64')
headers.append('authorization', `Basic ${encoded}`)
return headers
}

describe('GitHub admin route', function() {
const validCredentials = {
username: '',
password: '7'.repeat(40),
}

let sandbox
beforeEach(function() {
sandbox = sinon.createSandbox()
// Make this work when there is no `shieldsSecret` defined.
serverSecrets.shieldsSecret = undefined
sandbox
.stub(serverSecrets, 'shieldsSecret')
.value(validCredentials.password)
})
afterEach(function() {
sandbox.restore()
})

const baseUrl = `http://127.0.0.1:${config.port}`

let camp
before(function(done) {
camp = Camp.start({ port: config.port, hostname: '::' })
camp.on('listening', () => done())
})
after(function(done) {
if (camp) {
camp.close(() => done())
camp = undefined
}
})

before(function() {
setRoutes(camp)
})

context('the password is correct', function() {
it('returns a valid JSON response', async function() {
const res = await fetch(`${baseUrl}/$github-auth/tokens`, {
headers: createAuthHeader(validCredentials),
})
expect(res.ok).to.be.true
expect(await res.json()).to.be.ok
})
})

// Disabled because this code isn't modified often and the test is very
// slow. I wasn't able to make this work with fake timers:
// https://github.com/sinonjs/sinon/issues/1739
// context('the password is missing', function() {
// it('returns the expected message', async function() {
// this.timeout(11000)
// const res = await fetch(`${baseUrl}/$github-auth/tokens`)
// expect(res.ok).to.be.true
// expect(await res.text()).to.equal('"Invalid secret."')
// })
// })
})

0 comments on commit 39d3930

Please sign in to comment.