fix(dns): authenticate Umbrella with OAuth2 client credentials, not Basic (#3271)

apps/api/src/services/dnsProviders/umbrella.test.ts

@@ -0,0 +1,170 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { UmbrellaProvider } from './umbrella';
+import { DnsProviderHttpError, requestJson } from './http';
+
+// Same shape as the AdGuard/Pi-hole provider tests: transport is not exercised,
+// only the provider's request shaping and response handling. DnsProviderHttpError
+// is kept REAL because the auth-retry path branches on `instanceof`.
+vi.mock('./http', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('./http')>();
+  return {
+    ...actual,
+    requestJson: vi.fn()
+  };
+});
+
+const requestJsonMock = vi.mocked(requestJson);
+
+const TOKEN_URL = 'https://api.umbrella.com/auth/v2/token';
+const ORG_ID = 'org-123';
+
+function urlOf(call: unknown[]): string {
+  return String(call[0]);
+}
+function initOf(call: unknown[]): RequestInit & { headers?: Record<string, string> } {
+  return (call[1] ?? {}) as RequestInit & { headers?: Record<string, string> };
+}
+
+function makeProvider() {
+  return new UmbrellaProvider('key-abc', 'secret-xyz', {
+    organizationId: ORG_ID,
+    blocklistId: 'bl-1',
+    allowlistId: 'al-1'
+  });
+}
+
+/** A token response, then an empty activity page so syncEvents terminates. */
+function queueTokenThen(...bodies: unknown[]) {
+  const queue = [{ access_token: 'tok-1', token_type: 'bearer', expires_in: 3600 }, ...bodies];
+  requestJsonMock.mockImplementation(async () => {
+    if (!queue.length) throw new Error('requestJson mock exhausted');
+    return queue.shift() as never;
+  });
+}
+
+beforeEach(() => {
+  vi.clearAllMocks();
+});
+
+describe('UmbrellaProvider OAuth2 client-credentials auth (#3271)', () => {
+  it('exchanges key/secret for a bearer token before calling the API', async () => {
+    queueTokenThen({ requests: [] });
+
+    await makeProvider().syncEvents(new Date('2026-08-01'), new Date('2026-08-02'));
+
+    const tokenCall = requestJsonMock.mock.calls[0]!;
+    expect(urlOf(tokenCall)).toBe(TOKEN_URL);
+
+    const init = initOf(tokenCall);
+    expect(init.method).toBe('POST');
+    expect(init.body).toBe('grant_type=client_credentials');
+    expect(init.headers?.['Content-Type']).toBe('application/x-www-form-urlencoded');
+    // key:secret is Basic ONLY on the token exchange, never on the API itself.
+    expect(init.headers?.Authorization).toBe(
+      `Basic ${Buffer.from('key-abc:secret-xyz').toString('base64')}`
+    );
+  });
+
+  it('sends the bearer token — not Basic — to the reporting API', async () => {
+    queueTokenThen({ requests: [] });
+
+    await makeProvider().syncEvents(new Date('2026-08-01'), new Date('2026-08-02'));
+
+    const apiCall = requestJsonMock.mock.calls[1]!;
+    expect(urlOf(apiCall)).toContain('reports.api.umbrella.com');
+    expect(initOf(apiCall).headers?.Authorization).toBe('Bearer tok-1');
+  });
+
+  it('sends the bearer token to the policies API too', async () => {
+    queueTokenThen({});
+
+    await makeProvider().addBlocklistDomain('evil.example', 'because');
+
+    const apiCall = requestJsonMock.mock.calls[1]!;
+    expect(urlOf(apiCall)).toContain('api.umbrella.com/policies/v2/destinationlists');
+    expect(initOf(apiCall).headers?.Authorization).toBe('Bearer tok-1');
+  });
+
+  it('caches the token across calls instead of re-exchanging per request', async () => {
+    queueTokenThen({}, {});
+    const provider = makeProvider();
+
+    await provider.addBlocklistDomain('a.example');
+    await provider.addAllowlistDomain('b.example');
+
+    const tokenCalls = requestJsonMock.mock.calls.filter((c) => urlOf(c) === TOKEN_URL);
+    expect(tokenCalls).toHaveLength(1);
+    expect(requestJsonMock).toHaveBeenCalledTimes(3); // 1 token + 2 API
+  });
+
+  // The non-obvious one: Umbrella reports an EXPIRED token as 400
+  // invalid_request, not 401, so a plain retry-on-401 would never fire.
+  it('refreshes and retries when an expired token yields 400 invalid_request', async () => {
+    let call = 0;
+    requestJsonMock.mockImplementation(async (input) => {
+      call++;
+      if (String(input) === TOKEN_URL) {
+        return { access_token: `tok-${call}`, expires_in: 3600 } as never;
+      }
+      if (call === 2) {
+        throw new DnsProviderHttpError(400, 'Bad Request', '{"error":"invalid_request"}');
+      }
+      return {} as never;
+    });
+
+    await makeProvider().addBlocklistDomain('evil.example');
+
+    const tokenCalls = requestJsonMock.mock.calls.filter((c) => urlOf(c) === TOKEN_URL);
+    expect(tokenCalls).toHaveLength(2); // original + forced refresh
+    const retried = requestJsonMock.mock.calls[3]!;
+    expect(initOf(retried).headers?.Authorization).toBe('Bearer tok-3');
+  });
+
+  it('refreshes and retries on 401', async () => {
+    let call = 0;
+    requestJsonMock.mockImplementation(async (input) => {
+      call++;
+      if (String(input) === TOKEN_URL) {
+        return { access_token: `tok-${call}`, expires_in: 3600 } as never;
+      }
+      if (call === 2) {
+        throw new DnsProviderHttpError(401, 'Unauthorized', '{"data":{"error":"unauthorized"}}');
+      }
+      return {} as never;
+    });
+
+    await makeProvider().addAllowlistDomain('ok.example');
+
+    expect(requestJsonMock.mock.calls.filter((c) => urlOf(c) === TOKEN_URL)).toHaveLength(2);
+  });
+
+  // Control: without this, "retry on 400" would mask real validation errors and
+  // silently double-send writes.
+  it('does NOT retry a genuine validation 400', async () => {
+    let call = 0;
+    requestJsonMock.mockImplementation(async (input) => {
+      call++;
+      if (String(input) === TOKEN_URL) {
+        return { access_token: 'tok-1', expires_in: 3600 } as never;
+      }
+      throw new DnsProviderHttpError(400, 'Bad Request', '{"error":"destination is not a valid domain"}');
+    });
+
+    await expect(makeProvider().addBlocklistDomain('not a domain')).rejects.toMatchObject({ status: 400 });
+
+    expect(requestJsonMock.mock.calls.filter((c) => urlOf(c) === TOKEN_URL)).toHaveLength(1);
+    // One attempt only — no retry, so no risk of a duplicate write.
+    expect(requestJsonMock).toHaveBeenCalledTimes(2);
+  });
+
+  it('still requires apiSecret', async () => {
+    const provider = new UmbrellaProvider('key-abc', null, { organizationId: ORG_ID, blocklistId: 'bl-1' });
+    await expect(provider.addBlocklistDomain('x.example')).rejects.toThrow(/requires apiSecret/);
+    expect(requestJsonMock).not.toHaveBeenCalled();
+  });
+
+  it('fails loudly if the token endpoint returns no access_token', async () => {
+    requestJsonMock.mockImplementation(async () => ({ token_type: 'bearer' }) as never);
+    await expect(makeProvider().addBlocklistDomain('x.example')).rejects.toThrow(/no access_token/);
+  });
+});

apps/api/src/services/dnsProviders/umbrella.ts

@@ -1,5 +1,5 @@
 import type { DnsEvent, DnsProvider } from './index';
-import { requestJson } from './http';
+import { DnsProviderHttpError, requestJson } from './http';
 import { asArray, asBoolean, asNumber, asRecord, asString, asStringArray } from './helpers';
 
 export interface UmbrellaProviderConfig {
@@ -8,19 +8,115 @@ export interface UmbrellaProviderConfig {
   allowlistId?: string;
 }
 
+/** Cisco's OAuth2 client-credentials token endpoint (Umbrella API). */
+const UMBRELLA_TOKEN_URL = 'https://api.umbrella.com/auth/v2/token';
+
+/**
+ * Refresh this many ms before the advertised expiry so a token can't lapse
+ * mid-request. Umbrella tokens live 3600s, so 60s is ~1.7% of the lifetime.
+ */
+const TOKEN_EXPIRY_SAFETY_MS = 60_000;
+
+/** Fallback lifetime if Umbrella ever omits `expires_in` (documented as 3600). */
+const DEFAULT_TOKEN_LIFETIME_S = 3600;
+
 export class UmbrellaProvider implements DnsProvider {
+  private tokenCache: { accessToken: string; expiresAt: number } | null = null;
+  /** In-flight exchange, so concurrent calls share one token request. */
+  private tokenInFlight: Promise<string> | null = null;
+
   constructor(
     private readonly apiKey: string,
     private readonly apiSecret: string | null | undefined,
     private readonly config: UmbrellaProviderConfig
   ) {}
 
-  private basicAuthHeader(): string {
+  /**
+   * Umbrella retired direct Basic Auth on its APIs: the key/secret now buy a
+   * short-lived bearer token from the OAuth2 client-credentials endpoint, and
+   * every API call carries that token instead (#3271). Sending Basic straight
+   * at the API returns 401 for every key type, which is what made the old code
+   * look like a credentials problem rather than an auth-scheme one.
+   *
+   * One token covers every Umbrella surface — the reporting host and
+   * `policies/v2` alike — so it is cached on the provider instance.
+   */
+  private async getAccessToken(forceRefresh = false): Promise<string> {
     if (!this.apiSecret) {
       throw new Error('Cisco Umbrella integration requires apiSecret');
     }
-    const token = Buffer.from(`${this.apiKey}:${this.apiSecret}`).toString('base64');
-    return `Basic ${token}`;
+
+    if (!forceRefresh) {
+      const cached = this.tokenCache;
+      if (cached && cached.expiresAt > Date.now()) {
+        return cached.accessToken;
+      }
+      if (this.tokenInFlight) return this.tokenInFlight;
+    }
+
+    const basic = Buffer.from(`${this.apiKey}:${this.apiSecret}`).toString('base64');
+    const exchange = (async (): Promise<string> => {
+      const payload = await requestJson<Record<string, unknown>>(UMBRELLA_TOKEN_URL, {
+        method: 'POST',
+        headers: {
+          Authorization: `Basic ${basic}`,
+          'Content-Type': 'application/x-www-form-urlencoded'
+        },
+        body: 'grant_type=client_credentials'
+      });
+
+      const accessToken = asString(payload.access_token);
+      if (!accessToken) {
+        // Deliberately body-free: the token response is credential material.
+        throw new Error('Cisco Umbrella token endpoint returned no access_token');
+      }
+
+      const lifetimeS = asNumber(payload.expires_in) ?? DEFAULT_TOKEN_LIFETIME_S;
+      this.tokenCache = {
+        accessToken,
+        expiresAt: Date.now() + Math.max(0, lifetimeS * 1000 - TOKEN_EXPIRY_SAFETY_MS)
+      };
+      return accessToken;
+    })();
+
+    this.tokenInFlight = exchange;
+    try {
+      return await exchange;
+    } finally {
+      if (this.tokenInFlight === exchange) this.tokenInFlight = null;
+    }
+  }
+
+  /**
+   * Run an Umbrella API call with a bearer token, refreshing once if the token
+   * turns out to be dead.
+   *
+   * Note the status: an EXPIRED Umbrella token yields **400** with
+   * `{"error":"invalid_request"}`, not 401 — so a plain retry-on-401 would miss
+   * exactly the case a cache makes possible. 401 is still handled for a token
+   * revoked or scoped away mid-run. Genuine validation 400s (a malformed
+   * destination, say) are left alone by matching on the error body.
+   */
+  private async withAuth<T>(call: (authHeader: string) => Promise<T>): Promise<T> {
+    const attempt = async (forceRefresh: boolean): Promise<T> => {
+      const token = await this.getAccessToken(forceRefresh);
+      return call(`Bearer ${token}`);
+    };
+
+    try {
+      return await attempt(false);
+    } catch (error) {
+      if (!this.isAuthFailure(error)) throw error;
+      this.tokenCache = null;
+      return attempt(true);
+    }
+  }
+
+  private isAuthFailure(error: unknown): boolean {
+    if (!(error instanceof DnsProviderHttpError)) return false;
+    if (error.status === 401) return true;
+    // Umbrella signals an expired/invalid token as 400 invalid_request.
+    return error.status === 400 && /invalid_request|invalid_token|unauthorized/i.test(error.responseBody);
   }
 
   async syncEvents(since: Date, until: Date): Promise<DnsEvent[]> {
@@ -47,11 +143,11 @@ export class UmbrellaProvider implements DnsProvider {
         url.searchParams.set('page', String(page));
       }
 
-      const payload = await requestJson<Record<string, unknown>>(url, {
-        headers: {
-          Authorization: this.basicAuthHeader(),
-        }
-      });
+      const payload = await this.withAuth((authorization) =>
+        requestJson<Record<string, unknown>>(url, {
+          headers: { Authorization: authorization }
+        })
+      );
 
       const requests = asArray(payload.requests ?? payload.data);
       const mapped = requests.flatMap((entry): DnsEvent[] => {
@@ -152,58 +248,58 @@ export class UmbrellaProvider implements DnsProvider {
   async addBlocklistDomain(domain: string, reason?: string): Promise<void> {
     const listId = this.getDestinationListId('block');
     const url = `https://api.umbrella.com/policies/v2/destinationlists/${listId}/destinations`;
-    await requestJson(url, {
+    await this.withAuth((authorization) => requestJson(url, {
       method: 'POST',
       headers: {
-        Authorization: this.basicAuthHeader(),
+        Authorization: authorization,
         'Content-Type': 'application/json'
       },
       body: JSON.stringify({
         destination: domain,
         comment: reason
       })
-    });
+    }));
   }
 
   async removeBlocklistDomain(domain: string): Promise<void> {
     const listId = this.getDestinationListId('block');
     const url = new URL(`https://api.umbrella.com/policies/v2/destinationlists/${listId}/destinations`);
     url.searchParams.set('destination', domain);
 
-    await requestJson(url, {
+    await this.withAuth((authorization) => requestJson(url, {
       method: 'DELETE',
       headers: {
-        Authorization: this.basicAuthHeader()
+        Authorization: authorization
       }
-    });
+    }));
   }
 
   async addAllowlistDomain(domain: string): Promise<void> {
     const listId = this.getDestinationListId('allow');
     const url = `https://api.umbrella.com/policies/v2/destinationlists/${listId}/destinations`;
 
-    await requestJson(url, {
+    await this.withAuth((authorization) => requestJson(url, {
       method: 'POST',
       headers: {
-        Authorization: this.basicAuthHeader(),
+        Authorization: authorization,
         'Content-Type': 'application/json'
       },
       body: JSON.stringify({
         destination: domain
       })
-    });
+    }));
   }
 
   async removeAllowlistDomain(domain: string): Promise<void> {
     const listId = this.getDestinationListId('allow');
     const url = new URL(`https://api.umbrella.com/policies/v2/destinationlists/${listId}/destinations`);
     url.searchParams.set('destination', domain);
 
-    await requestJson(url, {
+    await this.withAuth((authorization) => requestJson(url, {
       method: 'DELETE',
       headers: {
-        Authorization: this.basicAuthHeader()
+        Authorization: authorization
       }
-    });
+    }));
   }
 }