Skip to content

Add Recaptcha App Check Provider stubs#1860

Open
a-maurice wants to merge 2 commits into
mainfrom
am-recaptcha_stubs
Open

Add Recaptcha App Check Provider stubs#1860
a-maurice wants to merge 2 commits into
mainfrom
am-recaptcha_stubs

Conversation

@a-maurice
Copy link
Copy Markdown
Contributor

Description

Provide details of the change, and generalize the change in the PR title above.

Add stub implementations for a new built-in provider, Recaptcha Enterprise. This is stubbed on all platforms, and will eventually be implemented for both Android and iOS.


Testing

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


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.

Copy link
Copy Markdown
Contributor

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

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 introduces the RecaptchaEnterpriseProviderFactory for App Check, adding the header and stub implementation files and updating CMakeLists.txt. The review feedback highlights a critical path error in CMakeLists.txt where the header path is missing the 'app_check/' subdirectory, and recommends initializing the 'internal_' member pointer to nullptr in the constructor to prevent undefined behavior.

Comment thread app_check/CMakeLists.txt Outdated
src/include/firebase/app_check/play_integrity_provider.h
src/include/firebase/app_check/device_check_provider.h
src/include/firebase/app_check/app_attest_provider.h
src/include/firebase/recaptcha_enterprise_provider.h
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.

high

The path to the header file is missing the app_check/ subdirectory. It should be src/include/firebase/app_check/recaptcha_enterprise_provider.h to match the actual file location and avoid build failures.

  src/include/firebase/app_check/recaptcha_enterprise_provider.h

return nullptr;
}

RecaptchaEnterpriseProviderFactory::RecaptchaEnterpriseProviderFactory() {}
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.

medium

The member variable internal_ is left uninitialized in the constructor. It is best practice to initialize member pointers to nullptr to prevent undefined behavior or garbage values.

Suggested change
RecaptchaEnterpriseProviderFactory::RecaptchaEnterpriseProviderFactory() {}
RecaptchaEnterpriseProviderFactory::RecaptchaEnterpriseProviderFactory() : internal_(nullptr) {}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant