[CLX-3799][Horizon] Refactor domain services api #3488
Conversation
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Mon, 26 Jan 2026 09:27:27 GMT |
|
Student Install Page |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary - Consolidate Redwood and Journey Services
I've completed a comprehensive review of this PR which introduces a GraphQL proxy pattern where Journey forwards Redwood operations. This is a significant architectural change with both benefits and concerns.
🎯 Architecture Overview
What Changed:
- Journey now acts as a proxy for all Redwood GraphQL operations
- New
ExecuteRedwoodQueryMutationwraps Redwood queries/mutations as JSON strings - Consolidated authentication using single Journey JWT token
- Added region-aware URL resolution with
{region}template placeholders - Removed separate
RedwoodModuleand related infrastructure
Files Changed: 27 files (+732/-488 lines)
- Core changes in
canvas-api-2module - Integration test updates in
horizonmodule - Login flow updates in
login-api-2module
🔴 Critical Issues (Must Fix)
- GraphQL Injection Risk - Proxy accepts arbitrary operations without validation
- Unvalidated JSON Deserialization - Missing error handling could cause crashes
- Missing Error Propagation - Redwood errors not checked before parsing
- No Unit Tests -
JourneyRedwoodManagerImplhas complex logic but no dedicated tests
⚠️ Important Issues (Should Fix)
- Region Validation - No sanitization of region string used in URLs
- Null Handling Bug -
canvasRegioncould overwrite valid values with null - Data Loss - Preference name change will lose existing tokens on upgrade
- Performance - Double JSON serialization adds overhead
✅ Positive Aspects
- Clean service consolidation
- Consistent Dagger Hilt patterns
- Type-safe proxy maintains compile-time safety
- All integration tests properly updated
📋 Recommendations
- Add operation allowlist for security validation
- Implement comprehensive error handling in proxy layer
- Create unit tests for
JourneyRedwoodManagerImpl - Add region format validation to prevent URL injection
- Document architectural tradeoffs in KDoc
- Consider feature flags for gradual rollout
📊 Testing Status
- ✅ Integration tests updated (8 test files)
- ❌ Unit tests missing for new manager class
- ❌ Region resolution not tested
- ❌ Proxy behavior not verified in integration tests
Please address the critical security and bug issues before merging. I've added inline comments with specific recommendations for each issue.
| ) | ||
|
|
||
| val mutation = ExecuteRedwoodQueryMutation(input) | ||
| val response = journeyClient.enqueueMutation(mutation).dataAssertNoErrors |
There was a problem hiding this comment.
🔴 CRITICAL - Security: GraphQL Injection Risk
The executeRedwoodQuery method forwards arbitrary GraphQL operations to the Journey proxy without validation. This creates a potential security vulnerability.
Recommendation: Add an allowlist of permitted Redwood operations:
companion object {
private val ALLOWED_REDWOOD_OPERATIONS = setOf(
"QueryNotes", "CreateNote", "UpdateNote", "DeleteNote",
"GetWidgetByTypeAndUser", "CreateWidget"
)
}
private suspend fun <D : Query.Data> executeRedwoodQuery(operation: Query<D>): D {
val operationName = operation.name()
require(operationName in ALLOWED_REDWOOD_OPERATIONS) {
"Unauthorized Redwood operation: $operationName"
}
// ... rest of implementation
}| } | ||
|
|
||
| val jsonString = gson.toJson(data) | ||
| val buffer = Buffer().writeUtf8(jsonString) |
There was a problem hiding this comment.
🔴 CRITICAL - Security: Unvalidated JSON Deserialization
The parseResponse method deserializes JSON from the Journey proxy without validation or error handling. This could lead to runtime crashes or type confusion.
Recommendation: Add proper error handling:
private fun <D : Operation.Data> parseResponse(
data: Any?,
operation: Operation<D>
): D {
if (data == null) {
throw IllegalStateException("Redwood query returned null data")
}
try {
val jsonString = gson.toJson(data)
val buffer = Buffer().writeUtf8(jsonString)
val jsonReader = buffer.jsonReader()
return operation.adapter().fromJson(jsonReader, customScalarAdapters)
} catch (e: Exception) {
Logger.e("Failed to parse Redwood response", e)
throw IllegalStateException("Invalid Redwood response format", e)
}
}| val input = RedwoodQueryInput( | ||
| query = queryString, | ||
| variables = Optional.presentIfNotNull(variables), | ||
| operationName = Optional.presentIfNotNull(operation.name()) |
There was a problem hiding this comment.
🔴 HIGH - Bug: Missing Error Propagation
The code checks for Journey-level errors with dataAssertNoErrors but doesn't check for Redwood-level errors returned in the response. The schema includes errors: [JSON!] field that should be validated.
Recommendation:
val apolloResponse = journeyClient.enqueueMutation(mutation)
val response = apolloResponse.dataAssertNoErrors
// Check for Redwood-level errors
val redwoodErrors = response.executeRedwoodQuery.errors
if (!redwoodErrors.isNullOrEmpty()) {
throw RedwoodApiException("Redwood query failed: $redwoodErrors")
}
return parseResponse(response.executeRedwoodQuery.data, operation)| return resolveRegion(baseUrlTemplate) | ||
| } | ||
|
|
||
| private fun resolveRegion(urlTemplate: String): String { |
There was a problem hiding this comment.
The resolveRegion method performs string replacement without validating the region value. If canvasRegion ever becomes controllable or contains malicious characters, it could manipulate URLs.
Recommendation: Add validation:
private fun resolveRegion(urlTemplate: String): String {
if (!urlTemplate.contains("{region}")) {
return urlTemplate
}
val region = ApiPrefs.canvasRegion?.takeIf { it.isNotEmpty() } ?: "us-east-1"
// Validate region format (alphanumeric + dashes only)
if (!region.matches(Regex("^[a-z0-9-]+$"))) {
Logger.w("Invalid region format: $region, using default")
return urlTemplate.replace("{region}", "us-east-1")
}
return urlTemplate.replace("{region}", region)
}| logEvent(AnalyticsEventConstants.LOGIN_SUCCESS, bundle) | ||
| refreshToken = token!!.refreshToken!! | ||
| accessToken = token.accessToken!! | ||
| canvasRegion = token.canvasRegion |
There was a problem hiding this comment.
The canvasRegion assignment could overwrite a valid existing region with null if the token response doesn't include a region value.
Recommendation: Only update if present:
refreshToken = token!!.refreshToken!!
accessToken = token.accessToken!!
token.canvasRegion?.let { canvasRegion = it } // Only update if present| } | ||
|
|
||
| object JourneyApiPref: DomainServicesApiPref("journey_api_prefs") { | ||
| object JourneyApiPref: DomainServicesApiPref("journey_api_pref") { |
There was a problem hiding this comment.
Changing the preference name from "journey_api_prefs" to "journey_api_pref" will cause existing Journey tokens to be lost on app upgrade, requiring users to re-authenticate.
Recommendation: Either revert to the original name or add migration logic:
object JourneyApiPref: DomainServicesApiPref("journey_api_pref") {
override var token: String? by NStringPref(null, "journey_token")
init {
// Migration from old preference name
if (token == null) {
val oldPrefs = context.getSharedPreferences("journey_api_prefs", Context.MODE_PRIVATE)
token = oldPrefs.getString("journey_token", null)
}
}
}| class JourneyRedwoodManagerImpl @Inject constructor( | ||
| @JourneyApolloClient private val journeyClient: ApolloClient | ||
| ) : RedwoodApiManager { | ||
|
|
There was a problem hiding this comment.
ℹ️ LOW - Code Quality: Gson Configuration
Using default Gson() without configuration may lead to issues with null handling, date formats, or special types.
Recommendation:
private val gson = GsonBuilder()
.setDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'")
.serializeNulls()
.create()
refs: CLX-3799
affects: Student
release note: none