Application crashes when refresh token returns an error

[orgads]

If tokens/refresh returns an error, the application crashes, and there is no way to catch and handle it properly.

Reproduced by:

import { DirectLine, ConnectionStatus } from 'botframework-directlinejs';
import WebSocket, { WebSocketServer } from 'ws';
import nock from 'nock';
import xhr2 from 'xhr2';

global.XMLHttpRequest = xhr2;
global.WebSocket = WebSocket;

const wss = new WebSocketServer({ port: 2222 });
wss.on('connection', function connection(ws) {
  ws.on('error', console.error);
  ws.send('{"activities": []}');
});

nock('https://directline.botframework.com')
.persist()
.post(uri => uri.startsWith('/v3/directline/conversations'))
.reply(
  200,
  JSON.stringify({
    conversationId: '123',
    token: '456',
    streamUrl: 'ws://localhost:2222'
  })
)
.post(uri => uri.includes('/refresh'))
.reply(403);

const directLine = new DirectLine({ token: '456' });
directLine.activity$
.subscribe(
  (activity) => console.log('Activity received: ', activity),
  (err) => console.error('**1 Error: ', err)
);
directLine.connectionStatus$
.subscribe(
  (connectionStatus) => console.log('DirectLine status: ' + ConnectionStatus[connectionStatus]),
  (err) => console.error('**2 Error: ', err)
);

process.on('uncaughtException', (err) => {
  console.error('**3 Error: ', err);
  process.exit(1);
});

If you don't want to wait 15 minutes, reduce the hardcoded value of lifetimeRefreshToken :)

Output:

DirectLine status: Connecting
DirectLine status: Online
DirectLine status: ExpiredToken
DirectLine status: ExpiredToken
**3 Error:  [AjaxError: ajax error 403] {
  xhr: <ref *1> XMLHttpRequest {
    onloadstart: null,
    onprogress: null,
    onabort: null,
    onerror: null,
    onload: null,
    ontimeout: [Function: xhrTimeout] {
      request: [Object],
      subscriber: [AjaxSubscriber],
      progressSubscriber: undefined
    },
    onloadend: null,
    _listeners: {},
    onreadystatechange: [Function: xhrReadyStateChange] {
      subscriber: [AjaxSubscriber],
      progressSubscriber: undefined,
      request: [Object]
    },
    _anonymous: undefined,
    readyState: 4,
    response: null,
    responseText: null,
    responseType: 'json',
    responseURL: 'https://directline.botframework.com/v3/directline/tokens/refresh',
    status: 403,
    statusText: 'Forbidden',
...

[stevkan]

William Wong (@compulim), Eugene (@OEvgeny)

[OEvgeny]

Thanks for investigating and for the repro. As a user-land solution you could also consider extending the DirectLine class with own refresh token implementation.

I've analyzed the issue and have a proposal for addressing it (cc William Wong (@compulim)):

Current Issue

The application crashes when the token refresh fails (e.g., with a 403 error). This occurs in the refreshTokenLoop method, which is called from checkConnection. As the refresh token loop doesn't interfere with connectionStatus$ subject, the error doesn't get propagated:

BotFramework-DirectLineJS/src/directLine.ts

Lines 564 to 565 in bb3d7ee

	if (!this.secret)
	this.refreshTokenLoop();

I see why this might be done on-purpose. The cause looks like the one we cannot recover direct line from, so trying to crash the runtime to give both: visibility and the ability to restart the whole process seems valid.

Proposal

We can probably enhance error handling though without changing the public API e.g. to allow replacing the DirctLine with a new one:

1. Modify refreshTokenLoop to catch errors and continue the loop:

   private refreshTokenLoop() {
     this.tokenRefreshSubscription = Observable.interval(intervalRefreshToken)
       .flatMap(_ => this.refreshToken()
         .pipe(
           catchError(error => {
             this.handleFatalError(error);
             return Observable.empty();
           })
         )
       )
       .subscribe(/* ... */);
   }

2. Update refreshToken to simplify error handling:

   private refreshToken(): Observable<string> {
     return this.checkConnection(true)
       .flatMap(/* ... */)
       .pipe(
         catchError(error => {
           if (error.status === 403 || error.status === 404) {
             return Observable.throw(error);
           }
           return Observable.throw(error).pipe(
             retryWhen(/* ... */)
           );
         })
       );
   }

3. Add handleFatalError to emit an error through connectionStatus$:

   private handleFatalError(error) {
     this.connectionStatus$.error(error);
   }

While addressing the issue this seems to:

• Add no changes to the public API
• Improve error handling without altering existing behavior
• Allow users to implement recovery logic

Usage

Users would be able to catch token expiration errors as follows:

directLine.connectionStatus$.subscribe(
  status => console.log('Status:', ConnectionStatus[status]),
  error => {
    console.error('Error:', error);
    // Implement recovery logic here
  }
);

This proposal could be considered a minor version change as it adds functionality without breaking existing API contracts, but also we do not crash as before, so this may be considered as a breaking change as well. We might be able to work further towards more compatible solution e.g. by checking the observers for error handling.

Any thoughts?

[orgads]

Any progress with this?

Eugene (@OEvgeny) William Wong (@compulim) Steven Kanberg (@stevkan)