Skip to content

Commit f8d69b6

Browse files
jpicklykclaude
andcommitted
fix: harden input validation — requireUUID, bounds checks, time ranges, cycle detection
- Add requireUUID() helper to BaseToolDefinition to eliminate !! non-null assertions - Replace extractUUID(required=true)!! with requireUUID() in 4 call sites - Add .coerceAtLeast(0) offset bounds check in SQLiteWorkItemRepository.findByFilters() - Add time range validation (createdAfter < createdBefore, etc.) in QueryItemsTool.executeSearch() - Replace repeat()-based ancestor walk with visited-set cycle detection in ItemHierarchyValidator Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent fb3a4c9 commit f8d69b6

File tree

7 files changed

+36
-10
lines changed

7 files changed

+36
-10
lines changed

current/src/main/kotlin/io/github/jpicklyk/mcptask/current/application/service/ItemHierarchyValidator.kt

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,14 @@ class ItemHierarchyValidator {
4545
}
4646

4747
// Guard: ancestor cycle check — walk up from parentId, ensure itemId is not an ancestor
48+
// Uses a visited set to detect pre-existing cycles in the hierarchy (not just depth-bounded)
49+
val visited = mutableSetOf<UUID>()
4850
var cursor: UUID? = parentId
49-
repeat(MAX_DEPTH + 1) {
50-
val cursorId = cursor ?: return@repeat
51-
val ancestorResult = repo.getById(cursorId)
52-
val ancestor = when (ancestorResult) {
51+
while (cursor != null && visited.size <= MAX_DEPTH) {
52+
if (!visited.add(cursor)) break // Pre-existing cycle — stop walking
53+
val ancestor = when (val ancestorResult = repo.getById(cursor)) {
5354
is Result.Success -> ancestorResult.data
54-
is Result.Error -> return@repeat
55+
is Result.Error -> break
5556
}
5657
if (ancestor.id == itemId) {
5758
throw ToolValidationException(

current/src/main/kotlin/io/github/jpicklyk/mcptask/current/application/tools/BaseToolDefinition.kt

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,20 @@ abstract class BaseToolDefinition : ToolDefinition {
277277
// UUID extraction (v3-specific)
278278
// ──────────────────────────────────────────────
279279

280+
/**
281+
* Extracts and parses a required UUID parameter, returning a non-null UUID.
282+
*
283+
* Wraps [extractUUID] with `required=true` so call sites avoid `!!` non-null assertions.
284+
*
285+
* @param params The input parameters
286+
* @param name The parameter name
287+
* @return The parsed UUID (never null)
288+
* @throws ToolValidationException if missing or not a valid UUID
289+
*/
290+
protected fun requireUUID(params: JsonElement, name: String): UUID =
291+
extractUUID(params, name, required = true)
292+
?: throw ToolValidationException("'$name' is required and must be a valid UUID")
293+
280294
/**
281295
* Extracts and parses a UUID parameter from a string value.
282296
*

current/src/main/kotlin/io/github/jpicklyk/mcptask/current/application/tools/dependency/QueryDependenciesTool.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ Returns dependencies with counts breakdown and optional graph traversal data.
9090
}
9191

9292
override suspend fun execute(params: JsonElement, context: ToolExecutionContext): JsonElement {
93-
val itemId = extractUUID(params, "itemId", required = true)!!
93+
val itemId = requireUUID(params, "itemId")
9494
val direction = optionalString(params, "direction") ?: "all"
9595
val typeFilter = optionalString(params, "type")?.let { DependencyType.fromString(it) }
9696
val includeItemInfo = optionalBoolean(params, "includeItemInfo", defaultValue = false)

current/src/main/kotlin/io/github/jpicklyk/mcptask/current/application/tools/items/QueryItemsTool.kt

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,17 @@ Operations: get, search, overview
343343
val offset = (optionalInt(params, "offset") ?: 0).coerceAtLeast(0)
344344
val includeAncestors = optionalBoolean(params, "includeAncestors", false)
345345

346+
// Validate time ranges — reject inverted ranges early
347+
if (createdAfter != null && createdBefore != null && createdAfter > createdBefore) {
348+
return errorResponse("createdAfter must be before createdBefore", ErrorCodes.VALIDATION_ERROR)
349+
}
350+
if (modifiedAfter != null && modifiedBefore != null && modifiedAfter > modifiedBefore) {
351+
return errorResponse("modifiedAfter must be before modifiedBefore", ErrorCodes.VALIDATION_ERROR)
352+
}
353+
if (roleChangedAfter != null && roleChangedBefore != null && roleChangedAfter > roleChangedBefore) {
354+
return errorResponse("roleChangedAfter must be before roleChangedBefore", ErrorCodes.VALIDATION_ERROR)
355+
}
356+
346357
// Parse role
347358
val role = roleStr?.let {
348359
Role.fromString(it)

current/src/main/kotlin/io/github/jpicklyk/mcptask/current/application/tools/notes/QueryNotesTool.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ Read-only query operations for Notes (get, list).
124124
// ──────────────────────────────────────────────
125125

126126
private suspend fun executeGet(params: JsonElement, context: ToolExecutionContext): JsonElement {
127-
val id = extractUUID(params, "id", required = true)!!
127+
val id = requireUUID(params, "id")
128128
val noteRepo = context.noteRepository()
129129

130130
return when (val result = noteRepo.getById(id)) {
@@ -146,7 +146,7 @@ Read-only query operations for Notes (get, list).
146146
// ──────────────────────────────────────────────
147147

148148
private suspend fun executeList(params: JsonElement, context: ToolExecutionContext): JsonElement {
149-
val itemId = extractUUID(params, "itemId", required = true)!!
149+
val itemId = requireUUID(params, "itemId")
150150
val role = optionalString(params, "role")
151151
val includeBody = optionalBoolean(params, "includeBody", defaultValue = true)
152152
val noteRepo = context.noteRepository()

current/src/main/kotlin/io/github/jpicklyk/mcptask/current/application/tools/workflow/GetNextStatusTool.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ Read-only status progression recommendation for a WorkItem.
5858
}
5959

6060
override suspend fun execute(params: JsonElement, context: ToolExecutionContext): JsonElement {
61-
val itemId = extractUUID(params, "itemId", required = true)!!
61+
val itemId = requireUUID(params, "itemId")
6262

6363
// Fetch the WorkItem
6464
val itemResult = context.workItemRepository().getById(itemId)

current/src/main/kotlin/io/github/jpicklyk/mcptask/current/infrastructure/repository/SQLiteWorkItemRepository.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ class SQLiteWorkItemRepository(private val databaseManager: DatabaseManager) : W
217217
val items = baseQuery
218218
.orderBy(sortColumn, order)
219219
.limit(limit)
220-
.offset(offset.toLong())
220+
.offset(offset.coerceAtLeast(0).toLong())
221221
.mapNotNull { mapRowToWorkItemSafe(it) }
222222

223223
Result.Success(items)

0 commit comments

Comments
 (0)