Skip to content

πŸ›‘οΈ Sentinel: [CRITICAL] λŒ€ν™”ν˜• ν”„λ‘¬ν”„νŠΈμ˜ μž…λ ₯ κ°•μ œ λ³€ν™˜ ν¬λž˜μ‹œ 취약점 μˆ˜μ • #295

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.

Sign up for GitHub

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

Open
seonghobae wants to merge 2 commits into
base: master
Choose a base branch
from
Open

πŸ›‘οΈ Sentinel: [CRITICAL] λŒ€ν™”ν˜• ν”„λ‘¬ν”„νŠΈμ˜ μž…λ ₯ κ°•μ œ λ³€ν™˜ ν¬λž˜μ‹œ 취약점 μˆ˜μ • #295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40304ec
Fix input coercion crash vulnerability in interactive prompts
seonghobae Aug 27, 2026
eb7864b
Fix input coercion crash vulnerability in interactive prompts
seonghobae Aug 27, 2026
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
5 changes: 5 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
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-27 - Fix unhandled NA coercion crash in interactive prompts
**Vulnerability:** Weak regex `^[0-9]+$` on interactive integer inputs allows large numbers which coerce to `NA` via `as.integer()`, breaking `if` conditions and causing unhandled exceptions.
**Learning:** Using overly permissive regex for bounded integer choices exposes the application to coercion crashes.
**Prevention:** Use strictly bounded exact-match regex like `^[12]$` when reading integer choices via `readline()` in R to prevent coercion crashes.
3 changes: 2 additions & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ 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
View file
Open in desktop
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)) {

@devin-ai-integration devin-ai-integration Bot Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

πŸ“ Info: Stricter regex changes accept/reject behavior

The old ^[0-9]+$ accepted any digit string, so inputs like 01, 3, or 99 reached as.integer and either coerced to an accepted value or hit a downstream stop. The new ^[12]$ accepts only 1 or 2 and otherwise loops to the 3-attempt cap. This narrows accepted inputs, consistent with the fix's intent to prevent NA coercion.

Devin Review

Was this helpful? React with πŸ‘ or πŸ‘Ž to provide feedback.

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
16 changes: 16 additions & 0 deletions tests/testthat/test-readline-coercion.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
test_that("interactive prompt handles large numbers correctly without crashing", {
# We mock readline to return a huge number that coerces to NA
my_readline <- function(...) "9999999999999999999999999"
mockery::stub(aFIPC::autoFIPC, 'readline', my_readline)
mockery::stub(aFIPC::autoFIPC, 'interactive', TRUE)
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

expect_error(
aFIPC::autoFIPC(
newformXData = data.frame(A=1),
oldformYData = data.frame(A=2),
newformCommonItemNames = c('A'),
oldformCommonItemNames = c('A')
),
"Too many invalid common item confirmation attempts"
Comment on lines +4 to +14

@devin-ai-integration devin-ai-integration Bot Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟑 Coercion test never exercises the prompt path

mockery::stub targets aFIPC::autoFIPC and the test then calls aFIPC::autoFIPC(...); a namespace-qualified call resolves from the package namespace and ignores the reassigned stub, so neither the readline nor interactive mock takes effect. In the non-interactive CI session the function stops with the "interactive session required" message, which does not match the asserted "Too many invalid common item confirmation attempts", so the test fails and never exercises the coercion path.

Prompt for agents 
The test in tests/testthat/test-readline-coercion.R uses mockery::stub(aFIPC::autoFIPC, 'readline', ...) and mockery::stub(aFIPC::autoFIPC, 'interactive', TRUE), then calls aFIPC::autoFIPC(...). mockery::stub works by reassigning a modified copy of the target function into the caller's environment; it cannot intercept a namespace-qualified call (pkg::fun), which always resolves directly from the package namespace and bypasses the stub. As a result the stubs never take effect: in a non-interactive session autoFIPC's checkCorrect() stops with the 'Common item confirmation requires an interactive session' error rather than the asserted 'Too many invalid common item confirmation attempts', so the test does not actually verify the coercion fix. Rework the test so the mocks apply β€” e.g. bring autoFIPC into scope as an unqualified binding (fn <- autoFIPC), stub that binding, and invoke it unqualified β€” and confirm mockery::stub can reach the internal readline/interactive calls (they are used inside nested helper functions, which may require the depth argument). Verify the test actually reaches the retry-exhaustion branch.
Devin Review

Was this helpful? React with πŸ‘ or πŸ‘Ž to provide feedback.

)
})
Loading