From 775f111192380def9ed9a0855d976505c1182c5e Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Mon, 18 Sep 2023 14:55:56 +0200 Subject: [PATCH] fix: consent and logout integration logic --- README.md | 2 + package-lock.json | 8 +- package.json | 2 +- src/index.ts | 47 +++++++++-- src/pkg/index.ts | 12 +++ src/pkg/route.ts | 11 ++- src/routes/consent.ts | 167 ++++++++++++++-------------------------- src/routes/csrfError.ts | 22 ++++++ src/routes/index.ts | 1 + src/routes/logout.ts | 89 +++++++++++++++++++++ 10 files changed, 241 insertions(+), 120 deletions(-) create mode 100644 src/routes/csrfError.ts create mode 100644 src/routes/logout.ts diff --git a/README.md b/README.md index 1f03aae0..fadff438 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,8 @@ Ory OAuth2 requires more setup to get CSRF cookies on the `/consent` endpoint. setup. This sets the CSRF cookies to `secure: false`, required for running locally. When using this setting, you must also set `CSRF_COOKIE_NAME` to a name without the `__Host-` prefix. +- `TRUSTED_CLIENT_IDS` (optional): A list of trusted client ids. They can be set + to skip the consent screen. Getting TLS working: diff --git a/package-lock.json b/package-lock.json index 095c3664..0257b107 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,7 @@ "axios": "1.2.6", "body-parser": "1.20.2", "cookie-parser": "1.4.6", - "csrf-csrf": "3.0.0", + "csrf-csrf": "3.0.1", "express": "4.18.2", "express-handlebars": "6.0.7", "express-jwt": "8.4.1", @@ -1854,9 +1854,9 @@ } }, "node_modules/csrf-csrf": { - "version": "3.0.0", - "resolved": "https://registry.npmjs.org/csrf-csrf/-/csrf-csrf-3.0.0.tgz", - "integrity": "sha512-fAF+gxIRKVlmVIOhbfQ/hPyza1RZz5rhqvP658CLdKZXAUnrK67bo2ZwGOumTNYQ6kfyamOeFsVDx3zx1GJNfQ==", + "version": "3.0.1", + "resolved": "https://registry.npmjs.org/csrf-csrf/-/csrf-csrf-3.0.1.tgz", + "integrity": "sha512-x+F6fV1Z+Vyl6XyNI6kjWg3MJjP3CfTb/CNmMi7c8klI9wtxeUFHqLt1GeJsxuo0s4qkUqT3vAbdHHHlmGrrsA==", "dependencies": { "http-errors": "^2.0.0" } diff --git a/package.json b/package.json index 6d05573e..c9267065 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "axios": "1.2.6", "body-parser": "1.20.2", "cookie-parser": "1.4.6", - "csrf-csrf": "3.0.0", + "csrf-csrf": "3.0.1", "express": "4.18.2", "express-handlebars": "6.0.7", "express-jwt": "8.4.1", diff --git a/src/index.ts b/src/index.ts index 380ef342..5f3d80c4 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,11 +6,10 @@ import { detectLanguage, handlebarsHelpers, } from "./pkg" -import { middleware as middlewareLogger } from "./pkg/logger" +import { logger, middleware as middlewareLogger } from "./pkg/logger" import { register404Route, register500Route, - registerConsentPostRoute, registerConsentRoute, registerErrorRoute, registerHealthRoute, @@ -22,8 +21,12 @@ import { registerStaticRoutes, registerVerificationRoute, registerWelcomeRoute, + registerLogoutRoute, } from "./routes" +import { csrfErrorHandler } from "./routes/csrfError" +import bodyParser from "body-parser" import cookieParser from "cookie-parser" +import { DoubleCsrfCookieOptions, doubleCsrf } from "csrf-csrf" import express, { Request, Response } from "express" import { engine } from "express-handlebars" import * as fs from "fs" @@ -34,10 +37,40 @@ const baseUrl = process.env.BASE_PATH || "/" const app = express() const router = express.Router() +const cookieOptions: DoubleCsrfCookieOptions = { + sameSite: "lax", + signed: true, + // set secure cookies by default (recommended in production) + // can be disabled through DANGEROUSLY_DISABLE_SECURE_COOKIES=true env var + secure: true, + ...(process.env.DANGEROUSLY_DISABLE_SECURE_CSRF_COOKIES && { + secure: false, + }), +} + +const cookieName = process.env.CSRF_COOKIE_NAME || "__Host-ax-x-csrf-token" +const cookieSecret = process.env.CSRF_COOKIE_SECRET + +// Sets up csrf protection +const { + invalidCsrfTokenError, + doubleCsrfProtection, // This is the default CSRF protection middleware. +} = doubleCsrf({ + getSecret: () => cookieSecret || "", // A function that optionally takes the request and returns a secret + cookieName: cookieName, // The name of the cookie to be used, recommend using Host prefix. + cookieOptions, + ignoredMethods: ["GET", "HEAD", "OPTIONS", "PUT", "DELETE"], // A list of request methods that will not be protected. + getTokenFromRequest: (req: Request) => { + logger.debug("Getting CSRF token from request", { body: req.body }) + return req.body._csrf + }, // A function that returns the token from the request +}) + app.use(middlewareLogger) app.use(cookieParser(process.env.COOKIE_SECRET || "")) app.use(addFavicon(defaultConfig)) app.use(detectLanguage) +app.use(bodyParser.urlencoded({ extended: false })) app.set("view engine", "hbs") app.engine( @@ -53,8 +86,6 @@ app.engine( registerHealthRoute(router) registerLoginRoute(router) -registerConsentRoute(app) -registerConsentPostRoute(app) registerRecoveryRoute(router) registerRegistrationRoute(router) registerSettingsRoute(router) @@ -67,10 +98,16 @@ router.get("/", (req: Request, res: Response) => { res.redirect(303, "welcome") }) +// all routes registered after this point are protected by CSRF +app.use(doubleCsrfProtection) +app.use(csrfErrorHandler(invalidCsrfTokenError)) + +registerConsentRoute(app) +registerLogoutRoute(app) + registerStaticRoutes(router) register404Route(router) register500Route(router) - app.use(baseUrl, router) const port = Number(process.env.PORT) || 3000 diff --git a/src/pkg/index.ts b/src/pkg/index.ts index bf3eb583..17272d34 100644 --- a/src/pkg/index.ts +++ b/src/pkg/index.ts @@ -38,6 +38,18 @@ export const defaultConfig: RouteOptionsCreator = () => { process.env.CSRF_COOKIE_NAME ? true : false, + shouldSkipConsent: (challenge) => { + let trustedClients: string[] = [] + if (process.env.TRUSTED_CLIENT_IDS) { + trustedClients = String(process.env.TRUSTED_CLIENT_IDS).split(",") + } + return challenge.skip || + challenge.client?.skip_consent || + (challenge.client?.client_id && + trustedClients.indexOf(challenge.client?.client_id) > -1) + ? true + : false + }, ...sdk, } } diff --git a/src/pkg/route.ts b/src/pkg/route.ts index 8fcdd30b..d9afa5ac 100644 --- a/src/pkg/route.ts +++ b/src/pkg/route.ts @@ -1,6 +1,11 @@ // Copyright © 2022 Ory Corp // SPDX-License-Identifier: Apache-2.0 -import { FrontendApi, IdentityApi, OAuth2Api } from "@ory/client" +import { + FrontendApi, + IdentityApi, + OAuth2Api, + OAuth2ConsentRequest, +} from "@ory/client" import { Theme } from "@ory/elements-markup" import { NextFunction, Request, Response, Router } from "express" @@ -15,6 +20,10 @@ export interface RouteOptions { // This is used to determine if the consent route should be registered // We need to check if the required environment variables are set isOAuthConsentRouteEnabled: () => boolean + // Checks if the OAuth2 consent request should be skipped + // In some cases Hydra will let us skip the consent request + // Setting `TRUSTED_CLIENT_IDS` will skip the consent request for the given client ids + shouldSkipConsent: (challenge: OAuth2ConsentRequest) => boolean logoUrl?: string faviconUrl?: string diff --git a/src/routes/consent.ts b/src/routes/consent.ts index a8b42d67..4eac9594 100644 --- a/src/routes/consent.ts +++ b/src/routes/consent.ts @@ -1,63 +1,17 @@ // Copyright © 2023 Ory Corp // SPDX-License-Identifier: Apache-2.0 -import { defaultConfig, logger, RouteCreator, RouteRegistrator } from "../pkg" +import { + defaultConfig, + logger, + RouteCreator, + RouteRegistrator, + setSession, +} from "../pkg" import { oidcConformityMaybeFakeSession } from "./stub/oidc-cert" import { AcceptOAuth2ConsentRequestSession } from "@ory/client" import { UserConsentCard } from "@ory/elements-markup" -import bodyParser from "body-parser" -import { doubleCsrf, DoubleCsrfCookieOptions } from "csrf-csrf" import { Request, Response, NextFunction } from "express" -const cookieOptions: DoubleCsrfCookieOptions = { - sameSite: "lax", - signed: true, - // set secure cookies by default (recommended in production) - // can be disabled through DANGEROUSLY_DISABLE_SECURE_COOKIES=true env var - secure: true, - ...(process.env.DANGEROUSLY_DISABLE_SECURE_CSRF_COOKIES && { - secure: false, - }), -} - -const cookieName = process.env.CSRF_COOKIE_NAME || "__Host-ax-x-csrf-token" -const cookieSecret = process.env.CSRF_COOKIE_SECRET - -// Sets up csrf protection -const { - invalidCsrfTokenError, - doubleCsrfProtection, // This is the default CSRF protection middleware. -} = doubleCsrf({ - getSecret: () => cookieSecret || "", // A function that optionally takes the request and returns a secret - cookieName: cookieName, // The name of the cookie to be used, recommend using Host prefix. - cookieOptions, - ignoredMethods: ["GET", "HEAD", "OPTIONS", "PUT", "DELETE"], // A list of request methods that will not be protected. - getTokenFromRequest: (req: Request) => { - logger.debug("Getting CSRF token from request", { body: req.body }) - return req.body._csrf - }, // A function that returns the token from the request -}) - -// Error handling, validation error interception -const csrfErrorHandler = ( - error: unknown, - req: Request, - res: Response, - next: NextFunction, -) => { - if (error == invalidCsrfTokenError) { - logger.debug("The CSRF token is invalid or could not be found.", { - req: req, - }) - next( - new Error( - "A security violation was detected, please fill out the form again.", - ), - ) - } else { - next() - } -} - const extractSession = ( req: Request, grantScope: string[], @@ -112,10 +66,16 @@ const extractSession = ( // A simple express handler that shows the Hydra consent screen. export const createConsentRoute: RouteCreator = - (createHelpers) => (req: Request, res: Response, next: NextFunction) => { - res.locals.projectName = "An application requests access to your data!" + (createHelpers) => + async (req: Request, res: Response, next: NextFunction) => { + res.locals.projectName = "Consent" - const { oauth2, isOAuthConsentRouteEnabled } = createHelpers(req, res) + const { + oauth2, + isOAuthConsentRouteEnabled, + shouldSkipConsent: canSkipConsent, + logoUrl, + } = createHelpers(req, res) if (!isOAuthConsentRouteEnabled()) { res.redirect("404") @@ -133,36 +93,21 @@ export const createConsentRoute: RouteCreator = return } - let trustedClients: string[] = [] - if (process.env.TRUSTED_CLIENT_IDS) { - trustedClients = String(process.env.TRUSTED_CLIENT_IDS).split(",") - } - // This section processes consent requests and either shows the consent UI or // accepts the consent request right away if the user has given consent to this // app before - oauth2 + await oauth2 .getOAuth2ConsentRequest({ consentChallenge: challenge }) // This will be called if the HTTP request was successful .then(async ({ data: body }) => { // If a user has granted this application the requested scope, hydra will tell us to not show the UI. - if ( - body.skip || - body.client?.skip_consent || - (body.client?.client_id && - trustedClients.indexOf(body.client?.client_id) > -1) - ) { - // You can apply logic here, for example grant another scope, or do whatever... - // ... - - let grantScope = req.body.grant_scope - if (!Array.isArray(grantScope)) { - grantScope = [grantScope] - } + if (canSkipConsent(body)) { + logger.debug("Skipping consent request and accepting it.") + let grantScope = body.requested_scope || [] const session = extractSession(req, grantScope) // Now it's time to grant the consent request. You could also deny the request if something went terribly wrong - return oauth2 + await oauth2 .acceptOAuth2ConsentRequest({ consentChallenge: challenge, acceptOAuth2ConsentRequest: { @@ -179,13 +124,19 @@ export const createConsentRoute: RouteCreator = }, }) .then(({ data: body }) => { + logger.debug("Consent request successfuly accepted") // All we need to do now is to redirect the user back to hydra! res.redirect(String(body.redirect_to)) }) + .catch(next) + return } // this should never happen if (!req.csrfToken) { + logger.warn( + "Expected CSRF token middleware to be set but received none.", + ) next( new Error( "Expected CSRF token middleware to be set but received none.", @@ -199,11 +150,14 @@ export const createConsentRoute: RouteCreator = card: UserConsentCard({ consent: body, csrfToken: req.csrfToken(true), - cardImage: body.client?.logo_uri || "/ory-logo.svg", - client_name: body.client?.client_name || "unknown client", - requested_scope: body.requested_scope, + cardImage: body.client?.logo_uri || logoUrl, + client_name: + body.client?.client_name || + body.client?.client_id || + "Unknown Client", + requested_scope: body.requested_scope || [], client: body.client, - action: (process.env.BASE_URL || "") + "/consent", + action: "consent", }), }) }) @@ -214,6 +168,7 @@ export const createConsentRoute: RouteCreator = export const createConsentPostRoute: RouteCreator = (createHelpers) => async (req, res, next) => { + res.locals.projectName = "Consent" // The challenge is a hidden input field, so we have to retrieve it from the request body const { oauth2, isOAuthConsentRouteEnabled } = createHelpers(req, res) @@ -222,19 +177,17 @@ export const createConsentPostRoute: RouteCreator = return } - const { consent_challenge: challenge, consent_action, remember } = req.body + const { + consent_challenge: challenge, + consent_action, + remember, + grant_scope, + } = req.body - // Here is also the place to add data to the ID or access token. For example, - // if the scope 'profile' is added, add the family and given name to the ID Token claims: - // if (grantScope.indexOf('profile')) { - // session.id_token.family_name = 'Doe' - // session.id_token.given_name = 'John' - // } - let grantScope = req.body.grant_scope + let grantScope = grant_scope if (!Array.isArray(grantScope)) { grantScope = [grantScope] } - // extractSession only gets the sesseion data from the request // You can extract more data from the Ory Identities admin API const session = extractSession(req, grantScope) @@ -242,14 +195,15 @@ export const createConsentPostRoute: RouteCreator = // Let's fetch the consent request again to be able to set `grantAccessTokenAudience` properly. // Let's see if the user decided to accept or reject the consent request.. if (consent_action === "accept") { + logger.debug("Consent request was accepted by the user") await oauth2 .getOAuth2ConsentRequest({ - consentChallenge: challenge, + consentChallenge: String(challenge), }) .then(async ({ data: body }) => { return oauth2 .acceptOAuth2ConsentRequest({ - consentChallenge: challenge, + consentChallenge: String(challenge), acceptOAuth2ConsentRequest: { // We can grant all scopes that have been requested - hydra already checked for us that no additional scopes // are requested accidentally. @@ -278,17 +232,22 @@ export const createConsentPostRoute: RouteCreator = }, }) .then(({ data: body }) => { + const redirectTo = String(body.redirect_to) + logger.debug("Consent request successfuly accepted", redirectTo) // All we need to do now is to redirect the user back! - res.redirect(String(body.redirect_to)) + res.redirect(redirectTo) }) }) .catch(next) + return } + logger.debug("Consent request denied by the user") + // Looks like the consent request was denied by the user await oauth2 .rejectOAuth2ConsentRequest({ - consentChallenge: challenge, + consentChallenge: String(challenge), rejectOAuth2Request: { error: "access_denied", error_description: "The resource owner denied the request", @@ -302,28 +261,18 @@ export const createConsentPostRoute: RouteCreator = .catch(next) } -var parseForm = bodyParser.urlencoded({ extended: false }) - -export const registerConsentRoute: RouteRegistrator = function ( +export const registerConsentRoute: RouteRegistrator = ( app, createHelpers = defaultConfig, -) { - return app.get( +) => { + app.get( "/consent", - doubleCsrfProtection, + setSession(createHelpers), createConsentRoute(createHelpers), ) -} - -export const registerConsentPostRoute: RouteRegistrator = function ( - app, - createHelpers = defaultConfig, -) { - return app.post( + app.post( "/consent", - parseForm, - doubleCsrfProtection, - csrfErrorHandler, + setSession(createHelpers), createConsentPostRoute(createHelpers), ) } diff --git a/src/routes/csrfError.ts b/src/routes/csrfError.ts new file mode 100644 index 00000000..1fa01063 --- /dev/null +++ b/src/routes/csrfError.ts @@ -0,0 +1,22 @@ +// Copyright © 2023 Ory Corp +// SPDX-License-Identifier: Apache-2.0 +import { logger } from "../pkg" +import { Request, Response, NextFunction } from "express" + +// Error handling, validation error interception +export const csrfErrorHandler = + (invalidCsrfTokenError: unknown) => + (error: unknown, req: Request, res: Response, next: NextFunction) => { + if (error == invalidCsrfTokenError) { + logger.debug("The CSRF token is invalid or could not be found.", { + req: req, + }) + next( + new Error( + "A security violation was detected, please fill out the form again.", + ), + ) + } else { + next() + } + } diff --git a/src/routes/index.ts b/src/routes/index.ts index 01cb7045..2dd856f4 100644 --- a/src/routes/index.ts +++ b/src/routes/index.ts @@ -14,3 +14,4 @@ export * from "./settings" export * from "./static" export * from "./verification" export * from "./welcome" +export * from "./logout" diff --git a/src/routes/logout.ts b/src/routes/logout.ts new file mode 100644 index 00000000..97dc6cd0 --- /dev/null +++ b/src/routes/logout.ts @@ -0,0 +1,89 @@ +// Copyright © 2023 Ory Corp +// SPDX-License-Identifier: Apache-2.0 +import { + RouteCreator, + RouteRegistrator, + defaultConfig, + logger, + setSession, +} from "../pkg" +import { UserLogoutCard } from "@ory/elements-markup" +import { Request, Response, NextFunction } from "express" + +export const createShowLogoutRoute: RouteCreator = + (createHelpers) => + async (req: Request, res: Response, next: NextFunction) => { + res.locals.projectName = "Logout" + + const { logout_challenge: logoutChallenge } = req.query + + if (typeof logoutChallenge !== "string") { + logger.debug("Expected a logout challenge to be set but received none.") + next( + new Error("Expected a logout challenge to be set but received none."), + ) + return + } + + // this should never happen + if (!req.csrfToken) { + logger.warn("Expected CSRF token middleware to be set but received none.") + next( + new Error( + "Expected CSRF token middleware to be set but received none.", + ), + ) + return + } + + res.render("logout", { + card: UserLogoutCard({ + csrfToken: req.csrfToken(true), + challenge: logoutChallenge, + action: "logout", + }), + }) + } + +export const createSubmitLogoutRoute: RouteCreator = + (createHelpers) => + async (req: Request, res: Response, next: NextFunction) => { + const { oauth2 } = createHelpers(req, res) + res.locals.projectName = "Logout" + + // The challenge is now a hidden input field, so let's take it from + // the request body instead. + const { challenge: logoutChallenge, submit } = req.body + + if (submit === "No") { + logger.debug("User rejected to log out.") + // The user rejected to log out, so we'll redirect to /ui/welcome + return oauth2 + .rejectOAuth2LogoutRequest({ logoutChallenge }) + .then(() => res.redirect("welcome")) + .catch(next) + } else { + logger.debug("User agreed to log out.") + // The user agreed to log out, let's accept the logout request. + return oauth2 + .acceptOAuth2LogoutRequest({ logoutChallenge }) + .then(({ data: body }) => res.redirect(body.redirect_to)) + .catch(next) + } + } + +export const registerLogoutRoute: RouteRegistrator = ( + app, + createHelpers = defaultConfig, +) => { + app.get( + "/logout", + setSession(createHelpers), + createShowLogoutRoute(createHelpers), + ) + app.post( + "/logout", + setSession(createHelpers), + createSubmitLogoutRoute(createHelpers), + ) +}