Conversation
…build-qa and build-release
…variables in Bitrise workflows
…nt build types in Bitrise workflows
….4 in Bitrise workflows
…s in Bitrise workflows
… in AppLogger" This reverts commit c194f42.
Summary of ChangesHello @SuHyeon00, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 Bitrise를 활용한 CI/CD 환경을 구축하여 개발 및 배포 프로세스를 자동화하고 효율성을 높이는 데 중점을 둡니다. 또한, Firebase 서비스를 통합하여 앱의 안정성 모니터링(Crashlytics), 원격 설정 관리(Remote Config), 사용자 분석(Analytics) 기능을 강화했습니다. 이와 함께 네트워크 통신 및 토큰 관리 로직을 개선하여 앱의 견고성을 향상시키고, 로깅 시스템을 중앙화하여 디버깅 및 문제 해결을 용이하게 했습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR successfully establishes the Bitrise CI/CD environment and integrates Firebase for remote configuration and crash reporting, including the introduction of a qa build type and dynamic base URL switching via Firebase Remote Config. It also refactors to a centralized AppLogger and makes TokenManager thread-safe. However, significant security concerns have been identified. The AppLogger performs insecure logging, potentially sending PII and authentication tokens to Firebase Crashlytics across all build types. Furthermore, the dynamic base URL override via Firebase Remote Config is unvalidated, which could lead to sensitive API request interception and user access token theft if the Remote Config source is compromised. Proper log filtering and domain validation are crucial to mitigate these risks. While the overall code quality is high, general improvements are also suggested for code duplication and potential threading issues.
| fun setDomain(domain: String) { | ||
| if (domain.isNotBlank() && domain != current) { | ||
| current = domain | ||
| } | ||
| } |
There was a problem hiding this comment.
The application allows overriding the API base URL via Firebase Remote Config without validation. This URL is used by HostSelectionInterceptor to redirect all API requests. Since AccessTokenInterceptor adds the user's access token to these requests, a compromised Remote Config could be used to redirect traffic to a malicious server and steal user credentials.
It is highly recommended to validate the domain string against an allowlist of trusted domains before updating the current domain.
| override fun clearCachedTokens() { | ||
| cachedAccessToken = null | ||
| cachedRefreshToken = null | ||
| runBlocking { | ||
| mutex.withLock { | ||
| cachedAccessToken = null | ||
| cachedRefreshToken = null | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override fun setCachedTokens(accessToken: String, refreshToken: String?) { | ||
| cachedAccessToken = accessToken | ||
| cachedRefreshToken = refreshToken | ||
| runBlocking { | ||
| mutex.withLock { | ||
| cachedAccessToken = accessToken | ||
| cachedRefreshToken = refreshToken | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
clearCachedTokens와 setCachedTokens 함수에서 runBlocking을 사용하고 있습니다. 이 함수들이 메인 스레드에서 호출될 경우 UI 스레드를 차단하여 ANR(Application Not Responding)을 유발할 수 있습니다. AccessTokenProvider 인터페이스의 제약으로 인해 suspend 키워드를 사용하기 어렵다면, 이 함수들이 백그라운드 스레드에서만 호출되도록 특별히 주의해야 합니다. 가능하다면 suspend 함수로 리팩토링하는 것을 장기적으로 고려해보시는 것이 좋습니다.
| fun d(tag: String, message: String) { | ||
| if (isDebug) Log.d(tag, message) | ||
| Firebase.crashlytics.log("$PREFIX_DEBUG$tag: $message") | ||
| } |
There was a problem hiding this comment.
The AppLogger.d implementation sends all log messages to Firebase Crashlytics regardless of the build type. While Log.d is conditionally called based on isDebug, Firebase.crashlytics.log is always executed. This can lead to sensitive information being sent to a third-party service even in release builds.
Specifically, BaseViewModel.kt (line 42) logs the entire dataResource.data object on success, which often contains PII such as user email addresses (see User and UserEntity models). Additionally, GoogleLoginHelper.kt (lines 91 and 104) logs Google ID tokens in debug builds, which are then transmitted to Crashlytics via this function.
| applicationIdSuffix = ".qa" | ||
| versionNameSuffix = "-qa" | ||
| matchingFallbacks += listOf("debug") | ||
| buildConfigField("String", "BASE_URL", "\"${localProperties["base_url"] ?: ""}\"") |
There was a problem hiding this comment.
qa 빌드 유형에서 buildConfigField를 다시 정의하고 있습니다. initWith(getByName("debug"))를 통해 debug 빌드 유형의 설정을 이미 상속받았기 때문에 이 부분은 중복됩니다. 코드의 간결성과 유지보수성을 위해 이 라인을 제거하는 것이 좋습니다.
|
|
||
| if (attempt < maxAttempts - 1) { | ||
| delay(backoff) | ||
| backoff = (backoff * 2).coerceAtMost(8_000L) |
There was a problem hiding this comment.
8_000L이라는 매직 넘버가 사용되었습니다. 이 값을 클래스 수준의 상수로 추출하면 코드의 가독성과 유지보수성이 향상됩니다. 예를 들어, 클래스 상단에 private const val MAX_BACKOFF_MS = 8_000L와 같이 정의하고, 이 라인에서는 backoff = (backoff * 2).coerceAtMost(MAX_BACKOFF_MS)와 같이 사용하는 것을 권장합니다.
| if [ -z "$BUILD_TYPE" ]; then | ||
| if [ "$BITRISE_GIT_BRANCH" = "main" ] || [ "$BITRISE_GIT_BRANCH" = "master" ]; then | ||
| BUILD_TYPE="release" | ||
| else | ||
| BUILD_TYPE="qa" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
BUILD_TYPE을 결정하는 로직이 중복되어 있습니다. 이 스크립트("Setup google-services.json files")에서 BUILD_TYPE을 결정하지만, 바로 다음 스크립트("Set environment variables")에서도 동일한 목적으로 BUILD_TYPE을 결정하고 envman을 통해 환경 변수로 설정하고 있습니다. 후자의 값이 빌드 전반에 사용되므로, 이 블록은 불필요한 중복입니다. 코드의 명확성을 위해 이 if [ -z "$BUILD_TYPE" ] 블록을 제거하는 것을 권장합니다.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| firebase-bom = { module = "com.google.firebase:firebase-bom", version.ref = "firebaseBom" } | ||
| firebase-config-ktx = { module = "com.google.firebase:firebase-config-ktx" } | ||
| firebase-analytics-ktx = { module = "com.google.firebase:firebase-analytics-ktx" } | ||
| firebase-crashlytics-ktx = { module = "com.google.firebase:firebase-crashlytics-ktx", version.ref = "crashlytics" } |
There was a problem hiding this comment.
Firebase Crashlytics version bypasses BOM management
Medium Severity
The firebase-crashlytics-ktx library has an explicit version (18.6.2) specified via version.ref = "crashlytics", while firebase-config-ktx and firebase-analytics-ktx rely on the BOM for version management. When using Firebase BOM with platform(), libraries with explicit versions override the BOM-managed versions. This inconsistency means Crashlytics will use version 18.6.2 while other Firebase libraries use whatever BOM 33.1.0 specifies, potentially causing runtime compatibility issues between Firebase components.
| mutex.withLock { | ||
| cachedAccessToken = prefDataSource.get(ACCESS_TOKEN_KEY, "").ifEmpty { null } | ||
| cachedRefreshToken = prefDataSource.get(REFRESH_TOKEN_KEY, "").ifEmpty { null } | ||
| } |
There was a problem hiding this comment.
Mutex held during I/O causes potential main thread blocking
Medium Severity
The preloadTokens() function holds the mutex while performing I/O via prefDataSource.get() (suspend calls). Meanwhile, clearCachedTokens() and setCachedTokens() use runBlocking { mutex.withLock {...} }. If either of these non-suspend interface methods is called from the main thread while preloadTokens() is holding the mutex during I/O, the main thread will block until the I/O completes, potentially causing ANR. The I/O operations ideally would be performed outside the mutex lock, with the lock only protecting the in-memory cache updates.
Additional Locations (1)
| .connectTimeout(TIME_OUT.toLong(), TimeUnit.SECONDS) | ||
| .readTimeout(TIME_OUT.toLong(), TimeUnit.SECONDS) | ||
| .writeTimeout(TIME_OUT.toLong(), TimeUnit.SECONDS) | ||
| .addInterceptor(hostSelectionInterceptor) |
There was a problem hiding this comment.
Refresh token API ignores dynamic domain configuration
High Severity
The new HostSelectionInterceptor is added to the main OkHttpClient (used by MainRetrofit) but not to RefreshRetrofit. This means main API calls respect the dynamic domain from DomainProvider (set via Firebase Remote Config), while token refresh calls always go to the static BuildConfig.BASE_URL. If the domain is updated at runtime, subsequent token refresh attempts will fail because they hit the wrong server, causing authentication failures and potential user logouts.
Additional Locations (1)
1 participant
📝 작업 내용
📸 스크린샷 (선택)
💬 리뷰 요구사항(선택)
Note
Medium Risk
Introduces dynamic base URL switching via Firebase Remote Config and adjusts token refresh/caching logic, which can affect networking/auth flows if misconfigured. CI/build variant changes (new
qatype, Google services/Crashlytics setup) could also impact release packaging and runtime configuration.Overview
Adds Bitrise CI (
bitrise.yml) withbuild-qaandbuild-releaseworkflows that generatelocal.properties, provisiongoogle-services.jsonper build type, build/sign APKs, deploy artifacts, and optionally notify Discord.Integrates Firebase (Analytics, Remote Config, Crashlytics) and adds a new
qabuild type plus debug/release suffixing; app startup now fetches Remote Config with retry/backoff to dynamically override the API base domain viaDomainProvider+HostSelectionInterceptor.Centralizes logging into
AppLogger(Logcat + Crashlytics) and adds Crashlytics user/build context (UserContext), wiring it into app init and Google login; also tightens token refresh/error handling (suspend refresh API, explicit refresh failures, broader unauthorized detection) and makesTokenManagerthread-safe with preloading + mutex.Written by Cursor Bugbot for commit dd37659. This will update automatically on new commits. Configure here.