Repository navigation
feat(sql) Add HTTP Network Transport and Header Resolution - #10523
stephenarosaj wants to merge 2 commits 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 introduces the HTTP transport layer (FirebaseSqlTransport) for Firebase SQL, which handles token resolution, telemetry headers, Cloud Workstation cookie forwarding, and automatic single-shot token refresh/retry on 401 responses. It also refactors header validation and normalization into a dedicated network/headers.ts module. Feedback on the changes suggests several improvements: optimizing validateHeaders by precomputing the lowercased disallowed keys set and using Object.keys to avoid unnecessary allocations; providing an explicit compare function when sorting entries in normalizeHeaders to prevent unexpected array-to-string conversion behavior; passing this.onAuthTokenChanged directly to avoid an unnecessary closure; and defensively checking if response.body.cancel is a function to prevent runtime errors in hybrid environments.
stephenarosaj
changed the title
dconeybe
left a comment
dconeybe
left a comment
There was a problem hiding this comment.
I've left comments written by AI, but all personally vetted and edited by me. I'll come back and do a human review once these comments are addressed or refuted.
| const [authToken, appCheckToken] = await Promise.all([ | ||
| this.authProvider | ||
| ? this.authProvider.getToken({ forceRefresh: false }).catch(() => null) | ||
| : Promise.resolve(null), | ||
| this.appCheckProvider | ||
| ? this.appCheckProvider | ||
| .getToken({ forceRefresh: false }) | ||
| .catch(() => null) | ||
| : Promise.resolve(null) | ||
| ]); |
There was a problem hiding this comment.
[HIGH] Auth getToken() failures are swallowed, silently sending a signed-in user's request unauthenticated. (AI code review)
FirebaseAuthProvider.fetchToken already maps the benign auth/token-not-initialized to null, so this .catch(() => null) only swallows real failures (e.g. network error during refresh, revoked/disabled user). The request then omits x-firebase-auth-token, and a query backed by row-level security runs as anonymous and can succeed with different results (empty or public-only rows), with no signal to the caller. The 401 retry doesn't help, since it requires a non-null authToken. Data Connect's getWithAuth lets these errors through for this reason.
Auth failures should reject (or at minimum be logged); App Check can reasonably stay fail-open. The test "omits token headers and still executes the request when getToken() rejects" would need updating accordingly.
| * Callback invoked when the Firebase Auth token is refreshed or changed. | ||
| */ | ||
| onAuthTokenChanged = (token: string | null): void => { | ||
| logDebug(`Auth Token Changed: ${token}`); |
There was a problem hiding this comment.
[HIGH] The raw Firebase Auth ID token is written to the debug log. (AI code review)
At debug log level this prints a valid bearer JWT (~1h lifetime). Debug logs are routinely pasted into GitHub issues, support tickets and CI output, leaking a credential that can be replayed as that user. Data Connect JS has the same pattern, which is worth fixing too (out of scope for this PR). App Check tokens are bearer credentials too, so any App Check token logging should be redacted the same way.
Log a partially redacted token instead, as the Data Connect Android SDK does in toScrubbedAccessToken(). Keeping a short prefix/suffix lets logs be correlated across token refreshes without exposing the token. It also logs the App Check "placeholder" token (sent when App Check fails to get a real token) unredacted and labeled, which is very helpful when debugging App Check issues. A TypeScript equivalent:
/**
* The App Check "placeholder" token, i.e. base64 of `{"error":"UNKNOWN_ERROR"}`,
* sent when App Check fails to obtain a real token.
* See `formatDummyToken(defaultTokenErrorData)` in `@firebase/app-check`.
*/
const PLACEHOLDER_APP_CHECK_TOKEN = 'eyJlcnJvciI6IlVOS05PV05fRVJST1IifQ==';
/**
* Returns a partially redacted access token, keeping only a short prefix and
* suffix so that tokens can be correlated in logs without exposing them.
*/
export function redactToken(token: string | null): string {
if (token === null) {
return 'null';
}
if (token === PLACEHOLDER_APP_CHECK_TOKEN) {
return `${token} (the "placeholder" App Check token)`;
}
if (token.length < 30) {
return '<redacted>';
}
return `${token.slice(0, 6)}<redacted>${token.slice(-6)}`;
}Usage: logDebug(`Auth Token Changed: ${redactToken(token)}`);
| export class FirebaseSqlTransport implements FirebaseSqlTransportInterface { | ||
| readonly settings?: SqlServiceSettings; | ||
| protected _appId?: string; | ||
| protected _authToken: string | null = null; |
There was a problem hiding this comment.
[LOW] _authToken is written by a listener but never read. (AI code review)
Every request gets its token from authProvider.getToken(), so this field, the listener registered in the constructor (L79), and the mutating onAuthTokenChanged on the public FirebaseSqlTransportInterface appear to be unused, and they add an Auth listener per FirebaseSql instance. Consider removing them until something reads them, or adding a comment noting which upcoming change will use them.
| ): Record<string, string> { | ||
| const headers = new Headers(); | ||
|
|
||
| mergeHeadersInit(headers, init?.headers); |
There was a problem hiding this comment.
[LOW] Reserved headers are only rejected in settings.headers; per-request init.headers can still set them. (AI code review)
validateHeaders only runs on settings.headers, while init.headers is merged as-is here. As a result:
x-firebase-auth-token/x-firebase-appcheckare only overwritten when the SDK has a token; otherwise a caller-supplied value goes out as-is (the transport test using'custom-user-jwt'relies on this).X-Firebase-SqlConnect-Affinity,X-Goog-Request-Params,X-Firebase-Client,X-Firebase-Client-Log-Typeandx-client-versionare never rejected.
go/firebasesql:sdk:headers specifies that setting a "reserved" header MUST result in an exception. Since init.headers will come from postgrest-js, where users can set headers (e.g. .setHeader()), consider applying the same check to init.headers and throwing FirebaseSqlError (Code.INVALID_ARGUMENT), as validateHeaders does for settings.headers. The 'custom-user-jwt' test would then need to expect a rejection.
| export const FirebaseHeaders = Object.freeze({ | ||
| GOOG_API_CLIENT: 'x-goog-api-client' as const satisfies FirebaseHeaders, | ||
| CLIENT_VERSION: 'x-client-version' as const satisfies FirebaseHeaders, | ||
| FIREBASE_GMPDID: 'x-firebase-gmpid' as const satisfies FirebaseHeaders, |
There was a problem hiding this comment.
[MEDIUM] Typo in constant name: FIREBASE_GMPDID → FIREBASE_GMPID. (AI code review)
| /** Lowerecased HTTP Header keys forbidden for FSQL SDK */ | ||
| const FORBIDDEN_HEADERS = new Set<string>([ | ||
| 'X-Firebase-SqlConnect-Affinity', | ||
| 'X-Goog-Request-Params', | ||
| 'X-Firebase-Client', | ||
| 'X-Firebase-Client-Log-Type' | ||
| ]); | ||
|
|
||
| /** @internal */ | ||
| export const LOWERCASE_DISALLOWED_HEADER_KEYS = new Set<string>([ | ||
| ...Object.values(FirebaseHeaders), | ||
| ...Array.from(FORBIDDEN_HEADERS).map(key => key.toLowerCase()) | ||
| ]); |
There was a problem hiding this comment.
[LOW] FORBIDDEN_HEADERS is documented as "Lowerecased" but holds mixed-case values, and LOWERCASE_DISALLOWED_HEADER_KEYS is an exported, mutable Set. (AI code review)
Consider writing the FORBIDDEN_HEADERS entries in lowercase (dropping the .map(key => key.toLowerCase())) so the comment holds and fixing the "Lowerecased" typo. Also consider typing the export as ReadonlySet<string> so importers can't .add() / .delete() and change the protected-header policy.
Also, please sync this list with the authoritative "reserved" headers list in go/firebasesql:sdk:headers and leave a comment here indicating such.
| appCheckToken && | ||
| headers.set(FirebaseHeaders.FIREBASE_APPCHECK, appCheckToken.token); |
There was a problem hiding this comment.
[LOW] Auth and App Check token checks are inconsistent: an empty App Check token string is still sent as a header. (AI code review)
Auth is only attached when authToken.accessToken is truthy, but App Check only checks the result object, so { token: '' } would send an empty x-firebase-appcheck header. Consider checking appCheckToken?.token for consistency. Note that the test "omits token headers when providers return null or empty tokens" doesn't cover this, since MockAppCheckProvider maps '' to null before it reaches resolveRequestHeaders.
| expect(result[FirebaseHeaders.FIREBASE_APPCHECK]).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('overwrites caller-supplied auth and App Check tokens over SDK tokens', () => { |
There was a problem hiding this comment.
[LOW] Test name reads as the opposite of what it asserts. (AI code review)
"overwrites caller-supplied auth and App Check tokens over SDK tokens" suggests the caller's tokens win, but the assertions verify the SDK tokens win. Consider renaming to match the equivalent test in transport.test.ts ("...with SDK tokens").
Add HTTP Network Transport and Header Resolution to Firebase SQL
Overview
✨ Implements
FirebaseSqlTransportand wire-level header resolution for@firebase/sql, handling Firebase Auth and App Check token attachment, telemetry headers, Cloud Workstation emulator credentials, and single-shot HTTP401token force-refresh retries.Highlights
✨ Key changes:
FirebaseSqlTransportto execute requests with concurrent Auth and App Check token resolution.401retry logic that force-refreshes the Firebase Auth token (getToken({ forceRefresh: true })), cancels the unconsumed401response body stream, and retries only when a new access token is obtained.HeadersInitmerging, and wire-level header resolution (x-firebase-auth-token,x-firebase-appcheck,x-goog-api-client,x-firebase-gmpid) intoheaders.ts, populatingDISALLOWED_HEADER_KEYSwith protected Firebase and telemetry headers.FirebaseSqlTransportinstantiation intoFirebaseSqland added unit test suites for header resolution and transport execution.Changelog
Implementation Changes
FirebaseSqlTransportInterface,FirebaseSqlTransportOptions, andFirebaseSqlTransportonAuthTokenChanged, Cloud Workstationcredentials: 'include'forwarding, and single-shot HTTP401force-refresh retryFirebaseHeaders,DISALLOWED_HEADER_KEYS,getGoogApiClientValue,mergeHeadersInit, andresolveRequestHeadersvalidateHeadersandnormalizeHeadersfromsettings.tsvalidateHeadersfromheaders.tsand removed placeholder header helpers_transport(FirebaseSqlTransport) with service settings,appId, and token providersFirebaseSqlinstancesFetchtype alias (typeof fetch)transport.tsfrom the network barrelTest Changes
onAuthTokenChanged,AbortSignaland network error propagation, Cloud Workstation credentials,HeadersInitformat parity, Auth/App Check/telemetry headers, and deduplicated HTTP401force-refresh retriesresolveRequestHeaderscovering custom headers, token attachment, token overwrites, and telemetry header precedenceMockAuthProviderandMockAppCheckProvidertest helpers