Merge 1.1.1.3 Release into master - #33
Conversation
Added connection options for timeout and connection limits.
1.1.0.docker
Fixed failing Jest tests that were causing TypeError and timeout issues: Token Service Tests (test/services/token-service.test.js): - Added mock userService to TokenService constructor - Implemented getUserTokenUUIDs mock with proper Jest functions - Updated all tests to handle async operations correctly - Tests now properly resolve/reject promises Auth Tests (test/auth.test.js): - Added express-mysql-session mock to prevent MySQL connection attempts during tests - Fixed duplicate test names (all three were "auth nih login called once") - Corrected test logic to mock the appropriate IDP client for each test - Added EventService mock to prevent database operations - Increased test timeout to 10 seconds for async operations - Added proper test lifecycle hooks (beforeAll, afterEach, afterAll) - Fixed mockLoginResult to include all required fields (name, lastName, tokens, email, idp) Result: All 25 tests now pass successfully without timeouts or errors.
Introduces a test workflow that runs on pushes, pull requests, and manual dispatch. The workflow checks out the repository, sets up Node.js, installs dependencies, runs tests, and uploads coverage reports to Coveralls.
Bump package version to 1.1.0 and add test-related scripts for running Jest, CI testing, and coverage reporting.
Added /coverage, .vscode/, and .DS_Store to .gitignore to prevent committing coverage reports, editor settings, and macOS system files.
Set mock database and secret-related environment variables in the GitHub Actions test workflow to support test execution with MySQL and required secrets.
Updated test/health.test.js to use consistent double quotes and improved formatting for readability. Removed unnecessary jest.useFakeTimers('legacy') call.
Replaced static mockLoginResult with createMockLoginResult function to generate mock login results based on the IDP being tested. This improves test accuracy by ensuring the idp field matches the provider under test.
CTDC-1884: Testing setup
chore: upgrade Node.js to v24 and fix 98 security vulnerabilities Baseline: crdc-ctdc-authn:1.1.0.69 had 99 vulnerabilities (3 critical, 14 high) Key Changes: - Upgrade Node.js 20.11.1 → 24.0.0, Alpine 3.19 → 3.23 - Upgrade npm to 11.14.1 (fixes bundled CVEs) - Update vulnerable packages: express, nodemailer, http-proxy-middleware, uuid, etc. - Add npm overrides for transitive dependencies (brace-expansion, picomatch, ip-address) - Remove unused packages: sequelize, session-file-store, newrelic - Add .dockerignore to prevent copying node_modules and secrets - Add USER node directive for non-root execution (CIS Docker compliance) - Add Node.js engine requirement (^24.0.0) and .nvmrc Result: 99 → 1 vulnerability (98.99% reduction) - Remaining: CVE-2025-60876 (MEDIUM) - busybox in Alpine base (requires upstream fix) - Compliance: 0 issues
Update .github/workflows/test.yml to use actions/setup-node@v4 with node-version set to 24.x (was 20.x). This runs the test workflow on a newer Node.js runtime while keeping npm caching unchanged.
- Upgrade jest from 28.1.3 to 30.3.0 for improved ES module support - Upgrade uuid to 11.1.1 (fixes CVE-2026-41907 HIGH) - Update test API: toBeCalledTimes → toHaveBeenCalledTimes (Jest 30 compatibility) - All 25 tests passing across 7 test suites
CTDC-2010: Security remediation - Node.js 24 upgrade and vulnerability fixes (99→1)
Refactor log directory creation to use recursive option and handle errors gracefully.
Remove the Dockerfile USER node entry so the container runs as the default user (root) and remove left over accessLogStream
📝 WalkthroughWalkthroughThe PR updates the project to Node.js 24, changes Docker packaging, adds database timeout settings, introduces build and test workflows, improves logging fallback behavior, and updates authentication, health, and token-service tests. ChangesRuntime and delivery updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Docker
participant Trivy
participant AWS OIDC
participant Amazon ECR
participant Slack
GitHub Actions->>Docker: Build and tag image
GitHub Actions->>Trivy: Scan image
GitHub Actions->>AWS OIDC: Authenticate
GitHub Actions->>Amazon ECR: Push image
GitHub Actions->>Slack: Send workflow status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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: 13
🧹 Nitpick comments (2)
Dockerfile (1)
1-1: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the production base image by digest.
node:24-alpine3.23is a mutable tag. A later build can pull different image layers without a source change. Use an approved digest and update it through a controlled dependency change. Docker documents that tags can change while digests are immutable. (docs.docker.com)🤖 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 `@Dockerfile` at line 1, Update the fnl_base_image FROM declaration to pin node:24-alpine3.23 to the approved immutable image digest, preserving the current Node and Alpine versions. Manage future digest updates through the controlled dependency-update process.Source: MCP tools
package.json (1)
20-33: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueIf New Relic is retired, remove its stale configuration. No runtime references to
newrelic,sequelize, orsession-file-storeremain. Onlynewrelic.js,NEW_RELIC_*entries in.env-template, and the.gitignorelog entry remain.🤖 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 `@package.json` around lines 20 - 33, Remove the stale New Relic configuration artifacts: delete newrelic.js, remove all NEW_RELIC_* entries from .env-template, and remove the New Relic log entry from .gitignore. Do not alter the dependency declarations shown in package.json.
🤖 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 @.github/workflows/build.yml:
- Around line 85-102: Reorder the workflow so the “Push to Amazon ECR” step
completes before “Create Git tag for Image” runs. Keep the existing tag commands
unchanged, ensuring Git tag creation occurs only after authentication and docker
push succeed.
- Around line 2-4: Update .github/workflows/build.yml lines 2-4 so build and
Trivy scanning use read-only contents permission and no id-token access. At
lines 22-26, scope Slack and AWS values only to the release steps that consume
them. At lines 30-33, disable checkout credential persistence and configure Git
push authentication only after scanning succeeds.
- Around line 8-10: Update the workflow_dispatch input named environment to be
required, ensuring every manual deployment supplies an environment before
authentication uses its environment-scoped AWS values.
- Around line 35-52: Update the Set Image Tag step to sort matching tags by
version rather than lexically before selecting the latest tag, so numeric
suffixes such as .10 correctly follow .9. Add job-level concurrency keyed by
repository, environment, and ref to serialize runs and prevent duplicate
image-tag allocation.
In @.github/workflows/test.yml:
- Around line 13-16: Add a MySQL service to the test job alongside runs-on,
configure it to expose port 3306 and include a health check, then ensure the
test steps run only after the service is healthy before invoking npm run
test:ci. Preserve the existing MySQL connection environment configuration.
- Around line 18-21: Update the actions/checkout@v4 step in the workflow to set
persist-credentials to false alongside the existing recursive submodules option,
preventing the checkout token from being stored in local Git configuration.
In `@app.js`:
- Around line 19-20: In the log-directory creation error handler, replace the
invalid logger.warn call with console.warn while preserving the existing warning
message and err.message details, so startup can fall back to stdout without
invoking the Morgan middleware factory.
- Around line 15-20: Update the accessLogStream setup around
fs.createWriteStream to attach an error handler for asynchronous open and write
failures. On error, switch Morgan’s active logging destination to process.stdout
through the existing accessLogStream reference, preventing unhandled stream
errors while preserving the synchronous fallback behavior.
In `@Dockerfile`:
- Around line 9-11: Update Dockerfile to copy package-lock.json alongside
package.json and use npm ci --omit=dev --ignore-scripts instead of npm install.
Remove package-lock.json from .dockerignore, and regenerate and commit the
lockfile with the required overrides entries so production dependency resolution
is reproducible.
- Around line 13-14: Update the Dockerfile before CMD to create
/usr/src/app/logs, assign ownership or write permissions to node, and switch
execution to USER node so the application runs non-root while retaining access
to logs/access.log.
In `@package.json`:
- Line 11: Update the package.json test:ci script to remove the
--passWithNoTests flag while preserving the existing timezone, CI, coverage,
Jest, and worker settings.
In `@services/mySQL/mySQL-operations.js`:
- Around line 11-15: Bound the connection queues in both pool configurations:
services/mySQL/mySQL-operations.js lines 11-15 and services/mysql-connection.js
lines 9-13. Update the pool options near waitForConnections to set a finite
queueLimit, or disable waiting, while preserving controlled overload behavior.
- Around line 12-15: Update all five MySQL helper functions to assign the
acquired connection to the outer connection variable rather than declaring an
inner currentConnection. Remove direct release calls from success and catch
paths, and release the outer connection once in finally when it exists,
preserving the original acquisition or query error.
---
Nitpick comments:
In `@Dockerfile`:
- Line 1: Update the fnl_base_image FROM declaration to pin node:24-alpine3.23
to the approved immutable image digest, preserving the current Node and Alpine
versions. Manage future digest updates through the controlled dependency-update
process.
In `@package.json`:
- Around line 20-33: Remove the stale New Relic configuration artifacts: delete
newrelic.js, remove all NEW_RELIC_* entries from .env-template, and remove the
New Relic log entry from .gitignore. Do not alter the dependency declarations
shown in package.json.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be12241d-f633-4a35-8b34-8675a62f5187
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.dockerignore.github/workflows/build.yml.github/workflows/test.yml.gitignore.nvmrcDockerfileapp.jspackage.jsonservices/mySQL/mySQL-operations.jsservices/mysql-connection.jstest/auth.test.jstest/health.test.jstest/services/token-service.test.js
| jobs: | ||
| test: | ||
| name: Test Changes | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Start the MySQL service for this job.
Lines 34-39 configure the application to use MySQL at localhost:3306, but this workflow does not start MySQL. Environment variables do not provision a database. Add a MySQL job service, map port 3306, and wait for its health check before npm run test:ci.
Proposed fix
jobs:
test:
name: Test Changes
runs-on: ubuntu-latest
+ services:
+ mysql:
+ image: mysql:8.0
+ env:
+ MYSQL_DATABASE: test
+ MYSQL_USER: test
+ MYSQL_PASSWORD: test
+ MYSQL_ROOT_PASSWORD: test-root-password
+ ports:
+ - 3306:3306
+ options: >-
+ --health-cmd="mysqladmin ping -h localhost -u test -ptest"
+ --health-interval=10s
+ --health-timeout=5s
+ --health-retries=5Also applies to: 32-42
🤖 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 @.github/workflows/test.yml around lines 13 - 16, Add a MySQL service to the
test job alongside runs-on, configure it to expose port 3306 and include a
health check, then ensure the test steps run only after the service is healthy
before invoking npm run test:ci. Preserve the existing MySQL connection
environment configuration.
| - name: Checkout Repository | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| submodules: 'recursive' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/test.yml
printf '%s\n' '--- package manifests ---'
git ls-files | grep -E '(^|/)(package\.json|npm-shrinkwrap\.json|package-lock\.json|yarn\.lock|pnpm-lock\.yaml)$' || true
printf '%s\n' '--- lifecycle-related scripts and git configuration references ---'
rg -n --hidden -S 'persist-credentials|npm (install|ci)|preinstall|install|postinstall|prepare|git config|credential' \
.github package.json package-lock.json npm-shrinkwrap.json 2>/dev/null || trueRepository: CBIIT/crdc-ctdc-authn
Length of output: 2796
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package.json ---'
cat -n package.json
printf '%s\n' '--- submodule configuration ---'
if [ -f .gitmodules ]; then
cat -n .gitmodules
else
printf '%s\n' 'No .gitmodules file'
fi
printf '%s\n' '--- repository Git metadata tracked by the worktree ---'
git ls-files -s | awk '$1 == "160000" {print}'Repository: CBIIT/crdc-ctdc-authn
Length of output: 2313
🌐 Web query:
actions/checkout v4 persist-credentials default local git config token official documentation
💡 Result:
The persist-credentials input for actions/checkout (including v4) has a default value of true [1][2][3]. When set to true, the action configures the repository to use the provided authentication token (typically the GITHUB_TOKEN) for subsequent git operations [4][3][5]. Regarding how it interacts with the local git configuration: - In versions prior to the changes introduced in later releases (like v6.0.0+), the authentication token was written directly into the local.git/config file [5]. - Starting with more recent versions (e.g., v6.0.0 and later), the action improved security by storing the credentials in a separate file within the $RUNNER_TEMP directory, using an includeIf directive in the local.git/config to reference it, rather than placing the token directly in the repository's configuration [6][7][5]. In all versions, the token is automatically removed during the post-job cleanup process [4][3]. If you wish to disable this behavior and prevent the action from configuring any credentials, you can set persist-credentials to false [4][3][5].
Citations:
- 1: https://github.com/actions/checkout/blob/v4/action.yml
- 2: https://github.com/actions/checkout/blob/v4.1.1/action.yml
- 3: https://github.com/actions/checkout
- 4: https://github.com/actions/checkout/blob/v4/README.md
- 5: Set
persist-credentials: falseon checkout steps cli/go-gh#225 - 6: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 7: https://github.com/actions/checkout/tree/v6.0.0
Disable checkout credential persistence.
actions/checkout@v4 stores the token in local Git configuration by default. npm install can run dependency lifecycle scripts. Set persist-credentials: false; no later Git command requires this token.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 18-21: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/test.yml around lines 18 - 21, Update the
actions/checkout@v4 step in the workflow to set persist-credentials to false
alongside the existing recursive submodules option, preventing the checkout
token from being stored in local Git configuration.
Source: Linters/SAST tools
| let accessLogStream = process.stdout; | ||
| try { | ||
| fs.mkdirSync(LOG_FOLDER, { recursive: true }); | ||
| accessLogStream = fs.createWriteStream(path.join(__dirname, LOG_FOLDER, 'access.log'), { flags: 'a' }); | ||
| } catch (err) { | ||
| logger.warn(`Unable to create log directory "${LOG_FOLDER}", falling back to stdout: ${err.message}`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
fd -t f 'app\.js$|package\.json$' . | head -80
printf '%s\n' '--- app.js outline ---'
if [ -f app.js ]; then
ast-grep outline app.js
printf '%s\n' '--- app.js lines 1-100 ---'
cat -n app.js | sed -n '1,100p'
else
file="$(fd -t f 'app\.js$' . | head -1)"
printf 'selected=%s\n' "$file"
ast-grep outline "$file"
cat -n "$file" | sed -n '1,120p'
fi
printf '%s\n' '--- accessLogStream references ---'
rg -n -C 4 'accessLogStream|createWriteStream|morgan|LOG_FOLDER' . -g '!node_modules' -g '!dist' -g '!build' || trueRepository: CBIIT/crdc-ctdc-authn
Length of output: 8007
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- asynchronous createWriteStream error is outside try/catch ---'
node - <<'JS'
const fs = require('fs');
const target = '/tmp/coderabbit-nonexistent-parent-verify/access.log';
let caught = false;
try {
const stream = fs.createWriteStream(target, { flags: 'a' });
stream.once('error', (err) => {
console.log(`event=${err.code}; caught=${caught}; syncWritable=${stream.writable}`);
process.exit(0);
});
caught = true;
} catch (err) {
console.log(`caught=${err.code}`);
process.exit(2);
}
setTimeout(() => {
console.log('no-error-event');
process.exit(3);
}, 500);
JS
printf 'probe_status=%s\n' "$?"
printf '%s\n' '--- unhandled stream error process behavior ---'
node - <<'JS'
const fs = require('fs');
const stream = fs.createWriteStream('/tmp/coderabbit-nonexistent-parent-verify/access.log', { flags: 'a' });
setTimeout(() => console.log('still-running'), 300);
JS
printf 'unhandled_probe_status=%s\n' "$?"
printf '%s\n' '--- installed/runtime versions ---'
node --versionRepository: CBIIT/crdc-ctdc-authn
Length of output: 960
🌐 Web query:
Node.js fs.createWriteStream error event asynchronous open documentation morgan stream error handling
💡 Result:
In Node.js, fs.createWriteStream initiates an asynchronous file opening process [1][2]. Because this process is asynchronous, errors encountered during the opening phase (such as ENOENT or EISDIR) are emitted as 'error' events on the stream instance itself [3][4]. Key points regarding this behavior and morgan integration: Asynchronous Error Handling: Since fs.createWriteStream does not block the event loop, errors are not thrown synchronously at the moment of the function call [1][2]. You must attach an 'error' event listener to the returned stream immediately to catch these errors [3][4]. Morgan Stream Integration: When using morgan with a file stream (e.g., app.use(morgan('common', { stream: fs.createWriteStream(...) }))), morgan simply writes to the provided stream [5][6]. It does not inherently handle errors emitted by that stream [7]. If the underlying stream fails (e.g., the log file directory does not exist or permissions are denied), the error event will be emitted on the stream object itself [3][4]. Best Practices for Error Handling: 1. Attach Listeners: Always attach an 'error' listener to your custom log streams [3]. 2. Directory Existence: Ensure the target directory for your log file exists before calling fs.createWriteStream, as the stream will not create the directory for you and will emit an ENOENT error if the path is invalid [4]. 3. Stream Lifecycle: By default, if autoClose is true (the default), the file descriptor will be closed automatically on 'error' or 'finish' [8][2]. 4. Alternative Log Management: For production environments, it is often recommended to use dedicated logging libraries (like Winston or Pino) which provide more robust built-in mechanisms for stream management, rotation, and error handling compared to passing a raw fs.createWriteStream to morgan [9].
Citations:
- 1: https://nodejs.org/docs/latest-v24.x/api/fs.html
- 2: https://nodejs.org/download/release/v25.5.0/docs/api/fs.html
- 3: fs.WriteStream error and finish event nodejs/node#15262
- 4: https://stackoverflow.com/questions/66343346/why-is-fs-createwritestream-throwing-a-error-enoent-no-such-file-or-directory
- 5: https://github.com/expressjs/morgan/
- 6: https://www.npmjs.com/package/morgan
- 7: Error: EBADF: bad file descriptor, write expressjs/morgan#250
- 8: https://nodejs.org/api/fs.html
- 9: Option to skip new line when writing to stream expressjs/morgan#70.
Handle WriteStream errors outside the try block.
fs.createWriteStream() reports asynchronous open and write failures through the stream's error event. Attach an error handler and switch a delegating stream to process.stdout when the file stream fails. Otherwise, the unhandled error can terminate the process while Morgan still writes to the failed stream.
🤖 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 `@app.js` around lines 15 - 20, Update the accessLogStream setup around
fs.createWriteStream to attach an error handler for asynchronous open and write
failures. On error, switch Morgan’s active logging destination to process.stdout
through the existing accessLogStream reference, preventing unhandled stream
errors while preserving the synchronous fallback behavior.
Source: MCP tools
| } catch (err) { | ||
| logger.warn(`Unable to create log directory "${LOG_FOLDER}", falling back to stdout: ${err.message}`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 '\b(logger|morgan)\b' app.jsRepository: CBIIT/crdc-ctdc-authn
Length of output: 1189
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- app.js ---'
sed -n '1,35p' app.js
printf '%s\n' '--- morgan declarations ---'
rg -n -C 3 '"morgan"|require\(['"'"']morgan['"'"']\)|logger\.(warn|info|error)' package.json package-lock.json npm-shrinkwrap.json yarn.lock pnpm-lock.yaml app.js 2>/dev/null || trueRepository: CBIIT/crdc-ctdc-authn
Length of output: 3236
Replace logger.warn with console.warn
logger is the Morgan middleware factory (morgan 1.10.1), which has no warn method. When log setup fails, this call throws and prevents the application from starting.
🤖 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 `@app.js` around lines 19 - 20, In the log-directory creation error handler,
replace the invalid logger.warn call with console.warn while preserving the
existing warning message and err.message details, so startup can fall back to
stdout without invoking the Morgan middleware factory.
Source: MCP tools
| COPY package.json ./ | ||
| # Use npm install instead of npm ci to apply npm overrides for transitive dependency CVE fixes | ||
| RUN npm install --omit=dev --ignore-scripts |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tracked files ---'
git ls-files -- Dockerfile .dockerignore package.json package-lock.json npm-shrinkwrap.json
printf '%s\n' '--- Dockerfile ---'
cat -n Dockerfile
printf '%s\n' '--- .dockerignore ---'
cat -n .dockerignore
printf '%s\n' '--- package manifests ---'
for f in package.json package-lock.json npm-shrinkwrap.json; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f ---"
if [ "$f" = package.json ]; then
cat -n "$f"
else
sed -n '1,80p' "$f"
fi
fi
done
printf '%s\n' '--- npm install/ci references ---'
rg -n --hidden -g '!node_modules' -g '!dist' -g '!build' 'npm (install|ci)|package-lock\.json|npm-shrinkwrap\.json|overrides' .Repository: CBIIT/crdc-ctdc-authn
Length of output: 6462
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import json
from pathlib import Path
lock = json.loads(Path("package-lock.json").read_text())
root = lock.get("packages", {}).get("", {})
print("lockfileVersion:", lock.get("lockfileVersion"))
print("root has overrides:", "overrides" in root)
print("manifest overrides:", json.loads(Path("package.json").read_text()).get("overrides"))
for name in ("brace-expansion", "picomatch", "ip-address"):
entries = []
prefix = "node_modules/" + name
for path, meta in lock.get("packages", {}).items():
if path == prefix or path.endswith("/" + prefix):
entries.append((path, meta.get("version"), meta.get("dev"), meta.get("resolved")))
print(name, entries)
print("top-level dependency entries:")
for name in ("brace-expansion", "picomatch", "ip-address"):
print(name, lock.get("dependencies", {}).get(name))
PY
printf '%s\n' '--- relevant lockfile entries and override metadata ---'
rg -n -C 3 '"(brace-expansion|picomatch|ip-address|overrides)"' package-lock.json | head -240
printf '%s\n' '--- repository npm and lockfile metadata ---'
rg -n --hidden -g '!node_modules' -g '!dist' -g '!build' \
'npm@|packageManager|lockfileVersion|npm install|npm ci|overrides' \
.github Dockerfile package.json package-lock.json README.md 2>/dev/null || trueRepository: CBIIT/crdc-ctdc-authn
Length of output: 6403
🌐 Web query:
npm ci package.json overrides package-lock.json synchronization behavior official npm documentation
💡 Result:
The npm ci command is designed to perform a strict, reproducible installation of dependencies based exclusively on the existing package-lock.json or npm-shrinkwrap.json file [1][2]. Because it treats the lockfile as the source of truth, it does not dynamically recalculate the dependency tree, nor does it apply changes from the overrides field in package.json at runtime [3]. Key behavior regarding overrides and npm ci: 1. Overrides are not dynamic: The overrides field in package.json is used by npm install to resolve the dependency tree and generate the package-lock.json [4]. Once the lockfile is generated, the overrides are effectively baked into the resolved versions recorded in that lockfile. 2. npm ci ignores local package.json overrides: If you modify the overrides field in your package.json but do not regenerate your package-lock.json (using npm install), npm ci will detect a discrepancy between the intended state (in package.json) and the locked state (in the lockfile) [1][2]. 3. Synchronization requirement: npm ci strictly enforces synchronization between package.json and the lockfile [1][5]. If your overrides result in a dependency tree that does not match the information present in your current package-lock.json, npm ci will exit with an error, requiring you to run npm install to update the lockfile before continuing [1][6]. To ensure overrides are applied correctly in your CI/CD environment, you must: - Run npm install locally after changing any overrides to regenerate and update the package-lock.json file [3]. - Commit the updated package-lock.json to your version control system [3]. - Ensure your CI environment uses a version of npm that supports overrides (npm v8.3.0 or later) [3]. If your lockfile is not updated to reflect the overrides, npm ci will either fail due to a sync error or proceed using the stale, pre-override versions stored in the lockfile [3].
Citations:
- 1: https://docs.npmjs.com/cli/v11/commands/npm-ci/
- 2: https://docs.npmjs.com/cli/v9/commands/npm-ci/?v=true
- 3: https://latchkey.dev/learn/node-js/npm-overrides-not-applied
- 4: https://docs.npmjs.com/cli/v9/configuring-npm/package-json/
- 5: https://docs.npmjs.com/cli/v7/commands/npm-ci/?v=true
- 6: [BUG] npm ci still does not work with overrides correctly npm/cli#4942
Keep the lockfile in the production build path.
Remove package-lock.json from .dockerignore. Regenerate and commit the lockfile with the overrides entries, copy it in the Dockerfile, and use npm ci --omit=dev --ignore-scripts. This makes production dependency resolution reproducible and applies the committed security overrides.
📍 Affects 2 files
Dockerfile#L9-L11(this comment).dockerignore#L4-L4
🤖 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 `@Dockerfile` around lines 9 - 11, Update Dockerfile to copy package-lock.json
alongside package.json and use npm ci --omit=dev --ignore-scripts instead of npm
install. Remove package-lock.json from .dockerignore, and regenerate and commit
the lockfile with the required overrides entries so production dependency
resolution is reproducible.
Source: MCP tools
| # Copy application files | ||
| COPY --chown=node:node . . |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- Dockerfile ---'
cat -n Dockerfile
printf '%s\n' '--- repository files relevant to runtime, permissions, and startup ---'
git ls-files | rg '(^|/)(Dockerfile|docker-compose[^/]*|package\.json|pnpm-lock\.yaml|yarn\.lock|package-lock\.json|.*entrypoint.*|.*start.*|.*log.*)$' || true
printf '%s\n' '--- Dockerfile directives ---'
rg -n '^(FROM|RUN|COPY|ADD|WORKDIR|USER|EXPOSE|ENTRYPOINT|CMD)\b|log|8082|node' DockerfileRepository: CBIIT/crdc-ctdc-authn
Length of output: 1183
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- package scripts and runtime dependencies ---'
cat package.json
printf '%s\n' '--- startup and filesystem-write behavior ---'
for f in bin/www app.js server.js index.js; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f"
fi
done
rg -n -i 'writeFile|appendFile|createWriteStream|mkdir|mkdirSync|fs\.|/var/log|log/|logs/|bento-event-logging|process\.cwd|tmp|8082' \
--glob '!package-lock.json' --glob '!node_modules/**' . || true
printf '%s\n' '--- tracked directories and files near runtime ---'
git ls-files | sed -n '1,160p'Repository: CBIIT/crdc-ctdc-authn
Length of output: 8512
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- ignore rules affecting logs and the build context ---'
cat -n .dockerignore
printf '\n--- git ignore rules affecting logs ---\n'
cat -n .gitignore
printf '\n--- filesystem writes and relative paths in application sources ---\n'
rg -n -C 2 'fs\.|writeFile|appendFile|createWriteStream|mkdir|logs|process\.cwd|__dirname' \
app.js bin services routes controllers neo4j idps config.js 2>/dev/null || true
printf '\n--- Dockerfile ownership-sensitive directives ---\n'
awk '
/^(FROM|WORKDIR|RUN|COPY|ADD|USER|CMD|ENTRYPOINT)/ { print NR ":" $0 }
' DockerfileRepository: CBIIT/crdc-ctdc-authn
Length of output: 2631
🌐 Web query:
Dockerfile reference COPY --chown USER default root WORKDIR ownership official Docker documentation
💡 Result:
In Dockerfiles, the USER instruction does not influence the ownership of files created or copied by subsequent COPY, ADD, or WORKDIR instructions [1][2][3]. By default, these operations create files and directories owned by the root user (UID 0, GID 0), regardless of the current USER setting [4][5][3]. For the COPY instruction, you must explicitly use the --chown flag to set non-root ownership during the copy process [4][2]. The flag supports either username and groupname strings (which are looked up in the container's /etc/passwd and /etc/group files) or direct integer UID and GID values [4][6]. Regarding WORKDIR, while it automatically creates a directory if it does not exist, that directory is typically created as root [7][1][8]. Depending on your builder version (e.g., BuildKit), there have been variations in behavior, but the established best practice is to explicitly create the directory with the desired permissions and ownership using a RUN instruction before switching to a non-root USER [7][1]. Commonly recommended patterns include: 1. Creating the non-root user and setting up directory ownership using RUN instructions while still as root. 2. Using --chown during COPY if you need specific ownership for files. 3. Keeping the USER instruction near the end of your Dockerfile to ensure that administrative tasks like installing dependencies or setting up file permissions are performed with root privileges [9]. top_results: [4][7][1][2][3][9]
Citations:
- 1: WORKDIR doesn't respect USER when creating directories moby/moby#20295
- 2: Dockerfile: ADD does not honor USER: files always owned by root moby/moby#6119
- 3: https://tally.wharflab.com/rules/tally/copy-after-user-without-chown
- 4: https://github.com/docker/docker-ce/blob/master/components/cli/docs/reference/builder.md
- 5: https://manpages.debian.org/bullseye/docker.io/Dockerfile.5.en.html
- 6: https://web.archive.org/web/20200218205347/https:/docs.docker.com/engine/reference/builder/
- 7: WORKDIR creates directories as root instead of USER moby/moby#36677
- 8: https://stackoverflow.com/questions/56980608/why-is-a-folder-created-by-workdir-owned-by-root-instead-of-user
- 9: https://stackoverflow.com/questions/65475377/exact-sequence-of-user-instruction-in-dockerfile-for-non-root-user
Run the application as node, not root.
Add USER node before CMD. Before switching users, create /usr/src/app/logs and make it writable by node; logs is excluded from the build context and the application creates logs/access.log at startup.
🤖 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 `@Dockerfile` around lines 13 - 14, Update the Dockerfile before CMD to create
/usr/src/app/logs, assign ownership or write permissions to node, and switch
execution to USER node so the application runs non-root while retaining access
to logs/access.log.
Source: MCP tools
| "start": "node ./bin/www" | ||
| "start": "node ./bin/www", | ||
| "test": "NODE_ENV=test jest", | ||
| "test:ci": "TZ=UTC CI=true jest --passWithNoTests --coverage --maxWorkers=2", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package.json ---'
cat -n package.json | sed -n '1,80p'
printf '%s\n' '--- Jest configuration files ---'
find . -maxdepth 3 -type f \( -iname '*jest*' -o -name 'package.json' \) -print | sort
printf '%s\n' '--- test-related files ---'
git ls-files | grep -Ei '(^|/)(__tests__|tests?)(/|$)|\.(test|spec)\.[^/]+$' | sed -n '1,200p'
printf '%s\n' '--- test script references ---'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'test:ci|passWithNoTests|jest' .Repository: CBIIT/crdc-ctdc-authn
Length of output: 39969
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- CI workflow ---'
cat -n .github/workflows/test.yml | sed -n '1,100p'
printf '%s\n' '--- deterministic package-script check ---'
python3 - <<'PY'
import json
from pathlib import Path
package = json.loads(Path("package.json").read_text())
script = package["scripts"]["test:ci"]
print("test:ci =", script)
print("contains --passWithNoTests:", "--passWithNoTests" in script)
print("contains --coverage:", "--coverage" in script)
print("test files tracked:", len([
p for p in __import__("subprocess").check_output(
["git", "ls-files"], text=True
).splitlines()
if __import__("re").search(r"(^|/)(__tests__|tests?)(/|$)|\\.(test|spec)\\.[^/]+$", p, __import__("re").I)
]))
PYRepository: CBIIT/crdc-ctdc-authn
Length of output: 1727
🌐 Web query:
Jest 30 CLI --passWithNoTests official documentation behavior when no tests are found
💡 Result:
The --passWithNoTests CLI flag in Jest is designed to allow the test suite to pass (exit with code 0) when no test files are found [1][2][3]. It is important to distinguish between "no tests found" and "empty test files": 1. No tests found: If Jest is invoked and no test files match the search patterns (e.g., testPathIgnorePatterns or testMatch), the --passWithNoTests flag successfully changes the exit status from 1 to 0 [3]. Without this flag, Jest exits with code 1 in this scenario [3]. 2. Empty test files: The --passWithNoTests flag does not prevent errors caused by empty test files (files that are found by Jest but contain no executable test cases) [4][5]. If a file exists that Jest considers a test file, it must contain at least one valid test; otherwise, Jest will report a "Test suite failed to run" error, regardless of whether --passWithNoTests is set [4][5]. Additionally, note that --passWithNoTests must be defined in the root configuration if used in a project with multiple configurations, as it is ignored when set within a project-specific config [6]. Users have historically reported that while the exit code correctly behaves as expected (0), Jest may sometimes still print a message suggesting it is exiting with code 1, which has been identified as a UI/messaging bug [3][7].
Citations:
- 1: https://jestjs.io/docs/30.0/cli
- 2: https://jestjs.io/docs/cli
- 3: Misleading message for passWithNoTests. jestjs/jest#8187
- 4: [Bug]: passWithNoTests flag ignored for empty test files jestjs/jest#15107
- 5:
--passWithNoTestsdoes not work in watch mode jestjs/jest#9020 - 6: https://jestjs.io/docs/configuration
- 7: Jest prints superfluous warning when ran with '--passWithNoTests' and there are no tests jestjs/jest#8594
Remove --passWithNoTests from test:ci.
The flag allows Jest to succeed when it finds no test files. A test-discovery failure could therefore produce a green CI job without running tests.
🤖 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 `@package.json` at line 11, Update the package.json test:ci script to remove
the --passWithNoTests flag while preserving the existing timezone, CI, coverage,
Jest, and worker settings.
Source: MCP tools
| multipleStatements: true, | ||
| connectTimeout: 60000, // 60 seconds | ||
| acquireTimeout: 60000, // 60 seconds | ||
| timeout: 60000, // 60 seconds | ||
| waitForConnections: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)(services/mySQL/mySQL-operations\.js|services/mysql-connection\.js|package\.json|package-lock\.json|yarn\.lock|pnpm-lock\.yaml)$' || true
printf '%s\n' '--- pool configuration ---'
for f in services/mySQL/mySQL-operations.js services/mysql-connection.js; do
if [ -f "$f" ]; then
echo "### $f"
cat -n "$f" | sed -n '1,90p'
fi
done
printf '%s\n' '--- mysql package declarations and pool calls ---'
rg -n -i --glob '!node_modules' --glob '!dist' \
'mysql2?|createPool|queueLimit|waitForConnections|acquireTimeout|connectTimeout|timeout' \
package.json package-lock.json yarn.lock pnpm-lock.yaml services 2>/dev/null || trueRepository: CBIIT/crdc-ctdc-authn
Length of output: 11092
🏁 Script executed:
#!/bin/bash
set -eu
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl -fsSL https://raw.githubusercontent.com/mysqljs/mysql/v2.18.1/lib/PoolConfig.js \
-o "$tmpdir/PoolConfig.js"
curl -fsSL https://raw.githubusercontent.com/mysqljs/mysql/v2.18.1/lib/Pool.js \
-o "$tmpdir/Pool.js"
curl -fsSL https://raw.githubusercontent.com/mysqljs/mysql/v2.18.1/Readme.md \
-o "$tmpdir/Readme.md"
printf '%s\n' '--- mysql 2.18.1 pool defaults and queue behavior ---'
cat -n "$tmpdir/PoolConfig.js" | sed -n '1,180p'
rg -n -C 5 'queueLimit|waitForConnections|acquireTimeout|_enqueueCallback|_connectionQueue' \
"$tmpdir/Pool.js" "$tmpdir/Readme.md"Repository: CBIIT/crdc-ctdc-authn
Length of output: 9978
Bound both MySQL pool queues.
Both mysql 2.18.1 pools use an unlimited queue because waitForConnections is enabled and queueLimit defaults to 0. The acquisition timeout does not include queue time. Set a finite queueLimit, or disable waiting to return controlled overload errors.
📍 Affects 2 files
services/mySQL/mySQL-operations.js#L11-L15(this comment)services/mysql-connection.js#L9-L13
🤖 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 `@services/mySQL/mySQL-operations.js` around lines 11 - 15, Bound the
connection queues in both pool configurations:
services/mySQL/mySQL-operations.js lines 11-15 and services/mysql-connection.js
lines 9-13. Update the pool options near waitForConnections to set a finite
queueLimit, or disable waiting, while preserving controlled overload behavior.
Source: MCP tools
| connectTimeout: 60000, // 60 seconds | ||
| acquireTimeout: 60000, // 60 seconds | ||
| timeout: 60000, // 60 seconds | ||
| waitForConnections: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="services/mySQL/mySQL-operations.js"
printf '%s\n' '--- outline ---'
ast-grep outline "$file" --lang javascript || true
printf '%s\n' '--- relevant source ---'
cat -n "$file" | sed -n '1,100p'
printf '%s\n' '--- currentConnection references ---'
rg -n -C 3 '\bcurrentConnection\b|connectTimeout|acquireTimeout|waitForConnections|timeout' "$file"Repository: CBIIT/crdc-ctdc-authn
Length of output: 12971
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
async function original(mode) {
const events = [];
const pool = {
getConnection(callback) {
if (mode === 'acquire-error') callback(new Error('acquire timeout'));
else callback(null, {
query(_sql, cb) {
if (mode === 'query-error') cb(new Error('query timeout'));
else cb(null, [{ data: JSON.stringify({ userInfo: { tokens: ['ok'] } }) }]);
},
release() { events.push('release'); }
});
}
};
let currentConnection = null;
try {
const currentConnection = await new Promise((resolve, reject) => {
pool.getConnection((err, conn) => {
if (err) reject(err);
else resolve(conn);
});
});
await new Promise((resolve, reject) => {
currentConnection.query('query', (err, rows) => {
if (err) reject(err);
else resolve(rows);
});
});
currentConnection.release();
return { result: 'ok', events };
} catch (error) {
try {
currentConnection.release();
return { result: 'handled', events };
} catch (cleanupError) {
return { result: cleanupError.name + ': ' + cleanupError.message, events };
}
} finally {
if (currentConnection) currentConnection.release();
}
}
async function corrected(mode) {
const events = [];
const pool = {
getConnection(callback) {
if (mode === 'acquire-error') callback(new Error('acquire timeout'));
else callback(null, {
query(_sql, cb) {
if (mode === 'query-error') cb(new Error('query timeout'));
else cb(null, [{ data: 'ok' }]);
},
release() { events.push('release'); }
});
}
};
let currentConnection = null;
try {
currentConnection = await new Promise((resolve, reject) => {
pool.getConnection((err, conn) => {
if (err) reject(err);
else resolve(conn);
});
});
await new Promise((resolve, reject) => {
currentConnection.query('query', (err, rows) => {
if (err) reject(err);
else resolve(rows);
});
});
return { result: 'ok', events };
} catch (error) {
return { result: error.message, events };
} finally {
if (currentConnection) currentConnection.release();
}
}
(async () => {
for (const mode of ['acquire-error', 'query-error', 'success']) {
console.log(mode, 'original=', await original(mode), 'corrected=', await corrected(mode));
}
})();
JSRepository: CBIIT/crdc-ctdc-authn
Length of output: 613
Fix connection cleanup in all five MySQL helpers.
When acquisition or query fails, the inner const currentConnection leaves the outer variable null. The catch block then masks the original error with a null .release() failure. Assign the connection to the outer variable, remove direct releases, and release it once in finally.
🤖 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 `@services/mySQL/mySQL-operations.js` around lines 12 - 15, Update all five
MySQL helper functions to assign the acquired connection to the outer connection
variable rather than declaring an inner currentConnection. Remove direct release
calls from success and catch paths, and release the outer connection once in
finally when it exists, preserving the original acquisition or query error.
Source: MCP tools
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 13
🧹 Nitpick comments (2)
Dockerfile (1)
1-1: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the production base image by digest.
node:24-alpine3.23is a mutable tag. A later build can pull different image layers without a source change. Use an approved digest and update it through a controlled dependency change. Docker documents that tags can change while digests are immutable. (docs.docker.com)🤖 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 `@Dockerfile` at line 1, Update the fnl_base_image FROM declaration to pin node:24-alpine3.23 to the approved immutable image digest, preserving the current Node and Alpine versions. Manage future digest updates through the controlled dependency-update process.Source: MCP tools
package.json (1)
20-33: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueIf New Relic is retired, remove its stale configuration. No runtime references to
newrelic,sequelize, orsession-file-storeremain. Onlynewrelic.js,NEW_RELIC_*entries in.env-template, and the.gitignorelog entry remain.🤖 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 `@package.json` around lines 20 - 33, Remove the stale New Relic configuration artifacts: delete newrelic.js, remove all NEW_RELIC_* entries from .env-template, and remove the New Relic log entry from .gitignore. Do not alter the dependency declarations shown in package.json.
🤖 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 @.github/workflows/build.yml:
- Around line 85-102: Reorder the workflow so the “Push to Amazon ECR” step
completes before “Create Git tag for Image” runs. Keep the existing tag commands
unchanged, ensuring Git tag creation occurs only after authentication and docker
push succeed.
- Around line 2-4: Update .github/workflows/build.yml lines 2-4 so build and
Trivy scanning use read-only contents permission and no id-token access. At
lines 22-26, scope Slack and AWS values only to the release steps that consume
them. At lines 30-33, disable checkout credential persistence and configure Git
push authentication only after scanning succeeds.
- Around line 8-10: Update the workflow_dispatch input named environment to be
required, ensuring every manual deployment supplies an environment before
authentication uses its environment-scoped AWS values.
- Around line 35-52: Update the Set Image Tag step to sort matching tags by
version rather than lexically before selecting the latest tag, so numeric
suffixes such as .10 correctly follow .9. Add job-level concurrency keyed by
repository, environment, and ref to serialize runs and prevent duplicate
image-tag allocation.
In @.github/workflows/test.yml:
- Around line 13-16: Add a MySQL service to the test job alongside runs-on,
configure it to expose port 3306 and include a health check, then ensure the
test steps run only after the service is healthy before invoking npm run
test:ci. Preserve the existing MySQL connection environment configuration.
- Around line 18-21: Update the actions/checkout@v4 step in the workflow to set
persist-credentials to false alongside the existing recursive submodules option,
preventing the checkout token from being stored in local Git configuration.
In `@app.js`:
- Around line 19-20: In the log-directory creation error handler, replace the
invalid logger.warn call with console.warn while preserving the existing warning
message and err.message details, so startup can fall back to stdout without
invoking the Morgan middleware factory.
- Around line 15-20: Update the accessLogStream setup around
fs.createWriteStream to attach an error handler for asynchronous open and write
failures. On error, switch Morgan’s active logging destination to process.stdout
through the existing accessLogStream reference, preventing unhandled stream
errors while preserving the synchronous fallback behavior.
In `@Dockerfile`:
- Around line 9-11: Update Dockerfile to copy package-lock.json alongside
package.json and use npm ci --omit=dev --ignore-scripts instead of npm install.
Remove package-lock.json from .dockerignore, and regenerate and commit the
lockfile with the required overrides entries so production dependency resolution
is reproducible.
- Around line 13-14: Update the Dockerfile before CMD to create
/usr/src/app/logs, assign ownership or write permissions to node, and switch
execution to USER node so the application runs non-root while retaining access
to logs/access.log.
In `@package.json`:
- Line 11: Update the package.json test:ci script to remove the
--passWithNoTests flag while preserving the existing timezone, CI, coverage,
Jest, and worker settings.
In `@services/mySQL/mySQL-operations.js`:
- Around line 11-15: Bound the connection queues in both pool configurations:
services/mySQL/mySQL-operations.js lines 11-15 and services/mysql-connection.js
lines 9-13. Update the pool options near waitForConnections to set a finite
queueLimit, or disable waiting, while preserving controlled overload behavior.
- Around line 12-15: Update all five MySQL helper functions to assign the
acquired connection to the outer connection variable rather than declaring an
inner currentConnection. Remove direct release calls from success and catch
paths, and release the outer connection once in finally when it exists,
preserving the original acquisition or query error.
---
Nitpick comments:
In `@Dockerfile`:
- Line 1: Update the fnl_base_image FROM declaration to pin node:24-alpine3.23
to the approved immutable image digest, preserving the current Node and Alpine
versions. Manage future digest updates through the controlled dependency-update
process.
In `@package.json`:
- Around line 20-33: Remove the stale New Relic configuration artifacts: delete
newrelic.js, remove all NEW_RELIC_* entries from .env-template, and remove the
New Relic log entry from .gitignore. Do not alter the dependency declarations
shown in package.json.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be12241d-f633-4a35-8b34-8675a62f5187
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.dockerignore.github/workflows/build.yml.github/workflows/test.yml.gitignore.nvmrcDockerfileapp.jspackage.jsonservices/mySQL/mySQL-operations.jsservices/mysql-connection.jstest/auth.test.jstest/health.test.jstest/services/token-service.test.js
🛑 Comments failed to post (4)
.github/workflows/build.yml (4)
2-4: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Separate untrusted build steps from release credentials.
The scanner runs with write-scoped GitHub access, AWS OIDC access, globally exported AWS values, and a persisted checkout credential. A compromised third-party action can push repository tags or assume the configured AWS role.
.github/workflows/build.yml#L2-L4: Run build and Trivy scanning in a job with read-only contents access and noid-tokenpermission..github/workflows/build.yml#L22-L26: Scope Slack and AWS values to only their consuming release steps..github/workflows/build.yml#L30-L33: Setpersist-credentials: false; provide Git push authentication only after the scan passes.📍 Affects 1 file
.github/workflows/build.yml#L2-L4(this comment).github/workflows/build.yml#L22-L26.github/workflows/build.yml#L30-L33🤖 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 @.github/workflows/build.yml around lines 2 - 4, Update .github/workflows/build.yml lines 2-4 so build and Trivy scanning use read-only contents permission and no id-token access. At lines 22-26, scope Slack and AWS values only to the release steps that consume them. At lines 30-33, disable checkout credential persistence and configure Git push authentication only after scanning succeeds.Source: Linters/SAST tools
8-10: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require the deployment environment.
The input is optional. A dispatch without
environmentleaves the environment-scoped AWS values empty and fails later during authentication.Proposed fix
environment: description: 'Which account the ECR repository is in' type: environment + required: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.environment: description: 'Which account the ECR repository is in' type: environment required: true🤖 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 @.github/workflows/build.yml around lines 8 - 10, Update the workflow_dispatch input named environment to be required, ensuring every manual deployment supplies an environment before authentication uses its environment-scoped AWS values.
35-52: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allocate image tags deterministically and exclusively.
Line 41 sorts tags lexically. After
.9and.10exist,tail -1selects.9and line 47 reuses.10. Concurrent manual runs can also allocate the same tag.Use version-aware sorting. Add job-level
concurrencyfor the repository, environment, and ref.Proposed fix
- tag=$(git tag -l $BRANCH_NAME* | tail -1) + tag=$(git tag --list "${BRANCH_NAME}.*" --sort=-version:refname | head -n1)build: + concurrency: + group: build-image-${{ github.repository }}-${{ inputs.environment }}-${{ github.ref }} + cancel-in-progress: false🤖 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 @.github/workflows/build.yml around lines 35 - 52, Update the Set Image Tag step to sort matching tags by version rather than lexically before selecting the latest tag, so numeric suffixes such as .10 correctly follow .9. Add job-level concurrency keyed by repository, environment, and ref to serialize runs and prevent duplicate image-tag allocation.
85-102: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Push the ECR image before creating the Git tag.
Lines 85-90 create release metadata before the image exists in ECR. If ECR authentication or
docker pushfails, the Git tag remains and a retry increments to a new tag. Move the Git tag creation step after line 102 succeeds.🧰 Tools
🪛 zizmor (1.29.0)
[warning] 89-89: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 90-90: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/build.yml around lines 85 - 102, Reorder the workflow so the “Push to Amazon ECR” step completes before “Create Git tag for Image” runs. Keep the existing tag commands unchanged, ensuring Git tag creation occurs only after authentication and docker push succeed.
4 participants
Merge the 1.1.1.3 release branch into master for the production release.
Official Release: https://github.com/CBIIT/crdc-ctdc-starter-kit/releases/tag/1.4.0.19
Auth Release: https://github.com/CBIIT/crdc-ctdc-authn/releases/tag/1.1.1.3
Summary by CodeRabbit