appcheck #1375
appcheck #1375
Conversation
not sure how this keeps cropping up
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces changes to the iOS project configuration, specifically in the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ios/Podfile (1)
72-77: LGTM: Module definition fix looks correctThe addition of
DEFINES_MODULE = 'YES'for specific targets is a standard fix for Swift/Objective-C interoperability issues in React Native projects. This change should help resolve build errors related to module imports.Note that this configuration ensures proper module interfaces for the specified targets. If you encounter similar build issues with other targets in the future, you may need to add them to this list.
ios/Converse.xcodeproj/project.pbxproj (1)
Line range hint
1-1: Overall Assessment of Build Pipeline FixesThe changes address two potential build issues:
- Swift/Objective-C module interface problems via
DEFINES_MODULEsettings- Google Services configuration management via project group restructuring
While the changes look reasonable for fixing build issues, please:
- Test the build pipeline with these changes
- Verify that Firebase/Google services continue to work as expected
- Consider adding comments in the code explaining why these changes were necessary to help future maintainers
Consider creating a troubleshooting guide in your documentation that covers:
- Common build issues and their solutions
- Steps to properly configure Google Services
- Module definition requirements for different target types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
ios/Converse.xcodeproj/project.pbxproj(1 hunks)ios/Podfile(1 hunks)
🔇 Additional comments (1)
ios/Converse.xcodeproj/project.pbxproj (1)
141-147: Verify Google Services configuration after group restructuring
The change replaces the synchronized Google Services group with a regular empty group. While this might help with build issues, we should ensure that:
- Required Google Services configuration files are still properly included in the build
- Firebase/Google services functionality is not affected
If you're experiencing issues with Google Services synchronization:
- Ensure GoogleService-Info.plist is properly added to the project
- Verify Firebase initialization in your AppDelegate
- Consider documenting the reason for removing the synchronized group to help with future maintenance
✅ Verification successful
Google Services configuration is properly maintained despite group restructuring
The verification shows that while the Google Services group was changed to a regular group, the essential configuration is still properly maintained:
- GoogleService-Info.plist is present in the iOS directory
- The plist file is correctly referenced in the project.pbxproj with proper build phase entries
- It's included in the Resources build phase for both main target and extension
The restructuring of the group appears to be a harmless organizational change that doesn't impact the actual Firebase configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Google Services configuration files and references
# Check for GoogleService-Info.plist
echo "Checking for GoogleService-Info.plist..."
fd "GoogleService-Info.plist"
# Check for Firebase configuration in the project
echo "Checking Firebase configuration..."
rg -l "FirebaseApp.configure"
Length of output: 258
Script:
#!/bin/bash
# Let's check the actual content of project.pbxproj for more context about Google Services references
# Check for any Google Services related entries in project.pbxproj
echo "Checking project.pbxproj for Google Services references..."
rg -A 5 "Google Services" "ios/Converse.xcodeproj/project.pbxproj"
# Check for Firebase related imports or configurations in source files
echo "Checking for Firebase usage in source files..."
rg -l "import Firebase|import FirebaseCore"
# Check if GoogleService-Info.plist is referenced in build phases
echo "Checking build phases for GoogleService-Info.plist..."
rg "GoogleService-Info.plist" "ios/Converse.xcodeproj/project.pbxproj"
Length of output: 1790
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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
utils/api.ts (3)
215-216: Acquire token earlier if possible.Retrieving the appCheckToken within getXmtpApiHeaders is reasonable. If this function is invoked repeatedly, consider caching the token if it has a valid TTL or transitions rarely to reduce overhead.
217-223: Comprehensive fallback or user messaging.The error message emphasizes potential tampering, which is excellent from a security standpoint. You might also consider providing user-friendly instructions for reinstalls or updates if they see this message.
395-399: Follow up on TODO comment.A TODO mentions request is “401ing after ephemeral account creation.” This is presumably a known bug. Consider opening a separate issue to track progress and avoid losing sight of it.
queries/QueryKeys.ts (2)
61-61: Double-check queries for missing accounts.If an account genuinely can be null, consider returning a fallback query key or skipping the fetch entirely to avoid no-op requests.
73-73: Ensure pinned frames handle no account states.When account is missing, you might short-circuit. This can avoid potential server calls that will fail anyway.
screens/Navigation/Navigation.tsx (1)
51-51: Button usage.Check that the new Button is used consistently with the design system. If not used or not fully integrated, remove it to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
queries/QueryKeys.ts(3 hunks)screens/Navigation/Navigation.tsx(2 hunks)utils/api.ts(8 hunks)
🔇 Additional comments (15)
utils/api.ts (6)
33-33: Ensure clarity with constant naming.
The introduction of FIREBASE_APP_CHECK_HEADER_KEY clarifies the header’s purpose. Good practice would be to ensure consistent naming conventions (e.g., “APP_CHECK_HEADER_KEY”) across other files that utilize this constant, if any.
37-37: Confirm import usage and side effects.
The tryGetAppCheckToken import addresses your app integrity checks. Verify that its usage doesn’t introduce additional asynchronous overhead in all contexts, especially if it is called frequently or in performance-critical flows.
149-149: Appropriate header assignment.
You are appending the appCheckToken to the FIREBASE_APP_CHECK_HEADER_KEY. Ensure the token is sanitized or validated before sending to prevent potential security issues if the token is user-supplied in the future.
201-201: Augmented type clarity.
Defining [FIREBASE_APP_CHECK_HEADER_KEY]: string in XmtpApiHeaders clarifies that a valid string token is always expected. Make sure consumers are aware of the mandatory nature of appCheckToken.
238-239: Review destructuring style consistency.
You’re spreading installationKeySignature here. Confirm consistent usage in other areas to keep the code base uniform.
547-547: Assess potential edge cases.
Using await getXmtpApiHeaders(address) is consistent with other calls. Confirm that address cannot be undefined or null, or wrap calls in conditionals if the address can be empty.
queries/QueryKeys.ts (7)
33-33: Safe chaining for .toLowerCase().
Using optional chaining avoids runtime errors. Ensure that query consumers gracefully handle the case when account is null/undefined (e.g., no logged-in user).
39-39: Confirm uniform usage across keys.
You’re using optional chaining in conversationQueryKey as well. This is coherent with the pattern applied in other keys.
56-56: Consistent naming and approach.
Again, applying account?.toLowerCase() helps keep the logic consistent. Make sure that the rest of your code handles an empty account string or undefined properly.
67-67: Group members query key consistency.
The same optional chaining approach is present. This is consistent with your overall change.
89-89: Permissions key usage.
Tying group permissions to the optional-chained account is consistent. Verify that other areas calling groupPermissionsQueryKey do not rely on a guaranteed lowercase string.
101-101: Join request key approach.
Same pattern as above. Possibly add fallback or early exit if account is undefined, preventing wasted lookups.
105-105: Pending requests behavior.
Again, the optional chaining is consistent. Confirm that the rest of the code handles undefined account or returns an empty array instead of attempting an API call.
screens/Navigation/Navigation.tsx (2)
5-6: Added React Hooks and RN components.
The introduction of useEffect, useState, View, and Text suggests potential UI changes. Verify that these additions are necessary for your new or existing logic. If you planned refactors that never fully materialized, consider removing unused imports to reduce code size.
53-53: Leverage logger for consistent debug logs.
Using logger is a good practice. Ensure that sensitive or personally identifiable info is not logged inadvertently, especially in production.
technoplato
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
features/GroupInvites/joinGroup/JoinGroup.client.ts (2)
Line range hint
132-186: Enhance error handling and retry strategyThe polling implementation could be improved in two ways:
- Add try-catch for network failures during polling
- Implement exponential backoff for retries
while (attemptsToRetryJoinGroup < GROUP_JOIN_REQUEST_POLL_MAX_ATTEMPTS) { logger.debug( `[liveAttemptToJoinGroup] Polling attempt ${ attemptsToRetryJoinGroup + 1 } of ${GROUP_JOIN_REQUEST_POLL_MAX_ATTEMPTS}` ); - const joinRequestData = await getGroupJoinRequest(joinRequestId); + try { + const joinRequestData = await getGroupJoinRequest(joinRequestId); + } catch (error) { + logger.error(`[liveAttemptToJoinGroup] Network error during polling: ${error}`); + // Continue polling despite network error + attemptsToRetryJoinGroup += 1; + const backoffMs = GROUP_JOIN_REQUEST_POLL_INTERVAL_MS * Math.pow(2, attemptsToRetryJoinGroup); + await wait(backoffMs); + continue; + } if (joinRequestData.status !== "PENDING") {
Line range hint
293-294: Fix TypeScript type issues in test fixturesThere are several @ts-expect-error comments indicating type mismatches in the test fixtures. These should be properly typed rather than suppressed.
Consider creating proper type definitions for the test fixtures:
type ConversationFixture = Pick<Conversation, | 'id' | 'createdAt' | 'members' | 'topic' | 'isGroupActive' | 'state' | 'creatorInboxId' | 'name' | 'addedByInboxId' | 'imageUrlSquare' | 'description' >; const createConversationFixture = (props: Partial<ConversationFixture>): Conversation => { return { id: 'default-id' as ConversationId, createdAt: new Date().getTime(), members: async () => [], topic: 'default-topic' as ConversationTopic, isGroupActive: true, state: 'allowed', creatorInboxId: async () => '0xabc' as InboxId, name: 'Default Group', addedByInboxId: '0x123' as InboxId, imageUrlSquare: 'https://example.com', description: 'Default Description', ...props } as Conversation; };Also applies to: 386-387, 486-487
🧹 Nitpick comments (2)
features/GroupInvites/joinGroup/JoinGroup.client.ts (1)
27-30: Consider making polling configuration environment-specificThe polling constants could be moved to a configuration file to allow different values for different environments (e.g., development, staging, production).
-const GROUP_JOIN_REQUEST_POLL_MAX_ATTEMPTS = 10; -const GROUP_JOIN_REQUEST_POLL_INTERVAL_MS = 1000; +import { config } from '@/config'; + +const GROUP_JOIN_REQUEST_POLL_MAX_ATTEMPTS = config.groupJoin.maxAttempts ?? 10; +const GROUP_JOIN_REQUEST_POLL_INTERVAL_MS = config.groupJoin.intervalMs ?? 1000;config.ts (1)
Line range hint
73-128: Consider adding runtime configuration validation.The configuration structure is well-organized, but consider adding runtime validation to ensure all required Firebase App Check configurations are present and correctly typed. This could help catch configuration issues early, especially for the new
appCheckDebugTokenproperty.Example approach:
type Config = { // ... other config types appCheckDebugToken: string | undefined; }; function validateConfig(config: Config): void { if (__DEV__ && !config.appCheckDebugToken) { console.warn('Missing appCheckDebugToken in development environment'); } // Add other validation rules }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
ios/Gemfile.lockis excluded by!**/*.lockios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
app.config.ts(1 hunks)config.ts(3 hunks)eas.json(2 hunks)features/GroupInvites/joinGroup/JoinGroup.client.ts(1 hunks)ios/Gemfile(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- eas.json
- app.config.ts
🔇 Additional comments (6)
config.ts (4)
94-94: LGTM! Correctly disabled debug token in preview environment.
Setting appCheckDebugToken to undefined in preview environment is the right approach to ensure debug tokens are not used outside development.
128-128: LGTM! Correctly disabled debug token in production environment.
Setting appCheckDebugToken to undefined in production environment is the right approach to ensure debug tokens are never used in production.
73-75: Verify Firebase App Check debug token usage.
The implementation correctly handles platform-specific debug tokens for development. However, ensure that:
- These debug tokens are only used in development builds
- The environment variables are properly secured and not committed to version control
Let's verify the environment variables usage:
✅ Verification successful
Firebase App Check debug tokens are properly secured
The implementation follows security best practices:
- Debug tokens are only accessed via environment variables with the
EXPO_PUBLIC_prefix - All
.env*files are properly gitignored - The
.xcode.envfile that is versioned doesn't contain any sensitive tokens - Debug tokens are only used in the development configuration, not in preview or production environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if debug tokens are accidentally committed
rg -g '!*.md' "FIREBASE_APP_CHECK_DEBUG_TOKEN"
# Check for any .env files that might contain these tokens
fd -H "\.env.*$"
Length of output: 239
Script:
#!/bin/bash
# Check the content of .xcode.env to ensure it doesn't contain debug tokens
cat ios/.xcode.env
# Check if the debug tokens are used only in development configuration
rg -B 5 -A 5 "FIREBASE_APP_CHECK_DEBUG_TOKEN"
# Check for any gitignore patterns related to env files
cat .gitignore | grep -i "env"
Length of output: 1121
132-132: Verify the usage of exported getConfig function.
The function is now exported, likely to support Firebase App Check integration. Let's verify its usage across the codebase.
✅ Verification successful
Let me verify the usage of the exported getConfig function with one more check to ensure we have a complete picture.
This will help us confirm:
- Any direct usage of getConfig() function
- How it's being used in appCheck.ts where we found the named import
Export of getConfig function is correctly implemented and used
The verification shows that:
- The
getConfigfunction is exported and used as the default export (export default getConfig()) - It's properly imported and used in
utils/appCheck.tsfor Firebase App Check configuration - Other files import the default export (the result of
getConfig()), which is the expected pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for direct imports of getConfig
rg "import.*\{.*getConfig.*\}.*from.*['\"].*config['\"]"
# Check for potential usage through default import
rg "import.*config.*from.*['\"].*config['\"]"
Length of output: 2342
Script:
#!/bin/bash
# Check for direct usage of getConfig function
rg "getConfig\(\)"
# Check the implementation in appCheck.ts where it's explicitly imported
cat utils/appCheck.ts
Length of output: 1798
ios/Gemfile (2)
3-4: LGTM! Good practice adding bundler version requirement.
Adding the minimum bundler version requirement is a proactive measure to ensure compatibility with Ruby 3.3.x, which is especially important given the Firebase-related changes in the project.
4-4: Verify bundler version in CI/CD environment.
Ensure that your CI/CD environment has a compatible bundler version (>= 2.4.0) to prevent potential build issues.
✅ Verification successful
Let me try to check the CI/CD configuration files more broadly to find any Ruby or bundler-related setup.
Let me try one more search specifically for Ruby and bundler setup in the iOS directory.
Let me try one final check to look for Ruby-related configuration in the iOS directory.
Based on the output from the searches, I can see that the project has a Gemfile.lock that shows bundler 2.6.1 is being used, which is already higher than the required version (>= 2.4.0) specified in the Gemfile. Additionally, the CI/CD workflows shown in the GitHub Actions configurations don't directly interact with Ruby/bundler as they use EAS (Expo Application Services) for building.
The bundler version requirement is already satisfied in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the current bundler version in package.json scripts or GitHub Actions workflow files
# Check for bundler version specifications in GitHub Actions workflow files
echo "Checking GitHub Actions workflow files for bundler version:"
fd -e yml -e yaml . .github/workflows/ --exec grep -l "bundler"
# Check for bundle commands in package.json scripts
echo "Checking package.json for bundle-related scripts:"
fd package.json -x jq -r '.scripts | to_entries | .[] | select(.value | contains("bundle"))'
Length of output: 396
Script:
#!/bin/bash
# Check for any CI configuration files that might contain Ruby or bundler setup
echo "Checking CI configuration files:"
fd -e yml -e yaml . .github/workflows/ -x cat {}
# Check if there are any other CI configuration files
echo -e "\nChecking for other CI config files:"
fd -e yml -e yaml . -x echo {}
# Check Ruby version files
echo -e "\nChecking Ruby version files:"
fd -g "*ruby*" -g "*gemfile*" -g ".ruby-version" -X cat {}
Length of output: 19267
Script:
#!/bin/bash
# Check for Ruby and bundler setup in iOS directory
echo "Checking iOS directory for Ruby and bundler setup:"
fd . ios/ -x cat {}
# Check for any EAS build configuration that might handle Ruby setup
echo -e "\nChecking EAS configuration:"
fd eas.json -x cat {}
Length of output: 66699
Script:
#!/bin/bash
# Check for Ruby and bundler setup in iOS directory, focusing on specific files
echo "Checking iOS Ruby configuration:"
fd -t f -e rb -e gemfile -e lock . ios/ -x cat {}
# Check for any Ruby version file
echo -e "\nChecking for Ruby version file:"
fd -t f -g ".ruby-version" . -x cat {}
Length of output: 69084
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
utils/logger.ts (1)
Line range hint
41-45: Add error handling for file operations.The file operations (
writeFile,appendFile) should include error handling to gracefully handle potential I/O failures.Consider wrapping file operations in try-catch blocks:
export const rotateLoggingFile = async () => { + try { if (loggingFilePath && (await RNFS.exists(loggingFilePath))) { await RNFS.unlink(loggingFilePath); } const tempDir = RNFS.TemporaryDirectoryPath; loggingFilePath = path.join(tempDir, `${uuidv4()}.converse.log.txt`); const appVersion = Constants.expoConfig?.version; const buildNumber = Platform.OS === "ios" ? Constants.expoConfig?.ios?.buildNumber : Constants.expoConfig?.android?.versionCode; await RNFS.writeFile( loggingFilePath, `Converse ${Platform.OS} logs - v${appVersion} (${buildNumber})\n\n` ); + } catch (error) { + console.error('Failed to rotate log file:', error); + // Consider sending this to Sentry + sentryTrackError('Log rotation failed', error); + } };Similarly for the append operation in
converseTransport:if (!loggingFilePath) { await rotateLoggingFile(); } + try { await RNFS.appendFile(loggingFilePath, `${props.msg}\n`, "utf8"); + } catch (error) { + console.error('Failed to append to log file:', error); + sentryTrackError('Log append failed', error); + }Also applies to: 66-69
🧹 Nitpick comments (5)
utils/getEnv.ts (1)
28-34: Consider caching the environment valueThe current implementation calls
getEnv()multiple times through the exported constants, which could lead to unnecessary environment checks.Consider caching the environment value:
+const currentEnv = (() => { + const isExpoEnvDev = process.env.EXPO_ENV === "dev"; + if (__DEV__ || isExpoEnvDev) { + return Environments.dev; + } else if (Constants.expoConfig?.extra?.ENV === "preview") { + return Environments.preview; + } else { + return Environments.prod; + } +})(); -export const getEnv = (): Environment => { +export const getEnv = (): Environment => currentEnv;utils/appCheck.ts (1)
17-18: Remove commented codeThere's commented out code that appears to be an alternative implementation. Either use it or remove it.
utils/apiConfig.ts (1)
69-73: Enhance process.env type declarationThe current type declaration is too permissive. Consider adding specific environment variable types.
declare const process: { env: { EXPO_PUBLIC_DEV_API_URI: string; EXPO_PUBLIC_PROD_API_URI?: string; NODE_ENV: 'development' | 'production' | 'test'; // Add other environment variables }; };config.ts (2)
76-78: Add validation for Firebase App Check debug tokens.While the implementation correctly handles platform-specific tokens, consider adding validation for the environment variables to fail fast if they're not properly configured.
Consider adding validation in the dev environment:
appCheckDebugToken: isAndroid - ? process.env.EXPO_PUBLIC_FIREBASE_APP_CHECK_DEBUG_TOKEN_ANDROID - : process.env.EXPO_PUBLIC_FIREBASE_APP_CHECK_DEBUG_TOKEN_IOS, + ? process.env.EXPO_PUBLIC_FIREBASE_APP_CHECK_DEBUG_TOKEN_ANDROID || + (function() { + throw new Error('Missing Android App Check debug token in dev environment'); + })() + : process.env.EXPO_PUBLIC_FIREBASE_APP_CHECK_DEBUG_TOKEN_IOS || + (function() { + throw new Error('Missing iOS App Check debug token in dev environment'); + })(),Also applies to: 97-97, 131-131
135-141: Add type safety for environment configuration.Consider adding type guards to ensure type safety when working with environment configurations.
Consider this improvement:
+type EnvConfig = typeof ENV[keyof typeof ENV]; + +function isValidEnv(env: unknown): env is EnvConfig { + return env === ENV.dev || env === ENV.preview || env === ENV.prod; +} + export const getConfig = () => { + let config: EnvConfig; if (isDev) { - return ENV.dev; + config = ENV.dev; } else if (isPreview) { - return ENV.preview; + config = ENV.preview; } else { - return ENV.prod; + config = ENV.prod; } + if (!isValidEnv(config)) { + throw new Error('Invalid environment configuration'); + } + return config; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app.config.ts(4 hunks)config.ts(5 hunks)package.json(2 hunks)utils/apiConfig.ts(1 hunks)utils/appCheck.ts(1 hunks)utils/getEnv.ts(1 hunks)utils/logger.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- app.config.ts
🔇 Additional comments (4)
utils/appCheck.ts (2)
26-44: Verify secure handling of debug tokens
The implementation includes debug tokens for development. This requires careful handling to ensure these tokens don't leak into production.
Let's verify the configuration:
✅ Verification successful
Let me check the config.ts file to understand how the debug tokens are handled.
Debug tokens are properly secured in the configuration
The implementation safely handles debug tokens through environment variables with proper environment segregation:
- Debug tokens are only enabled in development environment
- Preview and production environments explicitly set
appCheckDebugTokentoundefined - Debug tokens are sourced from environment variables (
EXPO_PUBLIC_FIREBASE_APP_CHECK_DEBUG_TOKEN_ANDROIDandEXPO_PUBLIC_FIREBASE_APP_CHECK_DEBUG_TOKEN_IOS) - The
__DEV__flag in appCheck.ts ensures debug provider is only used in development
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential debug token leaks in configuration files
rg -g '!node_modules' -g '!.git' -i 'appCheckDebugToken'
# Check EAS configuration
fd -g 'eas.json' -x cat {} | jq '.builds'
Length of output: 373
Script:
#!/bin/bash
# Check the implementation of debug tokens in config
cat config.ts
# Also check for any environment-specific configuration files
fd -g '*.env*' -x cat {}
Length of output: 4519
12-18: Address open questions in comments
The comments contain important questions about the implementation that should be addressed:
- "Do we need/want to use the limited use token?"
- "What endpoints are protected with app check?"
These decisions could have security implications.
Let's check the backend implementation:
✅ Verification successful
Use Limited Use Token for Enhanced Security
Based on the codebase analysis, I can provide clear answers to the implementation questions:
-
"Do we need/want to use the limited use token?" - Yes, the limited use token should be used. The code currently uses
getLimitedUseToken()which is the correct choice as it provides better security by limiting token reuse. -
"What endpoints are protected?" - The implementation shows App Check is used to protect:
- Authentication endpoint
/api/authenticate - All API requests through the
apiutility, as tokens are required in headers
The current implementation is secure because:
- App Check is properly initialized with platform-specific providers (Play Integrity for Android, App Attest for iOS)
- Debug tokens are only enabled in development
- Token auto-refresh is enabled
- API requests fail gracefully if App Check token is missing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the referenced middleware implementation
gh api repos/ephemeraHQ/converse-backend/contents/api/middlewares.ts | jq -r '.content' | base64 -d | grep -A 10 "app check"
Length of output: 246
Script:
#!/bin/bash
# Let's check the current repository for App Check implementation details
rg -A 5 "appCheck" --type ts
Length of output: 4406
Script:
#!/bin/bash
# Search for Firebase App Check configuration
rg -A 5 "getLimitedUseToken|getToken" --type ts
Length of output: 398
Script:
#!/bin/bash
# Look for any documentation about App Check setup
fd README.md | xargs cat | grep -i "app check" -A 5
Length of output: 53
utils/logger.ts (1)
13-13: LGTM! Environment check refactoring improves code organization.
The change to use isDev from a dedicated environment utility improves code organization by centralizing environment management.
Also applies to: 57-57
config.ts (1)
5-6: LGTM! API configuration modularization improves code organization.
Moving API URI configuration to a dedicated module improves maintainability and separation of concerns.
Also applies to: 57-58
| @@ -13,7 +13,7 @@ | |||
| "ios": "expo run:ios", | |||
| "run:ios": "EXPO_ENV=dev expo run:ios", | |||
| "postinstall": "patch-package && node scripts/wasm.js && husky install && cross-os postinstall", | |||
| "start": "EXPO_ENV=dev expo start", | |||
| "start": "EXPO_ENV=dev expo start --scheme converse-dev", | |||
There was a problem hiding this comment.
physical device runs weren't working without this
There was a problem hiding this comment.
this file fixes connecting to converse-backend on LAN via physical device
There was a problem hiding this comment.
i'll do better type validation on env later, just wanted to get this cleaned up for now
| @@ -5,6 +5,7 @@ | |||
| "android": "EXPO_ENV=dev expo run:android", | |||
| "android:build:dev": "eas build -p android --profile development --local", | |||
| "android:reverse": "adb reverse tcp:8081 tcp:8081 && adb reverse tcp:9875 tcp:9875", | |||
| "android:sendApk": "./scripts/android/sendApk.sh", | |||
There was a problem hiding this comment.
some android physical device qol improvements: https://www.loom.com/share/867a810a8df34a29bb45845d50e01a37
replaces #1369
some android physical device qol improvements: https://www.loom.com/share/867a810a8df34a29bb45845d50e01a37
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores