Skip to content

πŸ›‘οΈ Sentinel: [CRITICAL] μž…λ ₯ κ°’ 검증 κ°•ν™”λ₯Ό ν†΅ν•œ DoS(NA Coercion) 방지 #289

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 6 commits into
base: master
Choose a base branch
from
Open

πŸ›‘οΈ Sentinel: [CRITICAL] μž…λ ₯ κ°’ 검증 κ°•ν™”λ₯Ό ν†΅ν•œ DoS(NA Coercion) 방지 #289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0adaed3
πŸ›‘οΈ Sentinel: [CRITICAL] Fix unbounded numeric input validation (DoS v…
seonghobae Aug 25, 2026
3f08a91
πŸ›‘οΈ Sentinel: Fix security-audit CI workflow connection failure
seonghobae Aug 25, 2026
c3f118a
πŸ›‘οΈ Sentinel: Fix yamllint line length errors in CI
seonghobae Aug 25, 2026
8a19598
πŸ›‘οΈ Sentinel: Fix R CMD check missing mockery dependency
seonghobae Aug 25, 2026
1f24c24
πŸ›‘οΈ Sentinel: [CRITICAL] CI μ—λŸ¬ 및 μž…λ ₯ κ°’ 검증 κ°•ν™”λ₯Ό ν†΅ν•œ DoS(NA Coercion) 방지
seonghobae Aug 25, 2026
94964a1
πŸ›‘οΈ Sentinel: [CRITICAL] CI μ—λŸ¬ 및 μž…λ ₯ κ°’ 검증 κ°•ν™”λ₯Ό ν†΅ν•œ DoS 방지
seonghobae Aug 25, 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
12 changes: 10 additions & 2 deletions .github/workflows/code-quality.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ jobs:
uses: step-security/harden-runner@bf7454d06d71f1098171f2acdf0cd4708d7b5920
with:
egress-policy: audit
allowed-endpoints: >
github.com:443
objects.githubusercontent.com:443
release-assets.githubusercontent.com:443
127.0.0.1:48080
localhost:48080

- name: Checkout
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd
Expand All @@ -37,8 +43,10 @@ jobs:
ACTIONLINT_VERSION="1.7.10"
ACTIONLINT_FILE="actionlint_${ACTIONLINT_VERSION}_linux_amd64.tar.gz"
ACTIONLINT_BASE_URL="https://github.com/rhysd/actionlint/releases/download/v${ACTIONLINT_VERSION}"
curl -sSLo "$ACTIONLINT_FILE" "${ACTIONLINT_BASE_URL}/${ACTIONLINT_FILE}"
curl -sSLo actionlint_checksums.txt "${ACTIONLINT_BASE_URL}/actionlint_${ACTIONLINT_VERSION}_checksums.txt"
curl --retry 5 --retry-connrefused -sSLo "$ACTIONLINT_FILE" \
"${ACTIONLINT_BASE_URL}/${ACTIONLINT_FILE}"
curl --retry 5 --retry-connrefused -sSLo actionlint_checksums.txt \
"${ACTIONLINT_BASE_URL}/actionlint_${ACTIONLINT_VERSION}_checksums.txt"
grep "$ACTIONLINT_FILE" actionlint_checksums.txt | sha256sum -c -
tar -xzf "$ACTIONLINT_FILE" actionlint
chmod +x actionlint
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/r.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ jobs:
uses: step-security/harden-runner@bf7454d06d71f1098171f2acdf0cd4708d7b5920
with:
egress-policy: audit
allowed-endpoints: >
github.com:443
objects.githubusercontent.com:443
release-assets.githubusercontent.com:443
127.0.0.1:48080
localhost:48080

- name: Checkout
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd
Expand Down
18 changes: 14 additions & 4 deletions .github/workflows/security-audit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,23 @@ jobs:
uses: step-security/harden-runner@bf7454d06d71f1098171f2acdf0cd4708d7b5920
with:
egress-policy: audit
allowed-endpoints: >
github.com:443
objects.githubusercontent.com:443
release-assets.githubusercontent.com:443
Comment on lines 20 to +24

@devin-ai-integration devin-ai-integration Bot Aug 25, 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: allowed-endpoints has no effect under audit policy

The workflow adds allowed-endpoints while keeping egress-policy: audit. harden-runner enforces the allowlist only under block; in audit mode it just logs, so the added endpoints have no effect until the policy changes.

Open in Devin Review

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

127.0.0.1:48080
localhost:48080

- name: Checkout
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd

- name: Install gitleaks
run: |
GITLEAKS_FILE="gitleaks_8.24.2_linux_x64.tar.gz"
curl -sSLo "$GITLEAKS_FILE" "https://github.com/gitleaks/gitleaks/releases/download/v8.24.2/$GITLEAKS_FILE"
curl -sSLo gitleaks_checksums.txt "https://github.com/gitleaks/gitleaks/releases/download/v8.24.2/gitleaks_8.24.2_checksums.txt"
curl --retry 5 --retry-connrefused -sSLo "$GITLEAKS_FILE" \
"https://github.com/gitleaks/gitleaks/releases/download/v8.24.2/$GITLEAKS_FILE"
curl --retry 5 --retry-connrefused -sSLo gitleaks_checksums.txt \
"https://github.com/gitleaks/gitleaks/releases/download/v8.24.2/gitleaks_8.24.2_checksums.txt"
grep "$GITLEAKS_FILE" gitleaks_checksums.txt | sha256sum -c -
tar -xzf "$GITLEAKS_FILE" gitleaks
chmod +x gitleaks
Expand All @@ -40,8 +48,10 @@ jobs:
ACTIONLINT_VERSION="1.7.10"
ACTIONLINT_FILE="actionlint_${ACTIONLINT_VERSION}_linux_amd64.tar.gz"
ACTIONLINT_BASE_URL="https://github.com/rhysd/actionlint/releases/download/v${ACTIONLINT_VERSION}"
curl -sSLo "$ACTIONLINT_FILE" "${ACTIONLINT_BASE_URL}/${ACTIONLINT_FILE}"
curl -sSLo actionlint_checksums.txt "${ACTIONLINT_BASE_URL}/actionlint_${ACTIONLINT_VERSION}_checksums.txt"
curl --retry 5 --retry-connrefused -sSLo "$ACTIONLINT_FILE" \
"${ACTIONLINT_BASE_URL}/${ACTIONLINT_FILE}"
curl --retry 5 --retry-connrefused -sSLo actionlint_checksums.txt \
"${ACTIONLINT_BASE_URL}/actionlint_${ACTIONLINT_VERSION}_checksums.txt"
grep "$ACTIONLINT_FILE" actionlint_checksums.txt | sha256sum -c -
tar -xzf "$ACTIONLINT_FILE" actionlint
chmod +x actionlint
Expand Down
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-25 - Fix missing bounded validation for readline inputs
**Vulnerability:** Unbounded numeric regex validation (e.g., `^[0-9]+$`) for `readline()` allows large inputs that coerce to `NA` via `as.integer()`. This causes downstream `if (variable == 1)` conditions to fail with a `missing value where TRUE/FALSE needed` error, resulting in unhandled exception crashes.
**Learning:** When reading integer inputs for binary choices, using bounded exact-match regex like `^[12]$` is necessary to prevent coercion crashes and Denial of Service (DoS) vulnerabilities in interactive environments.
**Prevention:** Always use strict and bounded regex validation for `readline()` input instead of broad numeric matching.
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ 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 25, 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: Retry loop rejects non-1/2 digits differently

With ^[12]$ (R/aFIPC.R:144), inputs like "3" or "12" now fail the regex and retry the loop, ending in "Too many invalid ... attempts" after 3 tries. Previously ^[0-9]+$ accepted them and fell through to the confirm != 1 stop. Behavior is still safe; only the error path differs.

Open in Devin Review

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

@devin-ai-integration devin-ai-integration Bot Aug 25, 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: Regex tightening applied to all binary prompts

All three interactive readline prompts are binary 1/2 choices and were each updated from ^[0-9]+$ to ^[12]$. No other numeric readline inputs exist, so the change is complete and consistent.

Open in 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
26 changes: 23 additions & 3 deletions tests/testthat/test-sentinel-validation.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
test_that("autoFIPC validates boolean flags for newformBILOGprior, oldformBILOGprior, and confirmCommonItems", {
# newformBILOGprior
expect_error(
aFIPC::autoFIPC(
autoFIPC(
newformXData = data.frame(A=1),
oldformYData = data.frame(A=2),
newformCommonItemNames = c('A'),
Expand All @@ -13,7 +13,7 @@ test_that("autoFIPC validates boolean flags for newformBILOGprior, oldformBILOGp

# oldformBILOGprior
expect_error(
aFIPC::autoFIPC(
autoFIPC(
newformXData = data.frame(A=1),
oldformYData = data.frame(A=2),
newformCommonItemNames = c('A'),
Expand All @@ -25,7 +25,7 @@ test_that("autoFIPC validates boolean flags for newformBILOGprior, oldformBILOGp

# confirmCommonItems
expect_error(
aFIPC::autoFIPC(
autoFIPC(
newformXData = data.frame(A=1),
oldformYData = data.frame(A=2),
newformCommonItemNames = c('A'),
Expand All @@ -35,3 +35,23 @@ test_that("autoFIPC validates boolean flags for newformBILOGprior, oldformBILOGp
"Security Error: confirmCommonItems must be a single non-NA logical value or NULL"
)
})

test_that("large numbers do not cause NA coercion errors in autoFIPC readline prompt", {

# Give a huge number that passes ^[0-9]+$ but fails as.integer()
# Then give a valid '2' to continue if it rejects the first one
m <- mockery::mock("999999999999999999999999999999", "2", "2", "2")
mockery::stub(autoFIPC, 'readline', m)
mockery::stub(autoFIPC, 'interactive', TRUE)
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Comment on lines +44 to +45

@devin-ai-integration devin-ai-integration Bot Aug 25, 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: Test stubs depend on nested lexical scope

The test stubs readline/interactive on autoFIPC, but both run inside the nested checkCorrect() closure (aFIPC.R:129-149). Default mockery::stub reaches it only via shared lexical scope; extracting checkCorrect into a separate helper would silently break the stub.

Open in Devin Review

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


# Check that it rejects the large number correctly by prompting again (which gives 2) and thus stopping
expect_error({
autoFIPC(
newformXData = data.frame(A=c(1,0,1,0), B=c(1,1,0,0)),
oldformYData = data.frame(A=c(1,0,1,0), B=c(1,1,0,0)),
newformCommonItemNames = c('A'),
oldformCommonItemNames = c('A'),
confirmCommonItems = NULL
)
}, "Please write down pairs correctly")
})
Loading