chore: Add warning logs for cross-host CSRF and iss-based destination fetch (#6618)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: brodmart <brodmart@users.noreply.github.com>
Co-authored-by: sap-cloud-sdk-bot[bot] <274190970+sap-cloud-sdk-bot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Author: 4 people

.changeset/warn-cross-host-csrf.md

@@ -0,0 +1,5 @@
+---
+'@sap-cloud-sdk/http-client': patch
+---
+
+[Fixed Issue] Warn when the CSRF token fetch URL has a different host than the request URL, as sensitive headers would be forwarded to the cross-host endpoint.

.github/actions/check-public-api/index.js

@@ -287,7 +287,7 @@ describe('JWT type and selection strategies', () => {
         package: 'connectivity',
         messageContext: 'destination-accessor-service'
       });
-      const debugSpy = jest.spyOn(logger, 'debug');
+      const warnSpy = jest.spyOn(logger, 'warn');
 
       expect(
         await getDestinationFromDestinationService({
@@ -297,8 +297,9 @@ describe('JWT type and selection strategies', () => {
         })
       ).toMatchObject(parseDestination(certificateSingleResponse));
 
-      expect(debugSpy).toHaveBeenCalledWith(
-        'Using `iss` option instead of a full JWT to fetch a destination. No validation is performed.'
+      expect(warnSpy).toHaveBeenCalledWith(
+        'Using `iss` option instead of a full JWT to fetch a destination. No validation is performed.' +
+          'Passing a user-supplied value without verifying it may lead to unintended cross-tenant destination access.'
       );
     });
 

packages/connectivity/src/scp-cf/destination/destination-accessor-provider-subscriber-lookup.spec.ts

@@ -74,7 +74,10 @@ export interface DestinationAccessorOptions {
    * The value for `iss` is the issuer field of a JWT e.g. https://<your-subdomain>.localhost:8080/uaa/oauth/token.
    *
    * ATTENTION: If this property is used, no validation of the provided subdomain value is done.
-   * This is differs from how the `jwt` is handled.
+   * This differs from how the `jwt` is handled.
+   * Never pass a user-supplied or request-derived value directly to this field without verification.
+   * An unvalidated `iss` value controls which BTP tenant's destinations (including embedded credentials and
+   * OAuth tokens) are returned, enabling cross-tenant data access if an attacker can influence this value.
    * So be careful that the used value is not manipulated and breaks the tenant isolation of your application.
    */
   iss?: string;

packages/connectivity/src/scp-cf/destination/destination-accessor-types.ts

@@ -76,8 +76,9 @@ async function retrieveServiceToken(
 
 function getJwtForServiceToken(iss?: string, decodedUserJwt?: JwtPayload) {
   if (iss) {
-    logger.debug(
-      'Using `iss` option instead of a full JWT to fetch a destination. No validation is performed.'
+    logger.warn(
+      'Using `iss` option instead of a full JWT to fetch a destination. No validation is performed.' +
+        'Passing a user-supplied value without verifying it may lead to unintended cross-tenant destination access.'
     );
 
     return { ext_attr: { zdn: getIssuerSubdomain({ iss }) } };

packages/connectivity/src/scp-cf/destination/get-subscriber-token.ts

@@ -1,5 +1,5 @@
 import { URL } from 'url';
-import { removeTrailingSlashes } from '@sap-cloud-sdk/util';
+import { isValidUrl, removeTrailingSlashes } from '@sap-cloud-sdk/util';
 import type { JwtPayload } from './jsonwebtoken-type';
 
 /**
@@ -29,15 +29,6 @@ function getHost(url: URL): string {
   return host;
 }
 
-function isValidUrl(url: string): boolean {
-  try {
-    new URL(url);
-    return true;
-  } catch {
-    return false;
-  }
-}
-
 /**
  * @internal
  * Replaces the first part of the hostname (subdomain) in a URL.

packages/connectivity/src/scp-cf/subdomain-replacer.ts

@@ -1,5 +1,6 @@
 import nock from 'nock';
 import axios from 'axios';
+import { createLogger } from '@sap-cloud-sdk/util';
 import { circuitBreaker, timeout } from '@sap-cloud-sdk/resilience';
 import { circuitBreakers } from '@sap-cloud-sdk/resilience/internal';
 import { csrf } from './csrf-token-middleware';
@@ -250,19 +251,42 @@ describe('CSRF middleware', () => {
     ).resolves.not.toThrow();
   });
 
-  it('fetches the token with custom method', async () => {
-    nock(host).get('/some/path/').reply(200, {}, csrfResponseHeaders);
+  it('logs a warning when the CSRF token URL has a different host than the request URL', async () => {
+    const csrfHost = 'http://other.example.com';
+    nock(csrfHost).head('/csrf').reply(200, {}, csrfResponseHeaders);
     nock(host).post('/some/path').reply(200, {});
-    await expect(
-      executeHttpRequest(
-        { url: host },
-        {
-          method: 'POST',
-          url: 'some/path',
-          middleware: [csrf({ method: 'GET' })]
-        },
-        { fetchCsrfToken: false }
-      )
-    ).resolves.not.toThrow();
+    const logger = createLogger('csrf-middleware');
+    const warnSpy = jest.spyOn(logger, 'warn');
+    await executeHttpRequest(
+      { url: host },
+      {
+        method: 'POST',
+        url: 'some/path',
+        middleware: [csrf({ url: `${csrfHost}/csrf` })]
+      },
+      { fetchCsrfToken: false }
+    );
+    expect(warnSpy).toHaveBeenCalledWith(
+      expect.stringContaining('different host')
+    );
+  });
+
+  it('does not log a warning when the CSRF token URL has the same host as the request URL', async () => {
+    nock(host).head('/alternative/path').reply(200, {}, csrfResponseHeaders);
+    nock(host).post('/some/path').reply(200, {});
+    const logger = createLogger('csrf-middleware');
+    const warnSpy = jest.spyOn(logger, 'warn');
+    await executeHttpRequest(
+      { url: host },
+      {
+        method: 'POST',
+        url: 'some/path',
+        middleware: [csrf({ url: `${host}/alternative/path` })]
+      },
+      { fetchCsrfToken: false }
+    );
+    expect(warnSpy).not.toHaveBeenCalledWith(
+      expect.stringContaining('different host')
+    );
   });
 });

packages/http-client/src/csrf-token-middleware.spec.ts

@@ -1,8 +1,10 @@
+import { URL } from 'url';
 import {
   createLogger,
   ErrorWithCause,
   first,
   flatten,
+  isValidUrl,
   pickIgnoreCase,
   pickValueIgnoreCase,
   removeTrailingSlashes
@@ -166,13 +168,34 @@ function findCsrfHeader(
   return { 'x-csrf-token': csrfHeader, ...cookieHeader };
 }
 
+function isCrossHost(
+  csrfUrl: string | undefined,
+  requestUrl: string | undefined
+): boolean {
+  if (!csrfUrl || !requestUrl) {
+    return false;
+  }
+  if (!isValidUrl(csrfUrl) || !isValidUrl(requestUrl)) {
+    return false;
+  }
+  return new URL(csrfUrl).hostname !== new URL(requestUrl).hostname;
+}
+
 async function makeCsrfRequests(
   requestConfig: HttpRequestConfig,
   options: CsrfMiddlewareOptions & { context: HttpMiddlewareContext }
 ): Promise<CsrfHeaderWithCookie | undefined> {
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   const { data, params, parameterEncoder, ...requestConfigWithoutData } =
     requestConfig;
+
+  // TODO: In v5, make cross-host CSRF token fetching opt-in instead of just warning.
+  if (isCrossHost(options.url, requestConfig.baseURL)) {
+    logger.warn(
+      `The CSRF token fetch URL (${options.url}) has a different host than the request URL (${requestConfig.baseURL}). Sensitive headers will be forwarded to the CSRF token endpoint.`
+    );
+  }
+
   const axiosConfig: HttpRequestConfigWithOrigin = {
     ...requestConfigWithoutData,
     method: options.method || 'head',

packages/http-client/src/csrf-token-middleware.ts

@@ -1,5 +1,20 @@
 import axios from 'axios';
 
+/**
+ * Checks whether a string is a valid URL.
+ * @param url - String to check.
+ * @returns True if the string is a valid URL, false otherwise.
+ * @internal
+ */
+export function isValidUrl(url: string): boolean {
+  try {
+    new URL(url);
+    return true;
+  } catch {
+    return false;
+  }
+}
+
 /**
  * Checks whether a URL is existing via a head request.
  * @param url - URL to be checked.