Skip to content

Revert "Making proper use of strictOrigins" #1133

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.

Sign up for GitHub

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

Merged
kjetilk merged 1 commit into from
Mar 8, 2019
Merged

Revert "Making proper use of strictOrigins" #1133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 11 additions & 7 deletions lib/acl-checker.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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