From b6e8717e02a9014a354cfcf705cc5f885bc112b1 Mon Sep 17 00:00:00 2001 From: Ruben Verborgh Date: Thu, 14 Sep 2017 19:27:00 -0400 Subject: [PATCH 1/3] Require Read permissions for authProxy. Addresses read part of #595. --- lib/handlers/allow.js | 2 +- lib/handlers/auth-proxy.js | 3 ++- test/integration/auth-proxy-test.js | 2 +- test/resources/auth-proxy/.acl | 10 ++++++++++ test/resources/auth-proxy/index.html | 0 test/resources/auth-proxy/index.html.acl | 0 6 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 test/resources/auth-proxy/.acl create mode 100644 test/resources/auth-proxy/index.html create mode 100644 test/resources/auth-proxy/index.html.acl diff --git a/lib/handlers/allow.js b/lib/handlers/allow.js index 5a6abff8c..5e2ccadd1 100644 --- a/lib/handlers/allow.js +++ b/lib/handlers/allow.js @@ -8,7 +8,7 @@ var debug = require('../debug.js').ACL function allow (mode) { return function allowHandler (req, res, next) { - var ldp = req.app.locals.ldp + var ldp = req.app.locals.ldp || {} if (!ldp.webid) { return next() } diff --git a/lib/handlers/auth-proxy.js b/lib/handlers/auth-proxy.js index e6a8e38ca..4a22dc379 100644 --- a/lib/handlers/auth-proxy.js +++ b/lib/handlers/auth-proxy.js @@ -4,6 +4,7 @@ module.exports = addAuthProxyHandlers const proxy = require('http-proxy-middleware') const debug = require('../debug') +const allow = require('./allow') const PROXY_SETTINGS = { logLevel: 'silent', @@ -32,7 +33,7 @@ function addAuthProxyHandler (app, sourcePath, target) { }, PROXY_SETTINGS) // Activate the proxy - app.use(`${sourcePath}*`, proxy(settings)) + app.use(`${sourcePath}*`, allow('Read'), proxy(settings)) } // Adds a headers with authentication information diff --git a/test/integration/auth-proxy-test.js b/test/integration/auth-proxy-test.js index 238696721..fbf9d8df1 100644 --- a/test/integration/auth-proxy-test.js +++ b/test/integration/auth-proxy-test.js @@ -18,7 +18,7 @@ describe('Auth Proxy', () => { // Set up Solid server server = ldnode({ - root: path.join(__dirname, '../resources'), + root: path.join(__dirname, '../resources/auth-proxy'), authProxy: { '/server/a': 'http://server-a.org' }, diff --git a/test/resources/auth-proxy/.acl b/test/resources/auth-proxy/.acl new file mode 100644 index 000000000..c3dfa974e --- /dev/null +++ b/test/resources/auth-proxy/.acl @@ -0,0 +1,10 @@ +@prefix acl: . +@prefix foaf: . + +# All permissions on the root +<#owner> + a acl:Authorization; + acl:agent ; + acl:accessTo <./>; + acl:defaultForNew <./>; + acl:mode acl:Read, acl:Write, acl:Control. diff --git a/test/resources/auth-proxy/index.html b/test/resources/auth-proxy/index.html new file mode 100644 index 000000000..e69de29bb diff --git a/test/resources/auth-proxy/index.html.acl b/test/resources/auth-proxy/index.html.acl new file mode 100644 index 000000000..e69de29bb From 2c8adfc4b3877964ca39039e3c356e6dc321ffdb Mon Sep 17 00:00:00 2001 From: Ruben Verborgh Date: Fri, 15 Sep 2017 15:00:52 -0400 Subject: [PATCH 2/3] Require write permissions for unsafe proxy methods. Closes #595. --- lib/handlers/auth-proxy.js | 19 +++++- test/integration/auth-proxy-test.js | 97 ++++++++++++++++++++++++++--- test/resources/auth-proxy/.acl | 46 +++++++++++--- 3 files changed, 146 insertions(+), 16 deletions(-) diff --git a/lib/handlers/auth-proxy.js b/lib/handlers/auth-proxy.js index 4a22dc379..3c8a88eb4 100644 --- a/lib/handlers/auth-proxy.js +++ b/lib/handlers/auth-proxy.js @@ -2,7 +2,7 @@ // that sends a logged-in Solid user's details to a backend module.exports = addAuthProxyHandlers -const proxy = require('http-proxy-middleware') +const createProxy = require('http-proxy-middleware') const debug = require('../debug') const allow = require('./allow') @@ -10,6 +10,11 @@ const PROXY_SETTINGS = { logLevel: 'silent', changeOrigin: true } +const REQUIRED_PERMISSIONS = { + get: ['Read'], + options: ['Read'], + use: ['Read', 'Write'] +} // Registers Auth Proxy handlers for each target function addAuthProxyHandlers (app, targets) { @@ -33,7 +38,11 @@ function addAuthProxyHandler (app, sourcePath, target) { }, PROXY_SETTINGS) // Activate the proxy - app.use(`${sourcePath}*`, allow('Read'), proxy(settings)) + const proxy = createProxy(settings) + for (let action in REQUIRED_PERMISSIONS) { + const permissions = REQUIRED_PERMISSIONS[action] + app[action](`${sourcePath}*`, setOriginalUrl, ...permissions.map(allow), proxy) + } } // Adds a headers with authentication information @@ -46,3 +55,9 @@ function addAuthHeaders (proxyReq, req) { proxyReq.setHeader('Forwarded', `host=${headers.host}`) } } + +// Sets the original URL on the request (for the ACL handler) +function setOriginalUrl (req, res, next) { + res.locals.path = req.originalUrl + next() +} diff --git a/test/integration/auth-proxy-test.js b/test/integration/auth-proxy-test.js index fbf9d8df1..49608289a 100644 --- a/test/integration/auth-proxy-test.js +++ b/test/integration/auth-proxy-test.js @@ -5,7 +5,6 @@ const request = require('supertest') const { expect } = require('chai') const rm = require('../utils').rm -const HOST = 'solid.org' const USER = 'https://ruben.verborgh.org/profile/#me' describe('Auth Proxy', () => { @@ -15,6 +14,8 @@ describe('Auth Proxy', () => { // Set up test back-end server nock('http://server-a.org').persist() .get(/./).reply(200, function () { return this.req.headers }) + .options(/./).reply(200) + .post(/./).reply(200) // Set up Solid server server = ldnode({ @@ -36,18 +37,100 @@ describe('Auth Proxy', () => { describe('responding to /server/a', () => { let response - before(() => { - return request(server).get('/server/a') - .set('Host', HOST) + before(() => + request(server).get('/server/a/') .then(res => { response = res }) - }) + ) it('sets the User header on the proxy request', () => { expect(response.body).to.have.property('user', USER) }) + }) + + describe('responding to GET', () => { + describe('for a path with read permissions', () => { + let response + before(() => + request(server).get('/server/a/r') + .then(res => { response = res }) + ) + it('returns status code 200', () => { + expect(response).to.have.property('statusCode', 200) + }) + }) + + describe('for a path without read permissions', () => { + let response + before(() => + request(server).get('/server/a/wc') + .then(res => { response = res }) + ) + + it('returns status code 403', () => { + expect(response).to.have.property('statusCode', 403) + }) + }) + }) + + describe('responding to OPTIONS', () => { + describe('for a path with read permissions', () => { + let response + before(() => + request(server).options('/server/a/r') + .then(res => { response = res }) + ) + it('returns status code 200', () => { + expect(response).to.have.property('statusCode', 200) + }) + }) + + describe('for a path without read permissions', () => { + let response + before(() => + request(server).options('/server/a/wc') + .then(res => { response = res }) + ) + + it('returns status code 403', () => { + expect(response).to.have.property('statusCode', 403) + }) + }) + }) + + describe('responding to POST', () => { + describe('for a path with read and write permissions', () => { + let response + before(() => + request(server).post('/server/a/rw') + .then(res => { response = res }) + ) + it('returns status code 200', () => { + expect(response).to.have.property('statusCode', 200) + }) + }) + + describe('for a path without read permissions', () => { + let response + before(() => + request(server).post('/server/a/w') + .then(res => { response = res }) + ) + + it('returns status code 403', () => { + expect(response).to.have.property('statusCode', 403) + }) + }) + + describe('for a path without write permissions', () => { + let response + before(() => + request(server).post('/server/a/r') + .then(res => { response = res }) + ) - it('returns status code 200', () => { - expect(response).to.have.property('statusCode', 200) + it('returns status code 403', () => { + expect(response).to.have.property('statusCode', 403) + }) }) }) }) diff --git a/test/resources/auth-proxy/.acl b/test/resources/auth-proxy/.acl index c3dfa974e..ef847823b 100644 --- a/test/resources/auth-proxy/.acl +++ b/test/resources/auth-proxy/.acl @@ -1,10 +1,42 @@ @prefix acl: . @prefix foaf: . -# All permissions on the root -<#owner> - a acl:Authorization; - acl:agent ; - acl:accessTo <./>; - acl:defaultForNew <./>; - acl:mode acl:Read, acl:Write, acl:Control. +# All permissions on /server/a +[ + a acl:Authorization; + acl:accessTo ; + acl:agent ; + acl:mode acl:Read, acl:Write, acl:Control +]. + +# Only Read permissions on /server/a/r +[ + a acl:Authorization; + acl:accessTo ; + acl:agent ; + acl:mode acl:Read +]. + +# No Read permissions on /server/a/wc +[ + a acl:Authorization; + acl:accessTo ; + acl:agent ; + acl:mode acl:Write, acl:Control +]. + +# Only Write permissions on /server/a/w +[ + a acl:Authorization; + acl:accessTo ; + acl:agent ; + acl:mode acl:Write +]. + +# Read-Write permissions on /server/a/rw +[ + a acl:Authorization; + acl:accessTo ; + acl:agent ; + acl:mode acl:Read, acl:Write +]. From 4ed7e91acf277e0598985e1208dc8549d9b41520 Mon Sep 17 00:00:00 2001 From: Ruben Verborgh Date: Fri, 15 Sep 2017 15:31:15 -0400 Subject: [PATCH 3/3] Simplify status code assertions. --- test/integration/auth-proxy-test.js | 14 +++++++------- test/unit/auth-proxy-test.js | 10 +++++----- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/test/integration/auth-proxy-test.js b/test/integration/auth-proxy-test.js index 49608289a..6aa53aa50 100644 --- a/test/integration/auth-proxy-test.js +++ b/test/integration/auth-proxy-test.js @@ -55,7 +55,7 @@ describe('Auth Proxy', () => { .then(res => { response = res }) ) it('returns status code 200', () => { - expect(response).to.have.property('statusCode', 200) + expect(response.statusCode).to.equal(200) }) }) @@ -67,7 +67,7 @@ describe('Auth Proxy', () => { ) it('returns status code 403', () => { - expect(response).to.have.property('statusCode', 403) + expect(response.statusCode).to.equal(403) }) }) }) @@ -80,7 +80,7 @@ describe('Auth Proxy', () => { .then(res => { response = res }) ) it('returns status code 200', () => { - expect(response).to.have.property('statusCode', 200) + expect(response.statusCode).to.equal(200) }) }) @@ -92,7 +92,7 @@ describe('Auth Proxy', () => { ) it('returns status code 403', () => { - expect(response).to.have.property('statusCode', 403) + expect(response.statusCode).to.equal(403) }) }) }) @@ -105,7 +105,7 @@ describe('Auth Proxy', () => { .then(res => { response = res }) ) it('returns status code 200', () => { - expect(response).to.have.property('statusCode', 200) + expect(response.statusCode).to.equal(200) }) }) @@ -117,7 +117,7 @@ describe('Auth Proxy', () => { ) it('returns status code 403', () => { - expect(response).to.have.property('statusCode', 403) + expect(response.statusCode).to.equal(403) }) }) @@ -129,7 +129,7 @@ describe('Auth Proxy', () => { ) it('returns status code 403', () => { - expect(response).to.have.property('statusCode', 403) + expect(response.statusCode).to.equal(403) }) }) }) diff --git a/test/unit/auth-proxy-test.js b/test/unit/auth-proxy-test.js index fbd28d4b1..baf5a7b79 100644 --- a/test/unit/auth-proxy-test.js +++ b/test/unit/auth-proxy-test.js @@ -68,7 +68,7 @@ describe('Auth Proxy', () => { }) it('returns status code 200', () => { - expect(response).to.have.property('statusCode', 200) + expect(response.statusCode).to.equal(200) }) }) @@ -102,7 +102,7 @@ describe('Auth Proxy', () => { }) it('returns status code 200', () => { - expect(response).to.have.property('statusCode', 200) + expect(response.statusCode).to.equal(200) }) }) @@ -136,7 +136,7 @@ describe('Auth Proxy', () => { }) it('returns status code 200', () => { - expect(response).to.have.property('statusCode', 200) + expect(response.statusCode).to.equal(200) }) }) @@ -170,7 +170,7 @@ describe('Auth Proxy', () => { }) it('returns status code 200', () => { - expect(response).to.have.property('statusCode', 200) + expect(response.statusCode).to.equal(200) }) }) @@ -208,7 +208,7 @@ describe('Auth Proxy', () => { }) it('returns status code 200', () => { - expect(response).to.have.property('statusCode', 200) + expect(response.statusCode).to.equal(200) }) }) })