Merge pull request #835 from solid/fix/allow-subdomain-cookies

Block cookie auth from non-subdomains, but allow all other auth from everywhere

Author: RubenVerborgh

lib/create-app.js

@@ -169,6 +169,27 @@ function initWebId (argv, app, ldp) {
   const useSecureCookies = !!argv.sslKey // use secure cookies when over HTTPS
   const sessionHandler = session(sessionSettings(useSecureCookies, argv.host))
   app.use(sessionHandler)
+  // Reject cookies from third-party applications.
+  // Otherwise, when a user is logged in to their Solid server,
+  // any third-party application could perform authenticated requests
+  // without permission by including the credentials set by the Solid server.
+  app.use((req, res, next) => {
+    const origin = req.headers.origin
+    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) && !isLogoutRequest(req)) {
+      debug(`Rejecting session for ${userId} from ${origin}`)
+      // Destroy session data
+      delete req.session.userId
+      // Ensure this modified session is not saved
+      req.session.save = (done) => done()
+    }
+    next()
+  })
 
   let accountManager = AccountManager.from({
     authMethod: argv.auth,
@@ -187,30 +208,20 @@ function initWebId (argv, app, ldp) {
   // Set up authentication-related API endpoints and app.locals
   initAuthentication(app, argv)
 
-  // Protect against requests from third-party applications
-  app.use((req, res, next) => {
-    // Reject cookies from third-party applications.
-    // Otherwise, when a user is logged in to their Solid server,
-    // any third-party application could perform authenticated requests
-    // without permission by including the credentials set by the Solid server.
-    const origin = req.headers.origin
-    const userId = req.session.userId
-    if (!argv.host.allowsSessionFor(userId, origin)) {
-      debug(`Rejecting session for ${userId} from ${origin}`)
-      // Destroy session data
-      delete req.session.userId
-      // Ensure this modified session is not saved
-      req.session.save = done => done()
-    }
-    next()
-  })
-
-  // Set up per-host LDP middleware
   if (argv.multiuser) {
     app.use(vhost('*', LdpMiddleware(corsSettings)))
   }
 }
 
+/**
+ * Determines whether the given request is a logout request
+ */
+function isLogoutRequest (req) {
+  // TODO: this is a hack that hard-codes OIDC paths,
+  // this code should live in the OIDC module
+  return req.path === '/logout' || req.path === '/goodbye'
+}
+
 /**
  * Sets up authentication-related routes and handlers for the app.
  *

lib/models/solid-host.js

@@ -72,12 +72,14 @@ class SolidHost {
   allowsSessionFor (userId, origin) {
     // Allow no user or an empty origin
     if (!userId || !origin) return true
-    // Allow the server's main domain
-    if (origin === this.serverUri) return true
-    // Allow the user's subdomain
-    const userIdHost = userId.replace(/([^:/])\/.*/, '$1')
-    if (origin === userIdHost) return true
-    // Disallow everything else
+    // Allow the server and subdomains
+    const originHost = getHostName(origin)
+    const serverHost = getHostName(this.serverUri)
+    if (originHost === serverHost) return true
+    if (originHost.endsWith('.' + serverHost)) return true
+    // Allow the user's own domain
+    const userHost = getHostName(userId)
+    if (originHost === userHost) return true
     return false
   }
 
@@ -109,4 +111,9 @@ class SolidHost {
   }
 }
 
+function getHostName (url) {
+  const match = url.match(/^\w+:\/*([^/]+)/)
+  return match ? match[1] : ''
+}
+
 module.exports = SolidHost

test/unit/solid-host-test.js

@@ -55,23 +55,23 @@ describe('SolidHost', () => {
     })
 
     it('should allow a userId with empty origin', () => {
-      expect(host.allowsSessionFor('https://user.test.local/profile/card#me', '')).to.be.true
+      expect(host.allowsSessionFor('https://user.own/profile/card#me', '')).to.be.true
     })
 
     it('should allow a userId with the user subdomain as origin', () => {
-      expect(host.allowsSessionFor('https://user.test.local/profile/card#me', 'https://user.test.local')).to.be.true
+      expect(host.allowsSessionFor('https://user.own/profile/card#me', 'https://user.own')).to.be.true
     })
 
-    it('should disallow a userId with another subdomain as origin', () => {
-      expect(host.allowsSessionFor('https://user.test.local/profile/card#me', 'https://other.test.local')).to.be.false
+    it('should allow a userId with the server domain as origin', () => {
+      expect(host.allowsSessionFor('https://user.own/profile/card#me', 'https://test.local')).to.be.true
     })
 
-    it('should allow a userId with the server domain as origin', () => {
-      expect(host.allowsSessionFor('https://user.test.local/profile/card#me', 'https://test.local')).to.be.true
+    it('should allow a userId with a server subdomain as origin', () => {
+      expect(host.allowsSessionFor('https://user.own/profile/card#me', 'https://other.test.local')).to.be.true
     })
 
     it('should disallow a userId from a different domain', () => {
-      expect(host.allowsSessionFor('https://user.test.local/profile/card#me', 'https://other.remote')).to.be.false
+      expect(host.allowsSessionFor('https://user.own/profile/card#me', 'https://other.remote')).to.be.false
     })
   })