diff --git a/CHANGELOG.md b/CHANGELOG.md index 08219cb879..e7c080a16e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,12 @@ const session = await getSessionFromStorage(sessionId, { The following changes have been implemented but not released yet: +### Bugfix + +#### browser + +- Fixed an issue where `handleIncomingRedirect({ restorePreviousSession: true })` would redirect to the OAuth provider with expired client credentials, causing users to be stuck on an error page. The library now validates client expiration before attempting silent authentication and gracefully falls back to a logged-out state when the client has expired. + ## [3.1.0](https://github.com/inrupt/solid-client-authn-js/releases/tag/v3.1.0) - 2025-07-08 ### New feature diff --git a/packages/browser/src/ClientAuthentication.spec.ts b/packages/browser/src/ClientAuthentication.spec.ts index 0b4c3191ff..d867ff583c 100644 --- a/packages/browser/src/ClientAuthentication.spec.ts +++ b/packages/browser/src/ClientAuthentication.spec.ts @@ -92,17 +92,20 @@ describe("ClientAuthentication", () => { logoutHandler: mockLogoutHandler(defaultMockStorage), sessionInfoManager: mockSessionInfoManager(defaultMockStorage), issuerConfigFetcher: mockDefaultIssuerConfigFetcher(), + storage: defaultMockStorage, }; function getClientAuthentication( mocks: Partial = defaultMocks, ): ClientAuthentication { + const storage = mocks.storage ?? defaultMocks.storage; return new ClientAuthentication( mocks.loginHandler ?? defaultMocks.loginHandler, mocks.redirectHandler ?? defaultMocks.redirectHandler, mocks.logoutHandler ?? defaultMocks.logoutHandler, mocks.sessionInfoManager ?? defaultMocks.sessionInfoManager, mocks.issuerConfigFetcher ?? defaultMocks.issuerConfigFetcher, + storage, ); } @@ -601,4 +604,129 @@ describe("ClientAuthentication", () => { ); }); }); + + describe("isClientExpired", () => { + it("returns true when a confidential client has an expired timestamp", async () => { + const sessionId = "mySession"; + const expiredTimestamp = Math.floor(Date.now() / 1000) - 1000; // 1000 seconds ago + const mockedStorage = new StorageUtility( + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + isLoggedIn: "true", + }, + }), + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + clientId: "some-client-id", + clientSecret: "some-secret", + expiresAt: String(expiredTimestamp), + }, + }), + ); + const clientAuthn = getClientAuthentication({ + sessionInfoManager: mockSessionInfoManager(mockedStorage), + storage: mockedStorage, + }); + + await expect(clientAuthn.isClientExpired(sessionId)).resolves.toBe(true); + }); + + it("returns false when a confidential client has a valid timestamp", async () => { + const sessionId = "mySession"; + const futureTimestamp = Math.floor(Date.now() / 1000) + 10000; // 10000 seconds in future + const mockedStorage = new StorageUtility( + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + isLoggedIn: "true", + }, + }), + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + clientId: "some-client-id", + clientSecret: "some-secret", + expiresAt: String(futureTimestamp), + }, + }), + ); + const clientAuthn = getClientAuthentication({ + sessionInfoManager: mockSessionInfoManager(mockedStorage), + storage: mockedStorage, + }); + + await expect(clientAuthn.isClientExpired(sessionId)).resolves.toBe(false); + }); + + it("returns false when a confidential client never expires (expiresAt = 0)", async () => { + const sessionId = "mySession"; + const mockedStorage = new StorageUtility( + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + isLoggedIn: "true", + }, + }), + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + clientId: "some-client-id", + clientSecret: "some-secret", + expiresAt: "0", + }, + }), + ); + const clientAuthn = getClientAuthentication({ + sessionInfoManager: mockSessionInfoManager(mockedStorage), + storage: mockedStorage, + }); + + await expect(clientAuthn.isClientExpired(sessionId)).resolves.toBe(false); + }); + + it("returns false for public clients (no secret) regardless of expiration", async () => { + const sessionId = "mySession"; + const expiredTimestamp = Math.floor(Date.now() / 1000) - 1000; + const mockedStorage = new StorageUtility( + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + isLoggedIn: "true", + }, + }), + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + clientId: "some-client-id", + // No clientSecret - public client + expiresAt: String(expiredTimestamp), + }, + }), + ); + const clientAuthn = getClientAuthentication({ + sessionInfoManager: mockSessionInfoManager(mockedStorage), + storage: mockedStorage, + }); + + await expect(clientAuthn.isClientExpired(sessionId)).resolves.toBe(false); + }); + + it("returns true for legacy clients with missing expiresAt (confidential)", async () => { + const sessionId = "mySession"; + const mockedStorage = new StorageUtility( + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + isLoggedIn: "true", + }, + }), + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + clientId: "some-client-id", + clientSecret: "some-secret", + // No expiresAt - legacy case + }, + }), + ); + const clientAuthn = getClientAuthentication({ + sessionInfoManager: mockSessionInfoManager(mockedStorage), + storage: mockedStorage, + }); + + await expect(clientAuthn.isClientExpired(sessionId)).resolves.toBe(true); + }); + }); }); diff --git a/packages/browser/src/ClientAuthentication.ts b/packages/browser/src/ClientAuthentication.ts index dcdb363457..5e4d8a4c8c 100644 --- a/packages/browser/src/ClientAuthentication.ts +++ b/packages/browser/src/ClientAuthentication.ts @@ -28,6 +28,12 @@ import type { ISessionInfo, ISessionInternalInfo, ILoginOptions, + IStorageUtility, + ILoginHandler, + IIncomingRedirectHandler, + ILogoutHandler, + ISessionInfoManager, + IIssuerConfigFetcher, } from "@inrupt/solid-client-authn-core"; import { EVENTS, @@ -42,6 +48,23 @@ import type { EventEmitter } from "events"; * @hidden */ export default class ClientAuthentication extends ClientAuthenticationBase { + constructor( + loginHandler: ILoginHandler, + redirectHandler: IIncomingRedirectHandler, + logoutHandler: ILogoutHandler, + sessionInfoManager: ISessionInfoManager, + issuerConfigFetcher: IIssuerConfigFetcher, + protected storageUtility: IStorageUtility, + ) { + super( + loginHandler, + redirectHandler, + logoutHandler, + sessionInfoManager, + issuerConfigFetcher, + ); + } + // Define these functions as properties so that they don't get accidentally re-bound. // Isn't Javascript fun? login = async ( @@ -95,6 +118,38 @@ export default class ClientAuthentication extends ClientAuthenticationBase { return sessionInfo; }; + /** + * Checks if the stored client registration has expired. + * Returns true if the client is a confidential client (has a secret) and has expired. + * Returns false for public clients or clients that haven't expired. + */ + isClientExpired = async (sessionId: string): Promise => { + const [clientSecret, expiresAt] = await Promise.all([ + this.storageUtility.getForUser(sessionId, "clientSecret", { + secure: false, + }), + this.storageUtility.getForUser(sessionId, "expiresAt", { secure: false }), + ]); + + // Expiration only applies to confidential clients (those with secrets) + if (clientSecret === undefined) { + return false; + } + + // -1 identifies legacy cases when a value should have been stored but wasn't + // Treat as expired + const expirationDate = + expiresAt !== undefined ? Number.parseInt(expiresAt, 10) : -1; + + // expirationDate === 0 means the client registration never expires + if (expirationDate === 0) { + return false; + } + + // Check if current time (in seconds) is past the expiration date + return Math.floor(Date.now() / 1000) > expirationDate; + }; + handleIncomingRedirect = async ( url: string, eventEmitter: EventEmitter, diff --git a/packages/browser/src/Session.spec.ts b/packages/browser/src/Session.spec.ts index 2a0a8ea037..5ddfdf3a5e 100644 --- a/packages/browser/src/Session.spec.ts +++ b/packages/browser/src/Session.spec.ts @@ -466,6 +466,13 @@ describe("Session", () => { .mockReturnValue( validateCurrentSessionPromise, ) as typeof clientAuthentication.validateCurrentSession; + // Mock isClientExpired to return false (not expired) + const isClientExpiredPromise = Promise.resolve(false); + clientAuthentication.isClientExpired = jest + .fn() + .mockReturnValue( + isClientExpiredPromise, + ) as typeof clientAuthentication.isClientExpired; const mySession = new Session({ clientAuthentication }); // eslint-disable-next-line no-void @@ -475,6 +482,7 @@ describe("Session", () => { }); await incomingRedirectPromise; await validateCurrentSessionPromise; + await isClientExpiredPromise; expect(window.localStorage.getItem(KEY_CURRENT_URL)).toBe( "https://mock.current/location", ); @@ -544,6 +552,13 @@ describe("Session", () => { .mockReturnValue( validateCurrentSessionPromise, ) as typeof clientAuthentication.validateCurrentSession; + // Mock isClientExpired to return false (not expired) + const isClientExpiredPromise = Promise.resolve(false); + clientAuthentication.isClientExpired = jest + .fn() + .mockReturnValue( + isClientExpiredPromise, + ) as typeof clientAuthentication.isClientExpired; const incomingRedirectPromise = Promise.resolve(); clientAuthentication.handleIncomingRedirect = jest .fn() @@ -560,6 +575,7 @@ describe("Session", () => { }); await incomingRedirectPromise; await validateCurrentSessionPromise; + await isClientExpiredPromise; expect(clientAuthentication.login).toHaveBeenCalledWith( { sessionId: "mySession", @@ -761,6 +777,113 @@ describe("Session", () => { // The local storage should have been cleared by the auth error expect(window.localStorage.getItem(KEY_CURRENT_SESSION)).toBeNull(); }); + + it("does not attempt silent authentication if the stored client has expired", async () => { + const sessionId = "mySession"; + mockLocalStorage({ + [KEY_CURRENT_SESSION]: sessionId, + }); + mockLocation("https://mock.current/location"); + const mockedStorage = new StorageUtility( + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + isLoggedIn: "true", + }, + }), + mockStorage({}), + ); + const clientAuthentication = mockClientAuthentication({ + sessionInfoManager: mockSessionInfoManager(mockedStorage), + }); + + // Mock validateCurrentSession to return valid session info + const validateCurrentSessionPromise = Promise.resolve({ + issuer: "https://some.issuer", + clientAppId: "some client ID", + clientAppSecret: "some client secret", + redirectUrl: "https://some.redirect/url", + tokenType: "DPoP", + }); + clientAuthentication.validateCurrentSession = jest + .fn() + .mockReturnValue( + validateCurrentSessionPromise, + ) as typeof clientAuthentication.validateCurrentSession; + + // Mock isClientExpired to return true (expired client) + clientAuthentication.isClientExpired = ( + jest.fn() as any + ).mockResolvedValue(true); + + const incomingRedirectPromise = Promise.resolve(); + clientAuthentication.handleIncomingRedirect = jest + .fn() + .mockReturnValueOnce( + incomingRedirectPromise, + ) as typeof clientAuthentication.handleIncomingRedirect; + clientAuthentication.login = jest.fn(); + + const mySession = new Session({ clientAuthentication }); + const result = await mySession.handleIncomingRedirect({ + url: "https://some.redirect/url", + restorePreviousSession: true, + }); + + await incomingRedirectPromise; + await validateCurrentSessionPromise; + + // Silent auth should NOT have been attempted + expect(clientAuthentication.login).not.toHaveBeenCalled(); + // The stored session should be cleared to prevent retry loops + expect(window.localStorage.getItem(KEY_CURRENT_SESSION)).toBeNull(); + // The function should resolve (not hang) + expect(result).toBeUndefined(); + }); + + it("clears stored session when client has expired during silent auth attempt", async () => { + const sessionId = "mySession"; + mockLocalStorage({ + [KEY_CURRENT_SESSION]: sessionId, + }); + mockLocation("https://mock.current/location"); + const mockedStorage = new StorageUtility( + mockStorage({ + [`${USER_SESSION_PREFIX}:${sessionId}`]: { + isLoggedIn: "true", + }, + }), + mockStorage({}), + ); + const clientAuthentication = mockClientAuthentication({ + sessionInfoManager: mockSessionInfoManager(mockedStorage), + }); + + clientAuthentication.validateCurrentSession = ( + jest.fn() as any + ).mockResolvedValue({ + issuer: "https://some.issuer", + clientAppId: "some client ID", + clientAppSecret: "some client secret", + redirectUrl: "https://some.redirect/url", + }); + + clientAuthentication.isClientExpired = ( + jest.fn() as any + ).mockResolvedValue(true); + + clientAuthentication.handleIncomingRedirect = ( + jest.fn() as any + ).mockResolvedValue(undefined); + + const mySession = new Session({ clientAuthentication }); + await mySession.handleIncomingRedirect({ + url: "https://some.redirect/url", + restorePreviousSession: true, + }); + + // The stored session ID should have been cleared + expect(window.localStorage.getItem(KEY_CURRENT_SESSION)).toBeNull(); + }); }); describe("events.on", () => { diff --git a/packages/browser/src/Session.ts b/packages/browser/src/Session.ts index 5501887479..25b448011f 100644 --- a/packages/browser/src/Session.ts +++ b/packages/browser/src/Session.ts @@ -86,6 +86,14 @@ export async function silentlyAuthenticate( ): Promise { const storedSessionInfo = await clientAuthn.validateCurrentSession(sessionId); if (storedSessionInfo !== null) { + // Check if the client registration has expired before attempting silent auth + const clientExpired = await clientAuthn.isClientExpired(sessionId); + if (clientExpired) { + // Clear the stored session to prevent retry loops + window.localStorage.removeItem(KEY_CURRENT_SESSION); + return false; + } + // It can be really useful to save the user's current browser location, // so that we can restore it after completing the silent authentication // on incoming redirect. This way, the user is eventually redirected back diff --git a/packages/browser/src/__mocks__/ClientAuthentication.ts b/packages/browser/src/__mocks__/ClientAuthentication.ts index c30362779a..9246dc9237 100644 --- a/packages/browser/src/__mocks__/ClientAuthentication.ts +++ b/packages/browser/src/__mocks__/ClientAuthentication.ts @@ -19,6 +19,7 @@ // SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. // +import { jest } from "@jest/globals"; import type { IIssuerConfigFetcher, ILoginHandler, @@ -51,11 +52,16 @@ export const mockClientAuthentication = ( mocks?: Partial, ): ClientAuthentication => { const storage = mocks?.storage ?? mockStorageUtility({}); - return new ClientAuthentication( + const clientAuthn = new ClientAuthentication( mocks?.loginhandler ?? mockLoginHandler(), mocks?.redirectHandler ?? mockIncomingRedirectHandler(), mocks?.logoutHandler ?? mockLogoutHandler(storage), mocks?.sessionInfoManager ?? mockSessionInfoManager(storage), mocks?.issuerConfigFetcher ?? IssuerConfigFetcherMock, + storage, ); + // Add default mock for isClientExpired + // eslint-disable-next-line @typescript-eslint/no-explicit-any + clientAuthn.isClientExpired = (jest.fn() as any).mockResolvedValue(false); + return clientAuthn; }; diff --git a/packages/browser/src/dependencies.ts b/packages/browser/src/dependencies.ts index d139365c28..6056c8b869 100644 --- a/packages/browser/src/dependencies.ts +++ b/packages/browser/src/dependencies.ts @@ -105,5 +105,6 @@ export function getClientAuthenticationWithDependencies(dependencies: { new IWaterfallLogoutHandler(sessionInfoManager, redirector), sessionInfoManager, issuerConfigFetcher, + storageUtility, ); }