[jibin jose (jibin7jose)]

Fixes: #351

Description:
When multiple clients (Desktop, Web, Mobile) receive the OAuth card simultaneously, they each independently attempt a silent token exchange by sending a signin/tokenExchange invoke activity.

Previously, app.oauth.ts blindly emitted the signin event for each successful exchange without verifying if the token exchange id had already been processed. This resulted in the signin event firing multiple times for a single login flow.

This PR implements standard deduplication by caching processed activity.value.id strings in a short-lived, rolling array (max length 1000). If subsequent invoke activities carry an already processed id, the framework now gracefully ignores them and responds with an HTTP 200 OK status, preventing duplicate event emissions while remaining memory safe.

Co-authored-by: Jibin Jose jibinjose884@gmail.com

[Copilot]

Pull request overview

This PR aims to prevent duplicate signin event emissions when multiple clients concurrently send signin/tokenExchange invokes for the same OAuth card during a single login flow.

Changes:

• Adds an in-memory cache of processed token exchange activity.value.id values.
• Short-circuits subsequent signin/tokenExchange invokes with already-seen ids by returning HTTP 200 without emitting signin.
• Maintains the cache as a bounded rolling list (max length 1000) to limit memory growth.

Suppressed comments (2)

packages/apps/src/app.oauth.ts:49

• The token-exchange dedup only checks processedExchangeIds before the async exchange and only records the id after a successful exchange. Two signin/tokenExchange invokes with the same activity.value.id arriving concurrently can both pass the includes check and both emit signin, so this doesn’t reliably prevent duplicate sign-in events under the “simultaneous clients” scenario this PR targets. Consider serializing token exchange per id so later requests wait for the first to finish and then re-check processedExchangeIds.

    const exchangeId = activity.value.id;
    if (exchangeId && this.processedExchangeIds.includes(exchangeId)) {
      return { status: 200 };
    }

packages/apps/src/app.oauth.ts:76

• If you serialize token exchange per id, make sure the per-id lock is always released and removed (including when exchangeToken() throws). Otherwise later requests can hang awaiting a never-resolved lock, and the in-memory map can grow over time.

        }
      }

      this.events.emit('signin', { ...ctx, token, isSignedIn: true });
      next(ctx);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/apps/src/app.oauth.ts:56

• When a duplicate tokenExchange invoke arrives while another exchange with the same id is in-flight, the code awaits existingLock but swallows any rejection and still returns 200. If the in-flight exchange fails (and would return 412), other clients will incorrectly get 200 and may skip fallback auth, leaving them unsigned-in. Consider only returning 200 when the in-flight exchange succeeded, and retrying (or surfacing 412) when it failed.

      const existingLock = this.tokenExchangeLocks.get(exchangeId);
      if (existingLock) {
        await existingLock.catch(() => {});
        return { status: 200 };
      }

[Corina (corinagum)]

Thanks for the PR, jibin jose (@jibin7jose). Could you please see comments and add some unit tests? TIA!

[jibin jose (jibin7jose)]

Ok

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Suppressed comments (5)

packages/apps/src/app.ts:560

• The async wrapper does not return the invoke response from onVerifyState, so the router pipeline will treat this handler as returning undefined and may drop the intended InvokeResponse/status. Return the promise from onVerifyState instead of awaiting without returning.

      type: 'system',
      select: activity => activity.type === 'invoke' && activity.name === 'signin/verifyState',
      callback: async (ctx) => { await this.oauthHandlers.onVerifyState(ctx); },
    });

packages/apps/src/app.ts:567

• The async wrapper does not return the invoke response from onSignInFailure, so the router pipeline will treat this handler as returning undefined and may drop the intended InvokeResponse/status. Return the promise from onSignInFailure instead of awaiting without returning.

      name: 'signin.failure',
      type: 'system',
      select: activity => activity.type === 'invoke' && activity.name === 'signin/failure',
      callback: async (ctx) => { await this.oauthHandlers.onSignInFailure(ctx); },
    });

packages/apps/src/app.oauth.spec.ts:120

• The deduplication test currently verifies exchangeToken is called once, but it doesn’t verify the core regression (only one signin event / one next(ctx) call). Add assertions so the test fails if the handler still emits/executes twice.

      const next = jest.fn();
      
      const ctx: any = {
        api: mockApi,
        activity: mockActivity,

packages/apps/src/app.oauth.ts:117

• exchangeId is added to processedExchangeIds before emitting the signin event. If a signin listener throws, the exchange will be treated as failed (412) but the id will still be cached, causing subsequent retries with the same id to be ignored and return 200 without ever re-emitting signin. Cache the id only after the successful signin emission / next(ctx) call completes without throwing.

            if (exchangeId) {
              this.processedExchangeIds.push(exchangeId);
              if (this.processedExchangeIds.length > 1000) {
                this.processedExchangeIds.shift();
              }

packages/apps/src/app.oauth.spec.ts:64

• This test claims it "emits signin" but never asserts that the signin event was emitted. Adding an explicit assertion will ensure the test actually guards the intended behavior.

This issue also appears on line 116 of the same file.

      const next = jest.fn();
      
      const ctx: any = {
        api: mockApi,
        activity: mockActivity,

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Suppressed comments (5)

packages/apps/src/app.ts:559

• Avoid as unknown as Promise<void> here: it hides type mismatches and removes compile-time checking of the invoke response shape. Prefer returning the handler result with its actual type.

      callback: ctx => this.oauthHandlers.onVerifyState(ctx) as unknown as Promise<void>,

packages/apps/src/app.ts:566

• Avoid as unknown as Promise<void> here: it bypasses the type contract for invoke route handlers and can mask real typing issues. Prefer returning the handler result with the correct return type.

      callback: ctx => this.oauthHandlers.onSignInFailure(ctx) as unknown as Promise<void>,

packages/apps/src/app.ts:552

• Avoid as unknown as Promise<void> here: it hides the real route-handler type mismatch (if any) and can allow returning the wrong shape for invoke routes without compiler help. Prefer wiring the handler with its real return type (InvokeResponse/body) and fix the underlying typing issue instead of forcing Promise<void>.

This issue also appears in the following locations of the same file:

• line 559
• line 566

      callback: ctx => this.oauthHandlers.onTokenExchange(ctx) as unknown as Promise<void>,

packages/apps/src/app.ts:569

• Typing the error event payload as any removes the benefit of the existing IErrorEvent typing (error: Error, optional activity). Since this callback is contextually typed by AppEvents, the annotation is unnecessary; keep the typed destructuring so TypeScript can catch mistakes.

    this.event('error', ({ error }: any) => {

packages/apps/src/app.oauth.spec.ts:139

• The deduplication test only asserts exchangeToken is called once, but it doesn’t assert the key externally-visible behavior: signin and next() should run exactly once for a given exchangeId. Without those assertions, a regression could still fire duplicate signin events while keeping token exchange deduped.

      const next = jest.fn();
      
      const ctx: any = {
        api: mockApi,
        activity: mockActivity,

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (5)

packages/apps/src/app.ts:560

• Avoid forcing the route callback’s return type to Promise<void> via as unknown as. These OAuth invoke handlers return an InvokeResponse and the cast hides type mismatches. onVerifyState is an arrow field and can be passed directly.

    this.router.register({
      name: 'signin.verify-state',
      type: 'system',
      select: activity => activity.type === 'invoke' && activity.name === 'signin/verifyState',
      callback: ctx => this.oauthHandlers.onVerifyState(ctx) as unknown as Promise<void>,
    });

packages/apps/src/app.ts:567

• Avoid forcing the route callback’s return type to Promise<void> via as unknown as. onSignInFailure returns an InvokeResponse and can be passed directly as the route callback.

    this.router.register({
      name: 'signin.failure',
      type: 'system',
      select: activity => activity.type === 'invoke' && activity.name === 'signin/failure',
      callback: ctx => this.oauthHandlers.onSignInFailure(ctx) as unknown as Promise<void>,
    });

packages/apps/src/app.ts:575

• Typing the error event payload as any removes useful safety here (e.g., error may not be an Error). Prefer unknown + a small type guard so logging doesn’t rely on error.message existing.

    this.event('error', ({ error }: any) => {
      this.log.error(error.message);

      if (error instanceof AxiosError) {
        this.log.error(error.request.path);
        this.log.error(error.response?.data);
      }

packages/apps/src/app.ts:553

• Avoid forcing the route callback’s return type to Promise<void> via as unknown as. These OAuth invoke handlers return an InvokeResponse (e.g., { status: 412, body }), and the cast hides type mismatches and can mask incorrect responses during refactors. Since onTokenExchange is an arrow field, it can be passed directly without wrapping/casting.

This issue also appears in the following locations of the same file:

• line 555
• line 562

    this.router.register({
      name: 'signin.token-exchange',
      type: 'system',
      select: activity => activity.type === 'invoke' && activity.name === 'signin/tokenExchange',
      callback: ctx => this.oauthHandlers.onTokenExchange(ctx) as unknown as Promise<void>,
    });

packages/apps/src/app.oauth.spec.ts:150

• The deduplication test currently asserts only that exchangeToken was called once; it doesn’t directly assert the bug being fixed (duplicate signin emission / callback invocation). Add assertions that events.emit('signin', ...) and next(...) are called exactly once across the concurrent duplicate invokes.

      expect(results).toEqual([{ status: 200 }, { status: 200 }]);
      expect(mockApi.users.exchangeToken).toHaveBeenCalledTimes(1);
    });

[Corina (corinagum)]

jibin jose (@jibin7jose) thanks for the updates! Could you do a pass and resolve the addressed/stale comments? Looks like there's a few cleanup opportunities. :) Thanks in advance!

[jibin jose (jibin7jose)]

I've resolved all the addressed comments.

[Corina (corinagum)]

Thank you for the PR!