fix(server-elysia): update test mocks to match refactored http.createServer (#1204)#1210
fix(server-elysia): update test mocks to match refactored http.createServer (#1204)#1210kagura-agent wants to merge 3 commits intoVoltAgent:mainfrom
Conversation
…oltAgent#1206) Previously isDevRequest() treated any non-production NODE_ENV (including undefined) as a development environment, allowing auth bypass with a simple header. Deployed servers that forgot NODE_ENV=production were fully open. Now only NODE_ENV=development or NODE_ENV=test enable the dev bypass (fail-closed). Undefined/empty NODE_ENV is treated as production.
…ment helper Address CodeRabbit review: WebSocket auth functions in setup.ts also used the vulnerable NODE_ENV !== 'production' check. Extract isDevEnvironment() as a shared helper used by both HTTP and WebSocket auth paths. Also fix test names (empty vs undefined) and add isDevEnvironment unit tests.
…Server (VoltAgent#1204) Tests were mocking the old app.listen() API, but startServer() was refactored to use Node.js http.createServer() + server.listen(). Updated mocks to target node:http module instead, restoring test coverage for all 6 test cases.
🦋 Changeset detectedLatest commit: e639394 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis PR refactors authentication environment gating and updates Elysia server provider tests. It introduces a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (5)
packages/server-elysia/src/elysia-server-provider.spec.ts (2)
22-32: Module-levelmockServeris shared across tests.Since
mockServeris defined at module scope andvi.clearAllMocks()only resets call history (not implementations or properties), state likelistening: truepersists across tests. The currentbeforeEachre-applies implementations forlisten/close/once, which is sufficient for the present test set, but any future test that mutatesmockServer.listening(or adds new methods) will leak into other tests. Consider resettingmockServerproperties explicitly inbeforeEachto make the isolation boundary obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-elysia/src/elysia-server-provider.spec.ts` around lines 22 - 32, The module-level mockServer object is shared across tests causing state leakage; in the beforeEach where you reapply implementations for listen/close/once, also explicitly reset mockServer properties (e.g., set mockServer.listening = false or true as appropriate and reinitialize any added methods) so each test starts with a fresh known state; update the beforeEach to reassign mockServer.listening and any other mutable fields on the mockServer used by tests (while keeping vi.mock("node:http") and the mocked listen/close/once implementations intact).
139-160: Startup error simulation relies on registration order — worth a brief comment.The test works because the implementation calls
server.once("error", reject)beforeserver.listen(...)(seeelysia-server-provider.ts:87-91), soerrorHandleris captured beforemockServer.listensynchronously invokes it. If the implementation ever reorders these calls,errorHandlerwould beundefinedand the test would silently hang on an unresolved promise rather than fail loudly. A short inline comment noting this ordering dependency, or a defensiveexpect(errorHandler).toBeDefined()before triggering it, would make the failure mode more diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-elysia/src/elysia-server-provider.spec.ts` around lines 139 - 160, The test relies on server.once("error", ...) being called before server.listen(...) so errorHandler is set; add a defensive check (or short inline comment) in the test to make this dependency explicit and fail loudly if ordering changes: after mockServer.once.mockImplementation and before invoking mockServer.listen (or before calling provider.start()), assert that errorHandler is defined (e.g., expect(errorHandler).toBeDefined()) and/or add a brief comment referencing the ordering dependency; this targets the errorHandler captured in the it("should handle startup errors and release port") block and ensures the test won't hang silently if server.once is registered after listen.packages/server-core/src/auth/utils.spec.ts (1)
9-29: Consider adding an explicitundefinedNODE_ENV case.The suite covers
"development","test","production", and"", but the docstring onisDevEnvironment()emphasizes that undefinedNODE_ENVis the primary fail-closed scenario (deployments that forgot to set it).vi.stubEnv("NODE_ENV", undefined)(or deleting viadelete process.env.NODE_ENVinside the test) would pin down that behavior explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-core/src/auth/utils.spec.ts` around lines 9 - 29, Add an explicit test to assert that isDevEnvironment() returns false when NODE_ENV is undefined: inside the existing "isDevEnvironment" describe block add a case that clears or unsets the env (e.g., use vi.stubEnv("NODE_ENV", undefined) or delete process.env.NODE_ENV) and expect(isDevEnvironment()).toBe(false); this pins down the fail-closed behavior for undefined NODE_ENV and uses the same test helpers (vi.stubEnv) already present in the suite..changeset/fix-dev-auth-bypass-node-env.md (1)
5-12: Consider mentioningtestin the headline for completeness.The title says "require explicit NODE_ENV=development" but the body (and implementation) also accepts
NODE_ENV=test. A small wording tweak would prevent confusion for consumers scanning the changelog.📝 Proposed wording
-fix(auth): require explicit NODE_ENV=development for dev auth bypass +fix(auth): require explicit NODE_ENV=development|test for dev auth bypass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/fix-dev-auth-bypass-node-env.md around lines 5 - 12, Update the changelog headline to mention both development and test environments so it matches the implementation: change the title in .changeset/fix-dev-auth-bypass-node-env.md from "require explicit NODE_ENV=development for dev auth bypass" to something like "require explicit NODE_ENV=development or NODE_ENV=test for dev auth bypass" and ensure any summary lines referencing isDevRequest() or the dev bypass behavior also mention "test" to avoid consumer confusion.packages/server-core/src/auth/utils.ts (1)
45-58: Consider aligningshouldEnableSwaggerUIwith the new helper.
packages/server-core/src/server/app-setup.tsstill uses a directprocess.env.NODE_ENV === "production"check with fail-open semantics (any non-production value enables Swagger UI). It's not strictly a security issue since Swagger exposure is less sensitive than auth bypass, but for consistency — and to avoid the same "undefined NODE_ENV on deployed server" footgun — consider switching it to!isDevEnvironment()-style gating, or at minimum a shared constant, so env checks across the package stay uniform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-core/src/auth/utils.ts` around lines 45 - 58, The server app-setup uses a direct process.env.NODE_ENV === "production" check for shouldEnableSwaggerUI which is inconsistent with the new isDevEnvironment()/isDevRequest() helper pattern; update shouldEnableSwaggerUI in packages/server-core/src/server/app-setup.ts to use the shared isDevEnvironment() (or the inverse !isDevEnvironment()) instead of comparing NODE_ENV directly so that Swagger gating follows the same environment logic as isDevRequest(); ensure you import isDevEnvironment from auth/utils and replace the existing production-check branch with a call to isDevEnvironment() (or a shared constant) to maintain uniform behavior across the package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/fix-dev-auth-bypass-node-env.md:
- Around line 5-12: Update the changelog headline to mention both development
and test environments so it matches the implementation: change the title in
.changeset/fix-dev-auth-bypass-node-env.md from "require explicit
NODE_ENV=development for dev auth bypass" to something like "require explicit
NODE_ENV=development or NODE_ENV=test for dev auth bypass" and ensure any
summary lines referencing isDevRequest() or the dev bypass behavior also mention
"test" to avoid consumer confusion.
In `@packages/server-core/src/auth/utils.spec.ts`:
- Around line 9-29: Add an explicit test to assert that isDevEnvironment()
returns false when NODE_ENV is undefined: inside the existing "isDevEnvironment"
describe block add a case that clears or unsets the env (e.g., use
vi.stubEnv("NODE_ENV", undefined) or delete process.env.NODE_ENV) and
expect(isDevEnvironment()).toBe(false); this pins down the fail-closed behavior
for undefined NODE_ENV and uses the same test helpers (vi.stubEnv) already
present in the suite.
In `@packages/server-core/src/auth/utils.ts`:
- Around line 45-58: The server app-setup uses a direct process.env.NODE_ENV ===
"production" check for shouldEnableSwaggerUI which is inconsistent with the new
isDevEnvironment()/isDevRequest() helper pattern; update shouldEnableSwaggerUI
in packages/server-core/src/server/app-setup.ts to use the shared
isDevEnvironment() (or the inverse !isDevEnvironment()) instead of comparing
NODE_ENV directly so that Swagger gating follows the same environment logic as
isDevRequest(); ensure you import isDevEnvironment from auth/utils and replace
the existing production-check branch with a call to isDevEnvironment() (or a
shared constant) to maintain uniform behavior across the package.
In `@packages/server-elysia/src/elysia-server-provider.spec.ts`:
- Around line 22-32: The module-level mockServer object is shared across tests
causing state leakage; in the beforeEach where you reapply implementations for
listen/close/once, also explicitly reset mockServer properties (e.g., set
mockServer.listening = false or true as appropriate and reinitialize any added
methods) so each test starts with a fresh known state; update the beforeEach to
reassign mockServer.listening and any other mutable fields on the mockServer
used by tests (while keeping vi.mock("node:http") and the mocked
listen/close/once implementations intact).
- Around line 139-160: The test relies on server.once("error", ...) being called
before server.listen(...) so errorHandler is set; add a defensive check (or
short inline comment) in the test to make this dependency explicit and fail
loudly if ordering changes: after mockServer.once.mockImplementation and before
invoking mockServer.listen (or before calling provider.start()), assert that
errorHandler is defined (e.g., expect(errorHandler).toBeDefined()) and/or add a
brief comment referencing the ordering dependency; this targets the errorHandler
captured in the it("should handle startup errors and release port") block and
ensures the test won't hang silently if server.once is registered after listen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcbab152-366c-47a8-97b9-9b525ba07170
📒 Files selected for processing (6)
.changeset/fix-dev-auth-bypass-node-env.md.changeset/fix-elysia-server-provider-tests.mdpackages/server-core/src/auth/utils.spec.tspackages/server-core/src/auth/utils.tspackages/server-core/src/websocket/setup.tspackages/server-elysia/src/elysia-server-provider.spec.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Summary
Fixes #1204 — all 6 tests in
elysia-server-provider.spec.tswere failing because the test mocks targeted the oldapp.listen()API, butstartServer()was refactored to usehttp.createServer()+server.listen().Changes
vi.mock('node:http')to mockcreateServerinstead of relying onmockApp.listenbeforeEachto configure mock server behavior (listen/close/once callbacks)fetchto mock app (required by the HTTP request handler)createServerandserver.listencallsserver.close()is calledserver.once('error')handlerTesting
All 6 tests pass:
Closes #1204
Summary by cubic
Updates Elysia server tests to mock
node:http(createServer/server.listen) after the startServer refactor, restoring all six cases (fixes #1204). Also secures the dev auth bypass in@voltagent/server-coreby requiringNODE_ENV=developmentortestonly (fail-closed), and applies the same check to WebSocket paths.@voltagent/server-elysia: mocknode:http; assertcreateServer,server.listen, andserver.close; simulate startup errors viaserver.once('error').@voltagent/server-core: addisDevEnvironment(); updateisDevRequest()and WS dev bypass to use it; treat empty/undefinedNODE_ENVas production; add unit tests.Written for commit e639394. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests