🛡️ Sentinel: [MEDIUM] Fix integer coercion DoS vulnerability in readline - #202
🛡️ Sentinel: [MEDIUM] Fix integer coercion DoS vulnerability in readline#202seonghobae wants to merge 4 commits into
Conversation
…ine validation Replaced weak regex validation `^[0-9]+$` with strictly bounded exact-match `^[12]$` across all interactive `readline()` prompts in `R/aFIPC.R`. This prevents large numeric strings from coercing to `NA` via `as.integer()`, which causes unhandled runtime crashes when evaluated in conditionals. Also added tests to verify correct input validation and rejection behavior using `mockery`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
Changes대화형 입력 검증 강화
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/testthat/test-sentinel-validation.R (3)
44-44: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win정수 오버플로 입력을 별도 사례로 추가하십시오.
999999999는 R 정수의 오버플로 경로를 명확히 재현하지 않습니다. 현재 R 구현은 32비트 정수를 사용하며, 더 큰 값은NA로 변환될 수 있습니다. (stat.ethz.ch) 또한 기존 숫자 전용 정규식에서는 첫 입력3에서 함수가 반환되므로 세 번째 값도 오버플로 경로를 검증하지 않습니다.mockery::mock("x", "y", "2147483648")를 사용하는 별도 사례를 추가하십시오.🤖 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-sentinel-validation.R` at line 44, In the sentinel validation tests, add a separate overflow-input case using mockery::mock with "x", "y", and "2147483648". Ensure the test reaches the integer-conversion path rather than returning on the existing numeric input "3", and verify the expected overflow handling for the sentinel validation flow.
39-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win세 입력 검증 루프 모두에 회귀 테스트를 추가하십시오.
현재 invalid-input 테스트는 공통 문항 확인 루프만 실행합니다. valid-input 테스트는
mirt::mirt를 old-form 초기 추정 단계에서 실패시키므로checknewformBILOGprior에는 도달하지 않습니다.checkCorrect,checkoldformBILOGprior,checknewformBILOGprior각각에 대해 invalid input 3회와 해당 오류를 검증하십시오.Also applies to: 59-80
🤖 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-sentinel-validation.R` around lines 39 - 57, Extend the sentinel-validation tests beyond the common-item confirmation loop to cover checkCorrect, checkoldformBILOGprior, and checknewformBILOGprior. For each validation path, stub interactive input with three invalid responses and assert the corresponding “Too many invalid … attempts” error. Mock prerequisite estimation or validation calls as needed so each test reaches its target loop instead of failing earlier in mirt::mirt or another setup step.
42-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
readline호출 횟수를 검증하십시오.현재 테스트는 세 개의 잘못된 입력을 제공하지만 호출 횟수를 검증하지 않습니다.
readline_mock변수에 mock을 저장한 뒤expect_error()다음에mockery::expect_called(readline_mock, 3)을 추가하십시오.🤖 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-sentinel-validation.R` around lines 42 - 54, Update the test around aFIPC::autoFIPC by storing the mockery::mock used for readline in a readline_mock variable, passing it to the readline stub, and adding mockery::expect_called(readline_mock, 3) after expect_error() to verify all three invalid-input attempts occurred.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@tests/testthat/test-sentinel-validation.R`:
- Around line 43-44: Add mockery to DESCRIPTION under Suggests and update
packrat/packrat.lock accordingly so test-sentinel-validation.R dependencies are
declared; if retaining it as optional, add skip_if_not_installed("mockery")
before the mockery::stub/mockery::mock calls.
- Around line 67-77: Update the autoFIPC error assertion in the sentinel
validation test to require the specific expected error message by adding the
regexp pattern “Security Error: Initial estimation of oldFormModel completely
failed” to expect_error. Keep the existing test setup unchanged.
---
Nitpick comments:
In `@tests/testthat/test-sentinel-validation.R`:
- Line 44: In the sentinel validation tests, add a separate overflow-input case
using mockery::mock with "x", "y", and "2147483648". Ensure the test reaches the
integer-conversion path rather than returning on the existing numeric input "3",
and verify the expected overflow handling for the sentinel validation flow.
- Around line 39-57: Extend the sentinel-validation tests beyond the common-item
confirmation loop to cover checkCorrect, checkoldformBILOGprior, and
checknewformBILOGprior. For each validation path, stub interactive input with
three invalid responses and assert the corresponding “Too many invalid …
attempts” error. Mock prerequisite estimation or validation calls as needed so
each test reaches its target loop instead of failing earlier in mirt::mirt or
another setup step.
- Around line 42-54: Update the test around aFIPC::autoFIPC by storing the
mockery::mock used for readline in a readline_mock variable, passing it to the
readline stub, and adding mockery::expect_called(readline_mock, 3) after
expect_error() to verify all three invalid-input attempts occurred.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 134f46be-be0d-4d00-932f-1d9e45c32015
📒 Files selected for processing (3)
.jules/sentinel.mdR/aFIPC.Rtests/testthat/test-sentinel-validation.R
| mockery::stub(aFIPC::autoFIPC, 'interactive', TRUE) | ||
| mockery::stub(aFIPC::autoFIPC, 'readline', mockery::mock('3', '0', '999999999')) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 저장소 manifest에 mockery 선언이 있는지 확인합니다.
files="$(fd -HI -t f '^(DESCRIPTION|renv\.lock|pak\.lock)$' . || true)"
test -n "$files" || { echo "manifest를 찾지 못했습니다." >&2; exit 2; }
if ! printf '%s\n' "$files" | xargs -r rg -n '\bmockery\b'; then
echo "mockery가 manifest에 선언되지 않았습니다." >&2
exit 1
fiRepository: ContextualWisdomLab/aFIPC
Length of output: 2582
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tracked manifests ---'
git ls-files | grep -E '(^|/)(DESCRIPTION|renv\.lock|pak\.lock|packrat/[^/]+)$' | head -80
printf '%s\n' '--- root DESCRIPTION ---'
if test -f DESCRIPTION; then
cat -n DESCRIPTION
else
echo 'DESCRIPTION not found'
fi
printf '%s\n' '--- test references and dependency guards ---'
rg -n -C 3 'mockery|skip_if_not_installed|testthat' tests/testthat/test-sentinel-validation.R DESCRIPTION .github 2>/dev/null || true
printf '%s\n' '--- package-managed mockery references ---'
rg -n --glob 'DESCRIPTION' --glob 'renv.lock' --glob 'pak.lock' --glob 'packrat.lock' --glob '!packrat/lib/**' '\bmockery\b' . 2>/dev/null || trueRepository: ContextualWisdomLab/aFIPC
Length of output: 6784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- CI and package installation configuration ---'
git ls-files | grep -E '(^|/)(\.github/workflows/|appveyor|travis|Makefile|README|CONTRIBUTING)' | head -120
rg -n -C 3 'R CMD check|rcmdcheck|pak::|install|packrat|Suggests|mockery' \
.github DESCRIPTION README* Makefile .travis.yml appveyor.yml 2>/dev/null || true
printf '%s\n' '--- packrat lock entries for relevant packages ---'
rg -n -C 2 '"(mockery|testthat|mirt)"|Package: (mockery|testthat|mirt)' \
packrat/packrat.lock packrat 2>/dev/null | head -120 || true
printf '%s\n' '--- test file context ---'
cat -n tests/testthat/test-sentinel-validation.R | sed -n '1,95p'Repository: ContextualWisdomLab/aFIPC
Length of output: 9661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
description = Path("DESCRIPTION").read_text(encoding="utf-8")
test_file = Path("tests/testthat/test-sentinel-validation.R").read_text(encoding="utf-8")
workflow = Path(".github/workflows/r.yml").read_text(encoding="utf-8")
lockfile = Path("packrat/packrat.lock").read_text(encoding="utf-8")
suggests = re.search(
r"(?ms)^Suggests:\s*(.*?)(?=^[A-Za-z][A-Za-z0-9.-]*:|\Z)",
description,
)
suggests_text = suggests.group(1) if suggests else ""
print("mockery_in_DESCRIPTION_Suggests:", bool(re.search(r"\bmockery\b", suggests_text)))
print("mockery_test_calls:", len(re.findall(r"\bmockery::(?:stub|mock)\s*\(", test_file)))
print("test_dependency_guard:", bool(re.search(r"skip_if_not_installed\s*\(\s*[\"']mockery", test_file)))
print("r_ci_uses_setup_r_dependencies:", "r-lib/actions/setup-r-dependencies@" in workflow)
print("mockery_in_packrat_lock:", bool(re.search(r"(?m)^Package:\s*mockery\s*$", lockfile)))
PYRepository: ContextualWisdomLab/aFIPC
Length of output: 321
mockery를 Suggests에 추가하십시오.
현재 DESCRIPTION과 packrat/packrat.lock에 mockery가 없습니다. 테스트는 mockery::stub()와 mockery::mock()를 직접 호출하므로 깨끗한 환경의 R CMD check에서 실패합니다. .github/workflows/r.yml의 의존성 설정이 선언된 Suggests 패키지를 설치합니다. 선택적 의존성으로 유지하려면 skip_if_not_installed("mockery")를 추가하십시오.
🤖 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-sentinel-validation.R` around lines 43 - 44, Add mockery
to DESCRIPTION under Suggests and update packrat/packrat.lock accordingly so
test-sentinel-validation.R dependencies are declared; if retaining it as
optional, add skip_if_not_installed("mockery") before the
mockery::stub/mockery::mock calls.
| expect_error( | ||
| aFIPC::autoFIPC( | ||
| newformXData = data.frame(A=1, B=2), | ||
| oldformYData = data.frame(A=1, B=2), | ||
| newformCommonItemNames = c('A', 'B'), | ||
| oldformCommonItemNames = c('A', 'B'), | ||
| confirmCommonItems = NULL, | ||
| oldformBILOGprior = NULL, | ||
| newformBILOGprior = NULL | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 검증 이전 오류와 검증 이후 오류를 구분할 수 있는지 확인합니다.
rg -n -C 4 \
'expect_error\(|Common item confirmation requires|Initial estimation of oldFormModel' \
tests/testthat/test-sentinel-validation.R R/aFIPC.RRepository: ContextualWisdomLab/aFIPC
Length of output: 4500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test block ---'
cat -n tests/testthat/test-sentinel-validation.R | sed -n '40,90p'
printf '%s\n' '--- autoFIPC control flow ---'
cat -n R/aFIPC.R | sed -n '100,250p'
printf '%s\n' '--- relevant test helpers and stubs ---'
rg -n -C 5 \
'mirt::mirt|surveyFA|confirmCommonItems|oldformBILOGprior|newformBILOGprior|expect_error' \
tests/testthat RRepository: ContextualWisdomLab/aFIPC
Length of output: 50381
기대한 오류를 명시하십시오.
interactive()는 TRUE로 스텁되고 readline()은 "1"을 반환하므로 Line 136의 오류는 발생하지 않습니다. 그러나 regexp가 없어서 다른 오류도 테스트를 통과시킵니다. regexp = "Security Error: Initial estimation of oldFormModel completely failed"를 추가하십시오.
🤖 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-sentinel-validation.R` around lines 67 - 77, Update the
autoFIPC error assertion in the sentinel validation test to require the specific
expected error message by adding the regexp pattern “Security Error: Initial
estimation of oldFormModel completely failed” to expect_error. Keep the existing
test setup unchanged.
…ine validation Replaced weak regex validation `^[0-9]+$` with strictly bounded exact-match `^[12]$` across all interactive `readline()` prompts in `R/aFIPC.R`. This prevents large numeric strings from coercing to `NA` via `as.integer()`, which causes unhandled runtime crashes when evaluated in conditionals. Also added tests to verify correct input validation and rejection behavior using `mockery`. Also fixed R CMD check warning by adding `.semgrepignore` to `.Rbuildignore`.
…ine validation Replaced weak regex validation ^[0-9]+$ with strictly bounded exact-match ^[12]$ across all interactive readline() prompts in R/aFIPC.R. This prevents large numeric strings from coercing to NA via as.integer(), which causes unhandled runtime crashes when evaluated in conditionals. Also added tests to verify correct input validation and rejection behavior using mockery. Also fixed R CMD check warning by adding .semgrepignore to .Rbuildignore.
…ine validation Replaced weak regex validation ^[0-9]+$ with strictly bounded exact-match ^[12]$ across all interactive readline() prompts in R/aFIPC.R. This prevents large numeric strings from coercing to NA via as.integer(), which causes unhandled runtime crashes when evaluated in conditionals. Also added tests to verify correct input validation and rejection behavior using mockery. Also fixed R CMD check warnings by adding .semgrepignore to .Rbuildignore, removing top-level dummy files, and declaring test dependencies.
|
Closing as superseded by #193. The surviving PR applies the same exact |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
🚨 Severity: MEDIUM
💡 Vulnerability: Weak regex validation (
^[0-9]+$) on interactivereadline()inputs allowed huge numeric strings to pass validation, which would then coerce toNAwhen parsed byas.integer().🎯 Impact: When the
NAvalue is evaluated in subsequentifconditions, it causes a fatal unhandled exception ("condition has length > 1"), potentially crashing the application or automation workflows in a Denial of Service.🔧 Fix: Updated the regex validation on all three
readline()loops to use strictly bounded exact-match patterns (^[12]$), ensuring only valid inputs can proceed to integer coercion and logic evaluation. Also addedtestthatcoverage usingmockeryto verify the inputs are properly rejected after 3 failed attempts.✅ Verification: Ran
devtools::test()with the newly added tests; all 57 package tests passed successfully. Test coverage also verified to be intact.PR created automatically by Jules for task 14593451622102826199 started by @seonghobae
Summary by CodeRabbit
개선 사항
1또는2만 인식하도록 엄격해졌습니다.문서
테스트