[Snyk] Security upgrade react-native-google-places-autocomplete from 2.5.6 to 2.6.4 #82233
Conversation
The following vulnerabilities are fixed with an upgrade: - https://snyk.io/vuln/SNYK-JS-QS-15268416
melvin-bot
bot
requested review from
MarioExpensify
and removed request for
a team
|
@MarioExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
The Snyk auto-fix PR updated package.json to use [email protected] but did not regenerate package-lock.json, causing npm ci to fail in all CI checks.
|
Fixed the failing checks. All CI failures (TypeScript, ESLint, Prettier, Translation Dry Run, Spellcheck, etc.) had the same root cause: The Snyk auto-fix updated
I regenerated |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e95219764
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "react-native-gesture-handler": "2.28.0", | ||
| "react-native-get-random-values": "^1.11.0", | ||
| "react-native-google-places-autocomplete": "2.5.6", | ||
| "react-native-google-places-autocomplete": "2.6.4", |
There was a problem hiding this comment.
This change bumps react-native-google-places-autocomplete in package.json but leaves package-lock.json pinned to 2.5.6 (package-lock.json:112), so the upgrade is not actually reproducible. In this repo, CI setup runs npm ci (.github/actions/composite/setupNode/action.yml:56), and npm ci requires the manifest and lockfile to be in sync (it errors: npm ci can only install packages when your package.json and package-lock.json ... are in sync), which can break installs on cache misses; on cache hits, the cache key is derived from package-lock.json (action.yml:35), so workers can keep reusing node_modules with the old vulnerable version.
Useful? React with 👍 / 👎.
|
If this requires C+ testing, I can help |
The patch was created for v2.5.6 to add React 19 support by removing defaultProps and forwardRef usage. Version 2.6.4 already removes defaultProps and uses destructured defaults, making this patch unnecessary and incompatible with the new version.
|
Fixed the failing checks by removing the obsolete Root cause: The Snyk upgrade from v2.5.6 to v2.6.4 broke Why the patch is no longer needed: The patch added React 19 compatibility by removing What was removed:
|
|
The failing checks on the previous commit ( The latest commit ( No further code changes should be needed. |
…ete v2.6.4 The upgraded react-native-google-places-autocomplete package changed the Point interface to require both lat/lng and latitude/longitude properties. Updated recentWaypointsSelector to provide all four.
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
|
Fixed the failing Root cause: The upgrade of Fix: Added |
|
The failing checks have already been fixed in the latest commit (
The current CI run shows all completed checks passing (typecheck ✅, ESLint ✅, Prettier ✅, spellcheck ✅, validate-patches ✅, Storybook ✅, builds ✅). Tests and remaining builds are still in progress. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| @@ -48,6 +48,8 @@ function recentWaypointsSelector(waypoints: RecentWaypoint[] = []) { | |||
| location: { | |||
| lat: waypoint.lat ?? 0, | |||
There was a problem hiding this comment.
@MelvinBot do we still need the old lat/lng parameters? Or just the new latitude/longitude?
|
Both are needed. The interface Point {
lat: number;
lng: number;
latitude: number;
longitude: number;
}Since TypeScript enforces this, omitting either pair would cause a type error. The PR correctly provides all four ( The rest of the codebase (e.g., |
|
The failing check is Analysis: This check requires a reviewer to post a comment containing the Reviewer Checklist (from Evidence: All other checks pass (ESLint, typecheck, Prettier, tests, builds, verifySignedCommits, etc.). The failure is purely a process check, not a code issue. Resolution: A reviewer needs to post the Reviewer Checklist as a PR comment with all items checked off. This will cause the |
|
@situchan yes, please test, review and fill in the checklist. Thank you! |
| latitude: waypoint.lat ?? 0, | ||
| longitude: waypoint.lng ?? 0, |
There was a problem hiding this comment.
This is to fix lint error. These props were added in this upstream PR. Not used anywhere in our app.
Old lat/lng parameters are still needed.
There was a problem hiding this comment.
I investigated the full GooglePlacesAutocomplete.js source (v2.6.4) for all forward-reference / TDZ issues.
True TDZ violations (cause runtime ReferenceError): There are exactly 3, and the existing patch already fixes all of them:
| Variable | Referenced at | Declared at | Why it crashes | Patch fix |
|---|---|---|---|---|
_request |
Line 161 | Line 466 | useRef(_request) evaluates immediately |
Changed to useRef(null) |
_disableRowLoaders |
Line 450 (dep array) | Line 653 | useCallback dep arrays evaluate immediately |
Moved declaration to ~line 348 |
_disableRowLoaders |
Line 629 (dep array) | Line 653 | Same as above | Same move fixes both |
_renderDescription and getCurrentLocation are NOT true TDZ violations. They are "soft" forward references — referenced only inside function bodies (like _onPress) that execute lazily from event handlers, not during render. By the time a user taps a row and _onPress fires, the entire component body has already executed and all const declarations are initialized.
Full list of soft forward references (safe, no runtime errors):
| Referenced variable | Referenced inside | Ref lines | Declared at |
|---|---|---|---|
_renderDescription |
_onPress body |
696, 704, 762, 768 | 837 |
hideListView |
_onPress body |
670, 758, 767 | 795 |
_onBlur |
_onPress body |
694, 770 | 821 |
debounceData |
_onChangeText body |
782 | 1082 |
These 4 cases reference later-declared const functions, but only inside callback bodies that never run during render — they only run when triggered by user interaction or effects, at which point all declarations are initialized.
Bottom line: The current patch already covers all runtime TDZ crashes. The soft forward references are standard JavaScript and don't need reordering for correctness. That said, if you'd prefer to reorder them for code clarity/consistency, I can do that — just let me know.
There was a problem hiding this comment.
if you'd prefer to reorder them for code clarity/consistency, I can do that — just let me know.
Please do that
There was a problem hiding this comment.
Done. Updated the patch to reorder all forward-referenced declarations so they are declared before use:
Moved before _onPress:
_renderDescription(was in RENDER FUNCTIONS section)hideListView(was after_handleChangeText)isNewFocusInAutocompleteResultList(was afterhideListView)_onBlur(was afterisNewFocusInAutocompleteResultList)_onFocus(was after_onBlur)
Moved before _onChangeText:
debounceData(was in the EFFECTS section)
Commit: 4029453
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
The failing check is not related to the code changes in this PR. Analysis: The How to fix: A reviewer needs to copy the Reviewer Checklist, paste it as a PR comment, and check off all items after reviewing the changes. This is not something that can be resolved with a code push. All other checks (ESLint, typecheck, Prettier, tests, builds, signed commits, etc.) are passing. |
|
@MelvinBot if possible, can you add this change? - function recentWaypointsSelector(waypoints: RecentWaypoint[] = []) {
+ function recentWaypointsSelector(waypoints: RecentWaypoint[] = []): PredefinedPlace[] {Along with |
Co-authored-by: Situ Chandra Shil <[email protected]>
|
Done — pushed 43db18c with both changes:
|
|
@MelvinBot app crashes on this line: installHook.js:1 ReferenceError: Cannot access '_request' before initialization Screen.Recording.2026-02-12.at.6.41.27.PM.mov |
🔍 Crash AnalysisSummary: The app crashes at runtime because Root Cause: In This bug did not exist in v2.5.6, which had no Recommended Fix: Add a const requestRef = useRef(_request);to: const requestRef = useRef(null);This is safe because There is no v2.6.5 or newer available on npm — v2.6.4 is the latest published version, and no upstream fix exists. Detailed InvestigationRoot Cause AnalysisThe v2.6.x refactor (upstream PR #990) reorganized the component code into sections. The // Line 161 - STATE & REFS section
const requestRef = useRef(_request);But // Line 466 - API REQUEST FUNCTIONS section
const _request = (text) => {
_abortRequests();
// ...
};JavaScript Evidence Chain
Why the patch is safeThe // Line 1079 - runs on every render, AFTER _request is defined
requestRef.current = _request;
// Line 1082 - debounced function reads requestRef.current when invoked
const debounceData = useMemo(() => {
return debounce((text) => requestRef.current(text), debounceMs);
}, [debounceMs]);The initial value passed to Relevant Code
Next Steps: A |
|
The failing check is unrelated to this PR's changes. Failing check: Error: Analysis: This is a CI infrastructure issue — the GitHub Actions runner is missing CMake version Evidence:
This likely needs to be resolved by updating the CI runner image to include CMake 3.30.5, or pinning the expected CMake version in the Android build configuration. Please reach out in #expensify-open-source for infrastructure support. |
@MelvinBot let's create patch in this PR |
v2.6.4 has a bug where `useRef(_request)` references `_request` before its const declaration, causing a ReferenceError at runtime. This patch changes the initial value to `null`, which is safe because `requestRef.current` is reassigned to `_request` on every render before it can be invoked. Co-authored-by: Situ Chandra Shil <[email protected]>
|
Created the What the patch does: Changes Why it's safe: Files added:
The patch was tested locally and applies cleanly via |
|
App still crashes for me locally. @MelvinBot can you generate adhoc build for testing? |
|
I'm not able to trigger ad-hoc builds directly, but you can generate one yourself from the Actions tab:
Once the build completes, download links will be posted as a comment on this PR. Regarding the local crash — could you share the error/stack trace? That would help determine whether there's a runtime issue beyond the TypeScript fix that was already applied. |
|
The failing checks have been analyzed:
All other checks (ESLint, Prettier, spellcheck, all 8 test jobs, all app builds, validate-patches, verifySignedCommits) are passing. |
|
@MelvinBot your patch still doesn't fix the crash. Can you investigate more? |
|
@MarioExpensify can you please generate adhoc build? |
🔍 Issue AnalysisSummary: The Root Cause: The v2.6.4 rewrite introduced several Temporal Dead Zone (TDZ) issues and a new Recommended Fix: The v2.6.4 library needs a more comprehensive patch — or the upgrade should be reconsidered, since the library appears semi-abandoned (upstream issue #978). Issues FoundIssue 1 (Patched): Issue 2 (NOT Patched):
Babel's Issue 3: Diagnostic Steps@situchan — could you share the crash stack trace? That would confirm which of these issues is the actual cause. In the meantime:
RecommendationGiven that v2.6.4 has multiple issues and the library is semi-abandoned, I'd suggest one of: Option A: Expand the patch to fix all identified issues (TDZ for Option B: Pin to a safer version — if the Snyk vulnerability in v2.5.6 is in Detailed InvestigationRoot Cause AnalysisThe v2.6.4 rewrite of The TDZ flow for
With Babel's block-scoping transform, The uuid dependency chain:
Evidence Chain
Relevant Code
Next Steps: Please share the crash stack trace so we can confirm which issue is the culprit. Reply with |
|
@MelvinBot here's crash log: installHook.js:1 ReferenceError: Cannot access '_disableRowLoaders' before initialization |
🔍 Crash Analysis:
|
|
@MelvinBot can we move |
|
Yes, moving
This is safe because
I'll update the patch to include this move. |
The _disableRowLoaders useCallback was declared on line 653 but referenced in the dependency arrays of _requestNearby (line 450) and getCurrentLocation (line 629), causing a ReferenceError at render time. Move the definition above _requestNearby so it is in scope when the dependency arrays are evaluated. Co-authored-by: Situ Chandra Shil <[email protected]>
|
Updated the patch to also fix the Root cause: Fix: Moved The patch now fixes both TDZ crashes:
|
|
@MelvinBot there are a few more functions that need to be moved to the top: |
|
@situchan do you want me to generate the adhoc or wait for the bot to finish these changes? |
waiting for bot to finish the changes |
|
It seems the bot is stuck 😅 |
|
@MelvinBot implement the changes requested by @situchan in their review comment here. |
… patch Move _renderDescription, hideListView, isNewFocusInAutocompleteResultList, _onBlur, and _onFocus before _onPress so they are declared before use. Move debounceData before _onChangeText for the same reason. Co-authored-by: Mario Mazzotti <[email protected]>
4 participants
Explanation of Change
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari