From 4e907efe76c658683fb602f98fd2b5fa2bfd4051 Mon Sep 17 00:00:00 2001 From: Douglas DUTEIL Date: Tue, 9 Jun 2026 14:01:28 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20oidc:=20fix=20scope=20accumulati?= =?UTF-8?q?on=20in=20loadExistingGrant?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Problem** `addOIDCScope` and `addOIDCClaims` are additive — scopes from prior authorization requests (e.g. `organization`, `siret`) were retained on the existing grant even when a subsequent request no longer included them. This caused affiliation data (employer label, SIRET) to leak to clients that no longer requested the `organization` scope. **Proposal** Always create a fresh grant so scopes/claims reflect exactly what the current request asks for. The existing grant is still looked up, but only to preserve its expiry and keep it aligned with the session TTL. --- .github/workflows/end-to-end.yml | 1 + .../env.conf | 0 .../fixtures.sql | 89 +++++++++ .../index.cy.ts | 176 ++++++++++++++++++ src/config/oidc-provider.ts | 49 ++--- src/types/oidc-provider.d.ts | 14 ++ 6 files changed, 307 insertions(+), 22 deletions(-) create mode 100644 cypress/e2e/signin_scope_and_claims_isolation/env.conf create mode 100644 cypress/e2e/signin_scope_and_claims_isolation/fixtures.sql create mode 100644 cypress/e2e/signin_scope_and_claims_isolation/index.cy.ts diff --git a/.github/workflows/end-to-end.yml b/.github/workflows/end-to-end.yml index eacebb5be..93a65bfef 100644 --- a/.github/workflows/end-to-end.yml +++ b/.github/workflows/end-to-end.yml @@ -46,6 +46,7 @@ jobs: - set_info_after_account_provisioning - signin_from_proconnect_federation_client - signin_from_standard_client + - signin_scope_and_claims_isolation - signin_with_certification_dirigeant - signin_with_email_verification - signin_with_email_verification_renewal diff --git a/cypress/e2e/signin_scope_and_claims_isolation/env.conf b/cypress/e2e/signin_scope_and_claims_isolation/env.conf new file mode 100644 index 000000000..e69de29bb diff --git a/cypress/e2e/signin_scope_and_claims_isolation/fixtures.sql b/cypress/e2e/signin_scope_and_claims_isolation/fixtures.sql new file mode 100644 index 000000000..75d0b3db6 --- /dev/null +++ b/cypress/e2e/signin_scope_and_claims_isolation/fixtures.sql @@ -0,0 +1,89 @@ +INSERT INTO + users ( + id, + email, + email_verified, + email_verified_at, + encrypted_password, + created_at, + updated_at, + given_name, + family_name, + phone_number, + job + ) +VALUES + ( + 1, + 'unused1@yopmail.com', + true, + CURRENT_TIMESTAMP, + '$2a$10$kzY3LINL6..50Fy9shWCcuNlRfYq0ft5lS.KCcJ5PzrhlWfKK4NIO', + CURRENT_TIMESTAMP, + CURRENT_TIMESTAMP, + 'Jean', + 'Bon', + '0123456789', + 'Sbire' + ); + +INSERT INTO + organizations (id, cached_libelle, siret, created_at, updated_at) +VALUES + ( + 1, + 'Commune de lamalou-les-bains', + '21340126800130', + CURRENT_TIMESTAMP, + CURRENT_TIMESTAMP + ), + ( + 2, + 'Commune de clamart', + '21920023500014', + CURRENT_TIMESTAMP, + CURRENT_TIMESTAMP + ); + +INSERT INTO + users_organizations ( + user_id, + organization_id, + is_external, + verification_type, + has_been_greeted + ) +VALUES + (1, 1, false, 'domain', true), + (1, 2, false, 'domain', true); + +INSERT INTO + oidc_clients ( + client_name, + client_id, + client_secret, + redirect_uris, + post_logout_redirect_uris, + scope, + client_uri, + client_description, + userinfo_signed_response_alg, + id_token_signed_response_alg, + authorization_signed_response_alg, + introspection_signed_response_alg + ) +VALUES + ( + 'Oidc Test Client', + 'standard_client_id', + 'standard_client_secret', + ARRAY['http://localhost:4000/login-callback'], + ARRAY[]::varchar[], + 'openid email profile phone organization', + 'http://localhost:4000/', + 'ProConnect test client. More info: https://github.com/proconnect-gouv/proconnect-test-client.', + null, + null, + null, + null + ); diff --git a/cypress/e2e/signin_scope_and_claims_isolation/index.cy.ts b/cypress/e2e/signin_scope_and_claims_isolation/index.cy.ts new file mode 100644 index 000000000..f2403d55c --- /dev/null +++ b/cypress/e2e/signin_scope_and_claims_isolation/index.cy.ts @@ -0,0 +1,176 @@ +// + +describe("scope isolation across consecutive sign-ins", () => { + before(cy.seed); + + it("should not leak organization scope claims to a client that no longer requests it", function () { + cy.visit("http://localhost:4000"); + cy.updateCustomParams((customParams) => ({ + ...customParams, + scope: "openid email profile organization", + })); + cy.get("button#custom-connection").click({ force: true }); + cy.login("unused1@yopmail.com"); + + cy.getByLabel( + "Commune de lamalou-les-bains - Mairie (choisir cette organisation)", + ).click(); + + cy.contains('"label": "Commune de lamalou-les-bains - Mairie"'); + cy.contains('"scope": "openid email profile organization"'); + + // Second sign-in: same session, scope no longer includes organization + cy.visit("http://localhost:4000"); + cy.updateCustomParams((customParams) => ({ + ...customParams, + scope: "openid email profile", + })); + cy.get("button#custom-connection").click({ force: true }); + + cy.contains('"scope": "openid email profile"'); + cy.contains('"label"').should("not.exist"); + cy.contains('"siret"').should("not.exist"); + }); +}); + +describe("claims parameter isolation across consecutive sign-ins", () => { + before(cy.seed); + + it("should not leak claims-parameter claims to a subsequent request that omits them", () => { + // First sign-in: email granted only via claims parameter (no email scope) + cy.visit("http://localhost:4000"); + cy.updateCustomParams((customParams) => ({ + ...customParams, + scope: "openid", + claims: { userinfo: { email: null } }, + })); + cy.get("button#custom-connection").click({ force: true }); + cy.login("unused1@yopmail.com"); + cy.contains('"email": "unused1@yopmail.com"'); + + // Second sign-in: same session, claims parameter removed + cy.visit("http://localhost:4000"); + cy.updateCustomParams((customParams) => ({ + ...customParams, + scope: "openid", + claims: undefined, + })); + cy.get("button#custom-connection").click({ force: true }); + cy.contains('"email"').should("not.exist"); + }); +}); + +describe("claims parameter behaviour", () => { + before(cy.seed); + + it("should return email when requested via claims parameter with null value", function () { + // null is valid OIDC — means "include with no constraint" + cy.visit("http://localhost:4000"); + cy.updateCustomParams((customParams) => ({ + ...customParams, + scope: "openid", + claims: { + userinfo: { + email: null, + }, + }, + })); + cy.get("button#custom-connection").click({ force: true }); + cy.login("unused1@yopmail.com"); + + cy.contains('"email": "unused1@yopmail.com"'); + }); + + it("should NOT return organization claims when requested via claims parameter alone (no scope)", function () { + cy.visit("http://localhost:4000"); + cy.updateCustomParams((customParams) => ({ + ...customParams, + scope: "openid", + claims: { + userinfo: { + organization: { essential: true }, + phone: { essential: true }, + profile: { essential: false }, + }, + }, + })); + cy.get("button#custom-connection").click({ force: true }); + cy.login("unused1@yopmail.com"); + + cy.contains('"family_name"').should("not.exist"); + cy.contains('"label"').should("not.exist"); + cy.contains('"phone_number"').should("not.exist"); + cy.contains('"siret"').should("not.exist"); + }); + + it("should return label and siret when organization scope is requested", function () { + cy.visit("http://localhost:4000"); + cy.updateCustomParams((customParams) => ({ + ...customParams, + scope: "openid organization", + claims: undefined, + })); + cy.get("button#custom-connection").click({ force: true }); + cy.login("unused1@yopmail.com"); + + cy.getByLabel( + "Commune de lamalou-les-bains - Mairie (choisir cette organisation)", + ).click(); + + cy.contains('"label": "Commune de lamalou-les-bains - Mairie"'); + cy.contains('"siret": "21340126800130"'); + }); + + it("should NOT return uid claim when requested via claims parameter if client lacks uid scope", function () { + // uid is a valid provider leaf claim but standard_client_id is not allowed the uid scope + cy.visit("http://localhost:4000"); + cy.updateCustomParams((customParams) => ({ + ...customParams, + scope: "openid", + claims: { + userinfo: { + uid: null, + }, + }, + })); + cy.get("button#custom-connection").click({ force: true }); + cy.login("unused1@yopmail.com"); + + cy.contains('"uid"').should("not.exist"); + }); + + it("should return individual leaf claims when requested via claims parameter", function () { + cy.visit("http://localhost:4000"); + cy.updateCustomParams((customParams) => ({ + ...customParams, + scope: "openid", + claims: { + userinfo: { + given_name: null, + family_name: { essential: true }, + }, + }, + })); + cy.get("button#custom-connection").click({ force: true }); + cy.login("unused1@yopmail.com"); + + cy.contains('"given_name": "Jean"'); + cy.contains('"family_name": "Bon"'); + cy.contains('"label"').should("not.exist"); + }); +}); + +describe("invalid scope error", () => { + before(cy.seed); + + it("should show invalid_scope when requesting a scope not allowed for the client", () => { + cy.visit("http://localhost:4000"); + cy.updateCustomParams((customParams) => ({ + ...customParams, + scope: "openid uid", + })); + cy.get("button#custom-connection").click({ force: true }); + + cy.contains("invalid_scope"); + }); +}); diff --git a/src/config/oidc-provider.ts b/src/config/oidc-provider.ts index 2c3361dfe..1f2fb9041 100644 --- a/src/config/oidc-provider.ts +++ b/src/config/oidc-provider.ts @@ -175,32 +175,37 @@ export const oidcProviderConfiguration = ({ const oidcContextParams = ctx.oidc.params as OIDCContextParams; const grantId = ctx.oidc.session.grantIdFor(ctx.oidc.client.clientId); - let grant; + // Check whether an existing (non-expired) grant exists only to preserve + // its expiry — we always create a fresh grant so that scopes/claims + // reflect exactly what the current request asks for and never accumulate + // from prior requests (which would leak affiliation data like siret/label + // to clients that no longer request the organization scope). + const existingGrant = grantId + ? await ctx.oidc.provider.Grant.find(grantId) + : undefined; - if (grantId) { - grant = await ctx.oidc.provider.Grant.find(grantId); - // if the grant has expired, the grant can be undefined at this point. - if (grant) { - // Keep grant expiry aligned with session expiry to prevent consent - // prompt being requested when the grant is about to expire. - // The original code is overkill as session length is extended on every - // interaction. - grant.exp = epochTime() + sessionTtlInSeconds; - await grant.save(); - } - } + const grant = new ctx.oidc.provider.Grant({ + clientId: ctx.oidc.client.clientId, + accountId: ctx.oidc.session.accountId, + }); - if (!grant) { - grant = new ctx.oidc.provider.Grant({ - clientId: ctx.oidc.client.clientId, - accountId: ctx.oidc.session.accountId, - }); - } + // Keep grant expiry aligned with session expiry to prevent consent + // prompt being requested when the grant is about to expire. + grant.exp = existingGrant?.exp ?? epochTime() + sessionTtlInSeconds; - // event existing grant should be updated, as requested scopes might - // be different grant.addOIDCScope(oidcContextParams.scope); - grant.addOIDCClaims(Array.from(ctx.oidc.requestParamClaims || [])); + + // Only grant leaf claims that belong to scopes the client is authorised for, + // preventing clients from receiving restricted claims (e.g. uid) via the + // claims parameter when they lack the corresponding scope. + const claimsHelper = new ctx.oidc.provider.Claims({}, { ctx }); + claimsHelper.scope(ctx.oidc.client.scope as string); + grant.addOIDCClaims( + Array.from(ctx.oidc.requestParamClaims || []).filter( + (c) => c in claimsHelper.filter, + ), + ); + await grant.save(); return grant; }, diff --git a/src/types/oidc-provider.d.ts b/src/types/oidc-provider.d.ts index 33a42d22b..6c3d38484 100644 --- a/src/types/oidc-provider.d.ts +++ b/src/types/oidc-provider.d.ts @@ -9,6 +9,20 @@ import type { UnknownObject, } from "oidc-provider"; +declare module "oidc-provider" { + interface Provider { + // Public getter exposed at runtime but absent from the shipped type definitions. + // Returns the Claims class bound to this provider instance. + Claims: new ( + claims: Record, + options: { ctx: unknown }, + ) => { + filter: Record; + scope(scopeString: string): void; + }; + } +} + declare global { /** * Extends the {@link OIDCContext.params} type.