Skip to content

Commit de38bf8

Browse files
committed
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. Closes #524. Breaking change, needs new semver-major.
1 parent 95701d9 commit de38bf8

File tree

5 files changed

+177
-16
lines changed

5 files changed

+177
-16
lines changed

lib/create-app.js

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ const OidcManager = require('./models/oidc-manager')
2020
const config = require('./server-config')
2121
const defaults = require('../config/defaults')
2222
const options = require('./handlers/options')
23+
const debug = require('./debug').authentication
2324

2425
const corsSettings = cors({
2526
methods: [
@@ -143,9 +144,29 @@ function initViews (app, configPath) {
143144
function initWebId (argv, app, ldp) {
144145
config.ensureWelcomePage(argv)
145146

146-
// Use session cookies
147+
// Store the user's session key in a cookie
148+
// (for same-domain browsing by people only)
147149
const useSecureCookies = argv.webid // argv.webid forces https and secure cookies
148-
app.use(session(sessionSettings(useSecureCookies, argv.host)))
150+
const sessionHandler = session(sessionSettings(useSecureCookies, argv.host))
151+
app.use((req, res, next) => {
152+
sessionHandler(req, res, () => {
153+
// Reject cookies from third-party applications.
154+
// Otherwise, when a user is logged in to their Solid server,
155+
// any third-party application could perform authenticated requests
156+
// without permission by including the credentials set by the Solid server.
157+
const origin = req.headers.origin
158+
const userId = req.session.userId
159+
if (!argv.host.allowsSessionFor(userId, origin)) {
160+
debug(`Rejecting session for ${userId} from ${origin}`)
161+
// Destroy session data
162+
delete req.session.userId
163+
delete req.session.identified
164+
// Ensure this modified session is not saved
165+
req.session.save = (done) => done()
166+
}
167+
next()
168+
})
169+
})
149170

150171
let accountManager = AccountManager.from({
151172
authMethod: argv.auth,

lib/models/solid-host.js

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,25 @@ class SolidHost {
6161
}
6262
return this.parsedUri.protocol + '//' + accountName + '.' + this.host
6363
}
64+
/**
65+
* Determines whether the given user is allowed to restore a session
66+
* from the given origin.
67+
*
68+
* @param userId {?string}
69+
* @param origin {?string}
70+
* @return {boolean}
71+
*/
72+
allowsSessionFor (userId, origin) {
73+
// Allow no user or an empty origin
74+
if (!userId || !origin) return true
75+
// Allow the server's main domain
76+
if (origin === this.serverUri) return true
77+
// Allow the user's subdomain
78+
const userIdHost = userId.replace(/([^:/])\/.*/, '$1')
79+
if (origin === userIdHost) return true
80+
// Disallow everything else
81+
return false
82+
}
6483

6584
/**
6685
* Returns the /authorize endpoint URL object (used in login requests, etc).

test/integration/authentication-oidc.js

Lines changed: 99 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -145,18 +145,106 @@ describe('Authentication API (OIDC)', () => {
145145
fs.removeSync(path.join(aliceDbPath, 'users/users'))
146146
})
147147

148-
it('should login and be redirected to /authorize', (done) => {
149-
alice.post('/login/password')
150-
.type('form')
151-
.send({ username: 'alice' })
152-
.send({ password: alicePassword })
153-
.expect(302)
154-
.expect('set-cookie', /connect.sid/)
155-
.end((err, res) => {
156-
let loginUri = res.header.location
157-
expect(loginUri.startsWith(aliceServerUri + '/authorize'))
158-
done(err)
148+
describe('after performing a correct login', () => {
149+
let response, cookie
150+
before(done => {
151+
aliceUserStore.initCollections()
152+
aliceUserStore.createUser(aliceAccount, alicePassword)
153+
alice.post('/login/password')
154+
.type('form')
155+
.send({ username: 'alice' })
156+
.send({ password: alicePassword })
157+
.end((err, res) => {
158+
response = res
159+
cookie = response.headers['set-cookie'][0]
160+
done(err)
161+
})
162+
})
163+
164+
it('should redirect to /authorize', () => {
165+
const loginUri = response.headers.location
166+
expect(response).to.have.property('status', 302)
167+
expect(loginUri.startsWith(aliceServerUri + '/authorize'))
168+
})
169+
170+
it('should set the cookie', () => {
171+
expect(cookie).to.match(/connect.sid=/)
172+
})
173+
174+
it('should set the cookie with HttpOnly', () => {
175+
expect(cookie).to.match(/HttpOnly/)
176+
})
177+
178+
it('should set the cookie with Secure', () => {
179+
expect(cookie).to.match(/Secure/)
180+
})
181+
182+
describe('and performing a subsequent request', () => {
183+
describe('without that cookie', () => {
184+
let response
185+
before(done => {
186+
alice.get('/')
187+
.end((err, res) => {
188+
response = res
189+
done(err)
190+
})
191+
})
192+
193+
it('should return a 401', () => {
194+
expect(response).to.have.property('status', 401)
195+
})
196+
})
197+
198+
describe('with that cookie and a non-matching origin', () => {
199+
let response
200+
before(done => {
201+
alice.get('/')
202+
.set('Cookie', cookie)
203+
.set('Origin', bobServerUri)
204+
.end((err, res) => {
205+
response = res
206+
done(err)
207+
})
208+
})
209+
210+
it('should return a 401', () => {
211+
expect(response).to.have.property('status', 401)
212+
})
213+
})
214+
215+
describe('with that cookie but without origin', () => {
216+
let response
217+
before(done => {
218+
alice.get('/')
219+
.set('Cookie', cookie)
220+
.end((err, res) => {
221+
response = res
222+
done(err)
223+
})
224+
})
225+
226+
it('should return a 200', () => {
227+
expect(response).to.have.property('status', 200)
228+
})
229+
})
230+
231+
describe('with that cookie and a matching origin', () => {
232+
let response
233+
before(done => {
234+
alice.get('/')
235+
.set('Cookie', cookie)
236+
.set('Origin', aliceServerUri)
237+
.end((err, res) => {
238+
response = res
239+
done(err)
240+
})
241+
})
242+
243+
it('should return a 200', () => {
244+
expect(response).to.have.property('status', 200)
245+
})
159246
})
247+
})
160248
})
161249

162250
it('should throw a 400 if no username is provided', (done) => {
Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<#Owner>
22
a <http://www.w3.org/ns/auth/acl#Authorization> ;
33
<http://www.w3.org/ns/auth/acl#accessTo> <./>;
4-
<http://www.w3.org/ns/auth/acl#agent> <https://127.0.0.1:5000/profile/card#me>;
5-
<http://www.w3.org/ns/auth/acl#mode> <http://www.w3.org/ns/auth/acl#Read>, <http://www.w3.org/ns/auth/acl#Write>, <http://www.w3.org/ns/auth/acl#Control> .
4+
<http://www.w3.org/ns/auth/acl#agent> <https://localhost:7000/profile/card#me>;
5+
<http://www.w3.org/ns/auth/acl#mode> <http://www.w3.org/ns/auth/acl#Read>, <http://www.w3.org/ns/auth/acl#Write>, <http://www.w3.org/ns/auth/acl#Control> .

test/unit/solid-host.js

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ describe('SolidHost', () => {
2626
})
2727
})
2828

29-
describe('uriForAccount()', () => {
29+
describe('accountUriFor()', () => {
3030
it('should compose an account uri for an account name', () => {
3131
let config = {
3232
serverUri: 'https://test.local'
@@ -42,6 +42,39 @@ describe('SolidHost', () => {
4242
})
4343
})
4444

45+
describe('allowsSessionFor()', () => {
46+
let host
47+
before(() => {
48+
host = SolidHost.from({
49+
serverUri: 'https://test.local'
50+
})
51+
})
52+
53+
it('should allow an empty userId and origin', () => {
54+
expect(host.allowsSessionFor('', '')).to.be.true
55+
})
56+
57+
it('should allow a userId with empty origin', () => {
58+
expect(host.allowsSessionFor('https://user.test.local/profile/card#me', '')).to.be.true
59+
})
60+
61+
it('should allow a userId with the user subdomain as origin', () => {
62+
expect(host.allowsSessionFor('https://user.test.local/profile/card#me', 'https://user.test.local')).to.be.true
63+
})
64+
65+
it('should disallow a userId with another subdomain as origin', () => {
66+
expect(host.allowsSessionFor('https://user.test.local/profile/card#me', 'https://other.test.local')).to.be.false
67+
})
68+
69+
it('should allow a userId with the server domain as origin', () => {
70+
expect(host.allowsSessionFor('https://user.test.local/profile/card#me', 'https://test.local')).to.be.true
71+
})
72+
73+
it('should disallow a userId from a different domain', () => {
74+
expect(host.allowsSessionFor('https://user.test.local/profile/card#me', 'https://other.remote')).to.be.false
75+
})
76+
})
77+
4578
describe('cookieDomain getter', () => {
4679
it('should return null for single-part domains (localhost)', () => {
4780
let host = SolidHost.from({

0 commit comments

Comments
 (0)