Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: resolve catastrophic backtracing issue with semver regex #7

Merged
merged 2 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 10 additions & 15 deletions evaluation-core/src/commonMain/kotlin/SemanticVersion.kt
Original file line number Diff line number Diff line change
@@ -1,19 +1,14 @@
package com.amplitude.experiment.evaluation

// major and minor should be non-negative numbers separated by a dot
private const val MAJOR_MINOR_REGEX = "(\\d+)\\.(\\d+)"

// patch should be a non-negative number
private const val PATCH_REGEX = "(\\d+)"

// prerelease is optional. If provided, it should be a hyphen followed by a
// series of dot separated identifiers where an identifer can contain anything in [-0-9a-zA-Z]
private const val PRERELEASE_REGEX = "(-(([-\\w]+\\.?)*))?"

// version pattern should be major.minor(.patchAndPreRelease) where .patchAndPreRelease is optional
private const val VERSION_PATTERN = "$MAJOR_MINOR_REGEX(\\.$PATCH_REGEX$PRERELEASE_REGEX)?$"

private val regex = Regex(VERSION_PATTERN)
/**
* Copied from: https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
*
* Modified to:
* - Support versions starting with 0 (e.g. 01.01.01)
* - Support versions with only major and minor versions (e.g. 1.1)
*/
private const val VERSION_PATTERN = "^(0|[0-9]\\d*)\\.(0|[0-9]\\d*)(\\.(0|[0-9]\\d*)(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?)?$"
private val pattern = Regex(VERSION_PATTERN)

/**
* Implementation of Semantic version specification as per the spec in
Expand All @@ -39,7 +34,7 @@ internal data class SemanticVersion(
if (version == null) {
return null
}
val matchGroup = regex.matchEntire(version)?.groupValues ?: return null
val matchGroup = pattern.matchEntire(version)?.groupValues ?: return null
val major = matchGroup[1].toIntOrNull() ?: return null
val minor = matchGroup[2].toIntOrNull() ?: return null
val patch = matchGroup[4].toIntOrNull() ?: 0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the matchGroup indices still correct? Does it start from 0? What do index 0 and 3 represent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

Screenshot 2024-07-23 at 11 29 51 PM

0 is the full match
3 is everything after the 2nd .

Expand Down
18 changes: 18 additions & 0 deletions evaluation-core/src/commonTest/kotlin/SemanticVersionTest.kt
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
package com.amplitude.experiment.evaluation

import kotlinx.coroutines.DelicateCoroutinesApi
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import kotlinx.coroutines.runBlocking
import kotlin.test.Test
import kotlin.test.assertTrue
import kotlin.test.fail

@DelicateCoroutinesApi
class SemanticVersionTest {

@Test
Expand Down Expand Up @@ -106,6 +111,19 @@ class SemanticVersionTest {
assertVersionComparison("20.5.6-b1.2.x", EvaluationOperator.VERSION_GREATER_THAN, "20.5.5")
}

@Test
fun testCatastrophicBacktracing(): Unit = runBlocking {
val timeout = launch {
delay(1000)
throw RuntimeException("Semantic version parse took longer than 1 second")
}
launch {
SemanticVersion.parse("123.456.789-a-b-c-d-e-f-f-f-f-f-f-f-f-f-f-f-g-h-h-h-h-h-h-i-i-i-i]")
timeout.cancel()
}
timeout.join()
}

private fun assertInvalidVersion(ver: String?) {
SemanticVersion.parse(ver) ?: return // expect null
fail("Should have failed creating a semantic version for $ver")
Expand Down
Loading