🔒 oidc: fix scope accumulation in loadExistingGrant

.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

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
+  );

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");
+  });
+});

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;
   },

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<string, unknown>,
+      options: { ctx: unknown },
+    ) => {
+      filter: Record<string, unknown>;
+      scope(scopeString: string): void;
+    };
+  }
+}
+
 declare global {
   /**
    * Extends the {@link OIDCContext.params} type.