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

Ensure access tokens can only access specific allowed routes #827

Merged
merged 3 commits into from
Jul 29, 2022
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
2 changes: 1 addition & 1 deletion forge/ee/routes/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* @memberof forge.ee
*/
module.exports = async function (app) {
app.addHook('preHandler', app.verifyTokenOrSession)
app.addHook('preHandler', app.verifySession)
if (app.config.billing) {
await app.register(require('./billing'), { prefix: '/billing', logLevel: 'warn' })
}
Expand Down
8 changes: 4 additions & 4 deletions forge/routes/api/deviceLive.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ module.exports = async function (app) {
* The response will be a 200 if all is well.
* If the snapshot doesn't match the target, it will get a 409 (conflict)
*/
app.post('/state', async (request, reply) => {
app.post('/state', { config: { allowToken: true } }, async (request, reply) => {
await app.db.controllers.Device.updateState(request.device, request.body)
if (Object.hasOwn(request.body, 'project') && request.body.project !== (request.device.Project?.id || null)) {
reply.code(409).send({
Expand Down Expand Up @@ -61,15 +61,15 @@ module.exports = async function (app) {
reply.code(200).send({})
})

app.get('/state', async (request, reply) => {
app.get('/state', { config: { allowToken: true } }, async (request, reply) => {
reply.send({
project: request.device.Project?.id || null,
snapshot: request.device.targetSnapshot?.hashid || null,
settings: request.device.settingsHash || null
})
})

app.get('/snapshot', async (request, reply) => {
app.get('/snapshot', { config: { allowToken: true } }, async (request, reply) => {
if (!request.device.targetSnapshot) {
reply.send({})
} else {
Expand All @@ -94,7 +94,7 @@ module.exports = async function (app) {
}
})

app.get('/settings', async (request, reply) => {
app.get('/settings', { config: { allowToken: true } }, async (request, reply) => {
const response = {
hash: request.device.settingsHash,
env: {}
Expand Down
2 changes: 1 addition & 1 deletion forge/routes/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const Device = require('./device.js')
const ProjectType = require('./projectType.js')

module.exports = async function (app) {
app.addHook('preHandler', app.verifyTokenOrSession)
app.addHook('preHandler', app.verifySession)
app.decorate('getPaginationOptions', (request, defaults) => {
const result = { ...defaults }
if (request.query.limit !== undefined) {
Expand Down
1 change: 1 addition & 0 deletions forge/routes/api/project.js
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,7 @@ module.exports = async function (app) {
* @memberof forge.routes.api.project
*/
app.get('/:projectId/settings', {
config: { allowToken: true },
preHandler: (request, reply, done) => {
// check accessToken is project scope
if (request.session.ownerType !== 'project') {
Expand Down
30 changes: 25 additions & 5 deletions forge/routes/api/team.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,27 @@ module.exports = async function (app) {
if (request.params.teamId !== undefined) {
if (request.params.teamId) {
try {
// if User is not defined, check to see if request.context.config.allowAnonymous is set
if (request.session?.User === undefined && request.context.config?.allowAnonymous === true) {
if (!request.session.User) {
// If request.session.User is not defined, this request is being
// made with an access token. If it is a project access token,
// ensure that project is in this team
if (request.session.ownerType === 'project') {
// Want this to be as small a query as possible. Sequelize
// doesn't make it easy to just get `TeamId` without doing
// a join on Team table.
const project = await app.db.models.Project.findOne({
where: { id: request.session.ownerId },
include: {
model: app.db.models.Team,
attributes: ['hashid', 'id']
}
})
// Ensure the token's project is in the team being accessed
if (project && project.Team.hashid === request.params.teamId) {
return
}
}
reply.code(404).type('text/html').send('Not Found')
Steve-Mcl marked this conversation as resolved.
Show resolved Hide resolved
return
}
request.teamMembership = await request.session.User.getTeamMembership(request.params.teamId)
Expand Down Expand Up @@ -92,12 +111,13 @@ module.exports = async function (app) {
}
})

app.get('/:teamId/projects', { config: { allowAnonymous: true } }, async (request, reply) => {
app.get('/:teamId/projects', { config: { allowToken: true } }, async (request, reply) => {
const projects = await app.db.models.Project.byTeam(request.params.teamId)
if (projects) {
let result = app.db.views.Project.teamProjectList(projects)
const limitResponse = request.session?.User === undefined
if (limitResponse) {
if (request.session.ownerType === 'project') {
// This request is from a project token. Filter the list to return
// the minimal information needed
result = result.map(e => {
return { id: e.id, name: e.name }
})
Expand Down
2 changes: 1 addition & 1 deletion forge/routes/api/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ module.exports = async function (app) {
* @static
* @memberof forge.routes.api.user
*/
app.get('/', { config: { allowUnverifiedEmail: true } }, async (request, reply) => {
app.get('/', { config: { allowUnverifiedEmail: true, allowToken: true } }, async (request, reply) => {
const users = await app.db.views.User.userProfile(request.session.User)
reply.send(users)
})
Expand Down
15 changes: 4 additions & 11 deletions forge/routes/auth/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,6 @@ module.exports = fp(async function (app, opts, done) {

app.decorate('verifyToken', verifyToken)

app.decorate('verifyTokenOrSession', async function (request, reply) {
// Order is important, other way round breaks nr-auth plugin
if (request.sid) {
await verifySession(request, reply)
} else if (request.headers && request.headers.authorization) {
await verifyToken(request, reply)
} else if (!request.context.config.allowAnonymous) {
reply.code(401).send({ error: 'unauthorized' })
}
})

/**
* preHandler function that ensures the current request comes from an active
* session.
Expand All @@ -103,6 +92,10 @@ module.exports = fp(async function (app, opts, done) {
if (request.context.config.allowAnonymous) {
return
}
if (request.context.config.allowToken) {
await verifyToken(request, reply)
return
}
reply.code(401).send({ error: 'unauthorized' })
throw new Error()
}
Expand Down