From 0ab2b010eb3254a0d923b68de1c02c0d3dcaf083 Mon Sep 17 00:00:00 2001
From: Rituraj Sambherao <54317407+ritsam@users.noreply.github.com>
Date: Mon, 14 Sep 2020 16:29:10 +0100
Subject: [PATCH] QR Code stricter validation (EXPOSUREAPP-2570) (#1127)
* test for OnboardingFragment
* QR code stricter validations
* regex pattern improvement
* removed Obsolete test
* lint changes to fix pipeline issue
* logic refinement and test coverage improvement
* pipeline related fixes
* piepline related fixes : unnecessary test removed
* pipeline fixes and increased tests coverage
* QR code validation logic improvement
* detekt pipeline issue resolved
* pipeline issue Fix
* logic ovehaul and improvement for QRCode scans / validations
1, Data object for ScanResults
2. new Test for ScanResults
3. logic simplified
* removing unused imports from tests
* Small refactoring together with Rituraj.
* Adjust detekt, early abort via return is GOOD!
Co-authored-by: Matthias Urhahn <darken@darken.eu>
Co-authored-by: Matthias Urhahn <matthias.urhahn@sap.com>
---
Corona-Warn-App/config/detekt.yml | 2 +-
.../service/submission/QRScanResult.kt | 32 +++++++++++
.../service/submission/SubmissionConstants.kt | 4 --
.../service/submission/SubmissionService.kt | 13 +----
.../ui/viewmodel/SubmissionViewModel.kt | 9 ++--
.../service/submission/ScanResultTest.kt | 54 +++++++++++++++++++
.../submission/SubmissionConstantsTest.kt | 11 ++--
.../submission/SubmissionServiceTest.kt | 22 --------
.../ui/viewmodel/SubmissionViewModelTest.kt | 2 +-
9 files changed, 102 insertions(+), 47 deletions(-)
create mode 100644 Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/QRScanResult.kt
create mode 100644 Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/ScanResultTest.kt
diff --git a/Corona-Warn-App/config/detekt.yml b/Corona-Warn-App/config/detekt.yml
index 1a5fd4a6d..2555aa687 100644
--- a/Corona-Warn-App/config/detekt.yml
+++ b/Corona-Warn-App/config/detekt.yml
@@ -572,7 +572,7 @@ style:
excludedFunctions: 'equals'
excludeLabeled: false
excludeReturnFromLambda: true
- excludeGuardClauses: false
+ excludeGuardClauses: true
SafeCast:
active: true
SerialVersionUIDInSerializableClass:
diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/QRScanResult.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/QRScanResult.kt
new file mode 100644
index 000000000..783ec13a2
--- /dev/null
+++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/QRScanResult.kt
@@ -0,0 +1,32 @@
+package de.rki.coronawarnapp.service.submission
+
+import java.util.regex.Pattern
+
+data class QRScanResult(val rawResult: String) {
+
+ val isValid: Boolean
+ get() = guid != null
+ val guid: String? by lazy { extractGUID(rawResult) }
+
+ private fun extractGUID(rawResult: String): String? {
+ if (rawResult.length > MAX_QR_CODE_LENGTH) return null
+ if (rawResult.count { it == GUID_SEPARATOR } != 1) return null
+ if (!QR_CODE_REGEX.toRegex().matches(rawResult)) return null
+
+ val potentialGUID = rawResult.substringAfterLast(GUID_SEPARATOR, "")
+ if (potentialGUID.isBlank() || potentialGUID.length > MAX_GUID_LENGTH) return null
+
+ return potentialGUID
+ }
+
+ companion object {
+ // regex pattern for scanned QR code URL
+ val QR_CODE_REGEX: Pattern = Pattern.compile(
+ "^((^https:\\/{2}localhost)(\\/\\?)[A-Fa-f0-9]{6}" +
+ "[-][A-Fa-f0-9]{8}[-][A-Fa-f0-9]{4}[-][A-Fa-f0-9]{4}[-][A-Fa-f0-9]{4}[-][A-Fa-f0-9]{12})\$"
+ )
+ const val GUID_SEPARATOR = '?'
+ const val MAX_QR_CODE_LENGTH = 150
+ const val MAX_GUID_LENGTH = 80
+ }
+}
diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionConstants.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionConstants.kt
index 4ab404508..43de684e2 100644
--- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionConstants.kt
+++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionConstants.kt
@@ -14,10 +14,6 @@ object SubmissionConstants {
val TEST_RESULT_URL = "$VERSIONED_VERIFICATION_CDN_URL/$TEST_RESULT"
val TAN_REQUEST_URL = "$VERSIONED_VERIFICATION_CDN_URL/$TAN"
- const val MAX_QR_CODE_LENGTH = 150
- const val MAX_GUID_LENGTH = 80
- const val GUID_SEPARATOR = '?'
-
const val SERVER_ERROR_CODE_400 = 400
const val EMPTY_HEADER = ""
diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionService.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionService.kt
index defce20ed..e33668fa5 100644
--- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionService.kt
+++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionService.kt
@@ -65,19 +65,10 @@ object SubmissionService {
}
fun containsValidGUID(scanResult: String): Boolean {
- if (scanResult.length > SubmissionConstants.MAX_QR_CODE_LENGTH ||
- scanResult.count { it == SubmissionConstants.GUID_SEPARATOR } != 1
- )
- return false
-
- val potentialGUID = extractGUID(scanResult)
-
- return !(potentialGUID.isEmpty() || potentialGUID.length > SubmissionConstants.MAX_GUID_LENGTH)
+ val scanResult = QRScanResult(scanResult)
+ return scanResult.isValid
}
- fun extractGUID(scanResult: String): String =
- scanResult.substringAfterLast(SubmissionConstants.GUID_SEPARATOR, "")
-
fun storeTestGUID(guid: String) = LocalData.testGUID(guid)
fun deleteTestGUID() {
diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/viewmodel/SubmissionViewModel.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/viewmodel/SubmissionViewModel.kt
index 2fced7642..c5f670cb0 100644
--- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/viewmodel/SubmissionViewModel.kt
+++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/viewmodel/SubmissionViewModel.kt
@@ -9,6 +9,7 @@ import de.rki.coronawarnapp.exception.ExceptionCategory
import de.rki.coronawarnapp.exception.TransactionException
import de.rki.coronawarnapp.exception.http.CwaWebException
import de.rki.coronawarnapp.exception.reporting.report
+import de.rki.coronawarnapp.service.submission.QRScanResult
import de.rki.coronawarnapp.service.submission.SubmissionService
import de.rki.coronawarnapp.storage.LocalData
import de.rki.coronawarnapp.storage.SubmissionRepository
@@ -98,10 +99,10 @@ class SubmissionViewModel : ViewModel() {
_uiStateError
)
- fun validateAndStoreTestGUID(scanResult: String) {
- if (SubmissionService.containsValidGUID(scanResult)) {
- val guid = SubmissionService.extractGUID(scanResult)
- SubmissionService.storeTestGUID(guid)
+ fun validateAndStoreTestGUID(rawResult: String) {
+ val scanResult = QRScanResult(rawResult)
+ if (scanResult.isValid) {
+ SubmissionService.storeTestGUID(scanResult.guid!!)
_scanStatus.value = Event(ScanStatus.SUCCESS)
} else {
_scanStatus.value = Event(ScanStatus.INVALID)
diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/ScanResultTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/ScanResultTest.kt
new file mode 100644
index 000000000..2f7e78179
--- /dev/null
+++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/ScanResultTest.kt
@@ -0,0 +1,54 @@
+package de.rki.coronawarnapp.service.submission
+
+import io.kotest.matchers.shouldBe
+import io.mockk.MockKAnnotations
+import io.mockk.every
+import io.mockk.impl.annotations.MockK
+import io.mockk.mockkObject
+import org.junit.Before
+import org.junit.Test
+
+class ScanResultTest {
+ private val guid = "123456-12345678-1234-4DA7-B166-B86D85475064"
+
+ @MockK
+ private lateinit var scanResult: QRScanResult
+
+ @Before
+ fun setUp() {
+ MockKAnnotations.init(this)
+ mockkObject(scanResult)
+ every { scanResult.isValid } returns false
+ }
+
+ @Test
+ fun containsValidGUID() {
+ //valid test
+ scanResult = QRScanResult("https://localhost/?$guid")
+ scanResult.isValid shouldBe true
+
+ // more invalid tests checks
+ scanResult = QRScanResult("http://localhost/?$guid")
+ scanResult.isValid shouldBe false
+ scanResult = QRScanResult("https://localhost/?")
+ scanResult.isValid shouldBe false
+ scanResult = QRScanResult("htps://wrongformat.com")
+ scanResult.isValid shouldBe false
+ scanResult =
+ QRScanResult("https://localhost/%20?3D6D08-3567F3F2-4DCF-43A3-8737-4CD1F87D6FDA")
+ scanResult.isValid shouldBe false
+ scanResult =
+ QRScanResult("https://some-host.com/?3D6D08-3567F3F2-4DCF-43A3-8737-4CD1F87D6FDA")
+ scanResult.isValid shouldBe false
+ scanResult = QRScanResult("https://localhost/?3567F3F2-4DCF-43A3-8737-4CD1F87D6FDA")
+ scanResult.isValid shouldBe false
+ scanResult = QRScanResult("https://localhost/?4CD1F87D6FDA")
+ scanResult.isValid shouldBe false
+ }
+
+ @Test
+ fun extractGUID() {
+ QRScanResult("https://localhost/?$guid").guid shouldBe guid
+ }
+}
+
diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/SubmissionConstantsTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/SubmissionConstantsTest.kt
index 98efa5fec..b65d00b69 100644
--- a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/SubmissionConstantsTest.kt
+++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/SubmissionConstantsTest.kt
@@ -11,13 +11,16 @@ class SubmissionConstantsTest {
Assert.assertEquals(KeyType.GUID.name, "GUID")
Assert.assertEquals(KeyType.TELETAN.name, "TELETAN")
- Assert.assertEquals(SubmissionConstants.REGISTRATION_TOKEN_URL, "version/v1/registrationToken")
+ Assert.assertEquals(
+ SubmissionConstants.REGISTRATION_TOKEN_URL,
+ "version/v1/registrationToken"
+ )
Assert.assertEquals(SubmissionConstants.TEST_RESULT_URL, "version/v1/testresult")
Assert.assertEquals(SubmissionConstants.TAN_REQUEST_URL, "version/v1/tan")
- Assert.assertEquals(SubmissionConstants.MAX_QR_CODE_LENGTH, 150)
- Assert.assertEquals(SubmissionConstants.MAX_GUID_LENGTH, 80)
- Assert.assertEquals(SubmissionConstants.GUID_SEPARATOR, '?')
+ Assert.assertEquals(QRScanResult.MAX_QR_CODE_LENGTH, 150)
+ Assert.assertEquals(QRScanResult.MAX_GUID_LENGTH, 80)
+ Assert.assertEquals(QRScanResult.GUID_SEPARATOR, '?')
Assert.assertEquals(SubmissionConstants.SERVER_ERROR_CODE_400, 400)
diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/SubmissionServiceTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/SubmissionServiceTest.kt
index fef637070..f1d222553 100644
--- a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/SubmissionServiceTest.kt
+++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/SubmissionServiceTest.kt
@@ -163,26 +163,4 @@ class SubmissionServiceTest {
}
}
- @Test
- fun containsValidGUID() {
- // valid
- assertThat(
- SubmissionService.containsValidGUID("https://bs-sd.de/covid-19/?$guid"),
- equalTo(true)
- )
-
- // invalid
- assertThat(
- SubmissionService.containsValidGUID("https://no-guid-here"),
- equalTo(false)
- )
- }
-
- @Test
- fun extractGUID() {
- assertThat(
- SubmissionService.extractGUID("https://bs-sd.de/covid-19/?$guid"),
- equalTo(guid)
- )
- }
}
diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/viewmodel/SubmissionViewModelTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/viewmodel/SubmissionViewModelTest.kt
index a7fa73cbf..5a93e5ac2 100644
--- a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/viewmodel/SubmissionViewModelTest.kt
+++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/viewmodel/SubmissionViewModelTest.kt
@@ -51,7 +51,7 @@ class SubmissionViewModelTest {
// valid guid
val guid = "123456-12345678-1234-4DA7-B166-B86D85475064"
- viewModel.validateAndStoreTestGUID("https://bs-sd.de/covid-19/?$guid")
+ viewModel.validateAndStoreTestGUID("https://localhost/?$guid")
viewModel.scanStatus.value?.getContent().let { Assert.assertEquals(ScanStatus.SUCCESS, it) }
// invalid guid
--
GitLab