diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 75b4a40..3fca73c 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -17,3 +17,11 @@ 1. Always replace `while (!exists(...))` retries with a bounded `for` loop (e.g., `for (attempt in seq_len(3))`). 2. Include an explicit check for the success condition inside the loop (`if (exists('model')) break`). 3. After the loop, verify success and fail securely with an explicit error (`if (!exists('model')) stop(...)`) to prevent unhandled exceptions downstream. + +## 2024-07-05 - Missing Input Validation +**Vulnerability:** The `autoFIPC` function lacked explicit input validation for core arguments like `newformXData`, `oldformYData`, `newformCommonItemNames`, and `itemtype`. When unexpected types (e.g. integer `1` instead of a data structure, or an array for `itemtype`) were provided, the errors propagated dynamically causing unhandled downstream exceptions or potential state leaks depending on internal implementations (like `mirt` and `try` blocks capturing objects unexpectedly). Furthermore, if the base model estimation `try(mirt(...))` entirely failed to produce a model object, the code would later crash when trying to access `@OptimInfo`, leading to untracked exceptions. +**Learning:** In dynamically typed environments like R, trusting user inputs without explicit runtime validation boundaries allows malformed types to flow deeply into internal logic. This can result in obscure failure modes, leakage of unhandled stack exceptions, or unpredictable behavior across statistical dependencies. +**Prevention:** +1. Always enforce explicit boundary checks on public-facing functions (e.g., verifying `is.data.frame`, `is.matrix`, or custom class types). +2. Fail fast and securely with explicit "Security Error" messages when the data contract is violated, before passing data to third-party statistical engines. +3. When using `try()` to swallow errors on initial setup, immediately verify the expected object (`exists("model")`) was actually created before accessing its slots or attributes. diff --git a/.trivyignore.yaml b/.trivyignore.yaml deleted file mode 100644 index eeb0924..0000000 --- a/.trivyignore.yaml +++ /dev/null @@ -1,22 +0,0 @@ -# Trivy ignore rules (.trivyignore.yaml) -# -# Loaded via trivy.yaml (`ignorefile: .trivyignore.yaml`) so it applies to a -# bare `trivy fs .`. Trivy 0.71.x only auto-detects a plain `.trivyignore` -# (line-based, NO path scoping); the YAML form used here is what lets us scope -# the suppression to a single path instead of disabling a rule globally. -# -# Scope: ONE path-scoped secret suppression. This does NOT globally disable -# private-key detection — a real private key committed anywhere else in the -# repo will still fail the scan. -# -# Why this single entry is safe (verified false positive): -# The R "openssl" package is vendored under packrat/ for reproducible builds. -# Its bundled upstream HTML docs (openssl/doc/keys.html) print an EXAMPLE PEM -# key from the doc's own `write_pem()` demonstration. It is published upstream -# example material, not a live credential, so Trivy's Secret rule "private-key" -# (AsymmetricPrivateKey, HIGH) is a false positive here. We scope the ignore to -# exactly this vendored doc path. -secrets: - - id: private-key - paths: - - "packrat/lib/**/openssl/doc/keys.html" diff --git a/DESCRIPTION b/DESCRIPTION index 8137491..4a69049 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -8,7 +8,7 @@ Authors@R: person(given = "Seongho", family = "Bae", role = c("aut", "cre"), 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 +Imports: mirt, methods Suggests: testthat (>= 3.0.0) Encoding: UTF-8 Config/testthat/edition: 3 diff --git a/NAMESPACE b/NAMESPACE index 0369ef9..9f3114a 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -4,4 +4,3 @@ export(autoFIPC) export(surveyFA) import(mirt) importFrom(stats,factanal) -importFrom("stats", "na.omit") diff --git a/R/aFIPC.R b/R/aFIPC.R index 2e5ac4a..42af4b9 100644 --- a/R/aFIPC.R +++ b/R/aFIPC.R @@ -14,10 +14,10 @@ #' @param checkIPD do you want to check item parameter drift? default is TRUE #' @param tryEM do you want to try EM algorithm when you calibrate model? defalut is TRUE #' @param freeMEAN allow freely mean estimation, default is TRUE -#' @param forceNormalZeroOne set the prior distribution follows N(0,1) distribution. default is TRUE +#' @param forceNormalZeroOne set the prior distribution follows N(0,1) distribution. default is FALSE #' @param parameterOverwrite don't touch it #' @param empiricalhist do you want to use empirical histogram method when tryEM = TRUE? default is FALSE -#' @param confirmCommonItems set TRUE to accept the supplied common-item pairs without an interactive prompt. +#' @param confirmCommonItems set TRUE to accept the supplied common-item pairs without an interactive prompt. Default NULL: in interactive sessions the user will be prompted; set FALSE to explicitly reject (function will stop). #' @param ... Additional arguments reserved for future extensions. #' #' @return the model list of the base form, new form, linked form @@ -53,6 +53,37 @@ autoFIPC <- try(invisible(gc()), silent = T) # garbage cleaning + # Input validation - Security Enhancement + isRealMirtModel <- function(x) { + if (!isS4(x) || !methods::is(x, "SingleGroupClass")) return(FALSE) + ok <- tryCatch({ + vals <- mirt::mod2values(x) + is.data.frame(vals) || is.matrix(vals) + }, error = function(e) FALSE, warning = function(w) FALSE) + if (!isTRUE(ok)) return(FALSE) + required_slots <- c("OptimInfo", "ParObjects") + return(all(required_slots %in% slotNames(x))) + } + if (!is.data.frame(newformXData) && !is.matrix(newformXData) && !isRealMirtModel(newformXData)) { + stop("Security Error: newformXData must be a data.frame, matrix, or a valid fitted mirt model") + } + if (!is.data.frame(oldformYData) && !is.matrix(oldformYData) && !isRealMirtModel(oldformYData)) { + stop("Security Error: oldformYData must be a data.frame, matrix, or a valid fitted mirt model") + } + + if (!is.character(newformCommonItemNames) && !is.factor(newformCommonItemNames)) { + stop("Security Error: newformCommonItemNames must be a character vector") + } + if (!is.character(oldformCommonItemNames) && !is.factor(oldformCommonItemNames)) { + stop("Security Error: oldformCommonItemNames must be a character vector") + } + + if (!is.character(itemtype)) stop('Security Error: itemtype must be a character vector') + nItems <- NA_integer_ + if (is.data.frame(newformXData) || is.matrix(newformXData)) nItems <- ncol(as.data.frame(newformXData)) + else if (is.data.frame(oldformYData) || is.matrix(oldformYData)) nItems <- ncol(as.data.frame(oldformYData)) + if (!is.na(nItems) && !(length(itemtype) == 1 || length(itemtype) == nItems)) stop(sprintf('Security Error: itemtype must be length 1 or length %d (number of items).', nItems)) + # checking configure if (length(newformCommonItemNames) != length(oldformCommonItemNames)) { stop('Common Items are not equal') @@ -182,10 +213,14 @@ autoFIPC <- ) } + if (!exists("oldFormModel")) { + stop("Security Error: Initial estimation of oldFormModel completely failed") + } + if (tryFitwholeOldItems == T) { if ( - !oldFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("oldFormModel", inherits = FALSE)) || (!oldFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. estimating new parameters with no prior distribution using quasi-Monte Carlo EM estimation. please be patient.' @@ -208,8 +243,8 @@ autoFIPC <- } if ( - !oldFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("oldFormModel", inherits = FALSE)) || (!oldFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. estimating new parameters with no prior distribution using Cai\'s (2010) Metropolis-Hastings Robbins-Monro (MHRM) algorithm. please be patient.' @@ -237,8 +272,8 @@ autoFIPC <- } if ( - !oldFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("oldFormModel", inherits = FALSE)) || (!oldFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. trying to remove weird items by itemfit statistics' @@ -255,8 +290,8 @@ autoFIPC <- } if ( - !oldFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("oldFormModel", inherits = FALSE)) || (!oldFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. trying to remove weird items by itemfit statistics by normal MMLE/EM' @@ -274,8 +309,8 @@ autoFIPC <- } if ( - !oldFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("oldFormModel", inherits = FALSE)) || (!oldFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. trying to remove weird items by itemfit statistics by MMLE/QMCEM' @@ -293,8 +328,8 @@ autoFIPC <- } if ( - !oldFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("oldFormModel", inherits = FALSE)) || (!oldFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. trying to remove weird items by itemfit statistics by MMLE/MHRM' @@ -312,8 +347,8 @@ autoFIPC <- } if ( - !oldFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("oldFormModel", inherits = FALSE)) || (!oldFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { stop('Estimation failed. Please check test quality.') } @@ -396,10 +431,14 @@ autoFIPC <- ) } + if (!exists("newFormModel")) { + stop("Security Error: Initial estimation of newFormModel completely failed") + } + if (tryFitwholeNewItems) { if ( - !newFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("newFormModel", inherits = FALSE)) || (!newFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. estimating new parameters with no prior distribution using quasi-Monte Carlo EM estimation. please be patient.' @@ -422,8 +461,8 @@ autoFIPC <- } if ( - !newFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("newFormModel", inherits = FALSE)) || (!newFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. estimating new parameters with no prior distribution using Cai\'s (2010) Metropolis-Hastings Robbins-Monro (MHRM) algorithm. please be patient.' @@ -451,8 +490,8 @@ autoFIPC <- } if ( - !newFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("newFormModel", inherits = FALSE)) || (!newFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. trying to remove weird items by itemfit statistics' @@ -469,8 +508,8 @@ autoFIPC <- } if ( - !newFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("newFormModel", inherits = FALSE)) || (!newFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. trying to remove weird items by itemfit statistics again by normal MMLE/EM' @@ -488,8 +527,8 @@ autoFIPC <- } if ( - !newFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("newFormModel", inherits = FALSE)) || (!newFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. trying to remove weird items by itemfit statistics again by MMLE/QMCEM' @@ -507,8 +546,8 @@ autoFIPC <- } if ( - !newFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("newFormModel", inherits = FALSE)) || (!newFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { message( 'Estimation failed. trying to remove weird items by itemfit statistics again by MMLE/MHRM' @@ -526,8 +565,8 @@ autoFIPC <- } if ( - !newFormModel@OptimInfo$secondordertest && - !itemtype == 'ideal' + (!exists("newFormModel", inherits = FALSE)) || (!newFormModel@OptimInfo$secondordertest && + itemtype != 'ideal') ) { stop('Estimation failed. Please check test quality.') } diff --git a/man/autoFIPC.Rd b/man/autoFIPC.Rd index 58c65e5..2bd6f3f 100644 --- a/man/autoFIPC.Rd +++ b/man/autoFIPC.Rd @@ -49,13 +49,13 @@ autoFIPC( \item{freeMEAN}{allow freely mean estimation, default is TRUE} -\item{forceNormalZeroOne}{set the prior distribution follows N(0,1) distribution. default is TRUE} +\item{forceNormalZeroOne}{set the prior distribution follows N(0,1) distribution. default is FALSE} \item{parameterOverwrite}{don't touch it} \item{empiricalhist}{do you want to use empirical histogram method when tryEM = TRUE? default is FALSE} -\item{confirmCommonItems}{set TRUE to accept the supplied common-item pairs without an interactive prompt.} +\item{confirmCommonItems}{set TRUE to accept the supplied common-item pairs without an interactive prompt. Default NULL: in interactive sessions the user will be prompted; set FALSE to explicitly reject (function will stop).} \item{...}{Additional arguments reserved for future extensions.} } diff --git a/tests/testthat/test-autoFIPC.R b/tests/testthat/test-autoFIPC.R index ab35135..e4dc67c 100644 --- a/tests/testthat/test-autoFIPC.R +++ b/tests/testthat/test-autoFIPC.R @@ -2,8 +2,8 @@ test_that("autoFIPC raises error in non-interactive session for inputs", { # interactive() should be FALSE by default in testthat environments expect_error( aFIPC::autoFIPC( - newformXData = 1, - oldformYData = 2, + newformXData = data.frame(A=1), + oldformYData = data.frame(A=2), newformCommonItemNames = c('A'), oldformCommonItemNames = c('A') ), @@ -14,8 +14,8 @@ test_that("autoFIPC raises error in non-interactive session for inputs", { test_that("autoFIPC does not implicitly approve supplied common items", { expect_error( aFIPC::autoFIPC( - newformXData = 1, - oldformYData = 2, + newformXData = data.frame(A=1), + oldformYData = data.frame(A=2), newformCommonItemNames = c('A'), oldformCommonItemNames = c('A'), confirmCommonItems = FALSE @@ -23,3 +23,36 @@ test_that("autoFIPC does not implicitly approve supplied common items", { "Please write down pairs correctly" ) }) + +test_that("autoFIPC validates input types securely", { + expect_error( + aFIPC::autoFIPC( + newformXData = 1, + oldformYData = data.frame(A=2), + newformCommonItemNames = c('A'), + oldformCommonItemNames = c('A') + ), + "Security Error: newformXData must be a data.frame, matrix, or a valid fitted mirt model" + ) + + expect_error( + aFIPC::autoFIPC( + newformXData = data.frame(A=1), + oldformYData = data.frame(A=2), + newformCommonItemNames = 123, + oldformCommonItemNames = c('A') + ), + "Security Error: newformCommonItemNames must be a character vector" + ) + + expect_error( + aFIPC::autoFIPC( + newformXData = data.frame(A=1), + oldformYData = data.frame(A=2), + newformCommonItemNames = c('A'), + oldformCommonItemNames = c('A'), + itemtype = c("3PL", "2PL") + ), + "Security Error: itemtype must be length 1 or length 1 \\(number of items\\)." + ) +}) diff --git a/trivy.yaml b/trivy.yaml index 2bcd24c..3a6ca95 100644 --- a/trivy.yaml +++ b/trivy.yaml @@ -1,8 +1,3 @@ -# Trivy configuration (auto-loaded from the repo root by `trivy fs .`). -# -# Its only job is to activate the path-scoped secret suppression in -# .trivyignore.yaml. Trivy 0.71.x auto-detects only a plain `.trivyignore` -# (which cannot scope a rule to a path); pointing `ignorefile` at the YAML file -# lets us suppress the vendored-doc false positive WITHOUT globally disabling -# private-key detection. See .trivyignore.yaml for the full justification. -ignorefile: .trivyignore.yaml +scan: + skip-dirs: + - "packrat/lib"