Remove isomorphic-fetch dependency #3782

hsubox76 · 2020-09-15T20:27:04Z

The isomorphic-fetch dependency has not been updated in years and depends on an older version of node-fetch which has now been discovered to have security issues. Removing isomorphic-fetch and consuming node-fetch directly.

I tried another approach that involved passing the node-fetch implementation directly into the functions service, but it involved passing through many layers, and since it is passed into the Service constructor, the createTestService test util function needs to detect the environment and pass the correct fetch implementation as well which seems like a lot of places. This seems like the simplest approach and only touches the index.node.ts file.

Fixes #3768

changeset-bot · 2020-09-15T20:27:07Z

💥 No Changeset

Latest commit: fe963f8

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

google-oss-bot · 2020-09-15T21:01:54Z

Binary Size Report

Affected SDKs

@firebase/database

Type Base (f900417) Head (8e4bcda) Diff

browser 270 kB 270 kB +567 B (+0.2%)

esm2017 236 kB 236 kB +509 B (+0.2%)

main 270 kB 271 kB +344 B (+0.1%)

module 268 kB 269 kB +565 B (+0.2%)

`@firebase/firestore/exp`

Type	Base (`f900417`)	Head (`8e4bcda`)	Diff
browser	189 kB	189 kB	-122 B (-0.1%)
main	478 kB	478 kB	-7 B (-0.0%)
module	189 kB	189 kB	-122 B (-0.1%)
react-native	189 kB	189 kB	-122 B (-0.1%)

`@firebase/firestore/lite`

Type	Base (`f900417`)	Head (`8e4bcda`)	Diff
browser	64.0 kB	63.8 kB	-115 B (-0.2%)
main	141 kB	141 kB	-8 B (-0.0%)
module	64.0 kB	63.8 kB	-115 B (-0.2%)
react-native	64.2 kB	64.1 kB	-115 B (-0.2%)

`@firebase/functions`

Type	Base (`f900417`)	Head (`8e4bcda`)	Diff
browser	9.73 kB	9.77 kB	+40 B (+0.4%)
esm2017	7.31 kB	7.35 kB	+40 B (+0.5%)
main	9.77 kB	9.89 kB	+128 B (+1.3%)
module	9.46 kB	9.50 kB	+40 B (+0.4%)

@firebase/performance-exp

Type Base (f900417) Head (8e4bcda) Diff

browser ? 27.3 kB ? (?)

esm2017 ? 25.5 kB ? (?)

main ? 27.3 kB ? (?)

module ? 27.2 kB ? (?)
firebase

Type Base (f900417) Head (8e4bcda) Diff

firebase-database.js 187 kB 190 kB +3.31 kB (+1.8%)

firebase-functions.js 9.93 kB 9.94 kB +8 B (+0.1%)

firebase.js 830 kB 830 kB +353 B (+0.0%)

Test Logs

Base (f9004177): https://github.com/firebase/firebase-js-sdk/actions/runs/256442017
Head (8e4bcda6): https://github.com/firebase/firebase-js-sdk/actions/runs/261575237

google-oss-bot · 2020-09-15T21:02:07Z

Size Analysis Report

Affected Products

`@firebase/functions-exp`

getFunctions

Size

Type Base (f900417) Head (0182171) Diff

size 1.83 kB 1.86 kB +27 B (+1.5%)

size_with_ext_deps 5.71 kB 5.74 kB +27 B (+0.5%)

Dependencies

Type Base (f900417) Head (0182171) Diff

variables DEFAULT_REGION
FUNCTIONS_TYPE
factory
name
version DEFAULT_REGION
FUNCTIONS_TYPE
name
version - factory

`httpsCallable`

Size

Type	Base (`f900417`)	Head (`0182171`)	Diff
size	5.20 kB	5.21 kB	+7 B (+0.1%)
size_with_ext_deps	9.79 kB	9.80 kB	+5 B (+0.1%)

Dependencies

Type	Base (`f900417`)	Head (`0182171`)	Diff
variables	Click to show 8 deps `DEFAULT_REGION` `FUNCTIONS_TYPE` `LONG_TYPE` `UNSIGNED_LONG_TYPE` `errorCodeMap` `factory` `name` `version`	Click to show 7 deps `DEFAULT_REGION` `FUNCTIONS_TYPE` `LONG_TYPE` `UNSIGNED_LONG_TYPE` `errorCodeMap` `name` `version`	`- factory`

useFunctionsEmulator

Size

Type Base (f900417) Head (0182171) Diff

size 1.80 kB 1.82 kB +27 B (+1.5%)

size_with_ext_deps 5.65 kB 5.67 kB +27 B (+0.5%)

Dependencies

Type Base (f900417) Head (0182171) Diff

variables DEFAULT_REGION
FUNCTIONS_TYPE
factory
name
version DEFAULT_REGION
FUNCTIONS_TYPE
name
version - factory

`@firebase/performance-exp`

`registerPerformance`

Size

Type	Base (`f900417`)	Head (`0182171`)	Diff
size	?	16.4 kB	? (?)
size_with_ext_deps	?	44.4 kB	? (?)

Dependencies

Type	Base (`f900417`)	Head (`0182171`)	Diff
functions	?	Click to show 46 deps `addToQueue` `changeInitializationStatus` `configValid` `convertMetricValueToInteger` `createNetworkRequestEntry` `createUserTimingTrace` `dispatchQueueEvents` `getApplicationInfo` `getAuthTokenPromise` `getConfig` `getDocumentReadyComplete` `getEffectiveConnectionType` `getIid` `getIidPromise` `getInitializationPromise` `getRemoteConfig` `getServiceWorkerStatus` `getStoredConfig` `getVisibilityState` `initializePerf` `isPerfInitialized` `isValidCustomAttributeName` `isValidCustomAttributeValue` `isValidMetricName` `logNetworkRequest` `logTrace` `mergeStrings` `postToFlEndpoint` `processConfig` `processQueue` `registerPerformance` `sendEventsToFl` `sendLog` `sendTraceLog` `serializeNetworkRequest` `serializeTrace` `serializer` `setupApi` `setupNetworkRequests` `setupOobResources` `setupOobTraces` `setupTransportService` `setupUserTimingTraces` `shouldLogAfterSampling` `storeConfig` `transportHandler`	?
classes	?	`Api` `PerformanceController` `SettingsService` `Trace`	?
variables	?	Click to show 44 deps `ATTRIBUTE_FORMAT_REGEX` `CONFIG_EXPIRY_LOCAL_STORAGE_KEY` `CONFIG_LOCAL_STORAGE_KEY` `COULD_NOT_GET_CONFIG_MSG` `DEFAULT_CONFIGS` `DEFAULT_ENTRY_NAME` `DEFAULT_REMAINING_TRIES` `DEFAULT_SEND_INTERVAL_MS` `ERROR_DESCRIPTION_MAP` `ERROR_FACTORY` `FID_WAIT_TIME_MS` `FIRST_CONTENTFUL_PAINT_COUNTER_NAME` `FIRST_INPUT_DELAY_COUNTER_NAME` `FIRST_PAINT_COUNTER_NAME` `FIS_AUTH_PREFIX` `INITIAL_SEND_TIME_DELAY_MS` `MAX_ATTRIBUTE_NAME_LENGTH` `MAX_ATTRIBUTE_VALUE_LENGTH` `MAX_METRIC_NAME_LENGTH` `OOB_TRACE_PAGE_LOAD_PREFIX` `REMOTE_CONFIG_SDK_VERSION` `RESERVED_ATTRIBUTE_PREFIXES` `RESERVED_AUTO_PREFIX` `SDK_VERSION` `SERVICE` `SERVICE_NAME` `TRACE_MEASURE_PREFIX` `TRACE_START_MARK_PREFIX` `TRACE_STOP_MARK_PREFIX` `VisibilityState` `apiInstance` `consoleLogger` `iid` `initializationPromise` `initializationStatus` `isTransportSetup` `logger` `name` `oobMetrics` `queue` `remainingTries` `settingsServiceInstance` `version` `windowInstance`	?

External Dependencies

Module	Base (`f900417`)	Head (`0182171`)	Diff
`@firebase/logger`	?	`LogLevel` `Logger`	?
`@firebase/util`	?	`ErrorFactory` `isIndexedDBAvailable` `validateIndexedDBOpenable`	?
`@firebase/app`	?	`default export`	?
`@firebase/component`	?	`Component`	?

Test Logs

Base (f9004177): https://github.com/firebase/firebase-js-sdk/actions/runs/256442017
Head (01821716): https://github.com/firebase/firebase-js-sdk/actions/runs/257994294

Feiyang1 · 2020-09-16T16:24:37Z

packages-exp/functions-exp/src/index.node.ts

@@ -16,10 +16,22 @@
 */
 import { registerVersion } from '@firebase/app-exp';
 import { registerFunctions } from './config';
-import 'isomorphic-fetch';
+import 'whatwg-fetch';


Instead of including a new polyfill, we can replace Header with a string map. I prefer to not include a new polyfill that only provides Header and given that the workaround is really simple.

Fixed (it is a string-keyed object, fetch signature does not accept Maps).

Feiyang1 · 2020-09-16T16:44:48Z

packages-exp/functions-exp/src/index.node.ts

+ * https://github.com/apollographql/apollo-link/issues/513#issuecomment-548219023
+ */
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+(global as any).fetch = (nodeFetch as unknown) as typeof fetch;


I'm worried about introducing hard to debug issues in developers' app by patching globals. For example, they might use a different fetch polyfill which can be overwritten by our node-fetch polyfill depending on the import order. node-fetch might have a bug that the other polyfill doesn't have. It will be really hard for developers to track down the issue.

Though the chance of this kind of bug is slim, but it's not impossible, I think it's a good practice to not patch globals to future proof the SDK and developers' apps.

Fixed, it is passed in now.

Feiyang1 · 2020-09-18T01:34:46Z

packages-exp/functions-exp/src/config.ts

-};
-
-export function registerFunctions(): void {
+export function registerFunctions(fetchImpl: typeof fetch): void {
  const namespaceExports = {


Didn't notice it before, but exp packages don't use namespace exports, so it can be removed.

joandvgv · 2020-09-24T03:38:57Z

Hi! sorry to bother. Any ideas on when this one will be released? Thanks!

hsubox76 · 2020-09-24T15:42:01Z

It should be in this week's release, which will likely go out today sometime. I'm not sure what issue you are hoping to fix but it shouldn't affect functionality, it just fixes a potential vulnerability and will avoid an ugly warning message on npm install.

joandvgv · 2020-09-24T17:32:31Z

Thanks for the answer! Yeah I had the same vulnerability and had to use node-fetch as well but it still remains for this package, I followed the issues and discussions related to the vuln so just wanted to know when it'll be merged as my boss is always aiming for the 0 vulnerabilities when doing npm i or npm audit lol

inakie · 2020-10-04T16:31:11Z

Hi,

I'm trying to build a website with Gridsome, and the build process gives me the following error, which I believe is related with this commit:

ReferenceError: fetch is not defined
    at Object.iTTW (node_modules/@firebase/functions/dist/index.esm.js:652:26)

If I edit the file node_modules/@firebase/functions/dist/index.esm.js and add at the beginning of the file const fetch = require("node-fetch"); everything works well.

But I don't want to edit that file in node_modules... do you know if there is another way to solve this problem?

Thanks!

hsubox76 · 2020-10-05T18:17:19Z

See issue #3890

Just released a fix in 7.22.1

hsubox76 added 3 commits September 15, 2020 10:15

Remove isomorphic-fetch from functions

2df6538

Change to global, globalThis needs Node 12+

532d163

Update comment

bd58a1f

hsubox76 requested review from Feiyang1 and hiranya911 as code owners September 15, 2020 20:27

Feiyang1 reviewed Sep 16, 2020

View reviewed changes

Pass fetch implementation to functions service

fa623e0

Feiyang1 reviewed Sep 18, 2020

View reviewed changes

Remove namespace exports from functions-exp

fe963f8

Feiyang1 approved these changes Sep 19, 2020

View reviewed changes

hsubox76 merged commit 4c4b2d1 into master Sep 21, 2020

This was referenced Sep 28, 2020

Callable functions internal error #3849

Closed

Functions: Bind passed fetch function #3853

Merged

google-oss-bot mentioned this pull request Oct 5, 2020

Version Packages #3895

Merged

firebase locked and limited conversation to collaborators Oct 22, 2020

hsubox76 deleted the ch-fetchfix branch October 22, 2020 17:44

Remove isomorphic-fetch dependency #3782

Remove isomorphic-fetch dependency #3782

Uh oh!

Conversation

hsubox76 commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💥 No Changeset

Uh oh!

google-oss-bot commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

@firebase/database

@firebase/firestore/exp

@firebase/firestore/lite

@firebase/functions

@firebase/performance-exp

firebase

Test Logs

Uh oh!

google-oss-bot commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Analysis Report

Affected Products

@firebase/functions-exp

getFunctions

Size

Dependencies

httpsCallable

Size

Dependencies

useFunctionsEmulator

Size

Dependencies

@firebase/performance-exp

registerPerformance

Size

Dependencies

External Dependencies

Test Logs

Uh oh!

Feiyang1 Sep 16, 2020

Choose a reason for hiding this comment

Uh oh!

hsubox76 Sep 16, 2020

Choose a reason for hiding this comment

Uh oh!

Feiyang1 Sep 16, 2020

Choose a reason for hiding this comment

Uh oh!

hsubox76 Sep 16, 2020

Choose a reason for hiding this comment

Uh oh!

Feiyang1 Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

hsubox76 Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

joandvgv commented Sep 24, 2020

Uh oh!

hsubox76 commented Sep 24, 2020

Uh oh!

joandvgv commented Sep 24, 2020

Uh oh!

inakie commented Oct 4, 2020

Uh oh!

hsubox76 commented Oct 5, 2020

Uh oh!

Uh oh!

hsubox76 commented Sep 15, 2020 •

edited

Loading

changeset-bot bot commented Sep 15, 2020 •

edited

Loading

google-oss-bot commented Sep 15, 2020 •

edited

Loading

`@firebase/database`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/functions`

`@firebase/performance-exp`

`firebase`

google-oss-bot commented Sep 15, 2020 •

edited

Loading

`@firebase/functions-exp`

`getFunctions`

`httpsCallable`

`useFunctionsEmulator`

`@firebase/performance-exp`

`registerPerformance`