[PM-14484] ApiService showing html in error message (#14658)

* Make ApiService more testable

* Add ApiService tests

* Switch to only reading text/plain content

apps/browser/src/background/main.background.ts

@@ -720,6 +720,7 @@ export default class MainBackground {
       this.logService,
       (logoutReason: LogoutReason, userId?: UserId) => this.logout(logoutReason, userId),
       this.vaultTimeoutSettingsService,
+      { createRequest: (url, request) => new Request(url, request) },
     );
 
     this.fileUploadService = new FileUploadService(this.logService, this.apiService);

apps/cli/src/platform/services/node-api.service.ts

@@ -39,6 +39,7 @@ export class NodeApiService extends ApiService {
       logService,
       logoutCallback,
       vaultTimeoutSettingsService,
+      { createRequest: (url, request) => new Request(url, request) },
       customUserAgent,
     );
   }

libs/angular/src/services/injection-tokens.ts

@@ -13,6 +13,7 @@ import {
 import { Theme } from "@bitwarden/common/platform/enums";
 import { StateFactory } from "@bitwarden/common/platform/factories/state-factory";
 import { Message } from "@bitwarden/common/platform/messaging";
+import { HttpOperations } from "@bitwarden/common/services/api.service";
 import { SafeInjectionToken } from "@bitwarden/ui-common";
 // Re-export the SafeInjectionToken from ui-common
 export { SafeInjectionToken } from "@bitwarden/ui-common";
@@ -61,3 +62,5 @@ export const REFRESH_ACCESS_TOKEN_ERROR_CALLBACK = new SafeInjectionToken<() =>
 export const ENV_ADDITIONAL_REGIONS = new SafeInjectionToken<RegionConfig[]>(
   "ENV_ADDITIONAL_REGIONS",
 );
+
+export const HTTP_OPERATIONS = new SafeInjectionToken<HttpOperations>("HTTP_OPERATIONS");

libs/angular/src/services/jslib-services.module.ts

@@ -337,6 +337,7 @@ import {
   CLIENT_TYPE,
   DEFAULT_VAULT_TIMEOUT,
   ENV_ADDITIONAL_REGIONS,
+  HTTP_OPERATIONS,
   INTRAPROCESS_MESSAGING_SUBJECT,
   LOCALES_DIRECTORY,
   LOCKED_CALLBACK,
@@ -700,6 +701,10 @@ const safeProviders: SafeProvider[] = [
     },
     deps: [ToastService, I18nServiceAbstraction],
   }),
+  safeProvider({
+    provide: HTTP_OPERATIONS,
+    useValue: { createRequest: (url, request) => new Request(url, request) },
+  }),
   safeProvider({
     provide: ApiServiceAbstraction,
     useClass: ApiService,
@@ -712,6 +717,7 @@ const safeProviders: SafeProvider[] = [
       LogService,
       LOGOUT_CALLBACK,
       VaultTimeoutSettingsService,
+      HTTP_OPERATIONS,
     ],
   }),
   safeProvider({

libs/common/src/services/api.service.spec.ts

@@ -0,0 +1,203 @@
+import { mock, MockProxy } from "jest-mock-extended";
+import { of } from "rxjs";
+
+import { LogoutReason } from "@bitwarden/auth/common";
+
+import { TokenService } from "../auth/abstractions/token.service";
+import { DeviceType } from "../enums";
+import { VaultTimeoutSettingsService } from "../key-management/vault-timeout";
+import { ErrorResponse } from "../models/response/error.response";
+import { AppIdService } from "../platform/abstractions/app-id.service";
+import { Environment, EnvironmentService } from "../platform/abstractions/environment.service";
+import { LogService } from "../platform/abstractions/log.service";
+import { PlatformUtilsService } from "../platform/abstractions/platform-utils.service";
+
+import { ApiService, HttpOperations } from "./api.service";
+
+describe("ApiService", () => {
+  let tokenService: MockProxy<TokenService>;
+  let platformUtilsService: MockProxy<PlatformUtilsService>;
+  let environmentService: MockProxy<EnvironmentService>;
+  let appIdService: MockProxy<AppIdService>;
+  let refreshAccessTokenErrorCallback: jest.Mock<void, []>;
+  let logService: MockProxy<LogService>;
+  let logoutCallback: jest.Mock<Promise<void>, [reason: LogoutReason]>;
+  let vaultTimeoutSettingsService: MockProxy<VaultTimeoutSettingsService>;
+  let httpOperations: MockProxy<HttpOperations>;
+
+  let sut: ApiService;
+
+  beforeEach(() => {
+    tokenService = mock();
+    platformUtilsService = mock();
+    platformUtilsService.getDevice.mockReturnValue(DeviceType.ChromeExtension);
+
+    environmentService = mock();
+    appIdService = mock();
+    refreshAccessTokenErrorCallback = jest.fn();
+    logService = mock();
+    logoutCallback = jest.fn();
+    vaultTimeoutSettingsService = mock();
+    httpOperations = mock();
+
+    sut = new ApiService(
+      tokenService,
+      platformUtilsService,
+      environmentService,
+      appIdService,
+      refreshAccessTokenErrorCallback,
+      logService,
+      logoutCallback,
+      vaultTimeoutSettingsService,
+      httpOperations,
+      "custom-user-agent",
+    );
+  });
+
+  describe("send", () => {
+    it("handles ok GET", async () => {
+      environmentService.environment$ = of({
+        getApiUrl: () => "https://example.com",
+      } satisfies Partial<Environment> as Environment);
+
+      httpOperations.createRequest.mockImplementation((url, request) => {
+        return {
+          url: url,
+          cache: request.cache,
+          credentials: request.credentials,
+          method: request.method,
+          mode: request.mode,
+          signal: request.signal,
+          headers: new Headers(request.headers),
+        } satisfies Partial<Request> as unknown as Request;
+      });
+
+      tokenService.getAccessToken.mockResolvedValue("access_token");
+      tokenService.tokenNeedsRefresh.mockResolvedValue(false);
+
+      const nativeFetch = jest.fn<Promise<Response>, [request: Request]>();
+
+      nativeFetch.mockImplementation((request) => {
+        return Promise.resolve({
+          ok: true,
+          status: 200,
+          json: () => Promise.resolve({ hello: "world" }),
+          headers: new Headers({
+            "content-type": "application/json",
+          }),
+        } satisfies Partial<Response> as Response);
+      });
+
+      sut.nativeFetch = nativeFetch;
+
+      const response = await sut.send("GET", "/something", null, true, true, null, null);
+
+      expect(nativeFetch).toHaveBeenCalledTimes(1);
+      const request = nativeFetch.mock.calls[0][0];
+      // This should get set for users of send
+      expect(request.cache).toBe("no-store");
+      // TODO: Could expect on the credentials parameter
+      expect(request.headers.get("Device-Type")).toBe("2"); // Chrome Extension
+      // Custom user agent should get set
+      expect(request.headers.get("User-Agent")).toBe("custom-user-agent");
+      // This should be set when the caller has indicated there is a response
+      expect(request.headers.get("Accept")).toBe("application/json");
+      // If they have indicated that it's authed, then the authorization header should get set.
+      expect(request.headers.get("Authorization")).toBe("Bearer access_token");
+      // The response body
+      expect(response).toEqual({ hello: "world" });
+    });
+  });
+
+  const errorData: {
+    name: string;
+    input: Partial<Response>;
+    error: Partial<ErrorResponse>;
+  }[] = [
+    {
+      name: "json response in camel case",
+      input: {
+        json: () => Promise.resolve({ message: "Something bad happened." }),
+        headers: new Headers({
+          "content-type": "application/json",
+        }),
+      },
+      error: {
+        message: "Something bad happened.",
+      },
+    },
+    {
+      name: "json response in pascal case",
+      input: {
+        json: () => Promise.resolve({ Message: "Something bad happened." }),
+        headers: new Headers({
+          "content-type": "application/json",
+        }),
+      },
+      error: {
+        message: "Something bad happened.",
+      },
+    },
+    {
+      name: "json response with charset in content type",
+      input: {
+        json: () => Promise.resolve({ message: "Something bad happened." }),
+        headers: new Headers({
+          "content-type": "application/json; charset=utf-8",
+        }),
+      },
+      error: {
+        message: "Something bad happened.",
+      },
+    },
+    {
+      name: "text/plain response",
+      input: {
+        text: () => Promise.resolve("Something bad happened."),
+        headers: new Headers({
+          "content-type": "text/plain",
+        }),
+      },
+      error: {
+        message: "Something bad happened.",
+      },
+    },
+  ];
+
+  it.each(errorData)(
+    "throws error-like response when not ok response with $name",
+    async ({ input, error }) => {
+      environmentService.environment$ = of({
+        getApiUrl: () => "https://example.com",
+      } satisfies Partial<Environment> as Environment);
+
+      httpOperations.createRequest.mockImplementation((url, request) => {
+        return {
+          url: url,
+          cache: request.cache,
+          credentials: request.credentials,
+          method: request.method,
+          mode: request.mode,
+          signal: request.signal,
+          headers: new Headers(request.headers),
+        } satisfies Partial<Request> as unknown as Request;
+      });
+
+      const nativeFetch = jest.fn<Promise<Response>, [request: Request]>();
+
+      nativeFetch.mockImplementation((request) => {
+        return Promise.resolve({
+          ok: false,
+          status: 400,
+          ...input,
+        } satisfies Partial<Response> as Response);
+      });
+
+      sut.nativeFetch = nativeFetch;
+
+      await expect(
+        async () => await sut.send("GET", "/something", null, true, true, null, null),
+      ).rejects.toMatchObject(error);
+    },
+  );
+});

libs/common/src/services/api.service.ts

@@ -139,6 +139,10 @@ import { AttachmentResponse } from "../vault/models/response/attachment.response
 import { CipherResponse } from "../vault/models/response/cipher.response";
 import { OptionalCipherResponse } from "../vault/models/response/optional-cipher.response";
 
+export type HttpOperations = {
+  createRequest: (url: string, request: RequestInit) => Request;
+};
+
 /**
  * @deprecated The `ApiService` class is deprecated and calls should be extracted into individual
  * api services. The `send` method is still allowed to be used within api services. For background
@@ -166,6 +170,7 @@ export class ApiService implements ApiServiceAbstraction {
     private logService: LogService,
     private logoutCallback: (logoutReason: LogoutReason) => Promise<void>,
     private vaultTimeoutSettingsService: VaultTimeoutSettingsService,
+    private readonly httpOperations: HttpOperations,
     private customUserAgent: string = null,
   ) {
     this.device = platformUtilsService.getDevice();
@@ -217,7 +222,7 @@ export class ApiService implements ApiServiceAbstraction {
     const env = await firstValueFrom(this.environmentService.environment$);
 
     const response = await this.fetch(
-      new Request(env.getIdentityUrl() + "/connect/token", {
+      this.httpOperations.createRequest(env.getIdentityUrl() + "/connect/token", {
         body: this.qsStringify(identityToken),
         credentials: await this.getCredentials(),
         cache: "no-store",
@@ -1409,7 +1414,7 @@ export class ApiService implements ApiServiceAbstraction {
     }
     const env = await firstValueFrom(this.environmentService.environment$);
     const response = await this.fetch(
-      new Request(env.getEventsUrl() + "/collect", {
+      this.httpOperations.createRequest(env.getEventsUrl() + "/collect", {
         cache: "no-store",
         credentials: await this.getCredentials(),
         method: "POST",
@@ -1456,7 +1461,7 @@ export class ApiService implements ApiServiceAbstraction {
     const authHeader = await this.getActiveBearerToken();
 
     const response = await this.fetch(
-      new Request(keyConnectorUrl + "/user-keys", {
+      this.httpOperations.createRequest(keyConnectorUrl + "/user-keys", {
         cache: "no-store",
         method: "GET",
         headers: new Headers({
@@ -1481,7 +1486,7 @@ export class ApiService implements ApiServiceAbstraction {
     const authHeader = await this.getActiveBearerToken();
 
     const response = await this.fetch(
-      new Request(keyConnectorUrl + "/user-keys", {
+      this.httpOperations.createRequest(keyConnectorUrl + "/user-keys", {
         cache: "no-store",
         method: "POST",
         headers: new Headers({
@@ -1501,7 +1506,7 @@ export class ApiService implements ApiServiceAbstraction {
 
   async getKeyConnectorAlive(keyConnectorUrl: string) {
     const response = await this.fetch(
-      new Request(keyConnectorUrl + "/alive", {
+      this.httpOperations.createRequest(keyConnectorUrl + "/alive", {
         cache: "no-store",
         method: "GET",
         headers: new Headers({
@@ -1570,7 +1575,7 @@ export class ApiService implements ApiServiceAbstraction {
     const env = await firstValueFrom(this.environmentService.environment$);
     const path = `/sso/prevalidate?domainHint=${encodeURIComponent(identifier)}`;
     const response = await this.fetch(
-      new Request(env.getIdentityUrl() + path, {
+      this.httpOperations.createRequest(env.getIdentityUrl() + path, {
         cache: "no-store",
         credentials: await this.getCredentials(),
         headers: headers,
@@ -1711,7 +1716,7 @@ export class ApiService implements ApiServiceAbstraction {
     const env = await firstValueFrom(this.environmentService.environment$);
     const decodedToken = await this.tokenService.decodeAccessToken();
     const response = await this.fetch(
-      new Request(env.getIdentityUrl() + "/connect/token", {
+      this.httpOperations.createRequest(env.getIdentityUrl() + "/connect/token", {
         body: this.qsStringify({
           grant_type: "refresh_token",
           client_id: decodedToken.client_id,
@@ -1820,7 +1825,7 @@ export class ApiService implements ApiServiceAbstraction {
     };
     requestInit.headers = requestHeaders;
     requestInit.body = requestBody;
-    const response = await this.fetch(new Request(requestUrl, requestInit));
+    const response = await this.fetch(this.httpOperations.createRequest(requestUrl, requestInit));
 
     const responseType = response.headers.get("content-type");
     const responseIsJson = responseType != null && responseType.indexOf("application/json") !== -1;
@@ -1889,7 +1894,7 @@ export class ApiService implements ApiServiceAbstraction {
     let responseJson: any = null;
     if (this.isJsonResponse(response)) {
       responseJson = await response.json();
-    } else if (this.isTextResponse(response)) {
+    } else if (this.isTextPlainResponse(response)) {
       responseJson = { Message: await response.text() };
     }
 
@@ -1945,8 +1950,8 @@ export class ApiService implements ApiServiceAbstraction {
     return typeHeader != null && typeHeader.indexOf("application/json") > -1;
   }
 
-  private isTextResponse(response: Response): boolean {
+  private isTextPlainResponse(response: Response): boolean {
     const typeHeader = response.headers.get("content-type");
-    return typeHeader != null && typeHeader.indexOf("text") > -1;
+    return typeHeader != null && typeHeader.indexOf("text/plain") > -1;
   }
 }