fix(W-23695429): escape single quotes in SmartSQL IN-clause record IDs - #2992
Conversation
Escape single quotes in the ID lists passed to deleteRecordsFromLocalStore in SyncTarget.kt to satisfy scanner rule BUG-03 (SQL injection). The risk is academic since SmartStore is a per-user encrypted database and IDs come from the server, but sanitizing values before interpolating into SQL is correct hygiene.
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Reviewed at HEAD 1c5c37c. This escapes single quotes (' -> '') in each record ID before they are joined into the SmartSQL IN (...) literal in SyncTarget.deleteRecordsFromLocalStore. That is the correct SQLite/SmartSQL escaping, and it matches the already-approved-and-merged iOS twin (PR 4131, same W-23695429). Approving.
What I verified
- Correctness:
String.replace("'", "''")replaces every literal occurrence;SmartSqlHelperonly rewrites{soup:path}tokens, so quoted string literals (and the doubled quotes) pass through to SQLite unchanged. There is no bind-parameter API on the SmartSQL path, so escaping is the correct and only remediation here. - Callers: all three callers of
deleteRecordsFromLocalStore(SyncDownTarget.cleanGhosts,ParentChildrenSyncDownTarget.cleanGhosts,BriefcaseSyncDownTarget) flow through the single escaped path. - Cross-platform parity: same escaping semantics as the approved iOS change.
- Backward compatibility: internal method-body change only; no public API, signature, string, dependency, permission, or build change.
- CI: the two red checks look environmental rather than a regression from this change. The
unit-tests-pr (MobileSync)failures are dominated byREQUEST_LIMIT_EXCEEDED(shared test-org API quota) with NullPointerException / "Creates failed" cascades downstream, concentrated in sync-up tests that this sync-down change does not touch; the same failures appear on unrelated PRs in the same window, and the dev nightly (~9h later, after quota reset) is green (251/251 on API 31/32/33/34/36). Theui-tests-prfailures are the known AuthFlowTester Custom-Tab / creds-section flake. Recommend re-running both jobs on a fresh quota window before merge.
One follow-up worth filing (out of scope for this surgical fix): a sibling local-store SmartSQL path with the same trust boundary is still unescaped on both platforms -- see the inline note.
This review was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
…tIds in getQueryForChildren - Add ParentChildrenSyncTargetHelperTest: @smallTest verifying that parent IDs containing single quotes are doubled (001'Test -> 001''Test) in the SmartSQL IN-clause built by getQueryForChildren. - Fix getQueryForChildren in ParentChildrenSyncTargetHelper to escape single quotes in parentIds (parallel to the existing fix in deleteRecordsFromLocalStore). - Add SyncTargetSingleQuoteTest: @LargeTest exercising deleteRecordsFromLocalStore end-to-end — upserts a record with ID "001'Quote" and one with "001Normal", calls deleteRecordsFromLocalStore for the quote ID, asserts only "001Normal" remains.
|
Thanks for the quick turnaround, Wolf — appreciate you folding both notes into One thing to keep on the radar: the iOS twin (PR 4131) merged without the parent-children escaping, so that sibling path is still unescaped on iOS. Might be worth a small follow-up there to keep the two platforms in parity. This review was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
2 participants
Summary
deleteRecordsFromLocalStoreinSyncTarget.ktIN ('id1','id2',...)clause; without escaping a single quote in an ID could break the queryTest plan
./gradlew :libs:MobileSync:build— confirmed passing./gradlew :libs:MobileSync:connectedAndroidTest