Skip to content

Commit

Permalink
oidc: pass "next" path to IdP as "state"
Browse files Browse the repository at this point in the history
This makes the "next" value more trusted.

Closes getodk#1134
Closes getodk#1135
  • Loading branch information
alxndrsn committed Dec 4, 2024
1 parent 911739b commit 975b509
Showing 1 changed file with 14 additions and 9 deletions.
23 changes: 14 additions & 9 deletions lib/resources/oidc.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const ONE_HOUR = 60 * 60 * 1000;
// * https://bugzilla.mozilla.org/show_bug.cgi?id=1648993
// * https://bugs.chromium.org/p/chromium/issues/detail?id=1056543
const CODE_VERIFIER_COOKIE = (HTTPS_ENABLED ? '__Secure-' : '') + 'ocv';
const NEXT_COOKIE = (HTTPS_ENABLED ? '__Secure-' : '') + 'next'; // eslint-disable-line no-multi-spaces
const STATE_COOKIE = (HTTPS_ENABLED ? '__Secure-' : '') + 'next'; // eslint-disable-line no-multi-spaces
const callbackCookieProps = {
httpOnly: true,
secure: HTTPS_ENABLED,
Expand Down Expand Up @@ -95,6 +95,9 @@ const loaderTemplate = `
`;
parse(loaderTemplate); // caches template for future perf.

const stateFor = next => [ generators.state(), next ].join(':');
const nextFrom = state => state.substr(44);

module.exports = (service, endpoint) => {
if (!isEnabled()) return;

Expand All @@ -105,17 +108,19 @@ module.exports = (service, endpoint) => {

const code_challenge = generators.codeChallenge(code_verifier); // eslint-disable-line camelcase

const { next } = req.query ?? '';
const state = stateFor(next);

const authUrl = client.authorizationUrl({
scope: SCOPES.join(' '),
resource: `${envDomain}/v1`,
code_challenge,
code_challenge_method: CODE_CHALLENGE_METHOD,
state,
});

res.cookie(CODE_VERIFIER_COOKIE, code_verifier, { ...callbackCookieProps, maxAge: ONE_HOUR });

const { next } = req.query;
if (next) res.cookie(NEXT_COOKIE, next, { ...callbackCookieProps, maxAge: ONE_HOUR });
res.cookie(STATE_COOKIE, state, { ...callbackCookieProps, maxAge: ONE_HOUR }); // eslint-disable-line no-multi-spaces

redirect(307, authUrl);
} catch (err) {
Expand All @@ -131,15 +136,15 @@ module.exports = (service, endpoint) => {
service.get('/oidc/callback', endpoint.html(async (container, _, req, res) => {
try {
const code_verifier = req.cookies[CODE_VERIFIER_COOKIE]; // eslint-disable-line camelcase
const next = req.cookies[NEXT_COOKIE]; // eslint-disable-line no-multi-spaces
const state = req.cookies[STATE_COOKIE]; // eslint-disable-line no-multi-spaces
res.clearCookie(CODE_VERIFIER_COOKIE, callbackCookieProps);
res.clearCookie(NEXT_COOKIE, callbackCookieProps); // eslint-disable-line no-multi-spaces
res.clearCookie(STATE_COOKIE, callbackCookieProps); // eslint-disable-line no-multi-spaces

const client = await getClient();

const params = client.callbackParams(req);

const tokenSet = await client.callback(getRedirectUri(), params, { response_type: RESPONSE_TYPE, code_verifier });
const tokenSet = await client.callback(getRedirectUri(), params, { response_type: RESPONSE_TYPE, code_verifier, state });

const { access_token } = tokenSet;

Expand All @@ -157,7 +162,7 @@ module.exports = (service, endpoint) => {

await initSession(container, req, res, user);

const nextPath = safeNextPathFrom(next);
const nextPath = safeNextPathFrom(nextFrom(state));

// This redirect would be ideal, but breaks `SameSite: Secure` cookies.
// return redirect(303, nextPath);
Expand All @@ -180,7 +185,7 @@ function errorToFrontend(req, res, errorCode) {

loginUrl.searchParams.append('oidcError', errorCode);

const next = req.cookies[NEXT_COOKIE];
const next = nextFrom(req.cookies[STATE_COOKIE]);
if (next && !Array.isArray(next)) loginUrl.searchParams.append('next', next);

// Append query string manually, because Central Frontend expects search/hash
Expand Down

0 comments on commit 975b509

Please sign in to comment.