Conversation
There was a problem hiding this comment.
Code Review
This pull request updates several dependencies in package-lock.json and adds a uuid override to ^11.1.1 in package.json. However, several issues were identified: overriding uuid to v11 (an ESM-only version) will cause runtime ERR_REQUIRE_ESM errors in CommonJS-based transitive dependencies like @google-cloud/bigquery; gaxios was unexpectedly downgraded from 6.7.1 to 6.3.0; and the engine requirement for brace-expansion was updated to drop Node 18 support, which may conflict with other project components targeting Node 18.
| "inquirer": "^12.0.0", | ||
| "uuid": "^11.1.1" |
There was a problem hiding this comment.
Overriding uuid to ^11.1.1 will cause critical runtime failures. uuid v11 is an ESM-only package and does not support synchronous require() calls. Since @google-cloud/bigquery, google-gax, and other transitive dependencies are compiled as CommonJS and load uuid via require(), this override will trigger ERR_REQUIRE_ESM errors at runtime when these libraries are imported.
To resolve the vulnerability safely without breaking CommonJS compatibility, you should remove this override and instead use a version of uuid that still supports CommonJS (such as uuid v9.x or v10.x if compatible, or check if a patch release in the v9/v10 line is available), or wait for upstream dependencies to update their uuid usage.
"inquirer": "^12.0.0"There was a problem hiding this comment.
uuid 11 is not ESM-only. Its exports map has a require condition that points to ./dist/cjs/index.js, so CommonJS consumers still load it synchronously. I checked that require('uuid').v4 resolves to a function under 11.1.1 in this tree. uuid.v4() is the only API that bigquery, google-gax, teeny-request and gaxios call. The build passes and the unit tests pass. Keeping the override.
| "version": "6.7.1", | ||
| "resolved": "https://registry.npmjs.org/gaxios/-/gaxios-6.7.1.tgz", | ||
| "integrity": "sha512-LDODD4TMYx7XXdpwxAVRAIAuB0bzv0s+ywFonY46k126qzQHT9ygyoa9tncmOiQmmDrik65UYsEkv3lbfqQ3yQ==", | ||
| "version": "6.3.0", |
There was a problem hiding this comment.
The version of gaxios was downgraded from 6.7.1 to 6.3.0. This downgrade is likely an unintended side effect of the uuid override or the npm audit fix resolution. Downgrading production dependencies by multiple minor versions is risky as it can re-introduce resolved bugs or security vulnerabilities. Once the uuid override is addressed, please ensure gaxios is restored to its latest compatible version (e.g., 6.7.1).
There was a problem hiding this comment.
Good catch. npm audit fix picked 6.3.0 because it is the last gaxios release that doesn't depend on uuid. With the uuid override in place, 6.7.1 is fine, so I restored it in 3c2dfa5. It was the only downgrade in the lockfile. Audit is unchanged (2 moderate, dev-only).
| }, | ||
| "engines": { | ||
| "node": "18 || 20 || >=22" | ||
| "node": "20 || >=22" |
There was a problem hiding this comment.
The engine requirement for brace-expansion has been updated to "node": "20 || >=22", dropping support for Node 18. Since other parts of the project (such as the change-tracker library) still target "node": ">=18", this could cause installation warnings or compatibility issues for developers or CI environments running on Node 18.
There was a problem hiding this comment.
Leaving this as is. The move from brace-expansion 5.0.6 to 5.0.12 is what fixes the high-severity advisories, and it is a dev-only dependency that doesn't ship with the extension. The functions run on nodejs22 (extension.yaml), and the change-tracker has its own lockfile, so this change doesn't affect it. Node 18 is also past end-of-life.
Summary
Fixes 20 of the 22
npm auditissues infirestore-bigquery-export/functions.Changes
npm audit fix(semver-compatible updates only). This fixeswebsocket-driver(critical);brace-expansion,browserslistandjs-yaml(high); andprotobufjs,qs,express,body-parser,gaxiosandbaseline-browser-mapping(moderate).uuid: ^11.1.1override.@google-cloud/bigquery,google-gax,teeny-requestandgaxiospull inuuid9, which has a missing buffer bounds check (fixed in 11.1.1). They only callv4()through the CommonJS API, whichuuid11 still provides. This clears the transitive warnings onfirebase-admin,@google-cloud/*and the change-tracker.uuid(9 → 11), no top-level package in the lockfile changes major version.firebase-adminstays on 13 andfirebase-functionson 6.@firebaseextensions/firestore-bigquery-change-trackerat 2.0.4.npm audit fixhad also moved it to 2.2.1, which changes behaviour and is handled separately inchore/fbe-bump-change-tracker. Pinning it doesn't change the audit result.Remaining (2 moderate, dev-only)
firebase-functions-test3.5.0 (latest) depends onts-deepmerge^2, which has a prototype-method-override DoS (fixed in 8.0.0). v8 drops the default export thatfirebase-functions-testcalls, so overriding it would break the test helper. This needs an upstream fix, and it doesn't ship with the extension.Testing
Unit tests
npm run buildpasses.jest: 54/55 pass. The one failure ise2e.test.ts, which needs a real BigQuery table. It fails the same way onnext.Deployed comparison (
corie-testing)Deployed
functionsfromnext(before) and from this branch (after) side by side asfsexportbigquery(2nd gen) +syncBigQuery(task queue). Both use Node.js 22, the default params,LOG_LEVEL=debugand the task queue config fromextension.yaml. Each copy has its own collection and dataset, and both ran the same test script at the same time. Local-source extension installs aren't available on the project, so the functions were deployed as plain Cloud Functions with the extension's environment variables. BigQuery setup (initialize()) was run by hand, the same for both.next)dataJSON written to BigQuerydata+old_data)_raw_latestviewsyncBigQueryhandler via Cloud Tasks enqueuefsexportbigqueryenqueues →syncBigQueryretries until the table is backERR_REQUIRE_ESM/ module-load / startup errors since deployTimings are how long each row took to show up in BigQuery. They're single samples, and the differences are polling noise.
T8 behaved the same way on both: attempt 1 failed with a 404, attempts 2–3 failed with
Firestore has already been initialized(from change-tracker 2.0.4, not this change), and attempt 4 succeeded.bytesfields are dropped from the serialized data on both, which is existing behaviour.