Skip to content

Fix: Check flatbuffer integrity before parsing#1864

Open
AustinBenoit wants to merge 18 commits into
mainfrom
FixVulns
Open

Fix: Check flatbuffer integrity before parsing#1864
AustinBenoit wants to merge 18 commits into
mainfrom
FixVulns

Conversation

@AustinBenoit

Copy link
Copy Markdown
Contributor

Description

Provide details of the change, and generalize the change in the PR title above.
Fix: Check flatbuffer integrity before parsing
Updated flatbuffer to latest version to get verify buffer Use strol for key parsing to ensure exceptions do not result in a crash.


Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

Integration test in github


Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Flatbuffers dependency to a newer version, removes an obsolete patch file, and improves the robustness of the Remote Config desktop implementation. Specifically, it adds buffer verification before deserializing flexbuffers, enhances file path handling and error checking in the file manager, and replaces std::stoi with safer string-to-integer parsing in the metadata deserialization. The review feedback highlights critical improvements: ensuring robust overflow detection for std::strtol on LLP64 platforms (like Windows) by checking errno, preventing potential undefined behavior from a null package_name(), and explicitly including the necessary and headers.

Comment thread remote_config/src/desktop/metadata.cc
Comment thread remote_config/src/desktop/file_manager.cc Outdated
Comment thread remote_config/src/desktop/metadata.cc
@AustinBenoit AustinBenoit added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label Jun 17, 2026
@github-actions github-actions Bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels Jun 17, 2026
@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown

❌  Integration test FAILED

Requested by @AustinBenoit on commit 818d66e
Last updated: Wed Jun 24 14:12 PDT 2026
View integration test log & download artifacts

Failures Configs
analytics [TEST] [FLAKINESS] [Android] [1/3 os: macos] [1/4 android_device: emulator_ftl_target]
(1 failed tests)  CRASH/TIMEOUT
app_check [TEST] [FLAKINESS] [Android] [1/3 os: ubuntu] [1/4 android_device: emulator_ftl_target]
(1 failed tests)  CRASH/TIMEOUT
auth [TEST] [FLAKINESS] [Android] [1/3 os: ubuntu] [1/4 android_device: android_target]
(1 failed tests)  FirebaseAuthTest.TestLinkAnonymousUserWithEmailCredential
firestore
(4 items)[TEST] [ERROR] [Android] [1/3 os: ubuntu] [1/4 android_device: emulator_ftl_target]
[TEST] [FAILURE] [Android] [1/3 os: ubuntu] [1/4 android_device: emulator_ftl_target]
(1 failed tests)  WriteBatchTest.TestCanWriteTheSameDocumentMultipleTimes
[TEST] [FLAKINESS] [Android] [1/3 os: macos] [1/4 android_device: emulator_ftl_target]
(1 failed tests)  CRASH/TIMEOUT
[TEST] [FLAKINESS] [Android] [1/3 os: ubuntu] [1/4 android_device: emulator_ftl_latest]
(1 failed tests)  AggregateCountTest.TestCanQueryByDocumentId
functions [TEST] [FLAKINESS] [Android] [1/3 os: ubuntu] [1/4 android_device: emulator_ftl_target]
(1 failed tests)  CRASH/TIMEOUT
messaging [TEST] [FLAKINESS] [Android] [1/3 os: windows] [1/4 android_device: android_target]
(1 failed tests)  CRASH/TIMEOUT
remote_config [TEST] [FLAKINESS] [Android] [1/3 os: ubuntu] [1/4 android_device: emulator_ftl_target]
(1 failed tests)  CRASH/TIMEOUT

Add flaky tests to go/fpl-cpp-flake-tracker

@github-actions github-actions Bot added the tests: failed This PR's integration tests failed. label Jun 17, 2026
@firebase-workflow-trigger firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 17, 2026
@AustinBenoit

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates Flatbuffers to support buffer verification, adds integrity checks to prevent crashes on invalid data, and improves path resolution and error handling in Remote Config. The reviewer feedback highlights a critical bug where a boolean return value is incorrectly compared to nullptr, an issue with an unintended leading slash when the package name is empty, and suggests using default cases instead of case flexbuffers::FBT_MAX_TYPE to robustly handle corrupted or unknown flexbuffer types.

Comment thread app/rest/request_json.h
Comment thread remote_config/src/desktop/file_manager.cc Outdated
Comment thread app/src/variant_util.cc
Comment thread database/src/desktop/persistence/flatbuffer_conversions.cc
Comment thread remote_config/src/desktop/remote_config_response.cc
@AustinBenoit

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates Flatbuffers to a newer version, introduces buffer verification checks before deserializing flexbuffers, and hardens Remote Config desktop code with robust error and bounds checking. Feedback on the changes includes addressing a potential absolute path issue when package_name is empty or null, and adding a null pointer check for the configs parameter in RemoteConfigFileManager::Load to prevent crashes.

Comment thread remote_config/src/desktop/file_manager.cc Outdated
Comment thread remote_config/src/desktop/file_manager.cc Outdated
@wiz-9635d3485b

wiz-9635d3485b Bot commented Jun 24, 2026

Copy link
Copy Markdown

Wiz Scan Summary

⚠️ Many findings detected
Many findings were detected, but only a subset of the findings are displayed inline due to API constraints. To view all findings inline, please click here.
Scanner Findings
Vulnerability Finding Vulnerabilities 25 High 37 Medium 3 Low
Data Finding Sensitive Data -
Secret Finding Secrets -
IaC Misconfiguration IaC Misconfigurations -
SAST Finding SAST Findings -
Software Management Finding Software Management Findings -
Total 25 High 37 Medium 3 Low

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@AustinBenoit AustinBenoit added tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). and removed tests: failed This PR's integration tests failed. labels Jun 24, 2026
@github-actions github-actions Bot added tests: in-progress This PR's integration tests are in progress. tests: succeeded This PR's integration tests succeeded. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels Jun 24, 2026
@firebase-workflow-trigger firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 24, 2026
@github-actions github-actions Bot added tests: failed This PR's integration tests failed. and removed tests: succeeded This PR's integration tests succeeded. labels Jun 24, 2026
@AustinBenoit AustinBenoit requested a review from a-maurice June 24, 2026 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests: failed This PR's integration tests failed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant