Skip to content

handler: complete the OAuth2 handler port - #52

Merged
guimard merged 2 commits into
masterfrom
fix/oauth2-handler-port
Sep 8, 2026
Merged

guimard merged 2 commits into
masterfrom
fix/oauth2-handler-port

Conversation

@guimard

@guimard guimard commented Sep 8, 2026

Copy link
Copy Markdown
Member

Context

Reviewing the report behind GHSA-jj3p-7p8x-j82f (rejected: the jti of a LLNG
access token is the credential, and the Perl reference does not verify the JWT
signature on that path either) showed that the OAuth2 handler port is
incomplete. It used the jti claim, or the raw Bearer token, directly as a
user session id
, with a fallback to the main session storage.

Compared to Lemonldap::NG::Handler::Lib::OAuth2, the port diverged on every
point but the token parsing:

  • a genuine LLNG access token resolved to the access token session itself,
    which holds no user attribute — the forged identity headers were empty, so
    the handler simply did not work with real tokens;
  • conversely, any user session id was accepted as an access token;
  • token attributes were never exposed, making scope based rules unusable;
  • unauthenticated API calls were redirected to the portal instead of receiving
    the RFC 6750 challenge;
  • the access token was forwarded to the protected application;
  • offline access tokens were not handled.

Changes

fetchId() now resolves the Bearer token into its access token session (in
the OIDC storage), then follows user_session_id or offline_session_id, as
the Perl handler does. The token payload is still only used to find that session
id: the trust anchor is the session storage, not the token.

before after
token → session jti used as user session id jti → access token session → user_session_id / offline_session_id
unknown token falls back to the main session store falls back to the cookie, else 401
token attributes none _scope, _clientId, _clientConfKey, _oidc_grant_type, _audiences, _accessToken
unauthenticated 302 to the portal 401 + WWW-Authenticate: Bearer error="invalid_token"
Authorization header forwarded to the app removed
offline tokens unhandled handled

Supporting changes:

  • init.ts loads oidcStorage, oidcStorageOptions and
    oidcRPMetaDataOptions, which were declared in the TSV but never read — so
    the OAuth2 handler always used the main session storage, whatever the
    configuration said. The OIDC accessor is shared with the main one when both
    storages are identical.
  • fetchId() may now return a promise, since resolving a token requires a store
    lookup. run() awaits it; the other handler types are unchanged.
  • retrieveSession() receives the request as an optional second argument, to
    reach the per-request token attributes (the equivalent of Perl's $req->data).
    Attributes are merged into a copy of the session, so the session caches are
    never polluted.
  • @lemonldap-ng/jwt: documented why getAccessTokenSessionId() verifies no
    signature, and what callers must do with its result.

Tests

packages/handler/src/handlerOAuth2.test.ts — 21 tests, the first ones for this
handler type. They run the real handler over an Express app with a File session
backend, in two configurations (dedicated oidcStorage, and storage shared with
user sessions), and cover:

  • opaque and JWT access tokens, offline tokens, cookie fallback;
  • forged tokens (alg:none), including the GHSA-jj3p-7p8x-j82f scenario:
    a user session id presented as a Bearer token is now rejected in both
    configurations;
  • expired and dangling access token sessions;
  • the RFC 6750 challenge, and the hiding of Authorization and of the LLNG
    cookie;
  • scope based rules, and the absence of token attribute leaks between requests
    sharing the handler's internal session cache.

18 of the 21 fail against the previous implementation.

npm run test is green (139 tests), as are eslint, prettier --check and
npm run build --workspace=packages/handler. Test files are now excluded from
that package's tsc --noEmit, as they are in every other package.

@guimard
guimard force-pushed the fix/oauth2-handler-port branch 3 times, most recently from a832d05 to d0f8f2a Compare September 8, 2026 12:13
The OAuth2 handler used the `jti` claim of a Bearer token (or the raw token)
directly as a user session id, and fell back to the main session storage. As a
result it diverged from Lemonldap::NG::Handler::Lib::OAuth2 on every point but
the token parsing:

- a genuine LLNG access token resolved to the access token session itself,
  which holds no user attribute, so the forged headers were empty
- conversely, any user session id was accepted as an access token
- token attributes were never exposed, making scope based rules unusable
- unauthenticated API calls were redirected to the portal instead of getting
  the RFC 6750 challenge
- the access token was forwarded to the protected application
- offline access tokens were not handled

fetchId() now resolves the token into its access token session (in the OIDC
storage), then follows `user_session_id` or `offline_session_id`, as the Perl
handler does. The token payload is still only used to find that session id: the
trust anchor is the session storage.

To make this possible:

- init() loads `oidcStorage`, `oidcStorageOptions` and `oidcRPMetaDataOptions`,
  which were declared in the TSV but never read
- fetchId() may return a promise, since resolving a token needs a store lookup
- retrieveSession() receives the request, to reach the per-request token
  attributes

Also adds the first tests of the OAuth2 handler type, including the forged
token scenario reported in GHSA-jj3p-7p8x-j82f.

Co-Authored-By: Claude
@guimard
guimard force-pushed the fix/oauth2-handler-port branch from d0f8f2a to 6615247 Compare September 8, 2026 15:54
@guimard
guimard merged commit 3fe7fa0 into master Sep 8, 2026
3 of 5 checks passed
@guimard
guimard deleted the fix/oauth2-handler-port branch September 8, 2026 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant