fix: process cookies in failed responses

[padamstx]

This commit updates our support for cookies
to allow cookies to be scraped from non-2xx
responses in addition to 2xx responses.

Checklist

• npm test passes (tip: npm run lint-fix can correct most style issues)
• tests are included
• documentation is changed or added

[CarstenLeue]

I have added my review comments inline. I addition I would like to make a small recommendation to refactor the CookieInterceptor class like so:

/**
 * (C) Copyright IBM Corp. 2022, 2023.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

import { AxiosResponse, InternalAxiosRequestConfig, isAxiosError } from 'axios';
import extend from 'extend';
import { Cookie, CookieJar } from 'tough-cookie';
import logger from './logger';

const internalCreateCookieInterceptor = (cookieJar: CookieJar) => {
  /**
   * This is called by Axios when a request is about to be sent in order to
   * copy the cookie string from the URL to a request header.
   *
   * @param config the Axios request config
   * @returns the request config
   */
  async function requestInterceptor(config: InternalAxiosRequestConfig) {
    logger.debug('CookieInterceptor: intercepting request');
    if (config && config.url) {
      logger.debug(`CookieInterceptor: getting cookies for: ${config.url}`);
      const cookieHeaderValue = await cookieJar.getCookieString(config.url);
      if (cookieHeaderValue) {
        logger.debug('CookieInterceptor: setting cookie header');
        const cookieHeader = { cookie: cookieHeaderValue };
        config.headers = extend(true, {}, config.headers, cookieHeader);
      } else {
        logger.debug(`CookieInterceptor: no cookies for: ${config.url}`);
      }
    } else {
      logger.debug('CookieInterceptor: no request URL.');
    }
    return config;
  }

  /**
   * This is called by Axios when a 2xx response has been received.
   * We'll invoke the configured cookie jar's setCookie() method to handle
   * the "set-cookie" header.
   * @param response the Axios response object
   * @returns the response object
   */
  async function responseInterceptor(response: AxiosResponse) {
    logger.debug('CookieInterceptor: intercepting response.');
    if (response && response.headers) {
      logger.debug('CookieInterceptor: checking for set-cookie headers.');
      const cookies: string[] = response.headers['set-cookie'];
      if (cookies) {
        logger.debug(`CookieInterceptor: setting cookies in jar for URL ${response.config.url}.`);
        // Write cookies sequentially by chaining the promises in a reduce
        await cookies.reduce(
          (cookiePromise: Promise<Cookie>, cookie: string) =>
            cookiePromise.then(() => cookieJar.setCookie(cookie, response.config.url)),
          Promise.resolve(null)
        );
      } else {
        logger.debug('CookieInterceptor: no set-cookie headers.');
      }
    } else {
      logger.debug('CookieInterceptor: no response headers.');
    }
    return response;
  }

  /**
   * This is called by Axios when a non-2xx response has been received.
   * We'll simply invoke the "responseFulfilled" method since we want to
   * do the same cookie handler as for a success response.
   * @param error the Axios error object that describes the non-2xx response
   * @returns the error object
   */
  async function responseRejected(error: any) {
    logger.debug('CookieIntercepter: intercepting error response');

    if (isAxiosError(error)) {
      logger.debug('CookieIntercepter: delegating to responseInterceptor()');
      await responseInterceptor(error.response);
    } else {
      logger.debug('CookieInterceptor: no response field in error object, skipping...');
    }

    return Promise.reject(error);
  }

  return {
    requestInterceptor,
    responseInterceptor,
    responseRejected,
  }
}

export const createCookieInterceptor = (cookieJar: CookieJar | boolean) => {

  if (cookieJar) {
    if (cookieJar === true) {
      logger.debug('CookieInterceptor: creating new CookieJar');
      return internalCreateCookieInterceptor(new CookieJar());
    } else {
      logger.debug('CookieInterceptor: using supplied CookieJar');
      return internalCreateCookieInterceptor(cookieJar);
    }
  } else {
    throw new Error('Must supply a cookie jar or true.');
  }
}

The advantage is that now the method no longer depends on the this pointer, so they can be used without having to bind them or wrap them into a lambda expression (i.e. a more functional oriented style).

You can then use them like so:

    if (axiosOptions.jar) {
      const cookieInterceptor = createCookieInterceptor(axiosOptions.jar);
      this.axiosInstance.interceptors.request.use(cookieInterceptor.requestInterceptor);
      this.axiosInstance.interceptors.response.use(cookieInterceptor.responseInterceptor, cookieInterceptor.responseRejected);
    }

[CarstenLeue]

I recommend to write

    if (isAxiosError(error)) {

instead, because this gives you type safety on the error.response object

[CarstenLeue]

if the typing is correct, then this check is not needed, since response is guaranteed to exist and so is response.headers.

[dpopp07]

We still have many users that use this package with vanilla JS. I get that the Axios API probably guarantees this but it still may be prudent to check and prevent unnecessary reference exceptions. What do you think?

[CarstenLeue]

Just my 2cents: the response interceptor is used in the scope of axios.interceptors.response and the typing here is:

response: AxiosInterceptorManager<AxiosResponse>;

So axios guarantees that the interceptor is called with an AxiosResponse, the interceptor would not be called by a user directly, only via axios.

In such cases I rely on the contract provided by the library. To me it's one of the main advantages of TS to have these guarantees at compile time, so we do not need to check at runtime.

[dpopp07]

That is fair - I'll make that change

[CarstenLeue]

In case you are open to refactoring and you'd like to take it one step further:

/**
 * (C) Copyright IBM Corp. 2022, 2023.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

import { Axios, AxiosResponse, InternalAxiosRequestConfig, isAxiosError } from 'axios';
import extend from 'extend';
import { Cookie, CookieJar } from 'tough-cookie';
import logger from './logger';

const internalCreateCookieInterceptor = (cookieJar: CookieJar) => {
  /**
   * This is called by Axios when a request is about to be sent in order to
   * copy the cookie string from the URL to a request header.
   *
   * @param config the Axios request config
   * @returns the request config
   */
  async function requestInterceptor(config: InternalAxiosRequestConfig) {
    logger.debug('CookieInterceptor: intercepting request');
    if (config && config.url) {
      logger.debug(`CookieInterceptor: getting cookies for: ${config.url}`);
      const cookieHeaderValue = await cookieJar.getCookieString(config.url);
      if (cookieHeaderValue) {
        logger.debug('CookieInterceptor: setting cookie header');
        const cookieHeader = { cookie: cookieHeaderValue };
        config.headers = extend(true, {}, config.headers, cookieHeader);
      } else {
        logger.debug(`CookieInterceptor: no cookies for: ${config.url}`);
      }
    } else {
      logger.debug('CookieInterceptor: no request URL.');
    }
    return config;
  }

  /**
   * This is called by Axios when a 2xx response has been received.
   * We'll invoke the configured cookie jar's setCookie() method to handle
   * the "set-cookie" header.
   * @param response the Axios response object
   * @returns the response object
   */
  async function responseInterceptor(response: AxiosResponse) {
    logger.debug('CookieInterceptor: intercepting response.');
    if (response && response.headers) {
      logger.debug('CookieInterceptor: checking for set-cookie headers.');
      const cookies: string[] = response.headers['set-cookie'];
      if (cookies) {
        logger.debug(`CookieInterceptor: setting cookies in jar for URL ${response.config.url}.`);
        // Write cookies sequentially by chaining the promises in a reduce
        await cookies.reduce(
          (cookiePromise: Promise<Cookie>, cookie: string) =>
            cookiePromise.then(() => cookieJar.setCookie(cookie, response.config.url)),
          Promise.resolve(null)
        );
      } else {
        logger.debug('CookieInterceptor: no set-cookie headers.');
      }
    } else {
      logger.debug('CookieInterceptor: no response headers.');
    }
    return response;
  }

  /**
   * This is called by Axios when a non-2xx response has been received.
   * We'll simply invoke the "responseFulfilled" method since we want to
   * do the same cookie handler as for a success response.
   * @param error the Axios error object that describes the non-2xx response
   * @returns the error object
   */
  async function responseRejected(error: any) {
    logger.debug('CookieIntercepter: intercepting error response');

    if (isAxiosError(error)) {
      logger.debug('CookieIntercepter: delegating to responseInterceptor()');
      await responseInterceptor(error.response);
    } else {
      logger.debug('CookieInterceptor: no response field in error object, skipping...');
    }

    return Promise.reject(error);
  }

  return (axios: Axios) => {
    axios.interceptors.request.use(requestInterceptor);
    axios.interceptors.response.use(responseInterceptor, responseRejected);
  }
}

export const createCookieInterceptor = (cookieJar: CookieJar | boolean) => {

  if (cookieJar) {
    if (cookieJar === true) {
      logger.debug('CookieInterceptor: creating new CookieJar');
      return internalCreateCookieInterceptor(new CookieJar());
    } else {
      logger.debug('CookieInterceptor: using supplied CookieJar');
      return internalCreateCookieInterceptor(cookieJar);
    }
  } else {
    throw new Error('Must supply a cookie jar or true.');
  }
}

and usage is:

    // if a cookie jar is provided, register our CookieInterceptor methods with axios
    if (axiosOptions.jar) {
      createCookieInterceptor(axiosOptions.jar)(this.axiosInstance);
    }

The advantage of this approach over the previous is that now the method name requestInterceptor, responseInterceptor and responseRejected are completely internal to the closure internalCreateCookieInterceptor, so a packer (e.g. webpack) can mangle these names, resulting in more compact code.

[dpopp07]

@CarstenLeue thanks for the suggestions - they look good to me. I'm updating the branch and will push a new commit shortly.

[CarstenLeue]

LGTM

[ibm-devx-sdk]

🎉 This PR is included in version 4.1.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀