Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and test the github admin route #1886

Merged
merged 5 commits into from
Aug 11, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 }) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit annoying this must be done manually. We may want to consider a different request library like got or axios.

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."')
// })
// })
})