๐ก๏ธ Sentinel: [MEDIUM] Fix Integer Coercion Vulnerability - #176
๐ก๏ธ Sentinel: [MEDIUM] Fix Integer Coercion Vulnerability#176seonghobae wants to merge 4 commits into
Conversation
Fixed weak regex validation for integer conversion via readline() in R/aFIPC.R to prevent DoS or crashes via as.integer() type coercion.
|
๐ 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: 2 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 (1)
๐ WalkthroughWalkthroughautoFIPC์ surveyFA๋ฅผ ํจํค์ง API๋ก ์ถ๊ฐํ๊ณ , ์์๋ณ ์ถ์ ยท๊ณตํต ๋ฌธํญ ๋งํนยท์คํจ ๋ณต๊ตฌยท๊ฒฐ๊ณผ ์ฐ์ถ์ ๊ตฌํํ์ต๋๋ค. ํจํค์ง ๋ฌธ์์ R check ์ฐ์ถ๋ฌผ, ์ ๋ ฅ ๊ฒ์ฆ ํ ์คํธ, ๋ํํ ์ ๋ ฅ ๋ฐ ๋น๋ ์ ์ธ ๊ท์น๋ ๊ฐฑ์ ํ์ต๋๋ค. ChangesFIPC ๋ณด์ ๊ธฐ๋ฅ
ํจํค์ง ๋ฐฐํฌ ์ฐ์ถ๋ฌผ
์ ๋ ฅ ๋ฐ ๋น๋ ๋ณด์ ๊ฐํ
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant autoFIPC
participant surveyFA
participant mirt
Caller->>autoFIPC: old/new ๋ฐ์ดํฐ์ ๊ณตํต ๋ฌธํญ ์ ๋ฌ
autoFIPC->>mirt: ์์๋ณ ๋ชจ๋ธ ์ถ์
autoFIPC->>surveyFA: ์คํจ ๋ชจ๋ธ ๋ณต๊ตฌ ์์ฒญ
surveyFA->>mirt: EM/QMCEM/MHRM ์ฌ์ถ์
surveyFA-->>autoFIPC: ์ ํจ ๋ชจ๋ธ ๋๋ ๋ณต๊ตฌ ์คํจ
Suggested reviewers: ๐ฅ Pre-merge checks | โ 5โ Passed checks (5 passed)
โจ Finishing Touches๐งช Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR tightens interactive prompt input validation inside autoFIPC() to prevent unsafe as.integer() coercion outcomes (e.g., large numeric strings becoming NA) and documents the security lesson in the Sentinel log.
Changes:
- Restrict interactive prompt acceptance from
^[0-9]+$to the exact allowed options^[12]$for common-item confirmation and BILOG prior selection. - Preserve existing retry/stop behavior for repeated invalid inputs.
- Document the integer-coercion risk and prevention guidance in
.jules/sentinel.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| R/aFIPC.R | Hardens readline() option parsing so only 1/2 are accepted before as.integer() conversion in interactive flows. |
| .jules/sentinel.md | Adds a Sentinel entry documenting the coercion pitfall and the bounded-regex mitigation. |
๐ก Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
โฆ issue Fixed weak regex validation for integer conversion via readline() in R/aFIPC.R to prevent DoS or crashes via as.integer() type coercion. Added `.semgrepignore` to `.Rbuildignore` to fix `R CMD check` CI failure regarding unrecognized hidden files.
โฆ issue Fixed weak regex validation for integer conversion via readline() in R/aFIPC.R to prevent DoS or crashes via as.integer() type coercion. Added `.semgrepignore` to `.Rbuildignore` to fix `R CMD check` CI failure regarding unrecognized hidden files. Removed `test_dummy.R` and `test_validation.R` from the top level which were failing the check `R CMD check` due to non-standard files found.
| **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-07-26 - Integer Coercion Vulnerability from readline() |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
๐งน Nitpick comments (7)
aFIPC.Rcheck/aFIPC/R/aFIPC.rdb (1)
1-1: ๐ Maintainability & Code Quality | ๐ต Trivial | โก Quick win์์ฑ๋ R CMD check ์ฐ์ถ๋ฌผ์ ์ ์ฅ์์์ ์ ์ธํ์ธ์.
aFIPC.Rcheck/aFIPC/R/aFIPC.rdb๋ ์์ค๊ฐ ์๋ ์์ฑ ๋ฐ์ด๋๋ฆฌ์ ๋๋ค. ๋์ผํaFIPC.Rcheck/ํธ๋ฆฌ์ ๋ก๊ทธยทRDSยท๋์๋ง DBยท์ค์น ํจํค์ง ์ฌ๋ณธ๋ ํจ๊ป ์ ๊ฑฐํ๊ณ , ๊ฒ์ฆ ๊ฒฐ๊ณผ๋ CI ์ํฐํฉํธ๋ก ๋ณด๊ดํ์ธ์. ๊ทธ๋ ์ง ์์ผ๋ฉด ์ ์ฅ์ ์ฉ๋๊ณผ ๋ฆฌ๋ทฐ diff๊ฐ ๋ถํ์ํ๊ฒ ์ปค์ง๊ณ ๊ฒฐ๊ณผ๋ฌผ์ด ์ฝ๊ฒ ์ค๋๋ฉ๋๋ค.๐ค 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 `@aFIPC.Rcheck/aFIPC/R/aFIPC.rdb` at line 1, Remove the generated R CMD check artifacts under the aFIPC.Rcheck tree, including aFIPC.rdb and any logs, RDS files, help databases, and installed package copies; ensure the entire generated directory is excluded from version control and retain validation results through CI artifacts instead.aFIPC.Rcheck/aFIPC/Meta/Rd.rds (1)
1-1: ๐ Maintainability & Code Quality | ๐ต Trivial | โก Quick win์์ฑ๋ RDS ๋ฉํ๋ฐ์ดํฐ๋ฅผ ์์ค ์ ์ฅ์์ ์ปค๋ฐํ์ง ๋ง์ธ์.
aFIPC.Rcheck/aFIPC/Meta/*.rds๋ ํจํค์ง ๋น๋ยท์ฒดํฌ์์ ์ฌ์์ฑ๋๋ ์ฐ์ถ๋ฌผ์ ๋๋ค. ์์ค๋ namespace๊ฐ ๋ณ๊ฒฝ๋๋ฉด stale ์ํ๊ฐ ๋๊ณ ๋ฐ์ด๋๋ฆฌ diff๋ก ๊ฒํ ํ๊ธฐ๋ ์ด๋ ต์ต๋๋ค. CI์์ ์์ฑํ๋๋ก ํ๊ณ ํด๋น ์ฒดํฌ ์ฐ์ถ๋ฌผ์ ๋ฒ์ ๊ด๋ฆฌ์์ ์ ์ธํ์ธ์.๐ค 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 `@aFIPC.Rcheck/aFIPC/Meta/Rd.rds` at line 1, Remove the generated RDS metadata artifact under aFIPC.Rcheck/aFIPC/Meta from version control and add the corresponding aFIPC.Rcheck build/check output pattern to the repositoryโs ignore configuration so CI recreates it during package checks.aFIPC.Rcheck/aFIPC-Ex.R (1)
33-41: ๐ Maintainability & Code Quality | ๐ต Trivial | ๐๏ธ Heavy lift๊ณต๊ฐ API ์์ ๋ฅผ ์คํ ๊ฐ๋ฅํ ์ต์ ์์ ๋ก ์ ๊ณตํ์ธ์.
ํ์ฌ ์์ ๋
Not run์ํ์ด๊ณ ์ ๋ ฅ ๊ฐ์ฒด๋ ์ ์๋์ด ์์ง ์์ API ๊ณ์ฝ์ ๊ฒ์ฆํ์ง ๋ชปํฉ๋๋ค. ์ต์ํ์ ์คํ ๊ฐ๋ฅํ toy ์ ๋ ฅ์ ์ถ๊ฐํ๊ฑฐ๋, ํ์ํ ๋ชจ๋ธ ์์ฑ ์ ์ฐจ์ ์คํ ๋ถ๊ฐ ์ฌ์ ๋ฅผ ์๋ณธ Rd ๋ฌธ์์ ๋ช ์ํ ๋ค ์ฐ์ถ๋ฌผ์ ์ฌ์์ฑํ์ธ์.๐ค 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 `@aFIPC.Rcheck/aFIPC-Ex.R` around lines 33 - 41, autoFIPC ์์ ๋ฅผ ์คํ ๊ฐ๋ฅํ ์ต์ ์์ ๋ก ์์ ํ๊ณ , new_modelยทold_modelยทcommon_newยทcommon_old ๋ฑ ํ์ํ toy ์ ๋ ฅ๊ณผ ๋ชจ๋ธ ์์ฑ ์ ์ฐจ๋ฅผ ํฌํจํ์ธ์. ์์ ๋ฅผ ์ค์ ๋ก ์คํํด API ๊ณ์ฝ์ ๊ฒ์ฆํ ์ ์๋๋ก Not run ์ํ๋ฅผ ์ ๊ฑฐํ๊ณ ์ฐ์ถ๋ฌผ์ ์ฌ์์ฑํ๋ฉฐ, ์คํ์ด ๋ถ๊ฐ๋ฅํ ๊ฒฝ์ฐ์๋ ์๋ณธ Rd ๋ฌธ์์ ๊ทธ ์ฌ์ ์ ํ์ํ ์ ์ฐจ๋ฅผ ๋ช ์ํ ๋ค ์ฌ์์ฑํ์ธ์.aFIPC.Rcheck/tests/testthat/test-fixed-parameter-calibration.R (1)
83-88: ๐ Maintainability & Code Quality | ๐ต Trivial | โก Quick win
expect_false(any(...))๋ ๋น ๋ฒกํฐ์์๋ ํต๊ณผํ๋ฏ๋ก ๊ฒ์ฆ์ด ๋ฌด๋ ฅํ๋ ์ ์์ต๋๋ค.
linked_structural์ด 0-๊ธธ์ด๋ฉดany()๊ฐFALSE๋ฅผ ๋ฐํํด ํ ์คํธ๊ฐ ํญ์ ํต๊ณผํฉ๋๋ค. ์ค์ ๋ก g/u ํ์ด ์กด์ฌํ๋์ง ๋จผ์ ๋จ์ธํด ์ฃผ์ธ์.๐ ์ ์ ์์
+ expect_gt(length(linked_structural), 0) expect_false(any(linked_structural))๊ฐ์ ํจํด์ด L104(
expect_false(any(linked_fixed$est)))์ L115์๋ ์์ต๋๋ค.๐ค 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 `@aFIPC.Rcheck/tests/testthat/test-fixed-parameter-calibration.R` around lines 83 - 88, Update the assertions around linked_structural and the corresponding linked_fixed and L115 checks so they first assert that the filtered g/u rows exist and are non-empty, then verify their est values are false. Ensure empty filtered vectors fail the test instead of allowing expect_false(any(...)) to pass.aFIPC.Rcheck/tests/testthat/test-optimization-equivalence.R (1)
37-50: ๐ Maintainability & Code Quality | ๐ต Trivial | โก Quick win๊ตฌํ์ ํธ์ถํ์ง ์๊ณ ํํ์์ ๋ณต์ ํด ๊ฒ์ฆํ๋ฏ๋ก ํ๊ท ๊ฐ๋๋ก ๋์ํ์ง ์์ต๋๋ค.
new_idiom์autoFIPC()๋ด๋ถ ์ฝ๋๋ฅผ ์ฐธ์กฐํ๋ ๊ฒ์ด ์๋๋ผ ๋์ผํ ํํ์์ ํ ์คํธ ํ์ผ์ ๋ค์ ์ ์ด๋ ๊ฒ์ ๋๋ค. ์ค์ ๊ตฌํ(aFIPC.RL773-774)์ด ๋ฐ๋์ด๋ ์ด ํ ์คํธ๋ ๊ณ์ ํต๊ณผํฉ๋๋ค. ์นดํ ๊ณ ๋ฆฌ ์ ๋น๊ต ๋ก์ง์ ๋ด๋ถ ํฌํผ๋ก ์ถ์ถํด ํ ์คํธ๊ฐ ๊ทธ ํฌํผ๋ฅผ ํธ์ถํ๋๋ก ํ๋ฉด ์๋ํ ํ๊ท ๋ฐฉ์ง๊ฐ ์ฑ๋ฆฝํฉ๋๋ค.๐ค 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 `@aFIPC.Rcheck/tests/testthat/test-optimization-equivalence.R` around lines 37 - 50, Refactor the category-count logic used by autoFIPC into a shared internal helper, then update both the implementation and this test to call that helper instead of duplicating length(na.omit(unique(x))). Preserve the comparison with the legacy levels(as.factor(x)) expression and the existing expected-value assertions.aFIPC.Rcheck/tests/testthat/test-sentinel-validation.R (1)
1-37: ๐ Maintainability & Code Quality | ๐ต Trivial | โก Quick win์ด๋ฒ PR์ ํต์ฌ ๋ณ๊ฒฝ์ธ
^[12]$์ ๋ ฅ ๊ฒ์ฆ์ ๋ํ ์ปค๋ฒ๋ฆฌ์ง๊ฐ ์์ต๋๋ค.boolean ํ๋๊ทธ ๊ฒ์ฆ์ ์ ๋ค๋ฃจ๊ณ ์์ง๋ง, ์ ์
readline()ํ์ธ ๊ฒฝ๋ก(checkCorrect,checknewformBILOGprior,checkoldformBILOGprior)์ exact-match ๋์๊ณผ 3ํ ์ด๊ณผ ์ ์ค๋จ ๋์์ ๊ฒ์ฆ๋์ง ์์ต๋๋ค. ์ต์ํ ๋น๋ํํ ์ธ์ ์์confirmCommonItems = NULL์ผ ๋ ๋ช ํํ ์ค๋จ๋๋์ง,confirmCommonItems = FALSE์ผ ๋'Please write down pairs correctly'๋ก ์ค๋จ๋๋์ง๋ ์ถ๊ฐํ ์ ์์ต๋๋ค.์ ๊ฐ ํด๋น ํ ์คํธ ์ผ์ด์ค๋ฅผ ์์ฑํด ๋๋ฆด๊น์?
๐ค 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 `@aFIPC.Rcheck/tests/testthat/test-sentinel-validation.R` around lines 1 - 37, Add coverage for the new ^[12]$ input validation in the readline confirmation paths, targeting checkCorrect, checknewformBILOGprior, and checkoldformBILOGprior. Verify exact-match handling and termination after more than three invalid attempts, including non-interactive confirmCommonItems = NULL terminating clearly and confirmCommonItems = FALSE terminating with โPlease write down pairs correctlyโ.aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-autoFIPC.R (1)
1-25: ๐ฉบ Stability & Availability | ๐ต Trivial | ๐๏ธ Heavy liftPR์ ํต์ฌ ๋ณด์ ์์ (
checkCorrect()์ readline ์ ๊ท์^[0-9]+$โ^[12]$) ์์ฒด์ ๋ํ ํ๊ท ํ ์คํธ๊ฐ ์์ต๋๋ค.Line 1-12, 14-25์ ํ ์คํธ๋
interactive()๊ฐ FALSE์ผ ๋ ์กฐ๊ธฐ์ ์ค๋ฅ๊ฐ ๋ฐ์ํ๋ ๊ฒฝ๋ก๋ง ๊ฒ์ฆํ๋ฉฐ, ์ค์ ๋ก readline ์ ๋ ฅ์ ์ ๊ท์์ผ๋ก ๊ฒ์ฆํ๋ ๋ก์ง(๋ฐ 3ํ ์คํจ ์ "Too many invalid common item confirmation attempts" ์ข ๋ฃ ๊ฒฝ๋ก)์ ์ ํ ์คํ๋์ง ์์ต๋๋ค.interactive()์readline()์ ํจ๊ป ๋ชจํนํด^[0-9]+$์๋ค๋ฉดas.integer()์ค๋ฒํ๋ก์ฐ๋กNA๊ฐ ๋ฐ์ํ์ ํฐ ์(์:"999999999999")๋"abc"๊ฐ์ ์ ๋ ฅ์ด ์ฌ๋ฐ๋ฅด๊ฒ ๊ฑฐ๋ถ๋๋์ง ์ง์ ๊ฒ์ฆํ๋ ํ ์คํธ๋ฅผ ์ถ๊ฐํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค.๋ค๋ง base ํจ์(
interactive,readline)๋ฅผtestthat::local_mocked_bindings()๋ก ๋ชจํนํ๋ ค๋ฉด ํจํค์ง ๋ค์์คํ์ด์ค์ ํด๋น ๋ฐ์ธ๋ฉ(์:interactive <- NULL)์ด ๋ฏธ๋ฆฌ ์กด์ฌํด์ผ ํ๋ฉฐ,local_mocked_bindings()๋ testthat 3.1.x์์ ์คํ์ ์ผ๋ก ๋์ ๋์ด ์ดํ(3.2.0) ์์ ํ๋์์ต๋๋ค. ์ด ํ์ผ์library_context๋testthat (>= 3.0.0)๋ง ์๊ตฌํ๋ค๊ณ ๋ช ์ํ๋ฏ๋ก, CI์์ ์ค์ ๋ก ์ด ๊ธฐ๋ฅ์ ์ฌ์ฉํ ์ ์๋ testthat ๋ฒ์ ์ด ์ค์น๋๋์ง ํ์ธ์ด ํ์ํฉ๋๋ค.test_that("checkCorrect rejects overflow/non-matching readline input", { local_mocked_bindings( interactive = function() TRUE, readline = function(...) "999999999999", .package = "base" ) 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" ) })์ด ํ ์คํธ ์ปค๋ฒ๋ฆฌ์ง๋ฅผ ์ถ๊ฐํ๋ PR์ ๋์๋๋ฆด๊น์, ์๋๋ฉด ์ด์๋ก ๋ฑ๋กํด ๋๋ฆด๊น์?
๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-autoFIPC.R` around lines 1 - 25, Extend the tests around autoFIPC to exercise checkCorrectโs interactive readline validation directly, rather than only testing the non-interactive early exit. Mock interactive() and readline() using a testthat version-compatible approach, then verify oversized numeric input such as "999999999999" and non-matching input such as "abc" are rejected and repeated invalid responses reach the "Too many invalid common item confirmation attempts" error; ensure the packageโs testthat dependency supports the chosen mocking mechanism.
๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/man/autoFIPC.Rd`:
- Around line 38-48: Roxygen documentation in the source contains the typos โset
the thisโ and โdefalut.โ Update the corresponding parameter descriptions in the
roxygen comments for the aFIPC configuration documentation in R/aFIPC.R, then
regenerate the .Rd file instead of editing the generated documentation directly.
In `@aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R`:
- Around line 88-91: Restrict the itemtype validation near nItems to accept only
a single value, since downstream scalar conditions in the aFIPC processing flow
(including the checks at lines 166, 189, 385, 408, 607, 657, and 891) do not
support per-item vectors. Update the validation error to require length 1 and
remove the length nItems acceptance path.
- Around line 1-4: aFIPC.Rcheck/ ์ ์ฒด๋ R CMD check ์ฐ์ถ๋ฌผ์ด๋ฏ๋ก ๋ฒ์ ๊ด๋ฆฌ์์ ์ ๊ฑฐํ์ธ์.
aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R(1-4),
aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-optimization-equivalence.R(1-20),
aFIPC.Rcheck/tests/testthat/test-optimization-equivalence.R(1-20)์ ์ญ์ ํ๊ณ
R/aFIPC.R ๋ฐ tests/testthat/ ์๋ ์ ๋ณธ๋ง ์ ์งํ์ธ์. ์ฌ์์ฑ์ ๋ง๊ธฐ ์ํด .gitignore์ *.Rcheck/ ํจํด๋
์ถ๊ฐํ์ธ์.
- Around line 627-647: Handle the all-FALSE valid_idx case before constructing
IPDData: when no common items match, skip or return from the IPD generation
block using the existing function behavior. Otherwise, keep IPDItemCount
positive and generate column names from the matched item count without allowing
1:0 to produce invalid names.
- Around line 853-868: Update the forceNormalZeroOne branch to identify MEAN_1
instead of MEAN_11, and apply symmetric est = FALSE and value assignments for
both NewScaleParms and OldScaleParms. Ensure COV_11 remains fixed to 1 and
MEAN_1 is fixed to 0 in both parameter groups.
In `@aFIPC.Rcheck/aFIPC/DESCRIPTION`:
- Line 4: ํต์ผ๋ ์ ์ ๋ฒ์ ์ ์ ํํ ๋ค DESCRIPTION์ Version๊ณผ aFIPC ๋ฐฐ๋๊ฐ ๋์ผํ ๋ฒ์ ์ ์ฌ์ฉํ๋๋ก ์์ ํ์ธ์.
ํนํ DESCRIPTION์ Version: 0.1.0๊ณผ aFIPC.R์ ๋ฐฐ๋๋ฅผ ํจ๊ป ๊ฐฑ์ ํ๊ณ , ๋ ์์น์ ๋ฒ์ ํ๊ธฐ๋ฅผ ์ผ์น์ํค์ธ์.
- Line 11: Update aFIPC.Rcheck/aFIPC/DESCRIPTION:11 to use the simplified GPL-3
license declaration. Update aFIPC.Rcheck/aFIPC/LICENSE:1-2 as needed only if
retaining the file LICENSE reference, replacing the current template text with
the actual GPL-3 license body.
In `@aFIPC.Rcheck/aFIPC/html/00Index.html`:
- Around line 26-27: Update the \description{} content in the source surveyFA.Rd
documentation to describe surveyFAโs actual functionality instead of repeating
the function name, then regenerate the corresponding HTML package index output
so the surveyFA entry uses that description.
In `@aFIPC.Rcheck/aFIPC/html/R.css`:
- Line 127: Update the font-family declaration in the R.css stylesheet to quote
the multi-word โCourier Newโ font name, while preserving the existing Courier
fallback.
In `@aFIPC.Rcheck/tests/testthat/test-autoFIPC.R`:
- Around line 1-91: Remove the duplicated test-autoFIPC.R artifact under the
aFIPC.Rcheck/ directory from the repository, and remove any other committed R
CMD check output in that directory as applicable. Keep the canonical
tests/testthat/test-autoFIPC.R file unchanged.
In `@aFIPC.Rcheck/tests/testthat/test-surveyFA.R`:
- Around line 61-70: Update the surveyFA exhaustion test to call set.seed()
before generating the rbinom() data, using the established deterministic-test
pattern such as the L3 test. Keep the requested scope focused on
reproducibility; do not alter the dataset shape unless needed to exercise the
autofix repetition path.
---
Nitpick comments:
In `@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-autoFIPC.R`:
- Around line 1-25: Extend the tests around autoFIPC to exercise checkCorrectโs
interactive readline validation directly, rather than only testing the
non-interactive early exit. Mock interactive() and readline() using a testthat
version-compatible approach, then verify oversized numeric input such as
"999999999999" and non-matching input such as "abc" are rejected and repeated
invalid responses reach the "Too many invalid common item confirmation attempts"
error; ensure the packageโs testthat dependency supports the chosen mocking
mechanism.
In `@aFIPC.Rcheck/aFIPC-Ex.R`:
- Around line 33-41: autoFIPC ์์ ๋ฅผ ์คํ ๊ฐ๋ฅํ ์ต์ ์์ ๋ก ์์ ํ๊ณ ,
new_modelยทold_modelยทcommon_newยทcommon_old ๋ฑ ํ์ํ toy ์
๋ ฅ๊ณผ ๋ชจ๋ธ ์์ฑ ์ ์ฐจ๋ฅผ ํฌํจํ์ธ์. ์์ ๋ฅผ ์ค์ ๋ก
์คํํด API ๊ณ์ฝ์ ๊ฒ์ฆํ ์ ์๋๋ก Not run ์ํ๋ฅผ ์ ๊ฑฐํ๊ณ ์ฐ์ถ๋ฌผ์ ์ฌ์์ฑํ๋ฉฐ, ์คํ์ด ๋ถ๊ฐ๋ฅํ ๊ฒฝ์ฐ์๋ ์๋ณธ Rd ๋ฌธ์์ ๊ทธ ์ฌ์ ์
ํ์ํ ์ ์ฐจ๋ฅผ ๋ช
์ํ ๋ค ์ฌ์์ฑํ์ธ์.
In `@aFIPC.Rcheck/aFIPC/Meta/Rd.rds`:
- Line 1: Remove the generated RDS metadata artifact under
aFIPC.Rcheck/aFIPC/Meta from version control and add the corresponding
aFIPC.Rcheck build/check output pattern to the repositoryโs ignore configuration
so CI recreates it during package checks.
In `@aFIPC.Rcheck/aFIPC/R/aFIPC.rdb`:
- Line 1: Remove the generated R CMD check artifacts under the aFIPC.Rcheck
tree, including aFIPC.rdb and any logs, RDS files, help databases, and installed
package copies; ensure the entire generated directory is excluded from version
control and retain validation results through CI artifacts instead.
In `@aFIPC.Rcheck/tests/testthat/test-fixed-parameter-calibration.R`:
- Around line 83-88: Update the assertions around linked_structural and the
corresponding linked_fixed and L115 checks so they first assert that the
filtered g/u rows exist and are non-empty, then verify their est values are
false. Ensure empty filtered vectors fail the test instead of allowing
expect_false(any(...)) to pass.
In `@aFIPC.Rcheck/tests/testthat/test-optimization-equivalence.R`:
- Around line 37-50: Refactor the category-count logic used by autoFIPC into a
shared internal helper, then update both the implementation and this test to
call that helper instead of duplicating length(na.omit(unique(x))). Preserve the
comparison with the legacy levels(as.factor(x)) expression and the existing
expected-value assertions.
In `@aFIPC.Rcheck/tests/testthat/test-sentinel-validation.R`:
- Around line 1-37: Add coverage for the new ^[12]$ input validation in the
readline confirmation paths, targeting checkCorrect, checknewformBILOGprior, and
checkoldformBILOGprior. Verify exact-match handling and termination after more
than three invalid attempts, including non-interactive confirmCommonItems = NULL
terminating clearly and confirmCommonItems = FALSE terminating with โPlease
write down pairs correctlyโ.
๐ช 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: c63e58ea-be92-44ce-a4fc-178f882b04f1
โ Files ignored due to path filters (3)
aFIPC.Rcheck/00check.logis excluded by!**/*.logaFIPC.Rcheck/00install.outis excluded by!**/*.outaFIPC.Rcheck/aFIPC-Ex.pdfis excluded by!**/*.pdf
๐ Files selected for processing (51)
aFIPC.Rcheck/00_pkg_src/aFIPC/DESCRIPTIONaFIPC.Rcheck/00_pkg_src/aFIPC/LICENSEaFIPC.Rcheck/00_pkg_src/aFIPC/NAMESPACEaFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.RaFIPC.Rcheck/00_pkg_src/aFIPC/R/surveyFA.RaFIPC.Rcheck/00_pkg_src/aFIPC/README.mdaFIPC.Rcheck/00_pkg_src/aFIPC/man/autoFIPC.RdaFIPC.Rcheck/00_pkg_src/aFIPC/man/surveyFA.RdaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-autoFIPC.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-fixed-parameter-calibration.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-optimization-equivalence.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-package-api.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-sentinel-validation.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-surveyFA.RaFIPC.Rcheck/R_check_bin/RaFIPC.Rcheck/R_check_bin/RscriptaFIPC.Rcheck/aFIPC-Ex.RaFIPC.Rcheck/aFIPC-Ex.RoutaFIPC.Rcheck/aFIPC-Ex.timingsaFIPC.Rcheck/aFIPC/DESCRIPTIONaFIPC.Rcheck/aFIPC/INDEXaFIPC.Rcheck/aFIPC/LICENSEaFIPC.Rcheck/aFIPC/Meta/Rd.rdsaFIPC.Rcheck/aFIPC/Meta/features.rdsaFIPC.Rcheck/aFIPC/Meta/hsearch.rdsaFIPC.Rcheck/aFIPC/Meta/links.rdsaFIPC.Rcheck/aFIPC/Meta/nsInfo.rdsaFIPC.Rcheck/aFIPC/Meta/package.rdsaFIPC.Rcheck/aFIPC/NAMESPACEaFIPC.Rcheck/aFIPC/R/aFIPCaFIPC.Rcheck/aFIPC/R/aFIPC.rdbaFIPC.Rcheck/aFIPC/R/aFIPC.rdxaFIPC.Rcheck/aFIPC/help/AnIndexaFIPC.Rcheck/aFIPC/help/aFIPC.rdbaFIPC.Rcheck/aFIPC/help/aFIPC.rdxaFIPC.Rcheck/aFIPC/help/aliases.rdsaFIPC.Rcheck/aFIPC/help/paths.rdsaFIPC.Rcheck/aFIPC/html/00Index.htmlaFIPC.Rcheck/aFIPC/html/R.cssaFIPC.Rcheck/tests/startup.RsaFIPC.Rcheck/tests/testthat.RaFIPC.Rcheck/tests/testthat.RoutaFIPC.Rcheck/tests/testthat/test-autoFIPC.RaFIPC.Rcheck/tests/testthat/test-fixed-parameter-calibration.RaFIPC.Rcheck/tests/testthat/test-optimization-equivalence.RaFIPC.Rcheck/tests/testthat/test-package-api.RaFIPC.Rcheck/tests/testthat/test-sentinel-validation.RaFIPC.Rcheck/tests/testthat/test-surveyFA.Rtest_dummy.Rtest_validation.R
๐ค Files with no reviewable changes (2)
- test_validation.R
- test_dummy.R
โฆ issue Fixed weak regex validation for integer conversion via readline() in R/aFIPC.R to prevent DoS or crashes via as.integer() type coercion. Added `.semgrepignore` to `.Rbuildignore` to fix `R CMD check` CI failure regarding unrecognized hidden files. Removed `test_dummy.R` and `test_validation.R` from the top level and added `aFIPC.Rcheck` to `.gitignore` to fix `R CMD check` due to non-standard files found.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.jules/sentinel.md:6
- The new sentinel entry is dated 2024-07-26, but this PR is dated 2026-07-26. If this is meant to be a running incident log, the year should match the actual fix date to avoid confusion when auditing security learnings later.
## 2024-07-26 - Integer Coercion Vulnerability from readline()
| } | ||
| 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)) { | ||
| return(as.integer(n)) | ||
| } |
๐จ Severity: MEDIUM
๐ก Vulnerability: Unbounded input via
readline()coerced withas.integer()when checked against^[0-9]+$. If a very large number is provided,as.integer()yieldsNA, which leads to runtime errors or unhandled exceptions when passing lengths > 1 to conditional logic. This is an unexpected behavior/coercion bug that can be exploited for application DoS.๐ฏ Impact: Potential application crash/Denial of Service when automating or improperly interacting with prompts.
๐ง Fix: Changed the weak match
^[0-9]+$to exact-bounded selection bounds^[12]$ensuring safety ofas.integer()coercion.โ Verification: Ran
Rscript -e 'devtools::test()'and coverage tools. Tests passed and coverage is maintained. Recorded critical learning in.jules/sentinel.md.PR created automatically by Jules for task 15724802833890598210 started by @seonghobae
Summary by CodeRabbit