🛡️ Sentinel: [CRITICAL] Fix integer overflow coercion DoS in interactive prompts - #233
🛡️ Sentinel: [CRITICAL] Fix integer overflow coercion DoS in interactive prompts#233seonghobae wants to merge 4 commits into
Conversation
…ive prompts - Replaced weak regex `^[0-9]+$` with strict bound `^[12]$` in interactive `readline` validations within `autoFIPC()`. - Prevents unconstrained large integers from coercing to `NA` via `as.integer()`, which bypassed conditions and caused downstream errors. - Added explicit unit tests to `test-sentinel-validation-fix.R` to verify strict prompt rejection. - Updated `.jules/sentinel.md` journal with new security learnings.
|
👋 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: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (2)
📝 WalkthroughWalkthrough대화형 입력 검증이 공통 문항 확인과 BILOG-MG 사전분포 선택에서 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (1)
tests/testthat/test-sentinel-validation-fix.R (1)
1-16: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win세 입력 경로를 모두 회귀 테스트하세요.
현재 테스트는 공통 문항 확인 프롬프트만 실행합니다.
R/aFIPC.RLine [174]와 Line [393]의 BILOG-MG 검증은 실행되지 않습니다. 따라서 해당 정규식이 다시^[0-9]+$로 변경되어도 이 테스트가 회귀를 검출하지 못합니다. 구형 및 신형 BILOG-MG 프롬프트에 대한 oversized 입력 테스트를 추가하고, 유효한"1"및"2"입력도 검증하세요.🤖 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-fix.R` around lines 1 - 16, Extend the test around aFIPC::autoFIPC to exercise all three interactive validation paths: common-item confirmation plus legacy and current BILOG-MG prompts. Add oversized-input cases that must be rejected, and valid-input cases confirming “1” and “2” are accepted for their respective prompts. Keep the readline and interactive stubbing setup isolated per scenario so each prompt path is actually reached and the regex validation is covered.
🤖 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-fix.R`:
- Around line 2-5: Declare mockery as a test dependency in DESCRIPTION’s
Suggests, or replace mockery::mock() and mockery::stub() in the sentinel
validation test with testthat::local_mocked_bindings(). Ensure the test remains
runnable in a clean environment without relying on an undeclared package.
---
Nitpick comments:
In `@tests/testthat/test-sentinel-validation-fix.R`:
- Around line 1-16: Extend the test around aFIPC::autoFIPC to exercise all three
interactive validation paths: common-item confirmation plus legacy and current
BILOG-MG prompts. Add oversized-input cases that must be rejected, and
valid-input cases confirming “1” and “2” are accepted for their respective
prompts. Keep the readline and interactive stubbing setup isolated per scenario
so each prompt path is actually reached and the regex validation is covered.
🪄 Autofix
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: 4aa69acf-9174-453d-8000-d8a5ce2d3b2b
📒 Files selected for processing (4)
.jules/sentinel.mdR/aFIPC.Rtest_dummy.Rtests/testthat/test-sentinel-validation-fix.R
💤 Files with no reviewable changes (1)
- test_dummy.R
…ive prompts - Replaced weak regex `^[0-9]+$` with strict bound `^[12]$` in interactive `readline` validations within `autoFIPC()`. - Prevents unconstrained large integers from coercing to `NA` via `as.integer()`, which bypassed conditions and caused downstream errors. - Added explicit unit tests to `test-sentinel-validation-fix.R` to verify strict prompt rejection. - Updated `.jules/sentinel.md` journal with new security learnings. - Fixed `.Rbuildignore` to ignore `.semgrepignore` to resolve CI error.
…ive prompts - Replaced weak regex `^[0-9]+$` with strict bound `^[12]$` in interactive `readline` validations within `autoFIPC()`. - Prevents unconstrained large integers from coercing to `NA` via `as.integer()`, which bypassed conditions and caused downstream errors. - Added explicit unit tests to `test-sentinel-validation-fix.R` to verify strict prompt rejection. - Updated `.jules/sentinel.md` journal with new security learnings. - Fixed `.Rbuildignore` to ignore `.semgrepignore` to resolve CI error. - Added `mockery` to Suggests in DESCRIPTION to resolve CI test execution error.
…ive prompts - Replaced weak regex `^[0-9]+$` with strict bound `^[12]$` in interactive `readline` validations within `autoFIPC()`. - Prevents unconstrained large integers from coercing to `NA` via `as.integer()`, which bypassed conditions and caused downstream errors. - Added explicit unit tests to `test-sentinel-validation-fix.R` to verify strict prompt rejection. - Updated `.jules/sentinel.md` journal with new security learnings. - Fixed `.Rbuildignore` to ignore `.semgrepignore` to resolve CI error. - Added `mockery` to Suggests in DESCRIPTION to resolve CI test execution error.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Weak regex
^[0-9]+$on interactivereadlineprompts allowed unconstrained large input strings. When coerced viaas.integer(), values exceeding the 32-bit integer limit becomeNA, breaking conditional loops and leading to unhandled exceptions or potential DoS in edge environments.🎯 Impact: Prevents script crashes and infinite loops caused by unexpected integer limits during manual or semi-automated verification steps.
🔧 Fix: Changed regex to
^[12]$to strictly bound expected choices, completely averting the coercion risk. Added verification tests usingmockery.✅ Verification: Verified via
covr::package_coverage()anddevtools::test()locally. Tests explicitly assert correct error termination on out-of-bound strings.PR created automatically by Jules for task 3011607167211407000 started by @seonghobae
Summary by CodeRabbit
버그 수정
1또는2로 제한했습니다.테스트