Repository navigation
fix(common): preserve DEFAULT_PROJECT_ID_TOKEN in joinURIComponents() - #9315
akiliscodes wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a regression where the DEFAULT_PROJECT_ID_TOKEN was encoded, breaking lazy/ADC project-ID resolution. The changes update joinURIComponents to bypass encoding for this token and add corresponding unit tests. The review feedback points out two critical issues: a missing import of replaceProjectIdToken in the test file, and a potential circular dependency if DEFAULT_PROJECT_ID_TOKEN is imported from service into util. The reviewer suggests moving the token definition to util.ts to resolve the circular dependency cleanly.
ba0c03b to
8e247fb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes a regression where the DEFAULT_PROJECT_ID_TOKEN was being encoded by joinURIComponents, breaking lazy project-ID resolution for ADC clients. The changes bypass encoding for this token and add corresponding unit tests. Feedback on the changes highlights two critical issues: first, referencing DEFAULT_PROJECT_ID_TOKEN in util.ts introduces a circular dependency with service.ts that could cause the token to be undefined at runtime; second, the test file core/common/test/util.ts is missing an import for replaceProjectIdToken, which will result in a ReferenceError during test execution.
| const trimSlashesRegex = /^\/*|\/*$/g; | ||
| const trimmed = uriComponent.replace(trimSlashesRegex, ''); | ||
| const trimmed = uriComponent.replace(/^\/*|\/*$/g, ''); | ||
| if (trimmed === DEFAULT_PROJECT_ID_TOKEN) return trimmed; |
There was a problem hiding this comment.
Using DEFAULT_PROJECT_ID_TOKEN in util.ts introduces a circular dependency between util.ts and service.ts (since service.ts imports util.ts and util.ts imports service.ts). In Node.js, circular dependencies can cause imported constants to be undefined during module initialization. If DEFAULT_PROJECT_ID_TOKEN is undefined when util.ts is evaluated, the comparison trimmed === DEFAULT_PROJECT_ID_TOKEN will silently fail, and the token will be encoded anyway, defeating the fix.
To prevent this, please define DEFAULT_PROJECT_ID_TOKEN in util.ts (or a separate constants file) and export it from there. Then, service.ts can import it from util.ts and re-export it to maintain backward compatibility.
| joinURIComponents, | ||
| } from '../src/util'; |
There was a problem hiding this comment.
The test uses replaceProjectIdToken to verify that the placeholder is correctly substituted, but replaceProjectIdToken is not imported from ../src/util. This will cause a ReferenceError when running the tests. Please add replaceProjectIdToken to the import list.
encodeURIPath encodes '{{projectId}}' as '%7B%7BprojectId%7D%7D',
which prevents replaceProjectIdToken() from finding and substituting
the placeholder later in the request lifecycle (decorateRequest →
makeAuthenticatedRequest).
Skip encoding when the trimmed component equals DEFAULT_PROJECT_ID_TOKEN
so that ADC / lazy project-ID resolution continues to work for clients
that do not supply an explicit projectId (e.g. new BigQuery()).
Regression introduced by googleapis#9188.
Fixes googleapis#9256.
b5ab899 to
66b386b
Compare
This branch has not been deployed
1 participant
Summary
Preserve
DEFAULT_PROJECT_ID_TOKENwhen building request URIs withjoinURIComponents()so lazy project ID resolution can still replace the placeholder before the request is sent.Problem
#9188 hardened
joinURIComponents()against path traversal by encoding path components withencodeURIPath().This also causes the internal
DEFAULT_PROJECT_ID_TOKENplaceholder ({{projectId}}) to be encoded as%7B%7BprojectId%7D%7D.Project ID substitution happens later, when
decorateRequest()callsreplaceProjectIdToken(). Because that function looks for the literal{{projectId}}token, it can no longer recognize the encoded placeholder.As a result, clients relying on lazy project ID resolution can send the encoded placeholder to the backend instead of the resolved project ID, causing the request to fail.
Reported in #9256.
Fix
Update
joinURIComponents()to preserveDEFAULT_PROJECT_ID_TOKENwhen a trimmed path component exactly matches the library-owned token.Only this exact internal constant bypasses encoding. All other path components continue through
encodeURIPath(), preserving the path-traversal protection introduced in #9188.This allows the existing request flow to work as intended:
Testing
Added coverage for both the regression and the existing URI-encoding behavior:
test/util.tsDEFAULT_PROJECT_ID_TOKENsurvivesjoinURIComponents()and can subsequently be replaced byreplaceProjectIdToken(){{../../admin}}do not bypass encoding/path-traversal protectiontest/service.tsService.request_()preserves the literal{{projectId}}placeholder until the request-decoration phase when no explicit project ID is providedChecklist
DEFAULT_PROJECT_ID_TOKENbeforereplaceProjectIdToken()runs #9256Fixes #9256 🦕