Skip to content

Commit a4f0cbb

Browse files
kjetilkkjetilk
authored
Merge pull request #1133 from solid/revert-1118-fix/faulty-403
Revert "Making proper use of strictOrigins"
2 parents 332e7ae + 61d6455 commit a4f0cbb

14 files changed

Lines changed: 29 additions & 1487 deletions

‎lib/acl-checker.js‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,11 @@ class ACLChecker {
1616
constructor (resource, options = {}) {
1717
this.resource = resource
1818
this.resourceUrl = new URL(resource)
19-
this.agentOrigin = options.strictOrigin && options.agentOrigin ? rdf.sym(options.agentOrigin) : null
19+
this.agentOrigin = options.agentOrigin
2020
this.fetch = options.fetch
2121
this.fetchGraph = options.fetchGraph
22-
this.trustedOrigins = options.strictOrigin && options.trustedOrigins ? options.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
22+
this.strictOrigin = options.strictOrigin
23+
this.trustedOrigins = options.trustedOrigins
2324
this.suffix = options.suffix || DEFAULT_ACL_SUFFIX
2425
this.aclCached = {}
2526
this.messagesCached = {}
@@ -55,14 +56,17 @@ class ACLChecker {
5556
const aclFile = rdf.sym(acl.acl)
5657
const agent = user ? rdf.sym(user) : null
5758
const modes = [ACL(mode)]
58-
const agentOrigin = this.agentOrigin
59-
const trustedOrigins = this.trustedOrigins
59+
const agentOrigin = this.agentOrigin ? rdf.sym(this.agentOrigin) : null
60+
const trustedOrigins = this.trustedOrigins ? this.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
6061
const accessDenied = aclCheck.accessDenied(acl.graph, resource, directory, aclFile, agent, modes, agentOrigin, trustedOrigins)
61-
62-
if (accessDenied && user) {
62+
if (accessDenied && this.agentOrigin && this.resourceUrl.origin !== this.agentOrigin) {
6363
this.messagesCached[cacheKey].push(HTTPError(403, accessDenied))
64-
} else if (accessDenied) {
64+
} else if (accessDenied && user) {
65+
this.messagesCached[cacheKey].push(HTTPError(403, accessDenied))
66+
} else if (accessDenied && !user) {
6567
this.messagesCached[cacheKey].push(HTTPError(401, 'Unauthenticated'))
68+
} else if (accessDenied) {
69+
this.messagesCached[cacheKey].push(HTTPError(401, accessDenied))
6670
}
6771
this.aclCached[cacheKey] = Promise.resolve(!accessDenied)
6872
return this.aclCached[cacheKey]

‎lib/create-app.js‎

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -212,27 +212,14 @@ 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 = ldp.getTrustedOrigins(req)
215+
const trustedOrigins = argv.trustedOrigins
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-
//
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)) {
222+
if (!argv.host.allowsSessionFor(userId, origin, trustedOrigins) && !isLogoutRequest(req)) {
236223
debug.authentication(`Rejecting session for ${userId} from ${origin}`)
237224
// Destroy session data
238225
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, 401)
335+
assert.equal(response.statusCode, 403)
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, 401)
357+
assert.equal(response.statusCode, 403)
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, 401)
436+
assert.equal(response.statusCode, 403)
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, 401)
458+
assert.equal(response.statusCode, 403)
459459
done()
460460
})
461461
})

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

Lines changed: 9 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -179,39 +179,6 @@ 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-
215182
describe('with that cookie but without origin', () => {
216183
let response
217184
before(done => {
@@ -228,19 +195,6 @@ describe('Authentication API (OIDC)', () => {
228195
})
229196
})
230197

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-
244198
// How Mallory might set their cookie:
245199
describe('with malicious cookie but without origin', () => {
246200
let response
@@ -342,8 +296,8 @@ describe('Authentication API (OIDC)', () => {
342296
})
343297
})
344298

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

@@ -361,8 +315,8 @@ describe('Authentication API (OIDC)', () => {
361315
})
362316
})
363317

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

@@ -379,8 +333,8 @@ describe('Authentication API (OIDC)', () => {
379333
})
380334
})
381335

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

@@ -402,24 +356,7 @@ describe('Authentication API (OIDC)', () => {
402356
})
403357
})
404358

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-
359+
// Authenticated but origin not OK
423360
describe('with malicious cookie and a non-matching origin', () => {
424361
let response
425362
before(done => {
@@ -433,8 +370,8 @@ describe('Authentication API (OIDC)', () => {
433370
})
434371
})
435372

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

0 commit comments

Comments
 (0)