Skip to content

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

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.

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
merged 3 commits into from
Oct 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
49 changes: 30 additions & 19 deletions lib/create-app.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kidehen I copied this code from @smalinin, but I actually don't think that goodbye is necessary in this exception list (although it doesn't really hurt). The user needs to be authenticated for logout, but not for goodbye, right?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm..
I would suggest the workaround as is for now, we know it works (and it doesn't hurt in the worst case).

/cc @smalinin @cblakeley

}

/**
* Sets up authentication-related routes and handlers for the app.
*
Expand Down
19 changes: 13 additions & 6 deletions lib/models/solid-host.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -109,4 +111,9 @@ class SolidHost {
}
}

function getHostName (url) {
const match = url.match(/^\w+:\/*([^/]+)/)
return match ? match[1] : ''
}

module.exports = SolidHost
14 changes: 7 additions & 7 deletions test/unit/solid-host-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
})

Expand Down