feat: appcheck ios #1369
feat: appcheck ios #1369
Conversation
|
Warning Rate limit exceeded@technoplato has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughThis pull request introduces comprehensive changes to enhance the application's security and configuration, primarily focusing on integrating Firebase App Check across multiple platforms. The modifications span various files, including configuration settings, build scripts, and utility functions. Key updates include adding Firebase App Check support, updating iOS project configurations, modifying build processes, and implementing new utility functions for token management and environment-specific setups. Changes
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
scripts/build/ios/preview.js (1)
Line range hint
32-141: Consider extracting common configuration logicThe script shares significant code with production.js. Consider:
- Creating a shared configuration utility
- Implementing a configuration factory pattern
- Using a common base configuration with environment-specific overrides
+ // shared-config.js + const createConfig = (env) => { + const base = { + bundleId: `com.converse.${env}`, + displayName: env === 'preview' ? 'Converse PREVIEW' : 'Converse', + urlScheme: env === 'preview' ? 'converse-preview' : 'converse', + domain: env === 'preview' ? 'preview.converse.xyz' : 'converse.xyz' + }; + return base; + }; + // preview.js + const config = createConfig('preview'); const go = async () => { copyGoogleServiceInfo("preview"); // Use config object instead of string literals // ... };
🧹 Nitpick comments (15)
scripts/build/build.utils.js (1)
15-25: Consider improving error handling and file search specificityThe function could be enhanced in the following ways:
- Use a more specific path pattern to avoid unintended matches
- Add more detailed error logging to help with debugging
const findGoogleServiceFiles = () => { try { - const result = execSync('find ios -name "GoogleService-Info*.plist"', { + const result = execSync('find ios/Google\\ Services -name "GoogleService-Info-*.plist"', { encoding: "utf8", }); return result.split("\n").filter(Boolean); } catch (error) { - console.warn(`Failed to find GoogleService-Info files: ${error.message}`); + console.warn( + `Failed to find GoogleService-Info files: ${error.message}\n` + + `Command output: ${error.stdout}\n` + + `Command stderr: ${error.stderr}` + ); return []; } };scripts/build/build.js (2)
192-193: Improve readability of Android build file extension logicThe nested ternary operator makes the code harder to read and maintain.
Consider extracting this logic into a more readable format:
- ? "aab" - : "apk"; + ? "aab" // Android App Bundle for production releases + : "apk"; // APK for development buildsOr better yet, consider this more maintainable approach:
const getAndroidFileExtension = (env) => { const extensions = { production: 'aab', // Android App Bundle for production releases preview: 'aab', // Android App Bundle for preview releases development: 'apk' // APK for development builds }; return extensions[env] || 'apk'; };
Line range hint
1-276: Consider architectural improvements for better maintainabilityThe build script handles multiple responsibilities and could benefit from better separation of concerns.
Consider these architectural improvements:
Split into smaller modules:
- Platform-specific build configuration
- Environment management
- Cleanup operations
- Build submission logic
Create a BuildConfig class to encapsulate build settings:
class BuildConfig { constructor(platform, env, isLocal, isAdvanced) { this.platform = platform; this.env = env; this.isLocal = isLocal; this.isAdvanced = isAdvanced; } getBuildArgs() { // Extract build args logic here } getFileExtension() { // Extract file extension logic here } }
- Create a dedicated cleanup manager:
class CleanupManager { constructor(platform, env) { this.platform = platform; this.env = env; } register(cleanupFn) { // Register cleanup tasks } async cleanup() { // Execute cleanup tasks } }This would make the code more maintainable, testable, and easier to extend.
utils/api.ts (1)
214-218: Simplify the error message for clarityThe error message thrown when
appCheckTokenis unavailable is multi-line and may contain unnecessary line breaks. Consider simplifying it for better readability.Apply this diff to improve the error message:
-if (!appCheckToken) { - throw new Error(` -No App Check Token Available. This indicates that we believe the app is not running on an authentic build of -our application on a device that has not been tampered with. -`); +if (!appCheckToken) { + throw new Error("No App Check Token Available. This may indicate that the app is not running on an authentic build or the device has been tampered with."); +}utils/appCheck.ts (3)
11-15: Remove commented-out code and TODOsThere are commented-out lines and questions within the code. It's best practice to remove such comments or convert them into actionable issues to maintain code cleanliness.
Apply this diff to clean up the comments:
-// App Check FAQ: -// Do we need/want to use the limited use token? - -// What endpoints are protected with app check? -// @see https://github.com/ephemeraHQ/converse-backend/blob/main/api/middlewares.ts#L27If these comments contain important information, consider documenting them appropriately.
30-31: Address the TODO regarding the App Check debug tokenThere's a TODO comment indicating the need to get the key and add it to the environment configuration. This is crucial for proper App Check functionality in production.
Would you like assistance in implementing this, or should we open a new GitHub issue to track this task?
20-21: Handle errors more specificallyCurrently, any error during token retrieval logs a generic error message. Consider handling specific error cases to provide more informative logging and error handling.
For example:
} catch (error) { - logger.error("Error getting token", error); + if (error instanceof FirebaseAppCheckError) { + logger.error("Firebase App Check error:", error.message); + } else { + logger.error("Unexpected error getting App Check token:", error); + } return undefined; }utils/mmkv.ts (1)
19-24: Add error handling to auth storage operationsWhile the memory leak prevention is good, the storage operations should handle potential errors.
Consider wrapping operations in try-catch:
set(name: AuthStorageKey, value: string) { + try { // Deleting before setting to avoid memory leak // https://github.com/mrousavy/react-native-mmkv/issues/440 authMMKV.delete(name); return authMMKV.set(name, value); + } catch (error) { + logger.error('Failed to set auth token:', error); + throw error; + } },README.md (1)
162-162: Use markdown link syntax instead of bare URLReplace the bare URL with proper markdown link syntax:
-Obtain the Firebase config files from the private repository at https://github.com/ephemeraHQ/converse-app-env +Obtain the Firebase config files from the private repository at [converse-app-env](https://github.com/ephemeraHQ/converse-app-env)🧰 Tools
🪛 Markdownlint (0.37.0)
162-162: null
Bare URL used(MD034, no-bare-urls)
config.ts (2)
73-75: Document the purpose and security implications of appCheckDebugTokenThe introduction of
appCheckDebugTokenwith platform-specific handling needs documentation for future maintainers.Add a comment explaining:
- The purpose of appCheckDebugToken
- Why it's handled differently for Android vs iOS
- Security implications of using debug tokens in development
+ // Firebase App Check debug token for development + // Android uses the default debug token provider + // iOS requires an explicit debug token from Firebase Console appCheckDebugToken: isAndroid ? undefined : process.env.EXPO_PUBLIC_FIREBASE_APP_CHECK_DEBUG_TOKEN_IOS,
Line range hint
132-140: Consider using environment constants for better type safetyThe environment selection logic could benefit from using constants instead of string literals.
Consider refactoring to use typed constants:
+const ENVIRONMENTS = { + PREVIEW: 'preview', + PRODUCTION: 'prod', +} as const; + export const getConfig = () => { if (__DEV__) { return ENV.dev; - } else if (Constants.expoConfig?.extra?.ENV === "preview") { + } else if (Constants.expoConfig?.extra?.ENV === ENVIRONMENTS.PREVIEW) { return ENV.preview; } else { return ENV.prod; } };package.json (1)
62-63: Consider pinning Firebase versions.The Firebase dependencies are correctly added, but consider pinning the versions to ensure consistent builds across environments.
Update the versions to be exact:
- "@react-native-firebase/app": "^21.4.0", - "@react-native-firebase/app-check": "^21.4.0", + "@react-native-firebase/app": "21.4.0", + "@react-native-firebase/app-check": "21.4.0",screens/Navigation/Navigation.tsx (1)
197-227: Consider enhancing the test screen implementationWhile the test screen serves its purpose for development, there are several improvements that could be made:
- The error message display could be overwhelming for long error objects
- The token truncation logic could fail if token is undefined
- The state management could be simplified
Consider this improved implementation:
const TestAppCheckScreen = () => { const [token, setToken] = useState<string | undefined>(undefined); - const [key, setKey] = useState<string | undefined>(undefined); // Unused state const [error, setError] = useState<string | undefined>(undefined); useEffect(() => { setupAppAttest(); }, []); + const formatToken = (token?: string) => { + if (!token) return "no token"; + return `${token.substring(0, 10)}...${token.substring(token.length - 10)}`; + }; return ( <View> <Text>TestAppCheckScreen</Text> <Button text="TestAppCheckScreen" onPress={() => { tryGetAppCheckToken() .then(setToken) .catch((error) => { logger.error("Error getting token", error); - setError(JSON.stringify(error)); + setError(error.message || JSON.stringify(error)); }); }} /> - <Text> - {`${token?.substring(0, 10)}...${token?.substring(token.length - 10)}` ?? - "no token"} - </Text> + <Text>Token: {formatToken(token)}</Text> <Text>Last Error: {error ?? "no error"}</Text> </View> ); };ios/ConverseNotificationExtension/Spam.swift (2)
Line range hint
188-192: Optimize the loop with awhereclause for better readabilityThe current implementation uses a nested if condition which can be simplified using Swift's
whereclause.Consider this improvement:
- for address in senderAddresses { - if try await client.preferences.addressState(address: address) == .denied { - return 1 - } - } + for address in senderAddresses where try await client.preferences.addressState(address: address) == .denied { + return 1 + }This change:
- Improves code readability
- Follows Swift's idiomatic patterns
- Maintains the same functionality
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 188-188:
whereclauses are preferred over a singleifinside afor(for_where)
Line range hint
11-41: Consider integrating App Check token verificationSince we're adding Firebase App Check to the application, the spam score API calls should be protected with App Check token verification.
Consider:
- Adding App Check token to the API requests
- Verifying tokens on the backend
- Implementing rate limiting based on App Check attestation
Would you like assistance in implementing these security enhancements?
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 188-188:
whereclauses are preferred over a singleifinside afor(for_where)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
.gitignore(1 hunks)App.tsx(2 hunks)README.md(6 hunks)app.config.ts(1 hunks)config.ts(3 hunks)eas.json(0 hunks)features/conversation/conversation-message/conversation-message-sender-avatar.tsx(0 hunks)ios/Converse.xcodeproj/project.pbxproj(22 hunks)ios/Converse/AppDelegate.mm(2 hunks)ios/Converse/Converse.entitlements(1 hunks)ios/ConverseNotificationExtension/Spam.swift(1 hunks)ios/Podfile(2 hunks)package.json(2 hunks)queries/QueryKeys.ts(1 hunks)screens/Navigation/Navigation.tsx(4 hunks)screens/NewAccount/NewAccountEphemeraScreen.tsx(0 hunks)scripts/build/build.js(2 hunks)scripts/build/build.utils.js(1 hunks)scripts/build/ios/preview.js(4 hunks)scripts/build/ios/production.js(5 hunks)utils/api.constants.ts(1 hunks)utils/api.ts(7 hunks)utils/appCheck.ts(1 hunks)utils/mmkv.ts(1 hunks)
💤 Files with no reviewable changes (3)
- screens/NewAccount/NewAccountEphemeraScreen.tsx
- eas.json
- features/conversation/conversation-message/conversation-message-sender-avatar.tsx
✅ Files skipped from review due to trivial changes (2)
- utils/api.constants.ts
- .gitignore
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
53-53: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
78-78: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
162-162: null
Bare URL used
(MD034, no-bare-urls)
🪛 LanguageTool
README.md
[style] ~99-~99: Consider shortening or rephrasing this to strengthen your wording.
Context: ...sts sh yarn test:perf:baseline Make changes to the code to see the performance impact ...
(MAKE_CHANGES)
[style] ~120-~120: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ... Branches Each release branch is based off of main or the release branch before it....
(EN_WORDINESS_PREMIUM_OFF_OF)
[style] ~152-~152: It might be better to use ‘times’ with the time-relative pronoun ‘when’. (Alternatively, use ‘in/on which’.)
Context: ... ``` ### Exceptions There are certain times where this flow does not work as intended. Fo...
(WHEN_WHERE)
[style] ~154-~154: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...Build scripts: These may need to be run off of the default main branch instead of feat...
(EN_WORDINESS_PREMIUM_OFF_OF)
🪛 SwiftLint (0.57.0)
ios/ConverseNotificationExtension/Spam.swift
[Warning] 188-188: where clauses are preferred over a single if inside a for
(for_where)
🔇 Additional comments (16)
scripts/build/ios/production.js (1)
Line range hint 19-102: Consider implementing safer file modification strategies
The current implementation using string replacements could be fragile. Consider:
- Using a proper XML/plist parser for project file modifications
- Implementing file backups before modifications
- Validating modified content before saving
Here's a suggested approach for safer modifications:
+ const backupFile = (filePath) => {
+ const backupPath = `${filePath}.bak`;
+ fs.copyFileSync(filePath, backupPath);
+ return backupPath;
+ };
+ const restoreBackup = (filePath) => {
+ const backupPath = `${filePath}.bak`;
+ if (fs.existsSync(backupPath)) {
+ fs.renameSync(backupPath, filePath);
+ }
+ };
const go = async () => {
try {
copyGoogleServiceInfo("production");
const PROJ_PATH = "ios/Converse.xcodeproj/project.pbxproj";
+ const backup = backupFile(PROJ_PATH);
const projContent = fs.readFileSync(PROJ_PATH, "utf-8");
// Update Xcode Project Bundle ID
const newProjContent = projContent
.replace(
/PRODUCT_BUNDLE_IDENTIFIER = com\.converse\.dev/g,
"PRODUCT_BUNDLE_IDENTIFIER = com.converse.native"
)
.replace(
/ASSETCATALOG_COMPILER_APPICON_NAME = AppIconPreview/g,
"ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon"
);
+ // Validate modifications
+ if (!/PRODUCT_BUNDLE_IDENTIFIER = com\.converse\.native/.test(newProjContent)) {
+ throw new Error('Failed to update bundle identifier');
+ }
fs.writeFileSync(PROJ_PATH, newProjContent);
+ fs.unlinkSync(backup);
} catch (error) {
+ restoreBackup(PROJ_PATH);
throw error;
}
// ... rest of the function
};utils/api.ts (2)
262-262: Ensure appCheckToken is included in all authenticated requests
The appCheckToken is now added to the headers in getXmtpApiHeaders. Verify that all authenticated API calls are utilizing getXmtpApiHeaders to include the necessary headers.
122-122: Verify all usages of AuthParams are updated
The appCheckToken parameter in the AuthParams type has been changed from optional to required. Please ensure that all functions and methods that use AuthParams are updated accordingly to include the appCheckToken parameter.
Run the following script to find all usages of AuthParams and check if they provide appCheckToken:
✅ Verification successful
All usages of appCheckToken parameter are properly handled
Based on the verification results, all instances where AuthParams is used already include the appCheckToken parameter:
- The type definition in
utils/api.tscorrectly definesappCheckTokenas required - The
fetchAccessTokenfunction properly receives and usesappCheckTokenin both the request body and headers - All object literals passing
AuthParamsinclude theappCheckTokenproperty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all usages of AuthParams that might be missing appCheckToken
rg -t typescript 'AuthParams' -A 5
Length of output: 69
Script:
#!/bin/bash
# Search for AuthParams in all files (without file type restriction)
rg "AuthParams" -A 5
# Also search for type definition of AuthParams
ast-grep --pattern 'type AuthParams = {
$$$
}'
Length of output: 874
Script:
#!/bin/bash
# Search for any object literals being passed to functions where AuthParams might be used
rg -B 5 "appCheckToken.*,"
# Search for any function calls that might be passing AuthParams
ast-grep --pattern 'createAccessToken({
$$$
})'
Length of output: 1199
ios/Converse.xcodeproj/project.pbxproj (2)
676-676: Verify the updated deployment target
The IPHONEOS_DEPLOYMENT_TARGET has been increased to 16.0. Please confirm that this aligns with our supported iOS versions and that we are not unintentionally dropping support for users on earlier versions.
Consider checking the app usage statistics to determine the impact on users with iOS versions below 16.0.
Also applies to: 714-714, 759-759, 804-804
6-6: Update objectVersion with caution
The objectVersion has been updated from 54 to 60, indicating a project file format change. Ensure that all team members are using a compatible version of Xcode to avoid project file conflicts.
ios/Converse/Converse.entitlements (2)
7-11: Confirm associated domains configuration
The com.apple.developer.associated-domains key has been updated with new domains. Verify that applinks:dev.converse.xyz and applinks:dev.getconverse.app are correctly set up for Universal Links and that the necessary Apple App Site Association (AASA) files are in place on these domains.
12-13: Verify appattest-environment value
The com.apple.developer.devicecheck.appattest-environment is set to production. Confirm that this is intentional, especially if this build is intended for development or testing.
utils/mmkv.ts (1)
14-16: LGTM: Well-structured auth storage setup
Good practices observed:
- Separate MMKV instance for auth storage
- Clear type definition for auth keys
- Memory leak prevention implementation
ios/Converse/AppDelegate.mm (1)
26-35: Implement environment-specific Firebase configuration
The commented code shows the intended environment-specific configuration that should be implemented.
Let's verify the presence of environment-specific configuration files:
Would you like help implementing the environment-specific configuration logic?
queries/QueryKeys.ts (1)
44-44: Verify the necessity of optional chaining for account parameter
While adding optional chaining makes the code more defensive, it might mask underlying issues where account should never be null/undefined. Consider:
- Is this masking a bug where
accountshould always be defined? - Should we validate the
accountparameter at the function boundary instead?
Let's verify the usage of these query keys to determine if account can actually be undefined:
Also applies to: 48-48, 54-54, 58-58, 65-65, 70-70, 75-75, 80-80, 85-85, 90-90, 95-95, 100-100, 105-105, 110-110, 114-114, 121-121, 125-125, 132-132, 136-136, 142-142
✅ Verification successful
Let me analyze the search results to determine if account can be undefined in these query key functions.
Based on the search results, I can now generate the final response since I have enough information to make a determination about the optional chaining usage.
Optional chaining for account.toLowerCase() is appropriate and necessary
The optional chaining in these query key functions is correct and necessary because:
- All query hooks have explicit
enabledconditions that check for the presence of required parameters includingaccount(e.g.,enabled: !!account) - However, the query keys are constructed before these enable checks take effect, meaning
accountcould be undefined during the initial render - The pattern is consistently applied across all query key functions that handle account addresses
This is a proper defensive programming practice in React Query's context, where:
- Query keys need to be constructed during render, before enable conditions are checked
- The optional chaining prevents runtime errors during the initial render or when parameters are undefined
- The actual query function won't execute until the enable conditions are met
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to these query key functions
rg -l "conversationMessageQueryKey|conversationsQueryKey|conversationQueryKey|conversationWithPeerQueryKey|conversationMessagesQueryKey|conversationPreviewMessagesQueryKey|groupMembersQueryKey|groupNameQueryKey|groupDescriptionQueryKey|groupPhotoQueryKey|groupPinnedFrameQueryKey|groupPermissionsQueryKey|groupConsentQueryKey|dmConsentQueryKey|addedByQueryKey|groupIsActiveQueryKey|groupInviteQueryKey|groupJoinRequestQueryKey|pendingJoinRequestsQueryKey|groupPermissionPolicyQueryKey" | xargs rg -A 2 -B 2 "QueryKey"
Length of output: 36601
Script:
#!/bin/bash
# Search for places where these query key functions are called to check if account is ever undefined
rg -A 2 -B 2 "enabled: " "queries/"
Length of output: 7749
config.ts (1)
94-94: Verify the security of production environments
Good practice to explicitly set appCheckDebugToken to undefined in preview and production environments.
Let's verify no debug tokens are accidentally exposed in the environment:
Also applies to: 128-128
✅ Verification successful
Let me search for any potential debug tokens across all files in the repository to ensure a thorough verification.
Debug tokens are properly configured across environments
The verification confirms that the app check debug tokens are properly configured:
- Production environment has
appCheckDebugToken: undefined - Preview environment has
appCheckDebugToken: undefined - Development environment only enables debug tokens conditionally based on platform
- No hardcoded debug tokens found in environment files or configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to debug tokens in environment files
rg -i "app.?check.?debug.?token" .env* || echo "No debug tokens found in .env files"
Length of output: 169
Script:
#!/bin/bash
# Search for any references to debug tokens across all files
rg -i "app.?check.?debug.?token" -A 2
# Also search for any Firebase App Check configuration patterns
rg -i "firebase.?app.?check" -A 2
Length of output: 18890
ios/Podfile (2)
31-32: LGTM! Required Firebase configuration.
The $RNFirebaseAsStaticFramework = true setting is necessary for proper Firebase integration with static frameworks.
85-96: Verify if build settings are needed.
The commented build settings are recommended by Firebase documentation for handling architecture-related build errors. Consider enabling these settings if you encounter build issues related to architecture compatibility.
package.json (1)
243-243: LGTM! Required for native build configuration.
The expo-build-properties package is necessary for configuring native build settings with Expo.
screens/Navigation/Navigation.tsx (2)
51-53: LGTM: Required imports for App Check integration
The imports are correctly structured, bringing in necessary utilities for App Check functionality.
245-248: Verify the development-only test screen configuration
The test screen is correctly commented out but should be properly configured for development testing.
Let's verify the development configuration setup:
✅ Verification successful
Development test screen is properly configured but commented out
The verification shows that:
- The
TestAppCheckScreencomponent is properly implemented in the Navigation.tsx file with necessary functionality - The development environment configuration in
config.tscorrectly handles__DEV__conditions - The test screen is currently commented out in the navigation stack
The screen should remain commented out unless specifically needed for development testing. No issues found with the configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper development configuration setup
# Look for development environment configuration files
fd -t f "env|config" --exec grep -l "APP_CHECK"
Length of output: 59
Script:
#!/bin/bash
# Check the content of config.ts for APP_CHECK configuration
cat config.ts
# Look for any TestAppCheck related files or imports
rg "TestAppCheck" -A 3
# Check for __DEV__ conditional rendering patterns in Navigation files
rg "__DEV__" screens/Navigation/
Length of output: 5593
| ios: { | ||
| supportsTablet: true, | ||
| buildNumber: appBuildNumbers.expo.ios.buildNumber, | ||
| bundleIdentifier: "com.converse.dev", |
There was a problem hiding this comment.
Hardcoded development bundle identifier may cause issues in production
The bundle identifier is hardcoded to "com.converse.dev" for both iOS and Android. This should be environment-specific to support different builds.
Consider using environment variables or the existing environment checks:
- bundleIdentifier: "com.converse.dev",
+ bundleIdentifier: isDev ? "com.converse.dev" : isPreview ? "com.converse.preview" : "com.converse",Similar change needed for Android package name.
Also applies to: 46-46
app.config.ts
Outdated
| // TODO(lustig): dynamically set this based on env in Android PR for AppCheck | ||
| googleServicesFile: "./android/google-services.json", |
There was a problem hiding this comment.
💡 Codebase verification
Multiple environment-specific Google Services configurations found
The codebase already has environment-specific Google Services files in place:
android/app/src/preview/google-services.jsonandroid/app/src/prod/google-services.json
The TODO can be addressed by updating the configuration to use the appropriate file based on the build environment.
🔗 Analysis chain
Address TODO for dynamic Google Services configuration
The TODO comment indicates that the Google Services file path should be dynamic, but it's currently hardcoded.
Let's verify the presence of environment-specific Google Services files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for environment-specific Google Services files
echo "Checking for Google Services files..."
fd "google-services.*json" android/
fd "GoogleService-Info.*plist" ios/
Length of output: 322
|
ran into this - CocoaPods/CocoaPods#12671 - fixed by reverting an inadvertent change to |
not sure how this keeps cropping up
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
README.md (1)
Line range hint
85-89: Clarify required environment variablesThe testing section mentions a
.envfile but doesn't specify:
- Required variables
- Example values
- Where to obtain these values
Please add a subsection listing the required environment variables and their purposes.
🧰 Tools
🪛 Markdownlint (0.37.0)
78-78: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
🧹 Nitpick comments (2)
README.md (1)
154-157: Enhance exceptions list with security considerationsThe exceptions list should include guidance for security-sensitive changes:
- Configuration file updates
- Security patches
- Credential rotations
Consider adding a note about handling security-sensitive changes and their review requirements.
🧰 Tools
🪛 LanguageTool
[style] ~154-~154: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...Build scripts: These may need to be run off of the default main branch instead of feat...(EN_WORDINESS_PREMIUM_OFF_OF)
utils/api.ts (1)
215-222: Consider improving the multi-line error message format.While the error message is descriptive, multi-line template literals in error messages can make log parsing more challenging.
Consider this more log-friendly format:
- throw new Error(` -No App Check Token Available. This indicates that we believe the app is not running on an authentic build of -our application on a device that has not been tampered with. -`); + throw new Error( + "No App Check Token Available: App integrity check failed - possible unauthorized build or tampered device" + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(6 hunks)app.config.ts(1 hunks)ios/Google Services/firebase-setup.readme.md(1 hunks)utils/api.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app.config.ts
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
53-53: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
78-78: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
162-162: null
Bare URL used
(MD034, no-bare-urls)
ios/Google Services/firebase-setup.readme.md
5-5: null
Bare URL used
(MD034, no-bare-urls)
🪛 LanguageTool
README.md
[style] ~99-~99: Consider shortening or rephrasing this to strengthen your wording.
Context: ...sts sh yarn test:perf:baseline Make changes to the code to see the performance impact ...
(MAKE_CHANGES)
[style] ~120-~120: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ... Branches Each release branch is based off of main or the release branch before it....
(EN_WORDINESS_PREMIUM_OFF_OF)
[style] ~152-~152: It might be better to use ‘times’ with the time-relative pronoun ‘when’. (Alternatively, use ‘in/on which’.)
Context: ... ``` ### Exceptions There are certain times where this flow does not work as intended. Fo...
(WHEN_WHERE)
[style] ~154-~154: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...Build scripts: These may need to be run off of the default main branch instead of feat...
(EN_WORDINESS_PREMIUM_OFF_OF)
🔇 Additional comments (7)
README.md (2)
173-173: Complete the Android Firebase configuration instructions
The Android configuration section is marked as TODO. This matches a previous review comment that needs to be addressed.
Would you like me to help draft the Android configuration instructions or create a GitHub issue to track this task?
167-169: Fix inconsistent file naming in iOS config paths
The file paths are inconsistent with the structure used in the codebase:
- Current paths use "Google Services" directory
- Files have inconsistent naming patterns
Apply this naming convention for consistency:
-ios/Google Services/GoogleService-Info.plist (dev)
-ios/Google Services/GoogleService-Info-preview.plist (preview)
-ios/Google Services/GoogleService-Info-prod.plist (prod)
+ios/GoogleService-Info-dev.plist
+ios/GoogleService-Info-preview.plist
+ios/GoogleService-Info-prod.plistios/Google Services/firebase-setup.readme.md (1)
5-5: Security concern: Private repository URL exposure
The URL to the private repository containing sensitive configuration files should not be exposed in public documentation.
🧰 Tools
🪛 Markdownlint (0.37.0)
5-5: null
Bare URL used
(MD034, no-bare-urls)
utils/api.ts (4)
33-37: LGTM! Constants and imports are well organized.
The new Firebase App Check related additions follow the existing naming conventions and import organization patterns.
122-122: LGTM! Enhanced security with mandatory app check token.
The change from optional to required appCheckToken in AuthParams type strengthens the authentication requirements.
547-547: LGTM! Proper authentication headers added.
The checkUsernameValid function now correctly includes authentication headers.
395-399:
Critical: Authentication headers are commented out.
The TODO comments indicate authentication issues after ephemeral account creation, but commenting out the authentication headers is not a secure solution. This could expose endpoints to unauthorized access.
Let's verify the impact:
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ios/Google Services/firebase-setup.readme.md (1)
16-24: Improve documentation formatting and structureThe "Supporting Documentation" section needs formatting improvements:
- Use proper markdown link syntax instead of bare URLs
- Format blockquotes consistently without blank lines
- Consider adding a table of contents
Here's the suggested format:
-## Supporting Documentation this is Safe, If Not Advised Always +## Supporting Documentation + +### Security Considerations -https://stackoverflow.com/a/44937513/2441420 +[Stack Overflow discussion about Firebase config security](https://stackoverflow.com/a/44937513/2441420) -> While it's not the end of the world if you commit `GoogleService-Info.plist` (similarly, on Android, `google-services.json`), you're better off leaving it out for one big reason: **You're making it clear that others who build your code that they should be setting up their own Firebase project to host its configuration and data (because your project simply won't build with that file missing).** Furthermore, if your project has world-writable resources in it, you can expect that people who get a hold of your app and config so easily will inadvertently start writing data into it, which is probably not what you want. - -> Open source projects that require a Firebase setup should ideally have it stated in the readme that anyone who wants to work with this app needs to go through the process of setting up Firebase. - -> The data in these config files is not exactly private - they can be extracted fairly easily from an IPA or Android APK. It's just probably not in your best interest to share them as part of your app's code. +> While it's not the end of the world if you commit `GoogleService-Info.plist` (similarly, on Android, `google-services.json`), you're better off leaving it out for one big reason: **You're making it clear that others who build your code that they should be setting up their own Firebase project to host its configuration and data (because your project simply won't build with that file missing).** Furthermore, if your project has world-writable resources in it, you can expect that people who get a hold of your app and config so easily will inadvertently start writing data into it, which is probably not what you want. +> +> Open source projects that require a Firebase setup should ideally have it stated in the readme that anyone who wants to work with this app needs to go through the process of setting up Firebase. +> +> The data in these config files is not exactly private - they can be extracted fairly easily from an IPA or Android APK. It's just probably not in your best interest to share them as part of your app's code.🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...which is probably not what you want. > Open source projects that require a Firebase setup ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 Markdownlint (0.37.0)
21-21: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
23-23: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
18-18: null
Bare URL used(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ios/Converse.xcodeproj/project.pbxproj(25 hunks)ios/Google Services/GoogleService-Info-preview.plist(1 hunks)ios/Google Services/GoogleService-Info-prod.plist(1 hunks)ios/Google Services/firebase-setup.readme.md(1 hunks)ios/GoogleService-Info.plist(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- ios/Google Services/GoogleService-Info-prod.plist
- ios/Google Services/GoogleService-Info-preview.plist
🧰 Additional context used
🪛 LanguageTool
ios/Google Services/firebase-setup.readme.md
[style] ~8-~8: Consider a shorter alternative to avoid wordiness.
Context: ...ile in ios/GoogleService-Info.plist In order to ease development, we keep the file corr...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~11-~11: When ‘environment-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...y Firebase initialization. We keep the environment specific files tucked away in `ios/Google Servic...
(SPECIFIC_HYPHEN)
[uncategorized] ~22-~22: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...which is probably not what you want. > Open source projects that require a Firebase setup ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 Markdownlint (0.37.0)
ios/Google Services/firebase-setup.readme.md
21-21: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
23-23: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
18-18: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (5)
ios/Converse.xcodeproj/project.pbxproj (3)
240-243: Build configuration changes look good
The pod configurations and framework embedding are properly set up for both the main app and notification extension targets.
Also applies to: 487-512
13-14: Verify Firebase App Check configuration
The Firebase integration includes:
- GoogleService-Info.plist configuration
- Firebase Core bundles
- Firebase configuration script
Ensure that the GoogleService-Info.plist file is properly configured and contains the necessary App Check settings.
Also applies to: 411-414, 539-550
6-6: Verify Xcode compatibility across the team
The project format has been upgraded from version 54 to 60, which requires a newer version of Xcode. Ensure all team members are using a compatible Xcode version to prevent project file access issues.
ios/GoogleService-Info.plist (1)
11-12: Verify bundle identifier naming convention
The bundle ID com.converse.dev suggests this is a development configuration. Ensure this matches your bundle identifier scheme across different environments (dev/staging/prod).
✅ Verification successful
Bundle identifier scheme follows environment naming convention
The bundle identifier scheme is correctly following the environment naming pattern:
- Development:
com.converse.dev - Preview:
com.converse.preview - Production:
com.converse.prod
The current com.converse.dev bundle ID in GoogleService-Info.plist matches the development environment configuration, which is consistent with other environment configurations found in the Google Services directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other bundle identifiers in the project
rg "BUNDLE_ID|bundleIdentifier|CFBundleIdentifier" --type xml
Length of output: 63
Script:
#!/bin/bash
# Let's try a broader search for plist and xml files first
fd -e plist -e xml
# Then let's specifically look at the content of any iOS configuration files
find . -type f -name "*.xcodeproj" -o -name "*.pbxproj" -o -name "*.plist" | xargs cat 2>/dev/null
# Also search for any bundle identifier strings without file type restriction
rg -i "bundle.?id|bundleidentifier|com\.converse"
Length of output: 66949
Script:
#!/bin/bash
# Let's check the content of the Google Services directory for different environment configurations
cat "ios/Google Services/GoogleService-Info-preview.plist" 2>/dev/null
cat "ios/Google Services/GoogleService-Info-prod.plist" 2>/dev/null
# Also check the main Info.plist for bundle identifier
cat "ios/Converse/Info.plist" 2>/dev/null
Length of output: 5594
ios/Google Services/firebase-setup.readme.md (1)
1-26: Consider consolidating Firebase documentation
This file duplicates information from README.md and is located in an inconsistently named directory.
🧰 Tools
🪛 LanguageTool
[style] ~8-~8: Consider a shorter alternative to avoid wordiness.
Context: ...ile in ios/GoogleService-Info.plist In order to ease development, we keep the file corr...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~11-~11: When ‘environment-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...y Firebase initialization. We keep the environment specific files tucked away in `ios/Google Servic...
(SPECIFIC_HYPHEN)
[uncategorized] ~22-~22: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...which is probably not what you want. > Open source projects that require a Firebase setup ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 Markdownlint (0.37.0)
21-21: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
23-23: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
18-18: null
Bare URL used
(MD034, no-bare-urls)
| @@ -630,7 +669,7 @@ | |||
| "FB_SONARKIT_ENABLED=1", | |||
| ); | |||
| INFOPLIST_FILE = Converse/Info.plist; | |||
| IPHONEOS_DEPLOYMENT_TARGET = 14.0; | |||
| IPHONEOS_DEPLOYMENT_TARGET = 16.0; | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results and the Podfile showing iOS 14.0 as the current deployment target, along with Firebase App Check being version 11.4.0 which doesn't require iOS 16.0, I can generate the final response:
iOS 16.0 requirement may be too restrictive
The increase to iOS 16.0 appears unnecessary as:
- Current Podfile specifies iOS 14.0 as deployment target
- Firebase App Check 11.4.0 doesn't require iOS 16.0
- No other dependencies indicate a need for iOS 16.0
Consider keeping the deployment target at iOS 14.0 to maintain broader device compatibility unless there's a specific feature requirement for iOS 16.0.
🔗 Analysis chain
Verify iOS 16.0 requirement justification
The minimum iOS version has been increased to 16.0. This is a significant change that could impact the app's market reach. Please verify if this increase is specifically required by Firebase App Check or if we can support lower iOS versions.
Run this script to check for any documentation about iOS version requirements:
Also applies to: 710-710, 755-755, 800-800
Let me gather more information about Firebase App Check requirements.
Let me try one more search with corrected commands to find Firebase App Check requirements:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for iOS version requirements in documentation
rg -l "ios.*16|ios.*version" --type md
# Check podfile for minimum iOS version specifications
fd -t f "Podfile" -x cat {} \; | rg -i "platform.*ios"
Length of output: 904
Script:
#!/bin/bash
# Search for Firebase App Check related files and their iOS requirements
rg -l "firebase.*appcheck|FirebaseAppCheck" --type-add 'pod:*.podspec' --type pod --type swift --type objc
# Check package.json for Firebase dependencies and their versions
fd -t f "package.json" -x cat {} \; | jq -r '.dependencies | with_entries(select(.key | contains("firebase")))'
# Search for any iOS version requirements in podspec files
fd -t f "*.podspec" -x cat {} \; | rg -i "ios.*deployment.*target|platform.*ios"
Length of output: 693
Script:
#!/bin/bash
# Search for Firebase App Check documentation or configuration
rg -i "ios.*requirement|ios.*version|deployment.*target" --type yaml --type json --type md
# Check Podfile.lock for Firebase App Check version and requirements
fd Podfile.lock -x cat {} \;
# Search for any Firebase App Check related configuration
rg -i "firebaseappcheck|firebase.*app.*check" -A 5 -B 5
Length of output: 65720
| <key>API_KEY</key> | ||
| <string>AIzaSyD5G_3H8Q1wzUiRTuv9SefDyjlJxigHJh8</string> | ||
| <key>GCM_SENDER_ID</key> | ||
| <string>564961909146</string> | ||
| <key>PLIST_VERSION</key> | ||
| <string>1</string> | ||
| <key>BUNDLE_ID</key> | ||
| <string>com.converse.dev</string> | ||
| <key>PROJECT_ID</key> | ||
| <string>converse-unshut-labs</string> | ||
| <key>STORAGE_BUCKET</key> | ||
| <string>converse-unshut-labs.firebasestorage.app</string> | ||
| <key>IS_ADS_ENABLED</key> | ||
| <false></false> | ||
| <key>IS_ANALYTICS_ENABLED</key> | ||
| <false></false> | ||
| <key>IS_APPINVITE_ENABLED</key> | ||
| <true></true> | ||
| <key>IS_GCM_ENABLED</key> | ||
| <true></true> | ||
| <key>IS_SIGNIN_ENABLED</key> | ||
| <true></true> | ||
| <key>GOOGLE_APP_ID</key> | ||
| <string>1:564961909146:ios:779bcea169541560bd0223</string> |
There was a problem hiding this comment.
💡 Codebase verification
The verification reveals several security concerns:
- All Firebase configuration files (preview, production, and default) are currently tracked in git
- No .gitignore patterns exist to prevent committing these files
- Multiple environment configurations are exposed in the repository
Action items:
- Add
ios/Google Services/GoogleService-Info-*.plistandios/GoogleService-Info.plistto .gitignore - Remove tracked config files using
git rm --cached - Move Firebase configurations to a secure CI/CD pipeline
- Document the process for managing environment-specific configurations
🔗 Analysis chain
Review security implications of committed Firebase configuration
While these configuration values can be extracted from built apps anyway (as noted in the documentation), consider:
- Moving sensitive configuration to a secure CI/CD pipeline
- Using different configurations for development vs. production
- Adding this file to .gitignore to prevent accidental commits of production credentials
Let's verify the current security practices:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this file is properly gitignored
grep -l "GoogleService-Info.plist" .gitignore
# Check for any other committed Firebase configs
fd "GoogleService-Info" --type f
Length of output: 210
Script:
#!/bin/bash
# Check the contents of the different Firebase config files to verify environment separation
echo "=== Preview Config ==="
cat "ios/Google Services/GoogleService-Info-preview.plist" | grep -E "BUNDLE_ID|PROJECT_ID|API_KEY"
echo -e "\n=== Production Config ==="
cat "ios/Google Services/GoogleService-Info-prod.plist" | grep -E "BUNDLE_ID|PROJECT_ID|API_KEY"
echo -e "\n=== Default Config ==="
cat "ios/GoogleService-Info.plist" | grep -E "BUNDLE_ID|PROJECT_ID|API_KEY"
Length of output: 654
Script:
#!/bin/bash
# Let's check if there's a .gitignore pattern that might be excluding the Firebase configs
echo "=== Checking .gitignore patterns ==="
cat .gitignore | grep -i "firebase\|google"
# Check if these files are tracked by git
echo -e "\n=== Checking git status of config files ==="
git ls-files "ios/Google Services/GoogleService-Info-*.plist" "ios/GoogleService-Info.plist"
Length of output: 456
| <string>applinks:dev.getconverse.app</string> | ||
| </array> | ||
| <key>com.apple.developer.devicecheck.appattest-environment</key> | ||
| <string>production</string> |
There was a problem hiding this comment.
recommended via docs
There was a problem hiding this comment.
see for explanation of publicly committing: https://github.com/ephemeraHQ/converse-app/pull/1369/files#diff-629dc70c8ca0aa58dc08ce39715b7f4777a632e2e348bfbe6a0081246ec75e51
There was a problem hiding this comment.
see for explanation of publicly committing: https://github.com/ephemeraHQ/converse-app/pull/1369/files#diff-629dc70c8ca0aa58dc08ce39715b7f4777a632e2e348bfbe6a0081246ec75e51
| @@ -191,6 +194,39 @@ export function SignedInNavigation() { | |||
| ); | |||
| } | |||
|
|
|||
| // todo: remove once integration complete | |||
There was a problem hiding this comment.
leaving until android merged
| @@ -522,6 +544,7 @@ export const checkUsernameValid = async ( | |||
| ): Promise<string> => { | |||
| const { data } = await api.get("/api/profile/username/valid", { | |||
| params: { address, username }, | |||
| headers: await getXmtpApiHeaders(address), | |||
Summary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes
Chores