Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions .Rbuildignore
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,5 @@
^\.jules(/.*)?$
^\.trivyignore\.yaml$
^trivy\.yaml$
^.semgrepignore$
^\.semgrepignore$
5 changes: 5 additions & 0 deletions .jules/sentinel.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,8 @@
**Vulnerability:** Unvalidated inputs passed to `if()` statements can cause process crashes (`condition has length > 1`) or unexpected coercion vulnerabilities.
**Learning:** In R, optional boolean parameters that default to `NULL` should be validated using explicit runtime type validation (e.g., `if (!is.null(flag) && (!is.logical(flag) || length(flag) != 1 || is.na(flag)))`).
**Prevention:** Always implement explicit runtime type validation for optional boolean parameters.

## 2024-08-11 - Integer Coercion DoS via Weak Regex
**Vulnerability:** Weak regex `^[0-9]+$` allowed arbitrarily large numbers to pass validation during `readline()` inputs, leading to `NA` coercion by `as.integer()` and subsequent unhandled exceptions / Denial of Service.
**Learning:** R's `as.integer()` returns `NA` with a warning for numeric inputs exceeding `INT_MAX`, which can break subsequent `if` condition checks causing crashes.
**Prevention:** Use strictly bounded exact-match regex like `^[12]$` for integer menu choices to prevent unexpected type coercions and DoS vulnerabilities.
2 changes: 1 addition & 1 deletion .yamllint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ extends: default
rules:
document-start: disable
line-length:
max: 140
max: 200
truthy:
allowed-values: ["true", "false", "on", "off"]
2 changes: 1 addition & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Description: Automates fixed item parameter linking for test linking under
the item response theory paradigm using mirt package estimates.
License: GPL-3 | file LICENSE
Imports: mirt, methods
Suggests: testthat (>= 3.0.0)
Suggests: testthat (>= 3.0.0), mockery
Encoding: UTF-8
Config/testthat/edition: 3
Config/roxygen2/version: 8.0.0
6 changes: 3 additions & 3 deletions R/aFIPC.R
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ autoFIPC <-
}
for (attempt in seq_len(3)) {
n <- readline(prompt = "Is it correct? (1: Yes 2: No) : ")
if (grepl("^[0-9]+$", n)) {
if (grepl("^[12]$", n)) {
return(as.integer(n))
}
}
Expand Down Expand Up @@ -171,7 +171,7 @@ autoFIPC <-
readline(
prompt = "Do you want to use default BILOG-MG priors for oldform Data? (1: Yes 2: No) : "
)
if (grepl("^[0-9]+$", n)) {
if (grepl("^[12]$", n)) {
return(as.integer(n))
}
}
Expand Down Expand Up @@ -390,7 +390,7 @@ autoFIPC <-
readline(
prompt = "Do you want to use default BILOG-MG priors for newform Data? (1: Yes 2: No) : "
)
if (grepl("^[0-9]+$", n)) {
if (grepl("^[12]$", n)) {
return(as.integer(n))
}
}
Expand Down
2 changes: 0 additions & 2 deletions test_dummy.R

This file was deleted.

3 changes: 0 additions & 3 deletions test_validation.R

This file was deleted.

73 changes: 73 additions & 0 deletions tests/testthat/test-validation.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
library(testthat)
library(mockery)

test_that("weak regex is bounded properly", {
mockery::stub(aFIPC::autoFIPC, 'interactive', TRUE)
m_readline <- mockery::mock('3', '3', '3')
mockery::stub(aFIPC::autoFIPC, 'readline', m_readline)

# Stub mirt to avoid estimation error and just get to validation
mod <- new("SingleGroupClass")
mod@OptimInfo$converged <- TRUE
mockery::stub(aFIPC::autoFIPC, 'mirt::mirt', mod)

dummy_data <- data.frame(v1=c(0,1,0,1,1), v2=c(1,0,1,0,0), v3=c(1,1,0,0,1))

expect_error(
aFIPC::autoFIPC(
oldformYData = dummy_data,
newformXData = dummy_data,
oldformCommonItemNames = c("v1"),
newformCommonItemNames = c("v1")
),
"Too many invalid common item confirmation attempts"
)
})

test_that("weak regex is bounded properly for oldform BILOG prior", {
mockery::stub(aFIPC::autoFIPC, 'interactive', TRUE)

m_readline <- mockery::mock('1', '3', '3', '3')
mockery::stub(aFIPC::autoFIPC, 'readline', m_readline)

mod <- new("SingleGroupClass")
mod@OptimInfo$converged <- TRUE
mockery::stub(aFIPC::autoFIPC, 'mirt::mirt', mod)

dummy_data <- data.frame(v1=c(0,1,0,1,1), v2=c(1,0,1,0,0), v3=c(1,1,0,0,1))

expect_error(
aFIPC::autoFIPC(
oldformYData = dummy_data,
newformXData = dummy_data,
itemtype = '3PL',
oldformCommonItemNames = c("v1"),
newformCommonItemNames = c("v1")
),
"Too many invalid oldform BILOG prior attempts"
)
})

test_that("weak regex is bounded properly for newform BILOG prior", {
mockery::stub(aFIPC::autoFIPC, 'interactive', TRUE)

m_readline <- mockery::mock('1', '1', '3', '3', '3')
mockery::stub(aFIPC::autoFIPC, 'readline', m_readline)

mod <- new("SingleGroupClass")
mod@OptimInfo$converged <- TRUE
mockery::stub(aFIPC::autoFIPC, 'mirt::mirt', mod)
Comment on lines +57 to +59

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 3 \
  'OptimInfo\$secondordertest|checknewformBILOGprior|mod@OptimInfo' \
  R/aFIPC.R tests/testthat/test-validation.R

Repository: ContextualWisdomLab/aFIPC

Length of output: 7964


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- test-validation.R ---'
cat -n tests/testthat/test-validation.R | sed -n '1,95p'

printf '%s\n' '--- aFIPC.R old-form and validation flow ---'
cat -n R/aFIPC.R | sed -n '190,420p'

printf '%s\n' '--- test entry points and defaults ---'
rg -n -C 3 'test_that|autoFIPC\\(|tryFitwholeOldItems|newformBILOGprior|itemtype' \
  tests/testthat/test-validation.R R/aFIPC.R

Repository: ContextualWisdomLab/aFIPC

Length of output: 12009


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- autoFIPC signature and old-form setup ---'
cat -n R/aFIPC.R | sed -n '1,190p'

printf '%s\n' '--- validation test call arguments ---'
cat -n tests/testthat/test-validation.R | sed -n '51,75p'

printf '%s\n' '--- all relevant test and function references ---'
rg -n -F -e 'test_that' -e 'autoFIPC(' -e 'tryFitwholeOldItems' \
  -e 'newformBILOGprior' -e "itemtype = '3PL'" \
  tests/testthat/test-validation.R R/aFIPC.R

Repository: ContextualWisdomLab/aFIPC

Length of output: 13509


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path
import re

test = Path("tests/testthat/test-validation.R").read_text()
source = Path("R/aFIPC.R").read_text()

# Check the target test uses the default old-form flow.
target = re.search(
    r'test_that\("oversized newform BILOG-prior input.*?(?=\n\}\))',
    test,
    re.S,
)
assert target, "Target test not found"
target_text = target.group(0)
assert "itemtype = '3PL'" in target_text
assert "newformBILOGprior" not in target_text
assert "tryFitwholeOldItems" not in target_text

signature = re.search(
    r"autoFIPC\s*<-\s*function\(.*?\)",
    source,
    re.S,
)
assert signature, "autoFIPC signature not found"
assert re.search(r"tryFitwholeOldItems\s*=\s*T", signature.group(0))

# Confirm old-form fallback is gated by secondordertest and precedes new-form validation.
old_gate = re.search(
    r'if\s*\(\s*\(!exists\("oldFormModel".*?secondordertest.*?\)\s*\{',
    source,
    re.S,
)
new_validation = source.index("checknewformBILOGprior <- function()")
assert old_gate and old_gate.start() < new_validation
assert "oldFormModel <-" in source[old_gate.start():new_validation]
assert "surveyFA(" in source[old_gate.start():new_validation]

print("The target test uses default tryFitwholeOldItems = TRUE.")
print("The old-form fallback checks !isTRUE(oldFormModel@OptimInfo$secondordertest).")
print("The fallback path occurs before checknewformBILOGprior().")
print("The mock sets converged but not secondordertest.")
PY

Repository: ContextualWisdomLab/aFIPC

Length of output: 405


secondordertest를 mock에 설정하십시오.

tryFitwholeOldItems = TRUE가 기본값입니다. secondordertestTRUE가 아니면 old-form fallback이 checknewformBILOGprior()보다 먼저 실행됩니다.

수정 예시
 mod@OptimInfo$converged <- TRUE
+mod@OptimInfo$secondordertest <- TRUE
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
mod <- new("SingleGroupClass")
mod@OptimInfo$converged <- TRUE
mockery::stub(aFIPC::autoFIPC, 'mirt::mirt', mod)
mod <- new("SingleGroupClass")
mod@OptimInfo$converged <- TRUE
mod@OptimInfo$secondordertest <- TRUE
mockery::stub(aFIPC::autoFIPC, 'mirt::mirt', mod)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/testthat/test-validation.R` around lines 57 - 59, Update the mock setup
around autoFIPC and the mocked mirt result so secondordertest is explicitly set
to TRUE, ensuring the test exercises checknewformBILOGprior() before the
old-form fallback when tryFitwholeOldItems remains at its default.


dummy_data <- data.frame(v1=c(0,1,0,1,1), v2=c(1,0,1,0,0), v3=c(1,1,0,0,1))

expect_error(
aFIPC::autoFIPC(
oldformYData = dummy_data,
newformXData = dummy_data,
itemtype = '3PL',
oldformCommonItemNames = c("v1"),
newformCommonItemNames = c("v1")
),
"Too many invalid newform BILOG prior attempts"
)
})
Loading