๐ก๏ธ 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.
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๋ง ์ธ์ํ๋๋ก ์๊ฒฉํด์ก์ต๋๋ค.๋ฌธ์
ํ ์คํธ