Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
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: 8 additions & 4 deletions .github/workflows/security-audit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,19 @@
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

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.


- 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"

Check failure on line 32 in .github/workflows/security-audit.yml

View workflow job for this annotation

GitHub Actions / quality

32:141 [line-length] line too long (147 > 140 characters)
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"

Check failure on line 33 in .github/workflows/security-audit.yml

View workflow job for this annotation

GitHub Actions / quality

33:141 [line-length] line too long (168 > 140 characters)
grep "$GITLEAKS_FILE" gitleaks_checksums.txt | sha256sum -c -
tar -xzf "$GITLEAKS_FILE" gitleaks
chmod +x gitleaks
Expand All @@ -40,8 +44,8 @@
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"

Check failure on line 48 in .github/workflows/security-audit.yml

View workflow job for this annotation

GitHub Actions / quality

48:141 [line-length] line too long (147 > 140 characters)
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
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.
6 changes: 3 additions & 3 deletions R/aFIPC.R
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)) {

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.

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
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.
Resolved
Comment on lines +44 to +45

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