Skip to content

[Heartbeat Service] More null service protections added to triggerHeartBeat #7789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Nov 22, 2023
Merged

[Heartbeat Service] More null service protections added to triggerHeartBeat #7789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f3c14ce
further heatbeat service dection in triggerHeartbeat
DellaBitta Nov 16, 2023
a778f90
ensure heartbeatservie read returns an object with a heartbeats field
DellaBitta Nov 20, 2023
1d3cdb7
Added a changeset
DellaBitta Nov 20, 2023
f51db9d
lint fix
DellaBitta Nov 20, 2023
aa6822c
Merge branch 'master' into ddb-heartbeatcache-checks
DellaBitta Nov 20, 2023
529ccc9
remove fetchSignInMethodsForEmail returns the correct values
DellaBitta Nov 20, 2023
85039c2
put the fetchSignInMethodsForEmail test back
DellaBitta Nov 20, 2023
23fa7f1
revert node memory limit
DellaBitta Nov 20, 2023
be3d9cf
revert Node memory set
DellaBitta Nov 20, 2023
a96b6ae
execute full test on Auth instead of the changed target
DellaBitta Nov 20, 2023
c43d250
revert CI testing change
DellaBitta Nov 22, 2023
75e3a35
Merge branch 'master' into ddb-heartbeatcache-checks
DellaBitta Nov 22, 2023
e2a206a
Merge branch 'master' into ddb-heartbeatcache-checks
DellaBitta Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fast-moose-approve.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@firebase/app': patch
---

More safeguards to ensure that heartbeat objects queried from IndexedDB include a heartbeats field.
10 changes: 9 additions & 1 deletion packages/app/src/heartbeatService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ export class HeartbeatServiceImpl implements HeartbeatService {
const date = getUTCDateString();
if (this._heartbeatsCache?.heartbeats == null) {
this._heartbeatsCache = await this._heartbeatsCachePromise;
// If we failed to construct a heartbeats cache, then return immediately.
if (this._heartbeatsCache?.heartbeats == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hsubox76 perhaps this should just check if heartbeatsCache === null?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the root problem is here:

firebase-js-sdk/packages/app/src/heartbeatService.ts

Line 239 in b782bb2

return idbHeartbeatObject || { heartbeats: [] };
or even one function back (readHeartbeatsFromIndexedDB). It's checking to see if whatever comes back from indexedDB is falsy but if it's (as I suspect) an empty object, this will go wrong. We have to make sure that whatever gets returned from this read() function is an object with a heartbeats property which is an array (e.g., matches type HeartbeatsInIndexedDB), otherwise we are lying to typescript and so it can't protect us by catching errors further downstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I updated the read() method (below) to ensure the returned object has a heartbeats field.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this check should be needed anymore after the other change but I guess it doesn't hurt to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I feel like keeping it place just in case.

return;
}
}
// Do not store a heartbeat if one is already stored for this day
// or if a header has already been sent today.
Expand Down Expand Up @@ -236,7 +240,11 @@ export class HeartbeatStorageImpl implements HeartbeatStorage {
return { heartbeats: [] };
} else {
const idbHeartbeatObject = await readHeartbeatsFromIndexedDB(this.app);
return idbHeartbeatObject || { heartbeats: [] };
if (idbHeartbeatObject?.heartbeats) {
return idbHeartbeatObject;
} else {
return { heartbeats: [] };
}
}
}
// overwrite the storage with the provided heartbeats
Expand Down