fix: Patch the vulnerability for Bigquery and other REST based clients

[danieljbruce]

Description

This pull request adds URI path validation and RFC 3986 percent-encoding to @google-cloud/common to prevent path traversal and injection vulnerabilities across REST requests. It introduces helper utilities replicated from google-gax and updates Service and ServiceObject request handling to validate and encode path segments safely.

Impact

• Security Hardening: Rejects path traversal payloads (., .., %2e, %2E, %2e%2e, %2E%2E, foo/../bar) before requests are sent to the network.
• URI Encoding Compliance: Correctly encodes special characters, fragments (#), and query parameter (?) injection payloads in resource IDs while preserving valid colon-separated resource names and pre-encoded path segments without double encoding or decoding.

Testing

• Common Library Unit Tests: Added and verified 12 test cases in core/common/test/traversal.ts testing both ServiceObject and Service across all traversal and encoding scenarios (all passing).
• BigQuery Verification: Built and verified compilation of @google-cloud/bigquery with the new system test suite in handwritten/bigquery/system-test/traversal.ts.

Next Steps

• Release and publish the updated @google-cloud/common package to npm.
• Bump @google-cloud/common dependency in @google-cloud/bigquery.
• Set up a mock server for BigQuery testing to verify outgoing REST request paths end-to-end and add tests against the Bigquery service.
• Unskip (describe.skip -> describe) and run the BigQuery system tests in handwritten/bigquery/system-test/traversal.ts.
• Delete the temporary test suite in core/common/test/traversal.ts once the downstream tests are active.

[gemini-code-assist]

Code Review

This pull request introduces URI encoding and path traversal validation utilities to secure service requests against directory traversal attacks. However, the security review identified critical vulnerabilities where percent-encoded sequences (e.g., %2e%2e) can bypass the validation checks in validateUriPathSegment and validateUriPath. Additionally, encodeURIPath may double-encode already percent-encoded segments. To address these issues, it is recommended to decode path segments using decodeURIComponent before performing validation and encoding.

[gemini-code-assist]

validateUriPathSegment does not decode the input segment before checking if it is . or ... If a user passes a percent-encoded path traversal sequence (e.g., %2e%2e), this check will be bypassed. To prevent path traversal bypasses, the segment should be decoded using decodeURIComponent before validation.

export function validateUriPathSegment(propertyName: string, value: string): void {
  try {
    const decoded = decodeURIComponent(value);
    if (decoded === '.' || decoded === '..') {
      throw new Error(`Invalid value ${value} for ${propertyName}`);
    }
  } catch {
    if (value === '.' || value === '..') {
      throw new Error(`Invalid value ${value} for ${propertyName}`);
    }
  }
}

[danieljbruce]

Now it does decode the uri before doing the validation, but I think it is also worth noting here that we needed to carefully consider the impact of decoding uris on existing use cases since it means that the Bigquery server will be receiving a different uri which is also the case when encoding anyway. In consideration of the tradeoffs involved, I think decoding is worth it and for the sake of time we'll stick with it for now, but I'll suggest future work we may want to consider.

1. We should explore more combinations for tests against the Bigquery service, particularly with :, /, ? and .. plus . characters both pre-encoded and not encoded yet to see what the outcomes are. This involves setting up a test that sets up a dataset so that we can create tests that are expected to make successful API calls.
2. We could do an investigation with a mock server to see what strings really reach the server to get a understanding of exactly what is decoded and where. Understanding exactly what reaches the server would allow us to cut down on our test cases in datastore: benchmark datastore calls against protobuf endpoint #1 and decide what the outcomes really should be and what is possible.

[gemini-code-assist]

validateUriPath does not decode the path segments before checking for . or ... A user could bypass this check by using percent-encoded characters (e.g., %2e%2e). To ensure robust protection against path traversal, each segment should be decoded using decodeURIComponent before checking.

export function validateUriPath(propertyName: string, value: string): void {
  if (value) {
    const segments = value.split('/');
    const hasTraversal = segments.some(segment => {
      try {
        const decoded = decodeURIComponent(segment);
        return decoded === '.' || decoded === '..';
      } catch {
        return segment === '.' || segment === '..';
      }
    });
    if (hasTraversal) {
      throw new Error(
        `Value for ${propertyName} must not contain segments that are exactly . or ..`,
      );
    }
  }
}

[danieljbruce]

This is basically saying we should decode the path segments which the code now does.

[gemini-code-assist]

encodeURIPath currently double-encodes any path segments that are already percent-encoded (e.g., %20 becomes %2520). This will break legitimate requests that contain pre-encoded characters (such as spaces or non-ASCII characters in table/dataset IDs). Additionally, if the input is already percent-encoded, the path traversal checks in validateUriPathSegment can be bypassed.

To fix both issues, each segment/subpart should be decoded using decodeURIComponent before validation and encoding.

export function encodeURIPath(uri: string): string {
  const parts = uri.split('/');
  return parts
    .map(part => {
      if (part === '') {
        return '';
      }
      if (part.includes(':')) {
        const subparts = part.split(':');
        return subparts
          .map(subpart => {
            if (subpart === '') {
              return '';
            }
            let decoded = subpart;
            try {
              decoded = decodeURIComponent(subpart);
            } catch {
              // Fallback to raw subpart if decoding fails
            }
            validateUriPathSegment('path segment', decoded);
            return encodeWithSlashes(decoded);
          })
          .join(':');
      }
      let decoded = part;
      try {
        decoded = decodeURIComponent(part);
      } catch {
        // Fallback to raw part if decoding fails
      }
      validateUriPathSegment('path segment', decoded);
      return encodeWithSlashes(decoded);
    })
    .join('/');
}

[danieljbruce]

As discussed in other comments, we do the decoding now so I think this has been addressed.

[danieljbruce]

I tried fixing the linting issues with AI, but it caused regressions and the linting issues are mostly unrelated to the pull request anyway. I don't suggest we block this on the presubmit / lint check. If we really want to address the linting issues we should do so in a separate PR.

[feywind]

Nothing terribly bad, just a few suggestion comments.

[feywind]

I agree with you on most of the linter errors, maybe take a look at this one, though, it's in new code and looks straightforward.

[feywind]

Maybe not for this PR, but we might want to consider factoring this out one way or another. (Maybe putting it here and importing it in gax.)

[feywind]

Probably also do this lint fix.

[feywind]

It feels like we ought to keep unit tests next to the things being tested, especially if we end up later refactoring gax on this. Maybe not these specific ones, if they're BQ-specific, but I'm not sure we should delete it.