Skip to content

Commit eb6b62b

Browse files
kjetilkkjetilk
authored
Merge pull request #1118 from solid/fix/faulty-403
Making proper use of strictOrigins
2 parents e62fa81 + e4cf1e2 commit eb6b62b

14 files changed

Lines changed: 1487 additions & 29 deletions

‎lib/acl-checker.js‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,10 @@ class ACLChecker {
1616
constructor (resource, options = {}) {
1717
this.resource = resource
1818
this.resourceUrl = new URL(resource)
19-
this.agentOrigin = options.agentOrigin
19+
this.agentOrigin = options.strictOrigin && options.agentOrigin ? rdf.sym(options.agentOrigin) : null
2020
this.fetch = options.fetch
2121
this.fetchGraph = options.fetchGraph
22-
this.strictOrigin = options.strictOrigin
23-
this.trustedOrigins = options.trustedOrigins
22+
this.trustedOrigins = options.strictOrigin && options.trustedOrigins ? options.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
2423
this.suffix = options.suffix || DEFAULT_ACL_SUFFIX
2524
this.aclCached = {}
2625
this.messagesCached = {}
@@ -56,17 +55,14 @@ class ACLChecker {
5655
const aclFile = rdf.sym(acl.acl)
5756
const agent = user ? rdf.sym(user) : null
5857
const modes = [ACL(mode)]
59-
const agentOrigin = this.agentOrigin ? rdf.sym(this.agentOrigin) : null
60-
const trustedOrigins = this.trustedOrigins ? this.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
58+
const agentOrigin = this.agentOrigin
59+
const trustedOrigins = this.trustedOrigins
6160
const accessDenied = aclCheck.accessDenied(acl.graph, resource, directory, aclFile, agent, modes, agentOrigin, trustedOrigins)
62-
if (accessDenied && this.agentOrigin && this.resourceUrl.origin !== this.agentOrigin) {
63-
this.messagesCached[cacheKey].push(HTTPError(403, accessDenied))
64-
} else if (accessDenied && user) {
61+
62+
if (accessDenied && user) {
6563
this.messagesCached[cacheKey].push(HTTPError(403, accessDenied))
66-
} else if (accessDenied && !user) {
67-
this.messagesCached[cacheKey].push(HTTPError(401, 'Unauthenticated'))
6864
} else if (accessDenied) {
69-
this.messagesCached[cacheKey].push(HTTPError(401, accessDenied))
65+
this.messagesCached[cacheKey].push(HTTPError(401, 'Unauthenticated'))
7066
}
7167
this.aclCached[cacheKey] = Promise.resolve(!accessDenied)
7268
return this.aclCached[cacheKey]

‎lib/create-app.js‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -212,14 +212,27 @@ function initWebId (argv, app, ldp) {
212212
// without permission by including the credentials set by the Solid server.
213213
app.use((req, res, next) => {
214214
const origin = req.get('origin')
215-
const trustedOrigins = argv.trustedOrigins
215+
const trustedOrigins = ldp.getTrustedOrigins(req)
216216
const userId = req.session.userId
217217
// Exception: allow logout requests from all third-party apps
218218
// such that OIDC client can log out via cookie auth
219219
// TODO: remove this exception when OIDC clients
220220
// use Bearer token to authenticate instead of cookie
221221
// (https://github.com/solid/node-solid-server/pull/835#issuecomment-426429003)
222-
if (!argv.host.allowsSessionFor(userId, origin, trustedOrigins) && !isLogoutRequest(req)) {
222+
//
223+
// Authentication cookies are an optimization:
224+
// instead of going through the process of
225+
// fully validating authentication on every request,
226+
// we go through this process once,
227+
// and store its successful result in a cookie
228+
// that will be reused upon the next request.
229+
// However, that cookie can then be sent by any server,
230+
// even servers that have not gone through the proper authentication mechanism.
231+
// However, if trusted origins are enabled,
232+
// then any origin is allowed to take the shortcut route,
233+
// since malicious origins will be banned at the ACL checking phase.
234+
// https://github.com/solid/node-solid-server/issues/1117
235+
if (!argv.strictOrigin && !argv.host.allowsSessionFor(userId, origin, trustedOrigins) && !isLogoutRequest(req)) {
223236
debug.authentication(`Rejecting session for ${userId} from ${origin}`)
224237
// Destroy session data
225238
delete req.session.userId

‎test/integration/acl-tls-test.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,7 @@ describe('ACL with WebID+TLS', function () {
332332

333333
request.head(options, function (error, response, body) {
334334
assert.equal(error, null)
335-
assert.equal(response.statusCode, 403)
335+
assert.equal(response.statusCode, 401)
336336
done()
337337
})
338338
})
@@ -354,7 +354,7 @@ describe('ACL with WebID+TLS', function () {
354354

355355
request.head(options, function (error, response, body) {
356356
assert.equal(error, null)
357-
assert.equal(response.statusCode, 403)
357+
assert.equal(response.statusCode, 401)
358358
done()
359359
})
360360
})
@@ -433,7 +433,7 @@ describe('ACL with WebID+TLS', function () {
433433

434434
request.head(options, function (error, response, body) {
435435
assert.equal(error, null)
436-
assert.equal(response.statusCode, 403)
436+
assert.equal(response.statusCode, 401)
437437
done()
438438
})
439439
})
@@ -455,7 +455,7 @@ describe('ACL with WebID+TLS', function () {
455455

456456
request.head(options, function (error, response, body) {
457457
assert.equal(error, null)
458-
assert.equal(response.statusCode, 403)
458+
assert.equal(response.statusCode, 401)
459459
done()
460460
})
461461
})

‎test/integration/authentication-oidc-test.js‎

Lines changed: 72 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,39 @@ describe('Authentication API (OIDC)', () => {
179179
})
180180
})
181181

182+
describe('with that cookie and a non-matching origin', () => {
183+
let response
184+
before(done => {
185+
alice.get('/private-for-alice.txt')
186+
.set('Cookie', cookie)
187+
.set('Origin', bobServerUri)
188+
.end((err, res) => {
189+
response = res
190+
done(err)
191+
})
192+
})
193+
194+
it('should return a 403', () => {
195+
expect(response).to.have.property('status', 403)
196+
})
197+
})
198+
199+
describe('without that cookie and a non-matching origin', () => {
200+
let response
201+
before(done => {
202+
alice.get('/private-for-alice.txt')
203+
.set('Origin', bobServerUri)
204+
.end((err, res) => {
205+
response = res
206+
done(err)
207+
})
208+
})
209+
210+
it('should return a 401', () => {
211+
expect(response).to.have.property('status', 401)
212+
})
213+
})
214+
182215
describe('with that cookie but without origin', () => {
183216
let response
184217
before(done => {
@@ -195,6 +228,19 @@ describe('Authentication API (OIDC)', () => {
195228
})
196229
})
197230

231+
describe('with that cookie, private resource and no origin set', () => {
232+
before(done => {
233+
alice.get('/private-for-alice.txt')
234+
.set('Cookie', cookie)
235+
.end((err, res) => {
236+
response = res
237+
done(err)
238+
})
239+
})
240+
241+
it('should return a 200', () => expect(response).to.have.property('status', 200))
242+
})
243+
198244
// How Mallory might set their cookie:
199245
describe('with malicious cookie but without origin', () => {
200246
let response
@@ -296,8 +342,8 @@ describe('Authentication API (OIDC)', () => {
296342
})
297343
})
298344

299-
it('should return a 403', () => {
300-
expect(response).to.have.property('status', 403) // TODO: Should be 401?
345+
it('should return a 401', () => {
346+
expect(response).to.have.property('status', 401)
301347
})
302348
})
303349

@@ -315,8 +361,8 @@ describe('Authentication API (OIDC)', () => {
315361
})
316362
})
317363

318-
it('should return a 403', () => {
319-
expect(response).to.have.property('status', 403)
364+
it('should return a 401', () => {
365+
expect(response).to.have.property('status', 401)
320366
})
321367
})
322368

@@ -333,8 +379,8 @@ describe('Authentication API (OIDC)', () => {
333379
})
334380
})
335381

336-
it('should return a 403', () => {
337-
expect(response).to.have.property('status', 403)
382+
it('should return a 401', () => {
383+
expect(response).to.have.property('status', 401)
338384
})
339385
})
340386

@@ -356,7 +402,24 @@ describe('Authentication API (OIDC)', () => {
356402
})
357403
})
358404

359-
// Authenticated but origin not OK
405+
describe('with malicious cookie and our origin', () => {
406+
let response
407+
before(done => {
408+
var malcookie = cookie.replace(/connect\.sid=(\S+)/, 'connect.sid=l33th4x0rzp0wn4g3;')
409+
alice.get('/private-for-alice.txt')
410+
.set('Cookie', malcookie)
411+
.set('Origin', aliceServerUri)
412+
.end((err, res) => {
413+
response = res
414+
done(err)
415+
})
416+
})
417+
418+
it('should return a 401', () => {
419+
expect(response).to.have.property('status', 401)
420+
})
421+
})
422+
360423
describe('with malicious cookie and a non-matching origin', () => {
361424
let response
362425
before(done => {
@@ -370,8 +433,8 @@ describe('Authentication API (OIDC)', () => {
370433
})
371434
})
372435

373-
it('should return a 403', () => {
374-
expect(response).to.have.property('status', 403)
436+
it('should return a 401', () => {
437+
expect(response).to.have.property('status', 401)
375438
})
376439
})
377440
})

0 commit comments

Comments
 (0)