fix(agent): warn on empty LODESTAR_HMAC_SECRET; refuse to start in production - #517
Conversation
|
@MarcusDavidG Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThe agent now validates ChangesAgent HMAC secret validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 4
🧹 Nitpick comments (3)
agent/Dockerfile (1)
2-13: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin all container image references.
Floating tags make rebuilds non-reproducible and allow image contents to change without a reviewed source change.
- agent/Dockerfile#L2-L13: pin both
node:22-alpinestage images.- agent/docker-compose.yml#L52-L52: replace
mcuadros/ofelia:latestwith an immutable version or digest.🤖 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 `@agent/Dockerfile` around lines 2 - 13, Pin both node:22-alpine references in agent/Dockerfile lines 2-13 to an immutable version or digest, preserving the existing deps and runtime stages. Also replace mcuadros/ofelia:latest with an immutable version or digest in agent/docker-compose.yml line 52.agent/docker-compose.yml (1)
25-27: 🩺 Stability & Availability | 🔵 TrivialThe idle command bypasses container-level startup validation.
docker compose up -dstarts the sleep loop, notnode agent.js; a missing production secret is discovered only when Ofelia later executes the job. Add startup/health validation or use one-shot jobs so failed deployments are visible immediately.🤖 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 `@agent/docker-compose.yml` around lines 25 - 27, The compose service command currently runs only an idle sleep loop, so production startup validation is skipped. Update the service configuration around the command override to validate required startup configuration, including the production secret, when the container starts; alternatively configure the job as a one-shot execution so failures surface during docker compose up rather than only when Ofelia invokes node agent.js.agent/agent.test.js (1)
45-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the import-time startup failure.
This invokes the exported function after
agent.jshas already been imported, so removing the top-level validation call would still pass. Set production environment variables before an isolated import and assert that the import fails.Also applies to: 573-580
🤖 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 `@agent/agent.test.js` at line 45, Update the tests around the agent.js import and the related startup-failure case to set the required production environment variables, dynamically import agent.js in an isolated module context, and assert that the import rejects when validation fails. Ensure the test does not invoke an already-imported validateHmacSecret or rely on cached module state, so removing the top-level validation call causes the test to fail.
🤖 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 `@agent/agent.js`:
- Around line 51-52: Update validateHmacSecret in agent/agent.js to reject the
documented example sentinel, or apply an equivalent minimum validity check while
preserving fail-closed behavior. Change agent/.env.example at line 9 to use an
empty placeholder so deployments cannot unintentionally use the publicly known
example secret.
In `@agent/agent.test.js`:
- Around line 565-570: Rename the test case describing validateHmacSecret() so
its title states that development with an empty LODESTAR_HMAC_SECRET emits a
warning. Keep the existing assertions and test behavior unchanged.
In `@agent/docker-compose.yml`:
- Around line 57-60: Remove the direct Docker socket bind from the scheduler
service’s volumes configuration. Replace it with a safer scheduling approach,
such as a host-level scheduler, isolated runner, or least-privileged Docker
socket proxy, and update the service configuration to use that mechanism
instead.
- Around line 30-35: Configure production mode consistently: in
agent/docker-compose.yml at lines 30-35 and 71-75, add NODE_ENV=production to
the scheduled and one-shot services; in agent/Dockerfile at lines 39-44, set the
production environment for direct image runs and document HMAC_SECRET as
required in production.
---
Nitpick comments:
In `@agent/agent.test.js`:
- Line 45: Update the tests around the agent.js import and the related
startup-failure case to set the required production environment variables,
dynamically import agent.js in an isolated module context, and assert that the
import rejects when validation fails. Ensure the test does not invoke an
already-imported validateHmacSecret or rely on cached module state, so removing
the top-level validation call causes the test to fail.
In `@agent/docker-compose.yml`:
- Around line 25-27: The compose service command currently runs only an idle
sleep loop, so production startup validation is skipped. Update the service
configuration around the command override to validate required startup
configuration, including the production secret, when the container starts;
alternatively configure the job as a one-shot execution so failures surface
during docker compose up rather than only when Ofelia invokes node agent.js.
In `@agent/Dockerfile`:
- Around line 2-13: Pin both node:22-alpine references in agent/Dockerfile lines
2-13 to an immutable version or digest, preserving the existing deps and runtime
stages. Also replace mcuadros/ofelia:latest with an immutable version or digest
in agent/docker-compose.yml line 52.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dea7554-c47a-4f54-9cf0-d3521a79f052
📒 Files selected for processing (10)
agent/.dockerignoreagent/.env.exampleagent/Dockerfileagent/agent.jsagent/agent.test.jsagent/docker-compose.ymlfrontend/__tests__/RegistryPage.test.tsxfrontend/__tests__/useDebounce.test.tsfrontend/app/registry/page.tsxfrontend/hooks/useDebounce.ts
| export function validateHmacSecret() { | ||
| if (LODESTAR_HMAC_SECRET !== '') return; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
A known placeholder bypasses HMAC validation.
The validator accepts the non-empty example value, so copying the example can enable request signing with a publicly known key.
- agent/agent.js#L51-L52: reject the documented sentinel, or enforce an equivalent minimum validity rule.
- agent/.env.example#L9-L9: use an empty placeholder so production fails closed when the value is not replaced.
📍 Affects 2 files
agent/agent.js#L51-L52(this comment)agent/.env.example#L9-L9
🤖 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 `@agent/agent.js` around lines 51 - 52, Update validateHmacSecret in
agent/agent.js to reject the documented example sentinel, or apply an equivalent
minimum validity check while preserving fail-closed behavior. Change
agent/.env.example at line 9 to use an empty placeholder so deployments cannot
unintentionally use the publicly known example secret.
| it('does not emit a warning in development when LODESTAR_HMAC_SECRET is empty', () => { | ||
| process.env.NODE_ENV = 'development'; | ||
| // Should warn (not throw) — same as the general non-production case. | ||
| expect(() => validateHmacSecret()).not.toThrow(); | ||
| expect(logWarn).toHaveBeenCalledTimes(1); | ||
| expect(logWarn.mock.calls[0][0]).toMatchObject({ event: 'hmac_secret_missing' }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the development test description.
The test says it “does not emit” a warning but asserts that one warning is emitted. Rename the test to match the behavior being verified.
🤖 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 `@agent/agent.test.js` around lines 565 - 570, Rename the test case describing
validateHmacSecret() so its title states that development with an empty
LODESTAR_HMAC_SECRET emits a warning. Keep the existing assertions and test
behavior unchanged.
| environment: | ||
| # Defaults that can be overridden in .env | ||
| STELLAR_NETWORK: ${STELLAR_NETWORK:-testnet} | ||
| STELLAR_RPC_URL: ${STELLAR_RPC_URL:-https://soroban-testnet.stellar.org} | ||
| STELLAR_NETWORK_PASSPHRASE: ${STELLAR_NETWORK_PASSPHRASE:-Test SDF Network ; September 2015} | ||
| LOG_LEVEL: ${LOG_LEVEL:-info} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Production HMAC enforcement is not configured consistently.
agent.js requires the exact production environment, but Compose does not set it and the Dockerfile documents the HMAC secret as optional.
- agent/docker-compose.yml#L30-L35: set
NODE_ENV: productionfor the scheduled service. - agent/Dockerfile#L39-L44: document the secret as required in production and configure production mode for direct image runs.
- agent/docker-compose.yml#L71-L75: set
NODE_ENV: productionfor one-shot runs.
📍 Affects 2 files
agent/docker-compose.yml#L30-L35(this comment)agent/Dockerfile#L39-L44agent/docker-compose.yml#L71-L75
🤖 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 `@agent/docker-compose.yml` around lines 30 - 35, Configure production mode
consistently: in agent/docker-compose.yml at lines 30-35 and 71-75, add
NODE_ENV=production to the scheduled and one-shot services; in agent/Dockerfile
at lines 39-44, set the production environment for direct image runs and
document HMAC_SECRET as required in production.
| volumes: | ||
| # Ofelia needs access to the Docker socket to exec into containers. | ||
| - /var/run/docker.sock:/var/run/docker.sock:ro | ||
| restart: unless-stopped |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid exposing the Docker daemon socket directly.
A read-only bind does not make Docker API operations read-only. A compromised scheduler could control containers and potentially the host. Prefer a host-level scheduler, isolated runner, or least-privileged Docker socket proxy.
🤖 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 `@agent/docker-compose.yml` around lines 57 - 60, Remove the direct Docker
socket bind from the scheduler service’s volumes configuration. Replace it with
a safer scheduling approach, such as a host-level scheduler, isolated runner, or
least-privileged Docker socket proxy, and update the service configuration to
use that mechanism instead.
|
Hi @MarcusDavidG, This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
…t in production Closes Stellar-Ecosystem#368 An empty HMAC secret silently disabled request signing with no operator feedback. This change surfaces the gap at startup: - agent/agent.js: add exported validateHmacSecret() called immediately after the logger is constructed. • NODE_ENV=production + empty secret → throws Error (agent refuses to start), no warn log emitted. • Any other NODE_ENV + empty secret → logger.warn with event 'hmac_secret_missing' and an actionable message; agent continues. • Non-empty secret → returns immediately, no side effects. - agent/.env.example: expand the LODESTAR_HMAC_SECRET comment to state it is required in production and clarify the dev/test behaviour. - agent/agent.test.js: destructure validateHmacSecret from the top-level import; add 4 tests covering: 1. warn emitted (event + message) when secret is empty and not production 2. warn emitted (not thrown) when NODE_ENV=development 3. throws with correct message when NODE_ENV=production 4. production error message contains actionable instructions All 32 tests pass.
dd1d90b to
7077646
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent/agent.js (1)
502-523: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate
AGENT_SHUTDOWN_TIMEOUT_MSbefore scheduling the shutdown timer.Invalid values make
parseInt()returnNaN, andsetTimeout(NaN)schedules the callback after 1 millisecond. Use a finite, positive value or fall back to30000.Proposed fix
-let shutdownTimer = null; +let shutdownTimer = null; +const configuredShutdownTimeoutMs = Number(process.env.AGENT_SHUTDOWN_TIMEOUT_MS); +const SHUTDOWN_TIMEOUT_MS = + Number.isFinite(configuredShutdownTimeoutMs) && configuredShutdownTimeoutMs > 0 + ? configuredShutdownTimeoutMs + : 30000;🤖 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 `@agent/agent.js` around lines 502 - 523, Validate the value assigned to SHUTDOWN_TIMEOUT_MS before initiateShutdown schedules its timer: use a finite, positive parsed value from AGENT_SHUTDOWN_TIMEOUT_MS, otherwise fall back to 30000 milliseconds. Keep the existing shutdownTimer timeout behavior and logging unchanged.
🤖 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.
Outside diff comments:
In `@agent/agent.js`:
- Around line 502-523: Validate the value assigned to SHUTDOWN_TIMEOUT_MS before
initiateShutdown schedules its timer: use a finite, positive parsed value from
AGENT_SHUTDOWN_TIMEOUT_MS, otherwise fall back to 30000 milliseconds. Keep the
existing shutdownTimer timeout behavior and logging unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ea47df6-ef0a-4fdd-87d2-e936f05f478f
📒 Files selected for processing (3)
agent/.env.exampleagent/agent.jsagent/agent.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- agent/agent.test.js
2 participants
Closes #368
Problem
LODESTAR_HMAC_SECRETdefaulted to''with no feedback. An operator who forgot to set it would see the agent run successfully while all payment-record requests were silently unauthenticated — a security downgrade that can persist indefinitely.Changes
agent/agent.jsAdded
validateHmacSecret(), called immediately after the logger is constructed:NODE_ENV≠'production'logger.warnwithevent: 'hmac_secret_missing'and an actionable messageNODE_ENV === 'production'Error— agent refuses to startThe function is exported so the test suite can call it directly with controlled
NODE_ENVstate.agent/.env.exampleExpanded the
LODESTAR_HMAC_SECRETcomment to make the production requirement explicit and describe dev/test behaviour.agent/agent.test.jsAdded
validateHmacSecretto the top-level destructured import and 4 new tests:NODE_ENV=developmentNODE_ENV=productionTest results
Summary by CodeRabbit
New Features
Documentation