Skip to content

Commit

Permalink
Merge pull request #1133 from solid/revert-1118-fix/faulty-403
Browse files Browse the repository at this point in the history 
Revert "Making proper use of strictOrigins"
  • Loading branch information
@kjetilk
kjetilk authored Mar 8, 2019
2 parents 332e7ae + 61d6455 commit a4f0cbb
Show file tree
Hide file tree
Showing 14 changed files with 29 additions and 1,487 deletions.
18 changes: 11 additions & 7 deletions lib/acl-checker.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ class ACLChecker {
constructor (resource, options = {}) {
this.resource = resource
this.resourceUrl = new URL(resource)
this.agentOrigin = options.strictOrigin && options.agentOrigin ? rdf.sym(options.agentOrigin) : null
this.agentOrigin = options.agentOrigin
this.fetch = options.fetch
this.fetchGraph = options.fetchGraph
this.trustedOrigins = options.strictOrigin && options.trustedOrigins ? options.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
this.strictOrigin = options.strictOrigin
this.trustedOrigins = options.trustedOrigins
this.suffix = options.suffix || DEFAULT_ACL_SUFFIX
this.aclCached = {}
this.messagesCached = {}
Expand Down Expand Up @@ -55,14 +56,17 @@ class ACLChecker {
const aclFile = rdf.sym(acl.acl)
const agent = user ? rdf.sym(user) : null
const modes = [ACL(mode)]
const agentOrigin = this.agentOrigin
const trustedOrigins = this.trustedOrigins
const agentOrigin = this.agentOrigin ? rdf.sym(this.agentOrigin) : null
const trustedOrigins = this.trustedOrigins ? this.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
const accessDenied = aclCheck.accessDenied(acl.graph, resource, directory, aclFile, agent, modes, agentOrigin, trustedOrigins)

if (accessDenied && user) {
if (accessDenied && this.agentOrigin && this.resourceUrl.origin !== this.agentOrigin) {
this.messagesCached[cacheKey].push(HTTPError(403, accessDenied))
} else if (accessDenied) {
} else if (accessDenied && user) {
this.messagesCached[cacheKey].push(HTTPError(403, accessDenied))
} else if (accessDenied && !user) {
this.messagesCached[cacheKey].push(HTTPError(401, 'Unauthenticated'))
} else if (accessDenied) {
this.messagesCached[cacheKey].push(HTTPError(401, accessDenied))
}
this.aclCached[cacheKey] = Promise.resolve(!accessDenied)
return this.aclCached[cacheKey]
Expand Down
17 changes: 2 additions & 15 deletions lib/create-app.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,27 +212,14 @@ function initWebId (argv, app, ldp) {
// without permission by including the credentials set by the Solid server.
app.use((req, res, next) => {
const origin = req.get('origin')
const trustedOrigins = ldp.getTrustedOrigins(req)
const trustedOrigins = argv.trustedOrigins
const userId = req.session.userId
// Exception: allow logout requests from all third-party apps
// such that OIDC client can log out via cookie auth
// TODO: remove this exception when OIDC clients
// use Bearer token to authenticate instead of cookie
// (https://github.com/solid/node-solid-server/pull/835#issuecomment-426429003)
//
// Authentication cookies are an optimization:
// instead of going through the process of
// fully validating authentication on every request,
// we go through this process once,
// and store its successful result in a cookie
// that will be reused upon the next request.
// However, that cookie can then be sent by any server,
// even servers that have not gone through the proper authentication mechanism.
// However, if trusted origins are enabled,
// then any origin is allowed to take the shortcut route,
// since malicious origins will be banned at the ACL checking phase.
// https://github.com/solid/node-solid-server/issues/1117
if (!argv.strictOrigin && !argv.host.allowsSessionFor(userId, origin, trustedOrigins) && !isLogoutRequest(req)) {
if (!argv.host.allowsSessionFor(userId, origin, trustedOrigins) && !isLogoutRequest(req)) {
debug.authentication(`Rejecting session for ${userId} from ${origin}`)
// Destroy session data
delete req.session.userId
Expand Down
8 changes: 4 additions & 4 deletions test/integration/acl-tls-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ describe('ACL with WebID+TLS', function () {

request.head(options, function (error, response, body) {
assert.equal(error, null)
assert.equal(response.statusCode, 401)
assert.equal(response.statusCode, 403)
done()
})
})
Expand All @@ -354,7 +354,7 @@ describe('ACL with WebID+TLS', function () {

request.head(options, function (error, response, body) {
assert.equal(error, null)
assert.equal(response.statusCode, 401)
assert.equal(response.statusCode, 403)
done()
})
})
Expand Down Expand Up @@ -433,7 +433,7 @@ describe('ACL with WebID+TLS', function () {

request.head(options, function (error, response, body) {
assert.equal(error, null)
assert.equal(response.statusCode, 401)
assert.equal(response.statusCode, 403)
done()
})
})
Expand All @@ -455,7 +455,7 @@ describe('ACL with WebID+TLS', function () {

request.head(options, function (error, response, body) {
assert.equal(error, null)
assert.equal(response.statusCode, 401)
assert.equal(response.statusCode, 403)
done()
})
})
Expand Down
81 changes: 9 additions & 72 deletions test/integration/authentication-oidc-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,39 +179,6 @@ describe('Authentication API (OIDC)', () => {
})
})

describe('with that cookie and a non-matching origin', () => {
let response
before(done => {
alice.get('/private-for-alice.txt')
.set('Cookie', cookie)
.set('Origin', bobServerUri)
.end((err, res) => {
response = res
done(err)
})
})

it('should return a 403', () => {
expect(response).to.have.property('status', 403)
})
})

describe('without that cookie and a non-matching origin', () => {
let response
before(done => {
alice.get('/private-for-alice.txt')
.set('Origin', bobServerUri)
.end((err, res) => {
response = res
done(err)
})
})

it('should return a 401', () => {
expect(response).to.have.property('status', 401)
})
})

describe('with that cookie but without origin', () => {
let response
before(done => {
Expand All @@ -228,19 +195,6 @@ describe('Authentication API (OIDC)', () => {
})
})

describe('with that cookie, private resource and no origin set', () => {
before(done => {
alice.get('/private-for-alice.txt')
.set('Cookie', cookie)
.end((err, res) => {
response = res
done(err)
})
})

it('should return a 200', () => expect(response).to.have.property('status', 200))
})

// How Mallory might set their cookie:
describe('with malicious cookie but without origin', () => {
let response
Expand Down Expand Up @@ -342,8 +296,8 @@ describe('Authentication API (OIDC)', () => {
})
})

it('should return a 401', () => {
expect(response).to.have.property('status', 401)
it('should return a 403', () => {
expect(response).to.have.property('status', 403) // TODO: Should be 401?
})
})

Expand All @@ -361,8 +315,8 @@ describe('Authentication API (OIDC)', () => {
})
})

it('should return a 401', () => {
expect(response).to.have.property('status', 401)
it('should return a 403', () => {
expect(response).to.have.property('status', 403)
})
})

Expand All @@ -379,8 +333,8 @@ describe('Authentication API (OIDC)', () => {
})
})

it('should return a 401', () => {
expect(response).to.have.property('status', 401)
it('should return a 403', () => {
expect(response).to.have.property('status', 403)
})
})

Expand All @@ -402,24 +356,7 @@ describe('Authentication API (OIDC)', () => {
})
})

describe('with malicious cookie and our origin', () => {
let response
before(done => {
var malcookie = cookie.replace(/connect\.sid=(\S+)/, 'connect.sid=l33th4x0rzp0wn4g3;')
alice.get('/private-for-alice.txt')
.set('Cookie', malcookie)
.set('Origin', aliceServerUri)
.end((err, res) => {
response = res
done(err)
})
})

it('should return a 401', () => {
expect(response).to.have.property('status', 401)
})
})

// Authenticated but origin not OK
describe('with malicious cookie and a non-matching origin', () => {
let response
before(done => {
Expand All @@ -433,8 +370,8 @@ describe('Authentication API (OIDC)', () => {
})
})

it('should return a 401', () => {
expect(response).to.have.property('status', 401)
it('should return a 403', () => {
expect(response).to.have.property('status', 403)
})
})
})
Expand Down
Loading

0 comments on commit a4f0cbb

Please sign in to comment.