fix: validate path parameters and prevent traversal/injection in apiary request encodings

[danieljbruce]

Description

#9166 resolves a vulnerability for Gapic client libraries, but that same vulnerability exists for apiary libraries which this pull request patches. While this change is not on the apiary library, it is a change for the nodejs-googleapis-common package which all the apiary libraries use to do their requests.

Impact

Removes the potential for an exploit on apiary libraries.

Important Notes About Code Changes

1. All the methods in core/packages/nodejs-googleapis-common/src/transcoding.ts except validateAndEncodeParams and extractTemplateParams are exact duplicates of the methods in google-gax. It was important to reuse a lot of this logic so that we only have to scrutinize new code that was necessary due to the differences between apiary and gapic clients.
2. Tests are applied against createAPIRequest to capture the gax equivalent of both * and ** wildcards as well as all special characters.
3. To verify that apiary actually uses this code, apiary library tests are provided in core/packages/nodejs-googleapis-common/test/test.dialogflow.ts that explain what apiary passes into createAPIRequest so that we can understand how the code works end to end.

Next Steps

1. With this pull request we now have duplicate code like applyPattern in gax and common. We should find one place for this code to live and reference that place from both libraries.
2. The pull request removed the normalizePathParams method from this PR to keep it small in 9b02fe8 commit. However, we should add this method in again to improve code quality.
3. Swap "require.cache[commonPath] =" out when the new changes are released for this client library to avoid a hacky mock.

Tracked here

[gemini-code-assist]

Code Review

This pull request introduces path parameter validation and encoding to prevent directory traversal and injection attacks. It adds a new transcoding.ts module with utility functions to validate single-segment and multi-segment path parameters, percent-encode them strictly according to RFC 3986, and integrate this validation into the API request creation flow. Comprehensive tests are added to verify these security enhancements. The review feedback suggests simplifying the applyPattern function in transcoding.ts by removing redundant regular expression compilation, as the wildcard patterns are limited to '*' and '**'.

[gemini-code-assist]

Since extractTemplateParams only ever returns '*' or '**' as the wildcard pattern, the complex regular expression compilation and matching logic in applyPattern is entirely redundant and unreachable.\n\nWe can simplify applyPattern to directly handle '*' and '**' cases. This avoids compiling a new RegExp on every single parameter validation call, significantly improving performance and readability.

function applyPattern(\n  pattern: string,\n  fieldValue: string,\n  propertyName = 'resource',\n): string | undefined {\n  if (pattern === '*') {\n    validateUriPathSegment(propertyName, fieldValue);\n    return encodeWithSlashes(fieldValue);\n  }\n\n  if (pattern === '**') {\n    validateUriPath(propertyName, fieldValue);\n    return encodeWithoutSlashes(fieldValue);\n  }\n\n  return undefined;\n}

[danieljbruce]

Yes. But we also want this method to be an exact match with the gax method because that makes the solution way easier to understand.

[westarle]

A couple opportunities to simplify and hopefully avoid bugs. I notice a few lints, also.

[westarle]

could this be extracted like:

const urlTemplateString = options.url?.toString() ?? parameters.mediaUrl ?? undefined;

[danieljbruce]

I mostly agree with this, but with a few adjustments:

1. We should allow options.url to be used if it is available even if it is not an object that has toString method
2. I definitely think that inlining here instead of creating a urlTemplateString variable is the right approach with the main benefit being that it doesn't create an extra urlTemplateString variable that the reader has to wonder about in the rest of the method. If the intent of the fragment is unclear then maybe just adding a comment is best.

[danieljbruce]

Actually, my point #1 is irrelevant because a url string has a toString method so that will work either way.

[westarle]

I think this covers some advanced templating that I don't think we use in Apiary.

Consider:

function extractTemplateParams(urlTemplate: string): Array<{
  param: string;
  wildcard: '*' | '**';
}> {
  const paramMap = new Map<string, '*' | '**'>();
  
  // Natively skips {}, {#}, {?}, and {,} by demanding valid variable characters
  const matches = urlTemplate.matchAll(/\{(\+?)([a-zA-Z0-9_$-]+)\}/g);

  for (const match of matches) {
    const wildcard = match[1] === '+' ? '**' : '*';
    const paramName = match[2];
    
    if (wildcard === '**' || !paramMap.has(paramName)) {
      paramMap.set(paramName, wildcard);
    }
  }

  return Array.from(paramMap.entries()).map(([param, wildcard]) => ({
    param, wildcard,
  }));
}

[danieljbruce]

Sounds good. I've applied this code to simplify the method.

[westarle]

Can we replace with something like this and delete applyPattern:

const encodeParam = (val: string) => {
        validateUriPath(param, val);
        return encodeWithoutSlashes(val);
      };
      params[param] = Array.isArray(parameterValue)
        ? parameterValue.map(item => encodeParam(String(item)))
        : encodeParam(String(parameterValue));

[danieljbruce]

Yes. That's a good idea, but I've made sure to leave a comment that this achieves what applyPattern from gax achieves because I think pointing out the connection between the two is really important.