Making proper use of strictOrigins

lib/acl-checker.js

@@ -16,11 +16,10 @@ class ACLChecker {
   constructor (resource, options = {}) {
     this.resource = resource
     this.resourceUrl = new URL(resource)
-    this.agentOrigin = options.agentOrigin
+    this.agentOrigin = options.strictOrigin && options.agentOrigin ? rdf.sym(options.agentOrigin) : null
     this.fetch = options.fetch
     this.fetchGraph = options.fetchGraph
-    this.strictOrigin = options.strictOrigin
-    this.trustedOrigins = options.trustedOrigins
+    this.trustedOrigins = options.strictOrigin && options.trustedOrigins ? options.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
     this.suffix = options.suffix || DEFAULT_ACL_SUFFIX
     this.aclCached = {}
     this.messagesCached = {}
@@ -56,17 +55,14 @@ class ACLChecker {
     const aclFile = rdf.sym(acl.acl)
     const agent = user ? rdf.sym(user) : null
     const modes = [ACL(mode)]
-    const agentOrigin = this.agentOrigin ? rdf.sym(this.agentOrigin) : null
-    const trustedOrigins = this.trustedOrigins ? this.trustedOrigins.map(trustedOrigin => rdf.sym(trustedOrigin)) : null
+    const agentOrigin = this.agentOrigin
+    const trustedOrigins = this.trustedOrigins
     const accessDenied = aclCheck.accessDenied(acl.graph, resource, directory, aclFile, agent, modes, agentOrigin, trustedOrigins)
-    if (accessDenied && this.agentOrigin && this.resourceUrl.origin !== this.agentOrigin) {
-      this.messagesCached[cacheKey].push(HTTPError(403, accessDenied))
-    } else if (accessDenied && user) {
+
+    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.messagesCached[cacheKey].push(HTTPError(401, 'Unauthenticated'))
     }
     this.aclCached[cacheKey] = Promise.resolve(!accessDenied)
     return this.aclCached[cacheKey]

lib/create-app.js

@@ -206,14 +206,27 @@ 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 = argv.trustedOrigins
+    const trustedOrigins = ldp.getTrustedOrigins(req)
     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)
-    if (!argv.host.allowsSessionFor(userId, origin, trustedOrigins) && !isLogoutRequest(req)) {
+    //
+    // 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)) {
       debug.authentication(`Rejecting session for ${userId} from ${origin}`)
       // Destroy session data
       delete req.session.userId

test/integration/acl-tls-test.js

@@ -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, 403)
+        assert.equal(response.statusCode, 401)
         done()
       })
     })
@@ -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, 403)
+          assert.equal(response.statusCode, 401)
           done()
         })
       })
@@ -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, 403)
+        assert.equal(response.statusCode, 401)
         done()
       })
     })
@@ -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, 403)
+          assert.equal(response.statusCode, 401)
           done()
         })
       })

test/integration/authentication-oidc-test.js

@@ -178,6 +178,39 @@ 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 => {
@@ -194,6 +227,19 @@ 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
@@ -295,8 +341,8 @@ describe('Authentication API (OIDC)', () => {
               })
           })
 
-          it('should return a 403', () => {
-            expect(response).to.have.property('status', 403) // TODO: Should be 401?
+          it('should return a 401', () => {
+            expect(response).to.have.property('status', 401)
           })
         })
 
@@ -314,8 +360,8 @@ describe('Authentication API (OIDC)', () => {
               })
           })
 
-          it('should return a 403', () => {
-            expect(response).to.have.property('status', 403)
+          it('should return a 401', () => {
+            expect(response).to.have.property('status', 401)
           })
         })
 
@@ -332,8 +378,8 @@ describe('Authentication API (OIDC)', () => {
               })
           })
 
-          it('should return a 403', () => {
-            expect(response).to.have.property('status', 403)
+          it('should return a 401', () => {
+            expect(response).to.have.property('status', 401)
           })
         })
 
@@ -355,7 +401,24 @@ describe('Authentication API (OIDC)', () => {
           })
         })
 
-        // Authenticated but origin not OK
+        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)
+          })
+        })
+
         describe('with malicious cookie and a non-matching origin', () => {
           let response
           before(done => {
@@ -369,8 +432,8 @@ describe('Authentication API (OIDC)', () => {
               })
           })
 
-          it('should return a 403', () => {
-            expect(response).to.have.property('status', 403)
+          it('should return a 401', () => {
+            expect(response).to.have.property('status', 401)
           })
         })
       })