Skip to content
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

Respect SameSite in Auth Cookie #188

Merged
merged 1 commit into from
Feb 8, 2021
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
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@
[Full Changelog](https://github.com/auth0/express-openid-connect/compare/v2.1.0...v2.2.0)

**Added**
adamjmcgrath marked this conversation as resolved.
Show resolved Hide resolved
- afterCallback Hook [#168](https://github.com/auth0/express-openid-connect/pull/168) ([davidpatrick](https://github.com/davidpatrick))
- afterCallback Hook [#171](https://github.com/auth0/express-openid-connect/pull/171) ([davidpatrick](https://github.com/davidpatrick))

**Changed**
- Move transient cookies into single cookie [#171](https://github.com/auth0/express-openid-connect/pull/171) ([davidpatrick](https://github.com/davidpatrick))
- Move transient cookies into single cookie [#168](https://github.com/auth0/express-openid-connect/pull/168) ([davidpatrick](https://github.com/davidpatrick))
- Use native node hkdf when available (Node >=15) [#177](https://github.com/auth0/express-openid-connect/pull/177) ([panva](https://github.com/panva))

## [2.1.0](https://github.com/auth0/express-openid-connect/tree/v2.1.0) (2020-12-15)
Expand Down
26 changes: 16 additions & 10 deletions lib/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,26 +223,32 @@ class ResponseContext {
const authVerification = {
nonce: transient.generateNonce(),
state: encodeState(stateValue),
...(options.authorizationParams.max_age ? {
"max_age": options.authorizationParams.max_age
} : undefined)
}
const authParams = { ...options.authorizationParams, ...authVerification };
...(options.authorizationParams.max_age
adamjmcgrath marked this conversation as resolved.
Show resolved Hide resolved
? {
max_age: options.authorizationParams.max_age,
}
: undefined),
};
const authParams = {
...options.authorizationParams,
...authVerification,
};

if (usePKCE) {
authVerification.code_verifier = transient.generateNonce();

authParams.code_challenge_method= 'S256';
authParams.code_challenge_method = 'S256';
authParams.code_challenge = transient.calculateCodeChallenge(
authVerification.code_verifier
);
}

transient.store('auth_verification', req, res, {
sameSite: options.authorizationParams.response_mode === 'form_post'
? 'None'
: 'Lax',
value: JSON.stringify(authVerification)
sameSite:
options.authorizationParams.response_mode === 'form_post'
? 'None'
: config.session.cookie.sameSite,
adamjmcgrath marked this conversation as resolved.
Show resolved Hide resolved
value: JSON.stringify(authVerification),
});

const authorizationUrl = client.authorizationUrl(authParams);
Expand Down
53 changes: 49 additions & 4 deletions test/login.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,24 @@ const filterRoute = (method, path) => {
r.route && r.route.path === path && r.route.methods[method.toLowerCase()];
};

const fetchFromAuthCookie = (res, cookieName) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a utility function to test, and Refactor other function to use it.

Referenced below in my new test cases.

const fetchAuthCookie = (res) => {
const cookieHeaders = res.headers['set-cookie'];
const authCookie = cookieHeaders.filter(
return cookieHeaders.filter(
(header) => header.split('=')[0] === 'auth_verification'
)[0];
};

const fetchFromAuthCookie = (res, cookieName) => {
const authCookie = fetchAuthCookie(res);

if (!authCookie) {
return false
return false;
}

const decodedAuthCookie = querystring.decode(authCookie);
const cookieValuePart = decodedAuthCookie.auth_verification.split('; ')[0].split('.')[0];
const cookieValuePart = decodedAuthCookie.auth_verification
.split('; ')[0]
.split('.')[0];
const authCookieParsed = JSON.parse(cookieValuePart);

return authCookieParsed[cookieName];
Expand Down Expand Up @@ -356,4 +362,43 @@ describe('auth', () => {

assert.isDefined(fetchFromAuthCookie(res, 'code_verifier'));
});

it('should respect sameSite when response_mode is not form_post', async () => {
server = await createServer(
auth({
...defaultConfig,
clientSecret: '__test_client_secret__',
session: {
cookie: {
sameSite: 'Strict',
},
},
authorizationParams: {
response_mode: 'query',
response_type: 'code',
},
})
);
const res = await request.get('/login', { baseUrl, followRedirect: false });
assert.equal(res.statusCode, 302);

assert.include(fetchAuthCookie(res), 'SameSite=Strict');
});

it('should overwrite SameSite to None when response_mode is form_post', async () => {
server = await createServer(
auth({
...defaultConfig,
session: {
cookie: {
sameSite: 'Strict',
},
},
})
);
const res = await request.get('/login', { baseUrl, followRedirect: false });
assert.equal(res.statusCode, 302);

assert.include(fetchAuthCookie(res), 'SameSite=None');
});
});