chore: migrate-old-mock-api #431
Conversation
|
WalkthroughA new end-to-end mock API service is introduced for authentication testing, configured within the monorepo using Nx. Updates are made to existing test suites to use the mock API, journey configuration is normalized to handle baseUrl trailing slashes, and test credentials are changed to reference local mock instances instead of ForgeBlocks infrastructure. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as e2e Test
participant JourneyApp as Journey App
participant MockAPI as am-mock-api
participant RealAM as Real AM (LIVE)
alt LIVE Mode
Test->>JourneyApp: Navigate with clientId=tenant
JourneyApp->>MockAPI: POST /authenticate
MockAPI->>MockAPI: authorizeApp() - fetch AM token
MockAPI->>RealAM: POST /authenticate (with token)
RealAM-->>MockAPI: Session token
MockAPI-->>JourneyApp: Auth response
else Non-LIVE (Mock) Mode
Test->>JourneyApp: Navigate with clientId=tenant
JourneyApp->>MockAPI: POST /authenticate
MockAPI->>MockAPI: Match authIndexValue & callbacks
MockAPI-->>JourneyApp: Mock response (e.g., initialBasicLogin)
end
JourneyApp->>MockAPI: POST /tokenExchange
MockAPI-->>JourneyApp: OAuth tokens
alt Authorization Required
JourneyApp->>MockAPI: GET /resource/ig/authz-by-txn
MockAPI->>MockAPI: Check auth header/cookie
alt Authorization Success
MockAPI-->>JourneyApp: Resource access
else Authorization Needed
MockAPI-->>JourneyApp: 307 redirect with step-up URL
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Areas requiring extra attention during review:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)e2e/journey-app/main.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
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 |
ryanbas21
force-pushed
the
migrate-am-mock-api
branch
from
fde9406 to
9116a55
Compare
|
View your CI Pipeline Execution ↗ for commit 5dd2420
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Nitpick comments (2)
e2e/am-mock-api/.eslintrc.json (1)
17-28: Consider simplifying the overrides configuration.The configuration has three separate override blocks for file patterns, but all have empty rules. Since they inherit from the base config, you could consolidate them into a single override.
Apply this diff to simplify:
{ - "files": ["*.ts", "*.tsx", "*.js", "*.jsx"], - "rules": {} - }, - { - "files": ["*.ts", "*.tsx"], - "rules": {} - }, - { - "files": ["*.js", "*.jsx"], + "files": ["*.ts", "*.tsx", "*.js", "*.jsx"], "rules": {} }e2e/am-mock-api/src/app/responses.js (1)
70-76: Defensive URL handling in helpers (Tree URL/header and Txn header).Mirror the same fallback to avoid crashes when Referer is absent.
Apply this diff:
export const createTreeStepUpUrl = (url) => { // Grab the client's desired AM URL - const referer = new URL(url); - const amUrl = referer.searchParams.get('amUrl'); + const safe = url || 'http://localhost/'; + const referer = new URL(safe); + const amUrl = referer.searchParams.get('amUrl'); @@ export const createTxnStepUpHeader = (url) => { // Grab the client's desired AM URL - const referer = new URL(url); - const amUrl = referer.searchParams.get('amUrl') || AM_URL; + const safe = url || 'http://localhost/'; + const referer = new URL(safe); + const amUrl = referer.searchParams.get('amUrl') || AM_URL; @@ export const createTreeStepUpHeader = (url) => { // Grab the client's desired AM URL - const referer = new URL(url); - const amUrl = referer.searchParams.get('amUrl') || AM_URL; + const safe = url || 'http://localhost/'; + const referer = new URL(safe); + const amUrl = referer.searchParams.get('amUrl') || AM_URL;Also applies to: 88-99, 101-112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
e2e/am-mock-api/.babelrc(1 hunks)e2e/am-mock-api/.eslintrc.json(1 hunks)e2e/am-mock-api/package.json(1 hunks)e2e/am-mock-api/project.json(1 hunks)e2e/am-mock-api/src/app/app.auth.js(1 hunks)e2e/am-mock-api/src/app/constants.js(1 hunks)e2e/am-mock-api/src/app/env.config.js(1 hunks)e2e/am-mock-api/src/app/response.registration.js(1 hunks)e2e/am-mock-api/src/app/responses.js(1 hunks)e2e/am-mock-api/src/app/routes.auth.js(1 hunks)e2e/am-mock-api/src/app/routes.resource.js(1 hunks)e2e/am-mock-api/src/app/wait.js(1 hunks)e2e/am-mock-api/src/environments/environment.prod.ts(1 hunks)e2e/am-mock-api/src/environments/environment.ts(1 hunks)e2e/am-mock-api/src/index.js(1 hunks)e2e/am-mock-api/tsconfig.app.json(1 hunks)e2e/am-mock-api/tsconfig.json(1 hunks)e2e/am-mock-api/tsconfig.spec.json(1 hunks)package.json(2 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
e2e/am-mock-api/src/index.js (2)
e2e/am-mock-api/src/app/app.auth.js (1)
authorizeApp(16-36)e2e/am-mock-api/src/app/env.config.js (2)
MOCK_PORT(62-62)MOCK_PORT(62-62)
e2e/am-mock-api/src/app/app.auth.js (3)
e2e/am-mock-api/src/app/routes.resource.js (2)
response(47-55)response(70-70)e2e/am-mock-api/src/app/env.config.js (4)
AM_URL(64-64)AM_URL(64-64)REALM_PATH(68-68)REALM_PATH(68-68)e2e/am-mock-api/src/app/routes.auth.js (2)
un(258-258)pw(266-266)
e2e/am-mock-api/src/app/routes.resource.js (5)
e2e/am-mock-api/src/app/env.config.js (8)
FORGEOPS(67-67)FORGEOPS(67-67)AM_PORT(61-61)AM_PORT(61-61)REALM_PATH(68-68)REALM_PATH(68-68)AM_URL(64-64)AM_URL(64-64)e2e/am-mock-api/src/app/app.auth.js (3)
response(18-24)session(14-14)session(14-14)e2e/am-mock-api/src/app/wait.js (1)
wait(13-15)e2e/am-mock-api/src/app/routes.auth.js (2)
baz(57-59)baz(57-59)e2e/am-mock-api/src/app/responses.js (12)
authByTxnResponse(124-132)authByTxnResponse(124-132)createTxnStepUpHeader(88-99)createTxnStepUpHeader(88-99)createTxnStepUpUrl(51-68)createTxnStepUpUrl(51-68)createTreeStepUpHeader(101-112)createTreeStepUpHeader(101-112)createTreeStepUpUrl(70-86)createTreeStepUpUrl(70-86)authByTreeResponse(114-122)authByTreeResponse(114-122)
e2e/am-mock-api/src/app/env.config.js (1)
e2e/am-mock-api/src/app/responses.js (6)
realm(96-96)realm(109-109)amUrl(55-55)amUrl(73-73)amUrl(91-91)amUrl(104-104)
e2e/am-mock-api/src/environments/environment.prod.ts (1)
e2e/am-mock-api/src/environments/environment.ts (1)
environment(1-9)
e2e/am-mock-api/src/environments/environment.ts (1)
e2e/am-mock-api/src/environments/environment.prod.ts (1)
environment(1-3)
e2e/am-mock-api/src/app/routes.auth.js (4)
e2e/am-mock-api/src/app/env.config.js (4)
USERS(71-71)USERS(71-71)AM_URL(64-64)AM_URL(64-64)e2e/am-mock-api/src/app/constants.js (2)
authPaths(11-66)authPaths(11-66)e2e/am-mock-api/src/app/wait.js (1)
wait(13-15)e2e/am-mock-api/src/app/responses.js (38)
authSuccess(45-49)authSuccess(45-49)pingProtectInitialize(313-368)pingProtectInitialize(313-368)MetadataMarketPlaceInitialize(1205-1250)MetadataMarketPlaceInitialize(1205-1250)initialBasicLogin(198-215)initialBasicLogin(198-215)recaptchaEnterpriseCallback(1310-1349)recaptchaEnterpriseCallback(1310-1349)authFail(40-43)authFail(40-43)MetadataMarketPlacePingOneEvaluation(1252-1308)MetadataMarketPlacePingOneEvaluation(1252-1308)otpQRCodeCallbacks(500-584)otpQRCodeCallbacks(500-584)redirectCallbackFailureSaml(622-639)redirectCallbackFailureSaml(622-639)redirectCallbackSaml(604-621)redirectCallbackSaml(604-621)redirectCallback(586-603)redirectCallback(586-603)pingProtectEvaluate(288-311)pingProtectEvaluate(288-311)requestDeviceProfile(673-700)requestDeviceProfile(673-700)noSessionSuccess(424-424)noSessionSuccess(424-424)oauthTokens(13-20)oauthTokens(13-20)oauthTokensExpiringSoon(22-29)oauthTokensExpiringSoon(22-29)oauthTokensExpired(31-38)oauthTokensExpired(31-38)redirectUrl(57-57)redirectUrl(75-75)userInfo(665-671)userInfo(665-671)
e2e/am-mock-api/src/app/responses.js (2)
e2e/am-mock-api/src/app/routes.auth.js (5)
url(547-547)redirectUrl(453-455)redirectUrl(461-463)redirectUrl(470-472)redirectUrl(513-513)e2e/am-mock-api/src/app/env.config.js (6)
amUrl(49-49)AM_URL(64-64)AM_URL(64-64)RESOURCE_URL(69-69)RESOURCE_URL(69-69)realm(36-36)
🪛 Biome (2.1.2)
e2e/am-mock-api/src/app/response.registration.js
[error] 10-224: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
e2e/am-mock-api/src/index.js
[error] 9-10: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 10-11: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 11-12: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 12-13: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 13-14: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 14-15: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 15-16: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 16-17: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 17-18: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 18-19: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 19-20: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
e2e/am-mock-api/src/app/app.auth.js
[error] 10-11: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 11-12: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 13-14: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 15-36: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
e2e/am-mock-api/src/app/routes.resource.js
[error] 10-11: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 11-12: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 12-13: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 13-14: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 14-22: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 22-23: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 23-24: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 63-191: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
e2e/am-mock-api/src/app/env.config.js
[error] 10-11: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 59-60: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 60-61: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 61-62: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 63-64: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 64-65: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 65-66: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 66-67: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 67-68: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 68-69: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 69-70: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 70-71: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
e2e/am-mock-api/src/app/constants.js
[error] 10-66: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
e2e/am-mock-api/src/app/wait.js
[error] 12-15: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
e2e/am-mock-api/src/app/routes.auth.js
[error] 10-11: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 11-12: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 12-13: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 13-51: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 51-52: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 52-53: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 56-59: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 60-644: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
e2e/am-mock-api/src/app/responses.js
[error] 10-11: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 12-20: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 21-29: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 30-38: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 39-43: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 44-49: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 50-68: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 69-86: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 87-99: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 100-112: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 113-122: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 123-132: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 133-150: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 151-165: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 165-176: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 176-196: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 197-215: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 216-234: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 235-264: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 265-275: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 276-286: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 287-311: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 312-368: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 369-383: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 384-398: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 399-422: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 423-424: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 425-437: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 438-485: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 486-498: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 499-584: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 585-603: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 603-621: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 621-639: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 639-651: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 651-663: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 664-671: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 672-700: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 701-1033: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 1035-1122: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 1123-1203: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 1204-1250: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 1251-1308: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
[error] 1309-1349: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (9)
e2e/am-mock-api/.babelrc (1)
1-3: LGTM!Standard Nx Babel configuration with appropriate preset and polyfill strategy.
package.json (1)
65-65: LGTM!Addition of
@nx/expressaligns with the new e2e mock API project and maintains version consistency with other Nx dependencies at 21.2.3.tsconfig.json (1)
73-75: LGTM!The project reference correctly integrates the new e2e/am-mock-api into the TypeScript build configuration.
e2e/am-mock-api/tsconfig.spec.json (1)
1-9: LGTM!Standard test configuration with appropriate module format and file inclusion patterns.
e2e/am-mock-api/src/app/response.registration.js (1)
11-224: LGTM: Comprehensive registration flow mock data.The mock response structure provides thorough coverage of the registration flow with various callback types (username validation, password policies, KBA, terms and conditions, etc.). This will enable robust e2e testing.
e2e/am-mock-api/tsconfig.json (1)
1-13: LGTM: Standard TypeScript project reference configuration.The composite project structure correctly delegates to app and spec configurations.
e2e/am-mock-api/src/app/constants.js (1)
11-66: LGTM: Well-organized authentication path configuration.The
authPathsobject provides a clear categorization of authentication endpoints across different realms and variants, which will facilitate consistent routing in the mock API.e2e/am-mock-api/tsconfig.app.json (1)
4-4: Verify intended output directory configuration before applying changes.The
outDirmismatch betweentsconfig.app.json("../../dist/out-tsc") andproject.json("e2e/am-mock-api/dist") exists, but this pattern is already present in other e2e projects (protect-app,davinci-app). Before proceeding with the suggested fix, confirm:
- Whether Nx's
@nx/js:tscexecutor respectsproject.jsonoutputPathas authoritative or iftsconfig.app.jsonoutDirtakes precedence- Whether this inconsistency is intentional or a widespread issue across the workspace
- Whether the current build configuration works as expected
If the current setup is verified as working, no change is needed. If changes are required, align all e2e projects' output configurations consistently.
e2e/am-mock-api/src/app/routes.resource.js (1)
159-170: Case mismatch fix is correct, but suggested solution is wrong—isLivedoes not exist.The case mismatch at line 160 is a real bug:
env.NODE_ENV === 'live'should match the uppercase'LIVE'used everywhere else (lines 27, 43, 67, etc.). However, the variableisLivedoes not exist in the codebase and cannot be used.Correct fix:
- if (env.NODE_ENV === 'live') { + if (env.NODE_ENV === 'LIVE') {Likely an incorrect or invalid review comment.
| import request from 'superagent'; | ||
| import { AM_URL, REALM_PATH } from './env.config.js'; | ||
|
|
||
| export let session; |
There was a problem hiding this comment.
Avoid mutable module-level state.
Exporting a mutable session variable that gets modified in authorizeApp can lead to race conditions if multiple authorization attempts occur, and makes the module harder to test. Consider returning the session from the function and letting the caller manage state.
Refactor to avoid shared mutable state:
-export let session;
-
export async function authorizeApp({ un, pw }) {
try {
const response = await request
.post(`${AM_URL}/json/realms/${REALM_PATH}/authenticate`)
.set('Content-Type', 'application/json')
.set('Accept-API-Version', 'resource=2.0, protocol=1.0')
.set('X-OpenAM-Username', un)
.set('X-OpenAM-Password', pw)
.send({});
- session = response.body;
+ const session = response.body;
console.log(`REST app identity token: ${session.tokenId}`);
return session;
} catch (error) {Then update callers to store the returned session locally.
🧰 Tools
🪛 Biome (2.1.2)
[error] 13-14: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
🤖 Prompt for AI Agents
In e2e/am-mock-api/src/app/app.auth.js around line 14, the module currently
exports a mutable top-level variable "session" which is set by authorizeApp;
refactor authorizeApp to return the session object instead of mutating exported
state and remove the exported mutable variable. Update all callers to accept and
store the returned session locally (or pass it through callers) so state is
owned by the caller and not shared across the module, and remove any direct
imports/assignments to the old exported "session". Ensure tests and call sites
are updated accordingly.
There was a problem hiding this comment.
where is this exported session used that moving into the module won't break it?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for imports of 'session' from app.auth.js
echo "=== Searching for imports of 'session' from app.auth ==="
rg -n -C3 "import.*session.*from.*app\.auth" e2e/am-mock-api/
echo ""
echo "=== Searching for any imports from app.auth.js ==="
rg -n -C3 "from ['\"].*app\.auth" e2e/am-mock-api/
echo ""
echo "=== Searching for direct references to 'session' in am-mock-api ==="
rg -n -C3 "\bsession\b" e2e/am-mock-api/src/ --type js
echo ""
echo "=== Checking index.js for potential usage ==="
cat e2e/am-mock-api/src/index.js
Length of output: 7601
@ryanbas21, the exported session variable is imported and used in e2e/am-mock-api/src/app/routes.resource.js at line 13:
import { session } from './app.auth.js';It's then accessed in three places within that file to set authentication headers when making requests to the AM service:
- Line 54 - in the
authorizationmiddleware:.set('iPlanetDirectoryPro', session.tokenId) - Line 77 - for OAuth introspect validation:
.set('iPlanetDirectoryPro', session.tokenId) - Line 85 - for session validation:
.set('iPlanetDirectoryPro', session.tokenId)
The current flow works like this:
index.jscallsauthorizeApp()(without awaiting it) whenNODE_ENV === 'LIVE'authorizeApp()sets the module-levelsessionvariable- Later, when routes in
routes.resource.jsare hit, they accesssession.tokenIdfrom the shared module state
To refactor this without breaking it, you would need to:
- Make
index.jsawaitauthorizeApp()and store the result - Pass the session to
resourceRoutes(app, session)(and potentiallyauthRoutesif needed) - Update the default export in
routes.resource.jsto accept a session parameter:export default function (app, session) { ... } - Remove the import of
sessionfromapp.auth.jsinroutes.resource.js
Would you like me to provide a concrete refactoring example?
| } catch (error) { | ||
| console.warn('\n###################################################'); | ||
| console.warn('WARNING: REST app user for Step Up/Txn Auth missing'); | ||
| console.warn('###################################################\n'); | ||
| } |
There was a problem hiding this comment.
Propagate authentication failures to caller.
The catch block only logs a warning without throwing or returning an error indicator. Callers cannot determine whether authentication succeeded or failed, potentially leading to undefined behavior when accessing the session.
Apply this diff to signal failure:
} catch (error) {
console.warn('\n###################################################');
console.warn('WARNING: REST app user for Step Up/Txn Auth missing');
console.warn('###################################################\n');
+ throw error; // or return null/undefined to signal failure
}
}📝 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.
| } catch (error) { | |
| console.warn('\n###################################################'); | |
| console.warn('WARNING: REST app user for Step Up/Txn Auth missing'); | |
| console.warn('###################################################\n'); | |
| } | |
| } catch (error) { | |
| console.warn('\n###################################################'); | |
| console.warn('WARNING: REST app user for Step Up/Txn Auth missing'); | |
| console.warn('###################################################\n'); | |
| throw error; // or return null/undefined to signal failure | |
| } |
🤖 Prompt for AI Agents
In e2e/am-mock-api/src/app/app.auth.js around lines 31 to 35, the catch block
only logs a warning and does not propagate the authentication failure to
callers; update the catch block to propagate the error by either rethrowing the
caught error (throw error) after logging or return a clear failure indicator
(eg. return null/false or an object with success:false) so callers can detect
and handle authentication failure; ensure the chosen approach is consistent with
surrounding code (promise rejection vs return value) and include the original
error details when propagating.
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
|
|
||
| import { env } from 'process'; |
There was a problem hiding this comment.
🧩 Analysis chain
ESM vs. CommonJS: configure modules so Biome and Node agree.
Biome parse errors suggest these files are treated as scripts. Either mark this package as ESM or switch to require/module.exports.
Add a package.json in e2e/am-mock-api/:
+// e2e/am-mock-api/package.json
+{
+ "name": "am-mock-api",
+ "private": true,
+ "type": "module"
+}Alternatively, rename files to .mjs or configure Biome to parse ESM (sourceType: module). Based on learnings.
Also applies to: 60-71
🏁 Script executed:
#!/bin/bash
# Confirm Biome is parsing these files as scripts (look for the errors)
rg -n "Illegal use of an import declaration outside of a module" -g "!.git"Length of output: 0
I need to gather more information about the actual state of the codebase to verify if the Biome errors mentioned in the review actually exist.
🏁 Script executed:
#!/bin/bash
# Check if e2e/am-mock-api/package.json exists
if [ -f "e2e/am-mock-api/package.json" ]; then
echo "=== e2e/am-mock-api/package.json EXISTS ==="
cat "e2e/am-mock-api/package.json"
else
echo "=== e2e/am-mock-api/package.json DOES NOT EXIST ==="
fi
echo ""
echo "=== Directory structure of e2e/am-mock-api ==="
find "e2e/am-mock-api" -type f -name "*.json" -o -name "*.js" -o -name "*.mjs" | head -20
echo ""
echo "=== Content of env.config.js ==="
if [ -f "e2e/am-mock-api/src/app/env.config.js" ]; then
cat "e2e/am-mock-api/src/app/env.config.js"
else
echo "File not found"
fi
echo ""
echo "=== Check for Biome config ==="
if [ -f "biome.json" ] || [ -f "biome.jsonc" ]; then
echo "Found Biome config"
[ -f "biome.json" ] && cat "biome.json" || cat "biome.jsonc"
else
echo "No Biome config found at root"
fiLength of output: 4594
Fix module type mismatch: ESM syntax requires "type": "module".
The package.json specifies "type": "commonjs", but env.config.js uses ESM syntax (import/export statements). This will fail at runtime.
Note: The referenced Biome parse errors do not appear to be reported—the actual issue is the configuration mismatch.
Update e2e/am-mock-api/package.json:
{
"name": "am-mock-api",
"version": "0.0.0",
"private": true,
"description": "",
"keywords": [],
"license": "ISC",
"author": "",
- "type": "commonjs",
+ "type": "module",
"main": "./index.js",
...
}Alternatively, convert all files to CommonJS syntax (require/module.exports).
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 10-11: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
🤖 Prompt for AI Agents
In e2e/am-mock-api/src/app/env.config.js around line 11, the file uses ESM
import/export syntax while e2e/am-mock-api/package.json is set to "type":
"commonjs"; fix by either updating package.json to "type": "module" (so ESM
imports are valid) or convert this file to CommonJS (replace `import { env }
from 'process'` with a require and switch any exports to module.exports) and
ensure all other files in the package follow the same module style.
| export default function wait(req, res, next) { | ||
| setTimeout(() => next(), delay); | ||
| } |
There was a problem hiding this comment.
Fix the module system mismatch.
The file uses ES6 export syntax (export default), but the package.json declares "type": "commonjs". This will cause a runtime error in Node.js.
Apply this diff to use CommonJS syntax:
-export default function wait(req, res, next) {
+module.exports = function wait(req, res, next) {
setTimeout(() => next(), delay);
-}
+};Alternatively, if you want to use ES6 modules throughout the project, change the package.json:
- "type": "commonjs",
+ "type": "module",Note: Switching to ES6 modules will require updating all .js files in the project to use ES6 import/export syntax.
📝 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.
| export default function wait(req, res, next) { | |
| setTimeout(() => next(), delay); | |
| } | |
| module.exports = function wait(req, res, next) { | |
| setTimeout(() => next(), delay); | |
| }; |
🧰 Tools
🪛 Biome (2.1.2)
[error] 12-15: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
🤖 Prompt for AI Agents
In e2e/am-mock-api/src/app/wait.js around lines 13 to 15, the file uses ES6
export syntax (`export default`) while package.json is `"type": "commonjs"`,
causing a runtime error; replace the ES6 export with CommonJS by exporting the
function with module.exports = wait and keep the function declaration as-is (or
convert any imports/exports in this file to CommonJS require/module.exports if
needed) so the module format matches the project config.
| export const environment = { | ||
| production: true, | ||
| }; |
There was a problem hiding this comment.
Production environment configuration is incomplete.
The production environment only defines the production flag, but the development environment (environment.ts) includes several critical configuration properties: AM_URL, REALM_PATH, WEB_OAUTH_CLIENT, JOURNEY_LOGIN, JOURNEY_REGISTER, and API_URL. Code that imports this environment configuration will fail at runtime when accessing missing properties.
Apply this diff to include all required properties:
export const environment = {
+ AM_URL: 'https://openam-crbrl-01.forgeblocks.com/am/',
+ REALM_PATH: 'alpha',
+ WEB_OAUTH_CLIENT: 'WebOAuthClient',
+ JOURNEY_LOGIN: 'UsernamelessWebAuthn',
+ JOURNEY_REGISTER: 'Registration',
+ API_URL: 'http://localhost:9443',
- production: true,
+ production: 'production',
};Note: Update the URLs and configuration values as appropriate for your production environment.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In e2e/am-mock-api/src/environments/environment.prod.ts around lines 1 to 3, the
production export only sets production: true while the dev environment defines
AM_URL, REALM_PATH, WEB_OAUTH_CLIENT, JOURNEY_LOGIN, JOURNEY_REGISTER, and
API_URL; add these missing properties to the exported environment object with
production-appropriate values (replace with your real production URLs/ids), e.g.
include AM_URL, REALM_PATH, WEB_OAUTH_CLIENT, JOURNEY_LOGIN, JOURNEY_REGISTER,
and API_URL alongside production: true so any code that reads those keys at
runtime does not break.
| JOURNEY_LOGIN: 'UsernamelessWebAuthn', | ||
| JOURNEY_REGISTER: 'Registration', | ||
| API_URL: 'http://localhost:9443', | ||
| production: 'development', |
There was a problem hiding this comment.
Fix type mismatch: production should be a boolean.
The production property is set to the string 'development', but the related environment.prod.ts file shows this should be a boolean value. This type inconsistency could cause runtime errors if the code performs boolean checks.
Apply this diff:
- production: 'development',
+ production: false,📝 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.
| production: 'development', | |
| production: false, |
🤖 Prompt for AI Agents
In e2e/am-mock-api/src/environments/environment.ts around line 8, the production
property is incorrectly set to the string 'development'; change it to the
boolean false to match the type used in environment.prod.ts and ensure any
runtime boolean checks work correctly. Update the value only (production: false)
so the file's shape and typings remain consistent with the prod environment
file.
| app.use( | ||
| cors({ | ||
| exposedHeaders: ['www-authenticate'], | ||
| credentials: true, | ||
| origin: function (origin, callback) { | ||
| return callback(null, true); | ||
| }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Tighten CORS policy for the mock API.
The CORS configuration allows all origins by unconditionally returning true in the origin callback. Even for a mock API, this creates unnecessary exposure if the server is accessible beyond localhost.
Apply this diff to restrict to localhost origins:
app.use(
cors({
exposedHeaders: ['www-authenticate'],
credentials: true,
- origin: function (origin, callback) {
- return callback(null, true);
- },
+ origin: function (origin, callback) {
+ // Allow requests with no origin (like mobile apps or curl) or localhost
+ if (!origin || /^https?:\/\/(localhost|127\.0\.0\.1|0\.0\.0\.0)(:\d+)?$/.test(origin)) {
+ return callback(null, true);
+ }
+ return callback(new Error('Not allowed by CORS'));
+ },
}),
);📝 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.
| app.use( | |
| cors({ | |
| exposedHeaders: ['www-authenticate'], | |
| credentials: true, | |
| origin: function (origin, callback) { | |
| return callback(null, true); | |
| }, | |
| }), | |
| ); | |
| app.use( | |
| cors({ | |
| exposedHeaders: ['www-authenticate'], | |
| credentials: true, | |
| origin: function (origin, callback) { | |
| // Allow requests with no origin (like mobile apps or curl) or localhost | |
| if (!origin || /^https?:\/\/(localhost|127\.0\.0\.1|0\.0\.0\.0)(:\d+)?$/.test(origin)) { | |
| return callback(null, true); | |
| } | |
| return callback(new Error('Not allowed by CORS')); | |
| }, | |
| }), | |
| ); |
|
|
||
| app.get('/healthcheck', (req, res) => res.status(200).send('ok')); | ||
|
|
||
| env['NODE_TLS_REJECT_UNAUTHORIZED'] = 0; |
There was a problem hiding this comment.
Remove global disabling of TLS certificate verification.
Setting NODE_TLS_REJECT_UNAUTHORIZED=0 disables certificate validation for all outbound HTTPS requests from this process, creating a man-in-the-middle vulnerability. While this may be acceptable for an isolated mock server in a test environment, it should be clearly documented and ideally scoped more narrowly.
Consider one of these alternatives:
- If certificate validation must be disabled for specific requests, use per-request agent configuration with superagent's
.disableTLSCerts()method - If this is truly necessary for the mock environment, add a prominent comment explaining why and ensure the mock server runs in complete isolation
- Use self-signed certificates properly configured in the Node.js trust store
-env['NODE_TLS_REJECT_UNAUTHORIZED'] = 0;
+// TODO: Configure proper certificates or use per-request TLS optionsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In e2e/am-mock-api/src/index.js around line 55, remove the global disabling of
TLS certificate verification (env['NODE_TLS_REJECT_UNAUTHORIZED'] = 0) and
instead either: 1) scope certificate skipping to only the required requests
using per-request agent/config (e.g., superagent's per-request disableTLSCerts
or equivalent), 2) if disabling is unavoidable for this mock, keep the global
change out of production by adding a prominent comment explaining why, gating it
behind an explicit test-only flag and ensuring the mock runs in complete
isolation, or 3) preferably configure and trust a self-signed certificate in
Node’s trust store so TLS validation remains enabled.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (18.80%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #431 +/- ##
==========================================
+ Coverage 18.52% 18.80% +0.27%
==========================================
Files 138 140 +2
Lines 27402 27626 +224
Branches 963 980 +17
==========================================
+ Hits 5076 5194 +118
- Misses 22326 22432 +106
🚀 New features to boost your workflow:
|
@forgerock/davinci-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
|
Deployed f30f2ee to https://ForgeRock.github.io/ping-javascript-sdk/pr-431/f30f2eeb024a42fd634a9ce32dd3830264ed4bef branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.0 KB, -100.0%) 📊 Minor Changes📈 @forgerock/journey-client - 82.2 KB (+0.2 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
ryanbas21
force-pushed
the
migrate-am-mock-api
branch
from
9116a55 to
f200156
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
e2e/am-mock-api/src/environments/environment.prod.ts (1)
1-3: Production environment configuration remains incomplete.This issue was previously flagged: the production environment only defines
production: true, while the development environment includes critical properties (AM_URL,REALM_PATH,WEB_OAUTH_CLIENT,JOURNEY_LOGIN,JOURNEY_REGISTER,API_URL) that runtime code will expect. This will cause runtime failures when code attempts to access these missing properties.e2e/am-mock-api/src/app/wait.js (1)
13-15: Module system mismatch flagged in previous review.The ES6 export syntax incompatibility with the CommonJS package.json configuration has already been identified in the previous review.
e2e/am-mock-api/src/app/constants.js (1)
11-11: Module system mismatch affects this file.This file uses ES6 export syntax while package.json declares
"type": "commonjs". The same issue has been flagged in other files in this package. Update e2e/am-mock-api/package.json to"type": "module"to resolve this for all affected files.e2e/am-mock-api/src/app/routes.resource.js (5)
27-29: Host comparison bug previously identified.The comparison of
req.hostnameto the full URL inFORGEOPShas been flagged in the previous review with detailed remediation steps.
47-56: Unguarded external call previously identified.The missing try/catch and timeout handling for the superagent POST call has been flagged in the previous review with suggested fixes.
71-95: Missing error handling for OAuth introspection and SSO validation previously identified.The lack of error handling and timeouts for external authentication calls has been comprehensively documented in the previous review.
121-130: Missing Referer guard and deprecated API previously identified.The missing null check for
req.headers.refererand the deprecatedres.send(401, null)syntax have been flagged in the previous review.
145-154: Missing Referer guard and deprecated API previously identified.The same issues with Referer header handling and deprecated response API have been flagged for this code block in the previous review.
e2e/am-mock-api/src/app/responses.js (3)
11-11: Module system mismatch previously identified.The ES6 import syntax incompatibility with the CommonJS package.json configuration has been flagged in the previous review.
51-68: Debug log and unsafe URL parsing previously identified.The noisy
console.logand unguardednew URL(url)call have been flagged in the previous review with suggested fixes.
88-99: Guard against invalid URL in header construction.The unguarded
new URL(url)will throw ifurlis falsy or malformed.Apply this diff:
export const createTxnStepUpHeader = (url) => { // Grab the client's desired AM URL - const referer = new URL(url); + const safe = url || 'http://localhost/'; + const referer = new URL(safe); const amUrl = referer.searchParams.get('amUrl') || AM_URL;
🧹 Nitpick comments (1)
e2e/journey-app/main.ts (1)
47-47: Consider using ES6 object property shorthand.Since the property name matches the variable name, you can simplify
config: configto justconfig.Apply this diff:
- const journeyClient = await journey({ config: config, requestMiddleware }); + const journeyClient = await journey({ config, requestMiddleware });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.claude/settings.local.json(1 hunks)e2e/am-mock-api/.babelrc(1 hunks)e2e/am-mock-api/.eslintrc.json(1 hunks)e2e/am-mock-api/package.json(1 hunks)e2e/am-mock-api/project.json(1 hunks)e2e/am-mock-api/src/app/app.auth.js(1 hunks)e2e/am-mock-api/src/app/constants.js(1 hunks)e2e/am-mock-api/src/app/env.config.js(1 hunks)e2e/am-mock-api/src/app/response.registration.js(1 hunks)e2e/am-mock-api/src/app/responses.js(1 hunks)e2e/am-mock-api/src/app/routes.auth.js(1 hunks)e2e/am-mock-api/src/app/routes.resource.js(1 hunks)e2e/am-mock-api/src/app/wait.js(1 hunks)e2e/am-mock-api/src/environments/environment.prod.ts(1 hunks)e2e/am-mock-api/src/environments/environment.ts(1 hunks)e2e/am-mock-api/src/index.js(1 hunks)e2e/am-mock-api/tsconfig.app.json(1 hunks)e2e/am-mock-api/tsconfig.json(1 hunks)e2e/am-mock-api/tsconfig.spec.json(1 hunks)e2e/journey-app/main.ts(3 hunks)e2e/journey-app/server-configs.ts(1 hunks)e2e/journey-suites/playwright.config.ts(1 hunks)e2e/journey-suites/src/basic.test.ts(1 hunks)package.json(2 hunks)packages/journey-client/src/lib/journey.store.test.ts(1 hunks)packages/journey-client/src/lib/journey.store.ts(2 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .claude/settings.local.json
🚧 Files skipped from review as they are similar to previous changes (9)
- e2e/am-mock-api/src/index.js
- e2e/am-mock-api/.babelrc
- e2e/am-mock-api/tsconfig.json
- e2e/am-mock-api/tsconfig.spec.json
- e2e/am-mock-api/src/app/env.config.js
- e2e/am-mock-api/src/app/app.auth.js
- e2e/am-mock-api/src/app/routes.auth.js
- e2e/am-mock-api/package.json
- e2e/am-mock-api/src/environments/environment.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-27T17:55:33.855Z
Learnt from: cerebrl
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: e2e/journey-app/main.ts:83-87
Timestamp: 2025-10-27T17:55:33.855Z
Learning: In the e2e/journey-app and similar e2e test applications in this repository, XSS hardening and similar production security practices are not required since these are exclusively test/development applications.
Applied to files:
e2e/journey-suites/src/basic.test.tspackages/journey-client/src/lib/journey.store.test.ts
📚 Learning: 2025-10-28T14:44:26.883Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 427
File: .github/workflows/ci-fork.yml:50-56
Timestamp: 2025-10-28T14:44:26.883Z
Learning: Nx CLI accepts multiple targets with the short `-t` flag as space-separated tokens (e.g., `nx affected -t lint test build`) and also supports comma-separated values with the long form `--targets=` (e.g., `nx affected --targets=lint,test,build`). Both syntaxes are valid.
Applied to files:
e2e/am-mock-api/project.jsonpackage.json
📚 Learning: 2025-10-22T15:14:00.044Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
Applied to files:
packages/journey-client/src/lib/journey.store.ts
🧬 Code graph analysis (6)
e2e/am-mock-api/src/environments/environment.prod.ts (1)
e2e/am-mock-api/src/environments/environment.ts (1)
environment(1-9)
packages/journey-client/src/lib/journey.store.ts (2)
packages/journey-client/src/lib/config.types.ts (1)
JourneyClientConfig(11-15)packages/journey-client/src/lib/journey.store.utils.ts (1)
createJourneyStore(21-44)
packages/journey-client/src/lib/journey.store.test.ts (2)
packages/journey-client/src/lib/config.types.ts (1)
JourneyClientConfig(11-15)packages/journey-client/src/lib/journey.store.ts (1)
journey(45-194)
e2e/journey-app/main.ts (1)
packages/journey-client/src/lib/journey.store.ts (1)
journey(45-194)
e2e/am-mock-api/src/app/routes.resource.js (5)
e2e/am-mock-api/src/app/env.config.js (8)
FORGEOPS(67-67)FORGEOPS(67-67)AM_PORT(61-61)AM_PORT(61-61)REALM_PATH(68-68)REALM_PATH(68-68)AM_URL(64-64)AM_URL(64-64)e2e/am-mock-api/src/app/app.auth.js (3)
response(18-24)session(14-14)session(14-14)e2e/am-mock-api/src/app/wait.js (1)
wait(13-15)e2e/am-mock-api/src/app/routes.auth.js (2)
baz(57-59)baz(57-59)e2e/am-mock-api/src/app/responses.js (12)
authByTxnResponse(124-132)authByTxnResponse(124-132)createTxnStepUpHeader(88-99)createTxnStepUpHeader(88-99)createTxnStepUpUrl(51-68)createTxnStepUpUrl(51-68)createTreeStepUpHeader(101-112)createTreeStepUpHeader(101-112)createTreeStepUpUrl(70-86)createTreeStepUpUrl(70-86)authByTreeResponse(114-122)authByTreeResponse(114-122)
e2e/am-mock-api/src/app/responses.js (2)
e2e/am-mock-api/src/app/routes.auth.js (5)
url(547-547)redirectUrl(453-455)redirectUrl(461-463)redirectUrl(470-472)redirectUrl(513-513)e2e/am-mock-api/src/app/env.config.js (6)
amUrl(49-49)AM_URL(64-64)AM_URL(64-64)RESOURCE_URL(69-69)RESOURCE_URL(69-69)realm(36-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (10)
package.json (2)
37-37: LGTM: Watch script updated for Vite integration.The watch script change integrates Nx with Vite-based watching, which aligns with the Vite dependency in the project.
65-65: LGTM: @nx/express dependency added for mock API support.The addition of
@nx/expressat version 21.2.3 correctly enables Express integration within Nx tooling, supporting the new e2e mock API infrastructure.packages/journey-client/src/lib/journey.store.ts (2)
25-43: LGTM: baseUrl normalization correctly implemented.The
normalizeConfighelper ensures thatbaseUrlalways has a trailing slash, which is essential for correct URL resolution when using context paths (e.g.,/am). The implementation correctly:
- Checks for the presence of
serverConfig.baseUrl- Adds a trailing slash only when missing
- Returns the original config if already normalized
59-63: LGTM: Config normalization properly applied.The normalized config is correctly applied both when creating the store and when dispatching
setConfig, ensuring consistent URL handling throughout the journey client.packages/journey-client/src/lib/journey.store.test.ts (1)
241-298: LGTM: Comprehensive test coverage for baseUrl normalization.The new test suite thoroughly validates the
normalizeConfigbehavior across three important scenarios:
- Adding a trailing slash when missing for URLs with context paths
- Preserving an existing trailing slash
- Handling URLs without context paths
All tests correctly verify that the final request URL is properly constructed.
e2e/am-mock-api/tsconfig.app.json (1)
1-13: LGTM: Standard TypeScript configuration for Express application.The TypeScript configuration is appropriate for a Node/Express-based mock API:
- Node16 module system aligns with modern Node.js practices
- ES6 target is suitable for server-side code
- Proper type definitions for Node.js and Express
- Excludes test files from compilation
tsconfig.json (1)
79-81: LGTM: Project reference added for am-mock-api.The addition of the
./e2e/am-mock-apiproject reference correctly integrates the new mock API into the TypeScript composite project structure.e2e/am-mock-api/src/app/response.registration.js (2)
69-99: Verify: Is the commented NumberAttributeInputCallback intentional?There's a large block of commented-out code for a
NumberAttributeInputCallback(age field). If this callback is not needed for the registration flow tests, consider removing it entirely to keep the codebase clean. If it's intended for future use, consider adding a TODO comment explaining why it's disabled.
11-224: LGTM: Mock registration response structure is well-defined.The mock registration data comprehensively covers a typical user registration flow with appropriate callbacks for:
- Username validation with policies
- Personal information collection
- Email with validation
- Preference checkboxes
- Password with strength policies
- Security questions (KBA)
- Terms and conditions
The structure correctly follows AM callback patterns for e2e testing.
e2e/journey-suites/src/basic.test.ts (1)
26-27: LGTM: Hardcoded test credentials are appropriate for e2e mock environment.Replacing the imported credentials with hardcoded values ('sdkuser' and 'password') is acceptable for end-to-end tests against a mock API. This simplifies the test setup and makes the test more explicit about the test data being used.
| export const createTreeStepUpUrl = (url) => { | ||
| // Grab the client's desired AM URL | ||
| const referer = new URL(url); | ||
| const amUrl = referer.searchParams.get('amUrl'); | ||
| // Create the redirect URL | ||
| const redirectUrl = new URL(amUrl || AM_URL); | ||
| redirectUrl.searchParams.set('goto', `${RESOURCE_URL}/ig`); | ||
| redirectUrl.searchParams.set('realm', '/'); | ||
| redirectUrl.searchParams.set('authIndexType', 'composite_advice'); | ||
| redirectUrl.searchParams.set( | ||
| 'authIndexValue', | ||
| // eslint-disable-next-line max-len | ||
| '%3CAdvices%3E%3CAttributeValuePair%3E%3CAttribute%20name=%22AuthenticateToServiceConditionAdvice%22/%3E%3CValue%3E/sdk:ConfirmPassword%3C/Value%3E%3C/AttributeValuePair%3E%3C/Advices%3E', | ||
| ); | ||
|
|
||
| return redirectUrl.toString(); | ||
| }; |
There was a problem hiding this comment.
Apply the same URL parsing resilience as createTxnStepUpUrl.
This function has the same vulnerability as createTxnStepUpUrl (flagged in previous review): new URL(url) throws if url is falsy or invalid. Apply the same defensive pattern suggested for the other function.
Apply this diff:
export const createTreeStepUpUrl = (url) => {
// Grab the client's desired AM URL
- const referer = new URL(url);
+ const safe = url || 'http://localhost/';
+ const referer = new URL(safe);
const amUrl = referer.searchParams.get('amUrl');🤖 Prompt for AI Agents
In e2e/am-mock-api/src/app/responses.js around lines 70 to 86, new URL(url) can
throw for falsy/invalid input; make the same defensive change used in
createTxnStepUpUrl: attempt to construct the referer inside a try/catch (or
check truthiness) and if it fails set referer to new URL(AM_URL) (or another
safe default), then read amUrl from referer.searchParams and continue building
redirectUrl as before; this prevents exceptions on bad/missing url input while
preserving existing behavior.
| export const createTreeStepUpHeader = (url) => { | ||
| // Grab the client's desired AM URL | ||
| const referer = new URL(url); | ||
| const amUrl = referer.searchParams.get('amUrl') || AM_URL; | ||
|
|
||
| // Base 64 of {"AuthenticateToServiceConditionAdvice":["/sdk:ConfirmPassword"]} | ||
| const advices = | ||
| 'eyJBdXRoZW50aWNhdGVUb1NlcnZpY2VDb25kaXRpb25BZHZpY2UiOlsiL3NkazpDb25maXJtUGFzc3dvcmQiXX0='; | ||
| const realm = '/'; | ||
| const headerValue = `SSOADVICE realm="${realm}",advices="${advices}",am_uri="${amUrl}"`; | ||
| return headerValue; | ||
| }; |
There was a problem hiding this comment.
Guard against invalid URL in header construction.
The unguarded new URL(url) will throw if url is falsy or malformed.
Apply this diff:
export const createTreeStepUpHeader = (url) => {
// Grab the client's desired AM URL
- const referer = new URL(url);
+ const safe = url || 'http://localhost/';
+ const referer = new URL(safe);
const amUrl = referer.searchParams.get('amUrl') || AM_URL;📝 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.
| export const createTreeStepUpHeader = (url) => { | |
| // Grab the client's desired AM URL | |
| const referer = new URL(url); | |
| const amUrl = referer.searchParams.get('amUrl') || AM_URL; | |
| // Base 64 of {"AuthenticateToServiceConditionAdvice":["/sdk:ConfirmPassword"]} | |
| const advices = | |
| 'eyJBdXRoZW50aWNhdGVUb1NlcnZpY2VDb25kaXRpb25BZHZpY2UiOlsiL3NkazpDb25maXJtUGFzc3dvcmQiXX0='; | |
| const realm = '/'; | |
| const headerValue = `SSOADVICE realm="${realm}",advices="${advices}",am_uri="${amUrl}"`; | |
| return headerValue; | |
| }; | |
| export const createTreeStepUpHeader = (url) => { | |
| // Grab the client's desired AM URL | |
| const safe = url || 'http://localhost/'; | |
| const referer = new URL(safe); | |
| const amUrl = referer.searchParams.get('amUrl') || AM_URL; | |
| // Base 64 of {"AuthenticateToServiceConditionAdvice":["/sdk:ConfirmPassword"]} | |
| const advices = | |
| 'eyJBdXRoZW50aWNhdGVUb1NlcnZpY2VDb25kaXRpb25BZHZpY2UiOlsiL3NkazpDb25maXJtUGFzc3dvcmQiXX0='; | |
| const realm = '/'; | |
| const headerValue = `SSOADVICE realm="${realm}",advices="${advices}",am_uri="${amUrl}"`; | |
| return headerValue; | |
| }; |
🤖 Prompt for AI Agents
In e2e/am-mock-api/src/app/responses.js around lines 101 to 112 the call new
URL(url) is unguarded and will throw for falsy or malformed input; wrap the URL
parsing in a try/catch (or validate with a safe URL parse) and if parsing fails
fall back to using AM_URL as the amUrl (or a sensible default), then continue to
build and return the headerValue; ensure referer.searchParams.get is only
accessed when parsing succeeded and that the function never throws for bad
input.
| { | ||
| command: 'pnpm nx serve am-mock-api', | ||
| port: 5829, | ||
| ignoreHTTPSErrors: true, | ||
| reuseExistingServer: !process.env.CI, | ||
| cwd: workspaceRoot, | ||
| }, |
There was a problem hiding this comment.
Port conflict: am-mock-api and journey-app both configured for port 5829.
Both web servers are configured to use port 5829:
- Line 35: journey-app on port 5829
- Line 42: am-mock-api on port 5829
Two separate server processes cannot listen on the same port simultaneously. This will cause the second server to fail to start with an "EADDRINUSE" error.
Apply this diff to assign a different port to am-mock-api:
{
command: 'pnpm nx serve am-mock-api',
- port: 5829,
+ port: 9443,
ignoreHTTPSErrors: true,
reuseExistingServer: !process.env.CI,
cwd: workspaceRoot,
},Ensure that the journey-app is also configured to make requests to http://localhost:9443 (or whichever port you choose) for the mock AM API endpoints.
📝 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.
| { | |
| command: 'pnpm nx serve am-mock-api', | |
| port: 5829, | |
| ignoreHTTPSErrors: true, | |
| reuseExistingServer: !process.env.CI, | |
| cwd: workspaceRoot, | |
| }, | |
| { | |
| command: 'pnpm nx serve am-mock-api', | |
| port: 9443, | |
| ignoreHTTPSErrors: true, | |
| reuseExistingServer: !process.env.CI, | |
| cwd: workspaceRoot, | |
| }, |
🤖 Prompt for AI Agents
In e2e/journey-suites/playwright.config.ts around lines 40 to 46, the
am-mock-api server is configured to use port 5829 which conflicts with
journey-app (also 5829). Change the am-mock-api port to a different one (e.g.,
9443) in this block and update any related start/command args if necessary; then
ensure journey-app configuration/fixtures point to the mock AM API at
http://localhost:9443 (or whichever new port you pick) so tests call the correct
mock endpoint.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/davinci-suites/src/phone-number-field.test.ts (1)
109-111: Consider applying consistent assertion patterns across similar tests.The phone device registration test uses
.toPass()for retry logic, but the email device test (line 66) uses a directtoBeVisible()assertion without retries. If timing issues necessitate retry logic here, the email registration likely benefits from the same pattern.Apply this diff to make the email test consistent:
- await expect(page.getByText('EMAIL MFA Registered')).toBeVisible(); + await expect( + async () => await expect(page.getByText('EMAIL MFA Registered')).toBeVisible(), + ).toPass();Alternatively, verify whether the timing difference between phone and email registration is intentional and document the reason if so.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/davinci-suites/src/phone-number-field.test.ts(1 hunks)e2e/journey-suites/playwright.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (1)
e2e/journey-suites/playwright.config.ts (1)
40-46: Port conflict resolved and configuration verified.The port configuration is correct: journey-app runs on 5829 while am-mock-api runs on 9443. Verification confirms journey-app is properly configured in
e2e/journey-app/server-configs.ts(line 12) to call the mock AM API athttp://localhost:9443/am. No downstream issues identified.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
.claude/settings.local.json
Outdated
There was a problem hiding this comment.
We should probably .gitignore this as well, yeah?
e2e/journey-app/main.ts
Outdated
|
|
||
| const config = serverConfigs[searchParams.get('clientId') || 'basic']; | ||
|
|
||
| const tree = searchParams.get('tree') ?? 'UsernamePassword'; |
There was a problem hiding this comment.
I'm using journey and not tree in all the tests I've migrated over. Let's stick with that since "tree" is an outdated term.
There was a problem hiding this comment.
journey conflicts with the variable journey we can use a variation of journey if you prefer that?
| await expect( | ||
| async () => await expect(page.getByText('SMS/Voice MFA Registered')).toBeVisible(), | ||
| ).toPass(); |
There was a problem hiding this comment.
Did you mean to commit this?
There was a problem hiding this comment.
i was debugging a test error i can remove.
There was a problem hiding this comment.
It seems this wasn't removed. Can you remove it?
| basic: { | ||
| serverConfig: { | ||
| baseUrl: 'https://openam-sdks.forgeblocks.com/am/', | ||
| baseUrl: 'http://localhost:9443/am', | ||
| }, | ||
| realmPath: '/alpha', | ||
| realmPath: 'root', | ||
| }, |
There was a problem hiding this comment.
Should we have a different config object for the Mock API, rather than replacing our shared test tenant?
e2e/journey-suites/src/basic.test.ts
Outdated
| await page.getByLabel('User Name').fill('sdkuser'); | ||
| await page.getByLabel('Password').fill('password'); |
There was a problem hiding this comment.
Was this accidentally committed?
There was a problem hiding this comment.
no this was the user we had to use to make the test pass with mockapi
| @@ -7,7 +7,6 @@ | |||
|
|
|||
| import { expect, test } from '@playwright/test'; | |||
| import { asyncEvents } from './utils/async-events.js'; | |||
| import { username, password } from './utils/demo-user.js'; | |||
|
|
|||
| test('Test happy paths on test page', async ({ page }) => { | |||
There was a problem hiding this comment.
What's the "switch" that allows us to define the use of Mock vs Live server depending on the test run?
There was a problem hiding this comment.
I don't understand this question? If your asking for parity between the mock api and our tenant, have we ever had that?
ryanbas21
force-pushed
the
migrate-am-mock-api
branch
from
0a56dac to
5ff6bd8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
migrate-am-mock-api
branch
from
5ff6bd8 to
1e7af4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (12)
e2e/am-mock-api/src/index.js (3)
43-48: Hardcoded credentials must be removed.This issue was previously flagged but remains unaddressed. Hardcoded credentials pose a security risk even in test environments.
29-37: CORS policy allows all origins.This issue was previously flagged. The permissive CORS configuration should be tightened to localhost origins only.
55-55: TLS certificate verification is globally disabled.This issue was previously flagged. Consider scoping certificate skipping to specific requests or using properly configured certificates.
e2e/am-mock-api/src/app/app.auth.js (2)
14-14: Mutable module-level state is problematic.This issue was previously discussed. The exported mutable
sessionvariable can lead to race conditions and makes testing difficult. The conversation identified thatroutes.resource.jsimports this variable.
31-35: Authentication failures are not propagated.This issue was previously flagged. The catch block only logs a warning without throwing or returning an error indicator, preventing callers from detecting authentication failures.
e2e/am-mock-api/src/app/routes.resource.js (3)
14-74: Fix LIVE gating: hostname comparison is broken and the policy URL is malformed.
req.hostnamereturnsdefault.forgeops.petrov.ca, butFORGEOPSishttps://…; equality/inequality checks will never behave as intended, so LIVE requests skip auth enforcement. On top of that,${req.protocol}://${req.host}:${AM_PORT}double-inserts the port (req.host already holds host:port, or is undefined), yielding invalid resource URLs. ParseFORGEOPSonce (FORGEOPS_HOST) and compare hostnames, and buildfullURLwithreq.get('host')(orreq.hostname) without re-appending AM_PORT. This is critical for LIVE authorization to work. -->
47-80: Guard superagent calls—network failures currently crash the handler.Both the policy evaluate call and the OAuth/SSO validation paths await superagent without try/catch or timeouts. Any network error or slow AM host turns into an unhandled rejection, tearing down the request (or the process under load). Wrap those awaits in try/catch, add
.timeout({ response: 5000, deadline: 15000 }), and return a 5xx/401 instead of letting the exception propagate. -->
120-131: Don’t assume Referer exists.
req.headers.referer.includes('json')and the subsequent redirect/header builders dereference an optional header and feed it tonew URL(...). Missing/invalid referers throw here, breaking the route. Please guard with a safe default (empty string orAM_URL) before calling.includesorcreateTxnStepUp*. -->e2e/am-mock-api/src/app/routes.auth.js (3)
55-55: Drop the password log.Printing
USERS[0].pwleaks credentials to stdout/logs. Even in mocks this is a security smell—delete the log or hide the secret. -->
449-474: Handle missing Referer before constructing URLs.
new URL(req.get('Referer'))throws when the header is absent/invalid, which is common in automated tests. Add a fallback (e.g., derive fromreq.protocol/req.get('host')) before instantiating URL. -->
541-546: Cookie domain detection never matches localhost.
req.urlis just the path, so this branch always choosesexample.com, breaking local cookies. Usereq.hostname(orreq.get('host')) instead. -->e2e/am-mock-api/src/app/responses.js (1)
51-112: All step-up helpers need safe URL parsing.Each helper still calls
new URL(url)(and one logs to stdout). When Referer is missing or malformed, these throw—exactly the crash observed in routes.resource/IG flows. Remove the debugconsole.log, validate the input (fallback toAM_URL/RESOURCE_URL), and only readsearchParamswhen parsing succeeds. -->
🧹 Nitpick comments (2)
e2e/journey-app/utils/delete-webauthn-device.ts (1)
14-16: Consider parallel deletion for better performance.Sequential deletion with
awaitin a loop can be slow when multiple devices exist. UsingPromise.all()would execute deletions in parallel.Apply this diff to parallelize deletions:
-for (const device of devices) { - await dc.webAuthn.delete({ userId, realm, device }); -} +await Promise.all( + devices.map(device => dc.webAuthn.delete({ userId, realm, device })) +);packages/journey-client/src/lib/client.store.ts (1)
25-43: Consider edge case handling for robustness.The normalization logic handles the primary use cases well. For additional robustness, consider handling edge cases such as:
- Empty string baseUrl (
''→'/')- Multiple trailing slashes (
'http://test.com//')However, since this is a private helper and the primary use cases are covered, these edge cases may be better handled through upstream validation of the config object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
e2e/am-mock-api/.babelrc(1 hunks)e2e/am-mock-api/.eslintrc.json(1 hunks)e2e/am-mock-api/package.json(1 hunks)e2e/am-mock-api/project.json(1 hunks)e2e/am-mock-api/src/app/app.auth.js(1 hunks)e2e/am-mock-api/src/app/constants.js(1 hunks)e2e/am-mock-api/src/app/env.config.js(1 hunks)e2e/am-mock-api/src/app/response.registration.js(1 hunks)e2e/am-mock-api/src/app/responses.js(1 hunks)e2e/am-mock-api/src/app/routes.auth.js(1 hunks)e2e/am-mock-api/src/app/routes.resource.js(1 hunks)e2e/am-mock-api/src/app/wait.js(1 hunks)e2e/am-mock-api/src/environments/environment.prod.ts(1 hunks)e2e/am-mock-api/src/environments/environment.ts(1 hunks)e2e/am-mock-api/src/index.js(1 hunks)e2e/am-mock-api/tsconfig.app.json(1 hunks)e2e/am-mock-api/tsconfig.json(1 hunks)e2e/am-mock-api/tsconfig.spec.json(1 hunks)e2e/davinci-suites/src/phone-number-field.test.ts(1 hunks)e2e/journey-app/main.ts(2 hunks)e2e/journey-app/server-configs.ts(1 hunks)e2e/journey-app/utils/delete-webauthn-device.ts(1 hunks)e2e/journey-suites/playwright.config.ts(1 hunks)e2e/journey-suites/src/login.test.ts(0 hunks)package.json(2 hunks)packages/journey-client/src/lib/client.store.test.ts(1 hunks)packages/journey-client/src/lib/client.store.ts(2 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/journey-suites/src/login.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- e2e/davinci-suites/src/phone-number-field.test.ts
- e2e/am-mock-api/src/app/response.registration.js
- e2e/am-mock-api/tsconfig.app.json
- e2e/am-mock-api/package.json
- e2e/am-mock-api/tsconfig.spec.json
- e2e/am-mock-api/.babelrc
- e2e/am-mock-api/src/app/wait.js
- e2e/journey-app/server-configs.ts
- e2e/am-mock-api/src/environments/environment.ts
- e2e/journey-suites/playwright.config.ts
- e2e/journey-app/main.ts
- e2e/am-mock-api/src/environments/environment.prod.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-28T14:44:26.883Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 427
File: .github/workflows/ci-fork.yml:50-56
Timestamp: 2025-10-28T14:44:26.883Z
Learning: Nx CLI accepts multiple targets with the short `-t` flag as space-separated tokens (e.g., `nx affected -t lint test build`) and also supports comma-separated values with the long form `--targets=` (e.g., `nx affected --targets=lint,test,build`). Both syntaxes are valid.
Applied to files:
package.jsone2e/am-mock-api/project.json
📚 Learning: 2025-10-22T15:14:00.044Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
Applied to files:
packages/journey-client/src/lib/client.store.ts
📚 Learning: 2025-10-27T17:55:33.855Z
Learnt from: cerebrl
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: e2e/journey-app/main.ts:83-87
Timestamp: 2025-10-27T17:55:33.855Z
Learning: In the e2e/journey-app and similar e2e test applications in this repository, XSS hardening and similar production security practices are not required since these are exclusively test/development applications.
Applied to files:
packages/journey-client/src/lib/client.store.test.ts
🧬 Code graph analysis (9)
e2e/journey-app/utils/delete-webauthn-device.ts (1)
packages/device-client/src/lib/device.store.ts (1)
deviceClient(27-358)
e2e/am-mock-api/src/app/env.config.js (1)
e2e/am-mock-api/src/app/responses.js (6)
realm(96-96)realm(109-109)amUrl(55-55)amUrl(73-73)amUrl(91-91)amUrl(104-104)
e2e/am-mock-api/src/app/routes.resource.js (5)
e2e/am-mock-api/src/app/env.config.js (8)
FORGEOPS(67-67)FORGEOPS(67-67)AM_PORT(61-61)AM_PORT(61-61)REALM_PATH(68-68)REALM_PATH(68-68)AM_URL(64-64)AM_URL(64-64)e2e/am-mock-api/src/app/app.auth.js (3)
response(18-24)session(14-14)session(14-14)e2e/am-mock-api/src/app/wait.js (1)
wait(13-15)e2e/am-mock-api/src/app/routes.auth.js (2)
baz(57-59)baz(57-59)e2e/am-mock-api/src/app/responses.js (12)
authByTxnResponse(124-132)authByTxnResponse(124-132)createTxnStepUpHeader(88-99)createTxnStepUpHeader(88-99)createTxnStepUpUrl(51-68)createTxnStepUpUrl(51-68)createTreeStepUpHeader(101-112)createTreeStepUpHeader(101-112)createTreeStepUpUrl(70-86)createTreeStepUpUrl(70-86)authByTreeResponse(114-122)authByTreeResponse(114-122)
packages/journey-client/src/lib/client.store.ts (2)
packages/journey-client/src/lib/config.types.ts (1)
JourneyClientConfig(11-15)packages/journey-client/src/lib/client.store.utils.ts (1)
createJourneyStore(21-44)
packages/journey-client/src/lib/client.store.test.ts (2)
packages/journey-client/src/lib/config.types.ts (1)
JourneyClientConfig(11-15)packages/journey-client/src/lib/client.store.ts (1)
journey(45-194)
e2e/am-mock-api/src/app/routes.auth.js (4)
e2e/am-mock-api/src/app/env.config.js (4)
USERS(71-71)USERS(71-71)AM_URL(64-64)AM_URL(64-64)e2e/am-mock-api/src/app/constants.js (2)
authPaths(11-66)authPaths(11-66)e2e/am-mock-api/src/app/wait.js (1)
wait(13-15)e2e/am-mock-api/src/app/responses.js (44)
txnAuthz(640-651)txnAuthz(640-651)treeAuthz(652-663)treeAuthz(652-663)authSuccess(45-49)authSuccess(45-49)initialLoginWithEmailResponse(217-234)initialLoginWithEmailResponse(217-234)nameCallback(166-176)nameCallback(166-176)MetadataMarketPlaceInitialize(1205-1250)MetadataMarketPlaceInitialize(1205-1250)initialBasicLogin(198-215)initialBasicLogin(198-215)recaptchaEnterpriseCallback(1310-1349)recaptchaEnterpriseCallback(1310-1349)authFail(40-43)authFail(40-43)MetadataMarketPlacePingOneEvaluation(1252-1308)MetadataMarketPlacePingOneEvaluation(1252-1308)otpQRCodeCallbacks(500-584)otpQRCodeCallbacks(500-584)redirectCallbackFailureSaml(622-639)redirectCallbackFailureSaml(622-639)redirectCallbackSaml(604-621)redirectCallbackSaml(604-621)passwordCallback(277-286)passwordCallback(277-286)redirectCallback(586-603)redirectCallback(586-603)requestDeviceProfile(673-700)requestDeviceProfile(673-700)noSessionSuccess(424-424)noSessionSuccess(424-424)oauthTokens(13-20)oauthTokens(13-20)oauthTokensExpiringSoon(22-29)oauthTokensExpiringSoon(22-29)oauthTokensExpired(31-38)oauthTokensExpired(31-38)redirectUrl(57-57)redirectUrl(75-75)userInfo(665-671)userInfo(665-671)
e2e/am-mock-api/src/app/app.auth.js (2)
e2e/am-mock-api/src/app/routes.resource.js (2)
response(47-55)response(70-70)e2e/am-mock-api/src/app/env.config.js (4)
AM_URL(64-64)AM_URL(64-64)REALM_PATH(68-68)REALM_PATH(68-68)
e2e/am-mock-api/src/app/responses.js (2)
e2e/am-mock-api/src/app/routes.auth.js (5)
url(547-547)redirectUrl(453-455)redirectUrl(461-463)redirectUrl(470-472)redirectUrl(513-513)e2e/am-mock-api/src/app/env.config.js (6)
amUrl(49-49)AM_URL(64-64)AM_URL(64-64)RESOURCE_URL(69-69)RESOURCE_URL(69-69)realm(36-36)
e2e/am-mock-api/src/index.js (2)
e2e/am-mock-api/src/app/app.auth.js (1)
authorizeApp(16-36)e2e/am-mock-api/src/app/env.config.js (2)
MOCK_PORT(62-62)MOCK_PORT(62-62)
🪛 Gitleaks (8.29.0)
e2e/am-mock-api/src/app/routes.auth.js
[high] 194-194: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Mend Code Security Check
🔇 Additional comments (11)
e2e/journey-app/utils/delete-webauthn-device.ts (3)
3-7: LGTM! Clean type-safe initialization.The use of
Parameters<typeof deviceClient>[0]provides excellent type inference while avoiding duplication. The function signature is clear and the initialization is straightforward.
8-12: Verify silent error handling is acceptable.The function returns early without logging or propagating errors when device retrieval fails. While this might be intentional for a cleanup utility, it could make debugging test failures difficult if device deletion silently fails.
Consider whether errors should be logged or at least made visible for debugging:
const devices = await dc.webAuthn.get({ userId, realm }); if (!devices || 'error' in devices || !devices.length) { + if ('error' in devices) { + console.warn('Failed to retrieve WebAuthn devices for cleanup:', devices.error); + } return; }
14-16: Verify that deletion failures can be ignored.The delete operations can return error objects (per the device client API), but the code doesn't check the return values. This means individual deletion failures are silently ignored. Confirm this is acceptable for this e2e cleanup utility.
If any deletion failures should be surfaced, consider:
for (const device of devices) { const result = await dc.webAuthn.delete({ userId, realm, device }); if (result && 'error' in result) { console.warn('Failed to delete WebAuthn device:', result.error); } }packages/journey-client/src/lib/client.store.ts (1)
59-63: LGTM! Clean normalization implementation.The config normalization is applied correctly before store creation and dispatch, ensuring all downstream code receives a consistent, normalized baseUrl.
packages/journey-client/src/lib/client.store.test.ts (1)
241-298: LGTM! Comprehensive test coverage for baseUrl normalization.The test suite effectively validates the normalization logic across different baseUrl formats:
- Adding trailing slash when absent
- Preserving existing trailing slash
- Handling baseUrl without context path
The integration testing approach is appropriate, as it verifies the observable behavior (correct URL construction) rather than testing implementation details.
package.json (2)
64-64: LGTM!The addition of
@nx/expressaligns with the new Express-based mock API infrastructure introduced in this PR.
37-37: Verify the watch script change with git history and test watch command execution.The watch script now targets
vite:watch-depsinstead of a potentially more general target. While this change is coordinated in nx.json's Vite plugin configuration, the only project.json discovered uses@nx/js:tsc(not Vite). This raises concerns:
- The
vite:watch-depstarget is auto-generated by@nx/vite/pluginfor Vite-enabled projects only- Non-Vite projects in the included scope (
packages/**/**/*,e2e/**/**/*,tools/**/**/*) may not have this target- Requires verification that non-Vite projects either skip gracefully or won't be affected by the watch command
Please verify:
- What was the previous watch target (review git diff or commit history)
- Test running
pnpm watchto confirm it works for all projects in the monorepoe2e/am-mock-api/.eslintrc.json (1)
1-30: LGTM!The ESLint configuration follows standard patterns, properly extends the root configuration, and sets up appropriate ignore patterns for the mock API project.
e2e/am-mock-api/src/app/constants.js (1)
11-66: LGTM!The authentication path definitions comprehensively cover various OAuth/OIDC flows, realm configurations, and session management endpoints needed for end-to-end testing scenarios.
e2e/am-mock-api/tsconfig.json (1)
1-13: LGTM!Standard TypeScript project configuration using composite project references pattern.
tsconfig.json (1)
79-81: LGTM!The new project reference properly integrates the mock API into the monorepo's TypeScript configuration.
| "lint": { | ||
| "options": { | ||
| "fix": true, | ||
| "ignore-path": ".eslintignore", | ||
| "args": ["**/*.ts"] | ||
| } | ||
| } |
There was a problem hiding this comment.
Lint configuration excludes JavaScript files.
The lint target only includes TypeScript files (**/*.ts), but the project contains JavaScript files (e.g., index.js, routes.auth.js, routes.resource.js, app.auth.js). These files won't be linted with the current configuration.
Apply this diff to include JavaScript files:
"lint": {
"options": {
"fix": true,
"ignore-path": ".eslintignore",
- "args": ["**/*.ts"]
+ "args": ["**/*.ts", "**/*.js"]
}
}📝 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.
| "lint": { | |
| "options": { | |
| "fix": true, | |
| "ignore-path": ".eslintignore", | |
| "args": ["**/*.ts"] | |
| } | |
| } | |
| "lint": { | |
| "options": { | |
| "fix": true, | |
| "ignore-path": ".eslintignore", | |
| "args": ["**/*.ts", "**/*.js"] | |
| } | |
| } |
🤖 Prompt for AI Agents
In e2e/am-mock-api/project.json around lines 42 to 48, the lint args only target
TypeScript files ("**/*.ts"), so JavaScript files are omitted; update the args
to include JavaScript files as well (for example use a combined glob like
"**/*.{ts,js}" or add "**/*.js" alongside "**/*.ts") so the lint target picks up
.js files (keep existing options such as "fix" and "ignore-path" unchanged).
| // Ensure all domains are added to the security cert creation | ||
| app: process.env.NODE_ENV === 'LIVE' ? 'https://sdkapp.petrov.ca' : 'http://localhost', | ||
| forgeops: 'https://default.forgeops.petrov.ca', | ||
| mock: 'http://localhost', | ||
| resource: 'http://localhost', | ||
| }; | ||
| const paths = { | ||
| am: '/am', | ||
| }; | ||
| const ports = { | ||
| app: '8443', | ||
| forgeops: '443', | ||
| mock: '9443', | ||
| resource: '9443', | ||
| }; | ||
| const realm = 'root'; | ||
| const testUsers = [ | ||
| { | ||
| // Already exists in forgeops... | ||
| pw: 'password', | ||
| un: 'sdkuser', | ||
| }, | ||
| ]; | ||
|
|
||
| /** | ||
| * The below will be composed of the above values. | ||
| * Do not edit unless you know what you're doing. | ||
| */ | ||
| let amUrl; | ||
| let amPort; | ||
|
|
||
| if (env.LIVE) { | ||
| amUrl = origins.forgeops; | ||
| amPort = ports.forgeops; | ||
| } else { | ||
| amUrl = origins.mock; | ||
| amPort = ports.mock; | ||
| } |
There was a problem hiding this comment.
Stop mixing LIVE switches—this currently routes “LIVE” traffic to the mock AM URL.
With NODE_ENV='LIVE' (and no LIVE env var) the app picks the ForgeOps origin, but AM_URL keeps pointing at origins.mock because the later branch looks only at env.LIVE. Any “real” deployment will therefore call the mock AM host, breaking live auth. Conversely, setting LIVE="false" still activates the live branch because the string is truthy. Please collapse everything to a single boolean (e.g., const isLive = String(env.LIVE ?? env.NODE_ENV).toLowerCase() === 'live';) and reuse it for origins/ports/exports so live and non-live stay in sync. Also export the parsed ForgeOps hostname so callers stop comparing against the full URL (see routes.resource.js). -->
| app.get('/resource/rest/*', wait, authorization, async (req, res) => { | ||
| if (env.NODE_ENV === 'live') { | ||
| if (req.access.actions && req.access.actions.GET) { | ||
| res.json({ message: 'Successfully retrieved resource!' }); | ||
| } else if ( | ||
| req.access.advices && | ||
| (req.access.advices.TransactionConditionAdvice || | ||
| req.access.advices.AuthenticateToServiceConditionAdvice) | ||
| ) { | ||
| res.json(req.access); | ||
| } else { | ||
| res.status(401).send(); | ||
| } | ||
| } else { | ||
| if ( | ||
| req.cookies.iPlanetDirectoryPro === 'abcd1234' && | ||
| baz.canWithdraw && | ||
| (req.headers['x-txid'] === authByTxnResponse.advices.TransactionConditionAdvice[0] || | ||
| req.headers['x-tree'] === | ||
| authByTreeResponse.advices.AuthenticateToServiceConditionAdvice[0]) | ||
| ) { | ||
| baz.canWithdraw = false; | ||
| res.json({ message: 'Successfully retrieved resource!' }); | ||
| } else { | ||
| if (req.path.includes('authz-by-txn')) { | ||
| res.json(authByTxnResponse); | ||
| } else if (req.path.includes('authz-by-tree')) { | ||
| res.json(authByTreeResponse); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
env.NODE_ENV === 'live' never hits when other code checks 'LIVE'.
Elsewhere you gate logic on 'LIVE' (uppercase). Here it’s lowercase, so LIVE requests fall through to the non-live branch, skipping policy outputs. Normalize once (e.g., reuse an isLive boolean from env.config). -->
🤖 Prompt for AI Agents
In e2e/am-mock-api/src/app/routes.resource.js around lines 159 to 189, the
environment check uses env.NODE_ENV === 'live' while other code expects 'LIVE',
causing the live branch to never run; replace this direct lowercase comparison
with a normalized check (e.g., derive an isLive boolean once from env.NODE_ENV
by uppercasing or by reusing an existing env.config.isLive) and use that boolean
here so the live branch matches the rest of the codebase.
| if (env.NODE_ENV === 'LIVE') { | ||
| authorizeApp({ | ||
| un: '9190fcce-d6d7-4473-9449-412f281f9bc6', | ||
| pw: '7fh9sj7*NP$%F6978', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Authorization call should be awaited.
The authorizeApp call is not awaited, which could lead to race conditions if the mock routes depend on the session being established before handling requests. Consider awaiting the call or documenting that the session initializes asynchronously.
Apply this diff:
if (env.NODE_ENV === 'LIVE') {
- authorizeApp({
+ await authorizeApp({
un: '9190fcce-d6d7-4473-9449-412f281f9bc6',
pw: '7fh9sj7*NP$%F6978',
});
}Note: This requires wrapping the initialization in an async IIFE or refactoring the startup logic.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In e2e/am-mock-api/src/index.js around lines 43 to 48, the authorizeApp call is
invoked but not awaited which can cause race conditions; wrap the startup in an
async IIFE (or convert the surrounding init function to async), await
authorizeApp(...) and handle potential errors (try/catch or .catch) so the mock
routes only start after authorization completes; ensure the server/router
initialization that depends on the session is executed after the awaited call.
7803632 to
2a70f3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (17)
e2e/am-mock-api/src/environments/environment.ts (1)
8-8: Fix type mismatch:productionshould befalse(boolean).The
productionproperty is set to the string'development', but based onenvironment.prod.tsit should be a boolean. This inconsistency can cause runtime errors if code performs boolean checks.Apply this diff:
- production: 'development', + production: false,e2e/am-mock-api/src/app/env.config.js (2)
11-11: Fix module type mismatch: ESM syntax requires"type": "module".The package.json specifies
"type": "commonjs", but this file uses ESM syntax (import/export). This will cause runtime failures in Node.js.Update
e2e/am-mock-api/package.json:- "type": "commonjs", + "type": "module",
21-58: Consolidate LIVE environment detection to avoid routing inconsistencies.The code mixes three forms of LIVE detection:
env.LIVE,process.env.NODE_ENV === 'LIVE', and lowercase'live'elsewhere. This causes mismatched behavior where LIVE traffic may route to mock AM URLs or vice versa.Centralize to a single boolean check:
+const isLive = String(env.LIVE || env.NODE_ENV).toLowerCase() === 'live'; const origins = { - app: process.env.NODE_ENV === 'LIVE' ? 'https://sdkapp.petrov.ca' : 'http://localhost', + app: isLive ? 'https://sdkapp.petrov.ca' : 'http://localhost', forgeops: 'https://default.forgeops.petrov.ca', mock: 'http://localhost', resource: 'http://localhost', }; let amUrl; let amPort; -if (env.LIVE) { +if (isLive) { amUrl = origins.forgeops; amPort = ports.forgeops; } else { amUrl = origins.mock; amPort = ports.mock; } export const FORGEOPS = origins.forgeops; +export const FORGEOPS_HOST = new URL(FORGEOPS).hostname;e2e/am-mock-api/src/app/routes.resource.js (5)
27-29: Fix hostname comparison bug: comparing hostname to full URL.
req.hostnamereturns a hostname (e.g.,default.forgeops.petrov.ca) whileFORGEOPSis a full URL (https://default.forgeops.petrov.ca). The comparison will always fail, breaking LIVE logic.Apply this diff:
-import { AM_URL, AM_PORT, FORGEOPS, REALM_PATH } from './env.config.js'; +import { AM_URL, AM_PORT, FORGEOPS_HOST, REALM_PATH } from './env.config.js'; - if (env.NODE_ENV === 'LIVE' && req.hostname !== FORGEOPS) { + if (env.NODE_ENV === 'LIVE' && req.hostname !== FORGEOPS_HOST) {
47-56: Add error handling and timeouts for external policy evaluation calls.Unguarded
superagentcalls can crash the server on network failures or hangs.Apply this diff:
- const response = await request + let response; + try { + response = await request .post(`${AM_URL}/json/${realms}/policies/?_action=evaluate`) .set('Content-Type', 'application/json') .set('Accept-API-Version', 'resource=2.1') .set('iPlanetDirectoryPro', session.tokenId) + .timeout({ response: 5000, deadline: 15000 }) - .send(body); + .send(body); + } catch (err) { + console.error('Policy evaluate failed:', err.message); + req.access = {}; + return res.status(502).send(); + }
71-95: Add error handling and timeouts for OAuth introspection and SSO validation.These external calls lack error handling and can crash or hang the server.
Wrap both OAuth introspection and SSO validation in try/catch blocks with timeouts:
if (req.headers.authorization) { const authHeaderArr = req.headers.authorization.split(' '); + try { response = await request .post(`${AM_URL}/oauth2/introspect`) .set('Content-Type', 'application/json') .set('iPlanetDirectoryPro', session.tokenId) .set('Accept-API-Version', 'resource=1.2') + .timeout({ response: 5000, deadline: 15000 }) .send({ token: authHeaderArr[1] }); + } catch (e) { + console.error('Introspect failed:', e.message); + return res.status(502).send(); + } } else { + try { response = await request .post(`${AM_URL}/json/sessions/?_action=validate`) .set('Content-Type', 'application/json') .set('iPlanetDirectoryPro', session.tokenId) .set('Accept-API-Version', 'resource=2.1, protocol=1.0') + .timeout({ response: 5000, deadline: 15000 }) .send({ tokenId: req.cookies.iPlanetDirectoryPro }); + } catch (e) { + console.error('SSO validate failed:', e.message); + return res.status(502).send(); + } }
121-154: Guard against missing Referer header and use proper Express API.
req.headers.referer.includes('json')will throw if the header is absent;res.send(401, ...)is deprecated in Express.Apply this diff to both occurrences (lines 121-130 and 145-154):
+ const referer = req.headers.referer || ''; if ( - req.headers['x-authenticate-response'] && req.headers['x-authenticate-response'] === 'header' && - req.headers.referer.includes('json') + referer.includes('json') ) { - res.setHeader('WWW-Authenticate', createTxnStepUpHeader(req.headers.referer)); + res.setHeader('WWW-Authenticate', createTxnStepUpHeader(referer)); - res.send(401, null); + res.sendStatus(401); } else { - res.redirect(307, createTxnStepUpUrl(req.headers.referer)); + res.redirect(307, createTxnStepUpUrl(referer)); }
159-189: Fix environment check case mismatch.Line 160 uses lowercase
'live'while other code checks for uppercase'LIVE', causing the live branch to never execute.Replace the direct string comparison with a normalized check:
- if (env.NODE_ENV === 'live') { + const isLive = String(env.LIVE || env.NODE_ENV).toLowerCase() === 'live'; + if (isLive) {e2e/am-mock-api/src/app/routes.auth.js (4)
55-55: Remove password logging to avoid leaking secrets.This logs a clear-text test password unconditionally.
Apply this diff:
-console.log(`Your user password from 'env.config' file: ${USERS[0].pw}`); +// Password logging removed to avoid leaking secrets in logs
449-475: Guard against missing Referer header.
new URL(req.get('Referer'))will throw if the header is missing or invalid.Apply this diff to all three occurrences:
if (req.url.includes('SAMLFailure')) { - const referrer = new URL(req.get('Referer')); + const refHdr = req.get('Referer') || `${req.protocol}://${req.headers.host}/`; + const referrer = new URL(refHdr);
541-546: Fix domain detection: usereq.hostnameinstead ofreq.url.
req.urlcontains the path/query string, not the hostname. The check will never see'localhost'.Apply this diff:
- const domain = req.url.includes('localhost') ? 'localhost' : 'example.com'; + const domain = req.hostname === 'localhost' ? 'localhost' : 'example.com';
585-586: Fix typo: duplicated word in error message.- res.status(406).send('Middleware additions are missing missing.'); + res.status(406).send('Middleware additions are missing.');e2e/am-mock-api/src/app/responses.js (5)
11-11: Change"type"to"module"in package.json.All source files use ESM syntax but the package is configured as CommonJS, causing parse errors.
Update
e2e/am-mock-api/package.json:- "type": "commonjs", + "type": "module",
51-68: Remove debug log and guard URL parsing.
console.log(url)is noisy;new URL(url)throws ifurlis falsy/invalid.Apply this diff:
export const createTxnStepUpUrl = (url) => { - console.log(url); - const referer = new URL(url); + const safe = url || 'http://localhost/'; + const referer = new URL(safe); const amUrl = referer.searchParams.get('amUrl');
70-86: Apply the same URL parsing resilience ascreateTxnStepUpUrl.This function has the same vulnerability:
new URL(url)throws on falsy/invalid input.Apply this diff:
export const createTreeStepUpUrl = (url) => { - const referer = new URL(url); + const safe = url || 'http://localhost/'; + const referer = new URL(safe); const amUrl = referer.searchParams.get('amUrl');
88-99: Guard against invalid URL increateTxnStepUpHeader.Unguarded
new URL(url)will throw ifurlis falsy or malformed.Apply this diff:
export const createTxnStepUpHeader = (url) => { - const referer = new URL(url); + const safe = url || 'http://localhost/'; + const referer = new URL(safe); const amUrl = referer.searchParams.get('amUrl') || AM_URL;
101-112: Guard against invalid URL increateTreeStepUpHeader.Same issue: unguarded
new URL(url)will throw for falsy/malformed input.Apply this diff:
export const createTreeStepUpHeader = (url) => { - const referer = new URL(url); + const safe = url || 'http://localhost/'; + const referer = new URL(safe); const amUrl = referer.searchParams.get('amUrl') || AM_URL;
🧹 Nitpick comments (2)
e2e/am-mock-api/package.json (1)
10-18: Consider moving @types/express to devDependencies.Type definitions for Express are only needed at build/compilation time, not at runtime. Moving @types/express to a devDependencies section (if one is added) will keep the production dependency tree cleaner and avoid installing unused types in production environments.
If a devDependencies section is added to this package.json, apply this diff:
"dependencies": { - "@types/express": "^4.17.17", "body-parser": "^2.2.0", "cookie-parser": "^1.4.7", "cors": "^2.8.5", "express": "^4.21.2", "superagent": "^10.2.3", "uuid": "^13.0.0" + }, + "devDependencies": { + "@types/express": "^4.17.17" }packages/journey-client/src/lib/client.store.test.ts (1)
241-298: Good test coverage for baseUrl normalization.The test suite properly validates the normalization behavior across different baseUrl configurations (without trailing slash, with trailing slash, and without context path). All assertions correctly verify the constructed URLs.
Note: The AI summary mentions "The same 'baseUrl normalization' test suite appears duplicated, resulting in two identical test blocks," but only one describe block is present in the provided code.
Optional refinement: Consider parameterizing these structurally identical tests using
test.eachto reduce duplication and improve maintainability:describe('baseUrl normalization', () => { - test('should add trailing slash to baseUrl without one', async () => { - const configWithoutSlash: JourneyClientConfig = { - serverConfig: { - baseUrl: 'http://localhost:9443/am', - }, - realmPath: 'root', - }; - - const mockStepResponse: Step = { authId: 'test-auth-id', callbacks: [] }; - mockFetch.mockResolvedValue(new Response(JSON.stringify(mockStepResponse))); - - const client = await journey({ config: configWithoutSlash }); - await client.start(); - - expect(mockFetch).toHaveBeenCalledTimes(1); - const request = mockFetch.mock.calls[0][0] as Request; - expect(request.url).toBe('http://localhost:9443/am/json/realms/root/authenticate'); - }); - - test('should preserve trailing slash if already present', async () => { - const configWithSlash: JourneyClientConfig = { - serverConfig: { - baseUrl: 'http://localhost:9443/am/', - }, - realmPath: 'root', - }; - - const mockStepResponse: Step = { authId: 'test-auth-id', callbacks: [] }; - mockFetch.mockResolvedValue(new Response(JSON.stringify(mockStepResponse))); - - const client = await journey({ config: configWithSlash }); - await client.start(); - - expect(mockFetch).toHaveBeenCalledTimes(1); - const request = mockFetch.mock.calls[0][0] as Request; - expect(request.url).toBe('http://localhost:9443/am/json/realms/root/authenticate'); - }); - - test('should work with baseUrl without context path', async () => { - const configNoContext: JourneyClientConfig = { - serverConfig: { - baseUrl: 'http://localhost:9443', - }, - realmPath: 'root', - }; - - const mockStepResponse: Step = { authId: 'test-auth-id', callbacks: [] }; - mockFetch.mockResolvedValue(new Response(JSON.stringify(mockStepResponse))); - - const client = await journey({ config: configNoContext }); - await client.start(); - - expect(mockFetch).toHaveBeenCalledTimes(1); - const request = mockFetch.mock.calls[0][0] as Request; - expect(request.url).toBe('http://localhost:9443/json/realms/root/authenticate'); - }); + test.each([ + { + name: 'should add trailing slash to baseUrl without one', + baseUrl: 'http://localhost:9443/am', + expected: 'http://localhost:9443/am/json/realms/root/authenticate', + }, + { + name: 'should preserve trailing slash if already present', + baseUrl: 'http://localhost:9443/am/', + expected: 'http://localhost:9443/am/json/realms/root/authenticate', + }, + { + name: 'should work with baseUrl without context path', + baseUrl: 'http://localhost:9443', + expected: 'http://localhost:9443/json/realms/root/authenticate', + }, + ])('$name', async ({ baseUrl, expected }) => { + const config: JourneyClientConfig = { + serverConfig: { baseUrl }, + realmPath: 'root', + }; + + const mockStepResponse: Step = { authId: 'test-auth-id', callbacks: [] }; + mockFetch.mockResolvedValue(new Response(JSON.stringify(mockStepResponse))); + + const client = await journey({ config }); + await client.start(); + + expect(mockFetch).toHaveBeenCalledTimes(1); + const request = mockFetch.mock.calls[0][0] as Request; + expect(request.url).toBe(expected); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
.node-version(1 hunks)e2e/am-mock-api/.babelrc(1 hunks)e2e/am-mock-api/.eslintrc.json(1 hunks)e2e/am-mock-api/package.json(1 hunks)e2e/am-mock-api/project.json(1 hunks)e2e/am-mock-api/src/app/app.auth.js(1 hunks)e2e/am-mock-api/src/app/constants.js(1 hunks)e2e/am-mock-api/src/app/env.config.js(1 hunks)e2e/am-mock-api/src/app/response.registration.js(1 hunks)e2e/am-mock-api/src/app/responses.js(1 hunks)e2e/am-mock-api/src/app/routes.auth.js(1 hunks)e2e/am-mock-api/src/app/routes.resource.js(1 hunks)e2e/am-mock-api/src/app/wait.js(1 hunks)e2e/am-mock-api/src/environments/environment.prod.ts(1 hunks)e2e/am-mock-api/src/environments/environment.ts(1 hunks)e2e/am-mock-api/src/index.js(1 hunks)e2e/am-mock-api/tsconfig.app.json(1 hunks)e2e/am-mock-api/tsconfig.json(1 hunks)e2e/am-mock-api/tsconfig.spec.json(1 hunks)e2e/davinci-suites/src/phone-number-field.test.ts(1 hunks)e2e/journey-app/main.ts(3 hunks)e2e/journey-app/server-configs.ts(1 hunks)e2e/journey-suites/playwright.config.ts(1 hunks)e2e/journey-suites/src/login.test.ts(1 hunks)package.json(2 hunks)packages/journey-client/src/lib/client.store.test.ts(1 hunks)packages/journey-client/src/lib/client.store.ts(2 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- e2e/am-mock-api/.eslintrc.json
- .node-version
🚧 Files skipped from review as they are similar to previous changes (17)
- e2e/am-mock-api/tsconfig.json
- e2e/am-mock-api/tsconfig.app.json
- e2e/am-mock-api/src/app/constants.js
- e2e/am-mock-api/src/app/app.auth.js
- e2e/am-mock-api/src/app/wait.js
- e2e/am-mock-api/project.json
- package.json
- e2e/journey-app/server-configs.ts
- e2e/davinci-suites/src/phone-number-field.test.ts
- e2e/am-mock-api/tsconfig.spec.json
- e2e/journey-app/main.ts
- packages/journey-client/src/lib/client.store.ts
- tsconfig.json
- e2e/am-mock-api/src/index.js
- e2e/am-mock-api/.babelrc
- e2e/am-mock-api/src/environments/environment.prod.ts
- e2e/journey-suites/src/login.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T17:55:33.855Z
Learnt from: cerebrl
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: e2e/journey-app/main.ts:83-87
Timestamp: 2025-10-27T17:55:33.855Z
Learning: In the e2e/journey-app and similar e2e test applications in this repository, XSS hardening and similar production security practices are not required since these are exclusively test/development applications.
Applied to files:
packages/journey-client/src/lib/client.store.test.ts
🧬 Code graph analysis (6)
packages/journey-client/src/lib/client.store.test.ts (2)
packages/journey-client/src/lib/config.types.ts (1)
JourneyClientConfig(11-15)packages/journey-client/src/lib/client.store.ts (1)
journey(45-194)
e2e/am-mock-api/src/environments/environment.ts (1)
e2e/am-mock-api/src/environments/environment.prod.ts (1)
environment(1-3)
e2e/am-mock-api/src/app/env.config.js (1)
e2e/am-mock-api/src/app/responses.js (6)
realm(96-96)realm(109-109)amUrl(55-55)amUrl(73-73)amUrl(91-91)amUrl(104-104)
e2e/am-mock-api/src/app/routes.resource.js (5)
e2e/am-mock-api/src/app/env.config.js (8)
FORGEOPS(67-67)FORGEOPS(67-67)AM_PORT(61-61)AM_PORT(61-61)REALM_PATH(68-68)REALM_PATH(68-68)AM_URL(64-64)AM_URL(64-64)e2e/am-mock-api/src/app/app.auth.js (3)
response(18-24)session(14-14)session(14-14)e2e/am-mock-api/src/app/wait.js (1)
wait(13-15)e2e/am-mock-api/src/app/routes.auth.js (2)
baz(57-59)baz(57-59)e2e/am-mock-api/src/app/responses.js (12)
authByTxnResponse(124-132)authByTxnResponse(124-132)createTxnStepUpHeader(88-99)createTxnStepUpHeader(88-99)createTxnStepUpUrl(51-68)createTxnStepUpUrl(51-68)createTreeStepUpHeader(101-112)createTreeStepUpHeader(101-112)createTreeStepUpUrl(70-86)createTreeStepUpUrl(70-86)authByTreeResponse(114-122)authByTreeResponse(114-122)
e2e/am-mock-api/src/app/responses.js (2)
e2e/am-mock-api/src/app/routes.auth.js (5)
url(547-547)redirectUrl(453-455)redirectUrl(461-463)redirectUrl(470-472)redirectUrl(513-513)e2e/am-mock-api/src/app/env.config.js (6)
amUrl(49-49)AM_URL(64-64)AM_URL(64-64)RESOURCE_URL(69-69)RESOURCE_URL(69-69)realm(36-36)
e2e/am-mock-api/src/app/routes.auth.js (4)
e2e/am-mock-api/src/app/env.config.js (4)
USERS(71-71)USERS(71-71)AM_URL(64-64)AM_URL(64-64)e2e/am-mock-api/src/app/constants.js (2)
authPaths(11-66)authPaths(11-66)e2e/am-mock-api/src/app/wait.js (1)
wait(13-15)e2e/am-mock-api/src/app/responses.js (66)
txnAuthz(640-651)txnAuthz(640-651)treeAuthz(652-663)treeAuthz(652-663)initialMiscCallbacks(266-275)initialMiscCallbacks(266-275)initialPlatformLogin(236-264)initialPlatformLogin(236-264)authSuccess(45-49)authSuccess(45-49)initialLoginWithEmailResponse(217-234)initialLoginWithEmailResponse(217-234)nameCallback(166-176)nameCallback(166-176)pingProtectInitialize(313-368)pingProtectInitialize(313-368)selectIdPCallback(439-485)selectIdPCallback(439-485)MetadataMarketPlaceInitialize(1205-1250)MetadataMarketPlaceInitialize(1205-1250)initialBasicLogin(198-215)initialBasicLogin(198-215)recaptchaEnterpriseCallback(1310-1349)recaptchaEnterpriseCallback(1310-1349)authFail(40-43)authFail(40-43)MetadataMarketPlacePingOneEvaluation(1252-1308)MetadataMarketPlacePingOneEvaluation(1252-1308)otpQRCodeCallbacks(500-584)otpQRCodeCallbacks(500-584)redirectCallbackFailureSaml(622-639)redirectCallbackFailureSaml(622-639)redirectCallbackSaml(604-621)redirectCallbackSaml(604-621)textInputCallback(177-196)textInputCallback(177-196)passwordCallback(277-286)passwordCallback(277-286)messageCallback(400-422)messageCallback(400-422)pollingCallback(426-437)pollingCallback(426-437)secondFactorChoiceCallback(385-398)secondFactorChoiceCallback(385-398)secondFactorCallback(487-498)secondFactorCallback(487-498)redirectCallback(586-603)redirectCallback(586-603)pingProtectEvaluate(288-311)pingProtectEvaluate(288-311)noSessionSuccess(424-424)noSessionSuccess(424-424)oauthTokens(13-20)oauthTokens(13-20)oauthTokensExpiringSoon(22-29)oauthTokensExpiringSoon(22-29)oauthTokensExpired(31-38)oauthTokensExpired(31-38)redirectUrl(57-57)redirectUrl(75-75)userInfo(665-671)userInfo(665-671)wellKnownForgeRock(702-1033)wellKnownForgeRock(702-1033)newPiWellKnown(1124-1203)newPiWellKnown(1124-1203)
🪛 Gitleaks (8.29.0)
e2e/am-mock-api/src/app/routes.auth.js
[high] 194-194: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (3)
e2e/am-mock-api/package.json (1)
11-11: Good catch on the previous fix—@types/express is now correctly aligned with Express v4.The @types/express version has been corrected to v4.17.17, which properly matches the installed Express v4.21.2 runtime. This resolves the prior critical incompatibility issue.
e2e/journey-suites/playwright.config.ts (1)
40-46: Based on the verification results, all integration points between the mock API server, journey-app configuration, and Playwright setup are correctly aligned:
- The @nx/js:node executor runs the output of a build target, so
pnpm nx serve am-mock-apiwill correctly build and start the mock API server- Journey-app is configured to use the correct endpoint at
http://localhost:9443/am- The Playwright webServer configuration for port 9443 matches the expected endpoint
- The
reuseExistingServer: !process.env.CIpattern prevents server conflicts between local development and CI environmentsThe
ignoreHTTPSErrors: truesetting is unnecessary for an HTTP endpoint but harmless—it's only required for HTTPS certificate errors.Port conflict resolved; mock API integration verified.
The port conflict from the previous review has been successfully addressed. The am-mock-api server now runs on port 9443 while journey-app remains on 5829, and journey-app is correctly configured to communicate with the mock API at
http://localhost:9443/am.Note: The
ignoreHTTPSErrors: trueflag is unnecessary since the endpoint uses HTTP rather than HTTPS. Consider removing it or adding a comment explaining if this is intentional for future HTTPS support.e2e/am-mock-api/src/app/response.registration.js (1)
11-224: LGTM!The registration mock response is well-structured and provides comprehensive coverage of the registration flow with appropriate callback types, validation policies, and prompts for e2e testing.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/journey-suites/src/login.test.ts (1)
10-10: Minor: Unrelated import order change.The import order was changed from
{ username, password }to{ password, username }. This appears unrelated to the main refactoring and doesn't affect functionality.If this was an accidental change, consider reverting to maintain consistency with other test files (e.g.,
choice-confirm-poll.test.tsline 10 andno-session.test.tsline 10 still use{ username, password }). Otherwise, consider applying the same alphabetical ordering across all test files for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e/journey-suites/src/choice-confirm-poll.test.ts(2 hunks)e2e/journey-suites/src/custom-paths.test.ts(1 hunks)e2e/journey-suites/src/device-profile.test.ts(1 hunks)e2e/journey-suites/src/email-suspend.test.ts(1 hunks)e2e/journey-suites/src/login.test.ts(1 hunks)e2e/journey-suites/src/no-session.test.ts(2 hunks)e2e/journey-suites/src/otp-register.test.ts(1 hunks)e2e/journey-suites/src/protect.test.ts(1 hunks)e2e/journey-suites/src/registration.test.ts(2 hunks)e2e/journey-suites/src/request-middleware.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
e2e/journey-suites/src/no-session.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/journey-suites/src/registration.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/journey-suites/src/custom-paths.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/journey-suites/src/protect.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/journey-suites/src/email-suspend.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/journey-suites/src/device-profile.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/journey-suites/src/choice-confirm-poll.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/journey-suites/src/login.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (2)
asyncEvents(7-76)navigate(54-56)
e2e/journey-suites/src/request-middleware.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (10)
e2e/journey-suites/src/otp-register.test.ts (1)
14-14: Tree-based navigation aligns with mock APISwitching the OTP registration flow to
tree=TEST_OTPRegistrationkeeps this journey in sync with the new AM mock API routing. Looks good.e2e/journey-suites/src/registration.test.ts (1)
14-26: Consistent tree routing updateMatching the navigate call and URL assertion to
tree=Registrationkeeps this flow aligned with the tree-based journeys introduced by the mock API. Looks solid.e2e/journey-suites/src/request-middleware.test.ts (1)
14-36: Tree parameter covers middleware flowUpdating both the navigation URL and assertion to
tree=Loginkeeps this middleware scenario pointed at the correct journey under the new mock API structure. Good to go.e2e/journey-suites/src/email-suspend.test.ts (1)
14-14: Email suspend journey hooked to tree routeThe shift to
tree=TEST_LoginSuspendEmailkeeps this test aligned with the tree-based journeys. Looks correct.e2e/journey-suites/src/protect.test.ts (1)
14-14: Protect flow routed via tree parameterUsing
tree=TEST_Protectkeeps this skipped protect scenario consistent with the migrated routing. No issues spotted.e2e/journey-suites/src/device-profile.test.ts (1)
14-14: Device profile journey now uses tree routingPointing the device profile test at
tree=TEST_DeviceProfilekeeps it in sync with the rest of the tree-based migrations. Looks good.e2e/journey-suites/src/choice-confirm-poll.test.ts (1)
14-14: LGTM! Consistent parameter migration fromjourneytotree.The navigation and URL assertion have been updated consistently to use the
treequery parameter instead ofjourney, aligning with the mock API migration.Also applies to: 24-24
e2e/journey-suites/src/custom-paths.test.ts (1)
15-15: Parameter change looks good, but verify the skip status.The navigation parameter change from
journeytotreeis consistent with the broader refactoring.However, this test remains skipped with a comment stating "Skipping test until AM Mock API is available that supports custom paths" (line 12). Given that this PR is adding the mock API, please verify whether:
- The custom paths feature is intentionally not yet supported in the new mock API
- The test should now be enabled, or
- The skip comment should be updated to clarify the timeline
e2e/journey-suites/src/login.test.ts (1)
14-14: LGTM! Navigation updated to usetreeparameter.The navigation change from
/?journey=Loginto/?tree=Loginis consistent with the mock API migration applied across other test files.e2e/journey-suites/src/no-session.test.ts (1)
14-14: LGTM! Consistent parameter migration fromjourneytotree.Both the navigation path and URL assertion have been updated consistently to use the
treequery parameter instead ofjourney, maintaining theno-session=trueparameter alongside it.Also applies to: 24-24
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/journey-app/main.ts (1)
21-21: Consider a more descriptive variable name to address the 'tree' vs 'journey' terminology concern.Based on the past review comments, there's a preference for using 'journey' terminology instead of 'tree' since 'tree' is outdated. While
journeyconflicts with the imported function, alternative names likejourneyName,authTree, orflowNamecould address both the namespace conflict and the terminology preference.For example:
-const tree = searchParams.get('tree') ?? 'UsernamePassword'; +const journeyName = searchParams.get('tree') ?? 'UsernamePassword';Then update references accordingly throughout the file (lines 59, 86).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/journey-app/main.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/journey-app/main.ts (1)
packages/journey-client/src/lib/client.store.ts (1)
journey(45-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (1)
e2e/journey-app/main.ts (1)
85-88: LGTM! Logout restart logic correctly preserves journey state.The logout flow now properly restarts the journey using the same
treeparameter and preserves thenoSessionquery parameter, ensuring consistent behavior after logout. This mirrors the initial start call and maintains proper state management.
|
|
||
| (async () => { | ||
| const journeyClient = await journey({ config, requestMiddleware }); | ||
| const journeyClient = await journey({ config: config, requestMiddleware }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Restore property shorthand syntax.
The change from config to config: config is unnecessary and verbose. Modern JavaScript/TypeScript supports property shorthand notation, which is more concise and idiomatic.
Apply this diff:
- const journeyClient = await journey({ config: config, requestMiddleware });
+ const journeyClient = await journey({ config, requestMiddleware });📝 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.
| const journeyClient = await journey({ config: config, requestMiddleware }); | |
| const journeyClient = await journey({ config, requestMiddleware }); |
🤖 Prompt for AI Agents
In e2e/journey-app/main.ts around line 52, the object passed to journey uses the
verbose property form "config: config" instead of the shorthand; change the call
to use property shorthand by passing "config" (i.e., replace the verbose
property with the concise "config") while keeping requestMiddleware as-is so the
call becomes journey({ config, requestMiddleware }).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
migrate old mock api to repo chore: fix-tests
ryanbas21
force-pushed
the
migrate-am-mock-api
branch
from
54dfc81 to
bc3da80
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (15)
e2e/am-mock-api/src/app/app.auth.js (1)
14-36: Previously flagged issues remain unaddressed.The mutable module-level
sessionstate (line 14) and the catch block that swallows errors (lines 31-35) were already identified in previous reviews. Please address those concerns before merging.e2e/am-mock-api/src/app/env.config.js (1)
11-71: Previously flagged critical issues remain unaddressed.The module type mismatch (ESM syntax with CommonJS package.json) and inconsistent LIVE environment detection (line 22 uses
NODE_ENV, line 52 usesenv.LIVE) were already identified as critical issues in previous reviews. Please resolve these before merging.e2e/am-mock-api/src/app/wait.js (1)
13-15: Module system mismatch still present.The ES6 export syntax (
export default) remains incompatible with CommonJS module system. This critical issue was previously flagged but appears unresolved.e2e/am-mock-api/src/app/routes.resource.js (5)
27-29: Host comparison bug: comparing hostname to full URL.
req.hostnamereturns a hostname likedefault.forgeops.petrov.ca, whileFORGEOPScontains a full URL likehttps://default.forgeops.petrov.ca. This comparison will always fail, breaking the LIVE logic.
47-56: Add error handling and timeouts for external policy evaluation call.The
superagentPOST to AM_URL is unguarded and can throw on network failures or hang indefinitely, crashing the server.
71-88: Add error handling and timeouts for OAuth introspection.The OAuth introspection call lacks timeout and error handling, which can crash or hang the server on network failures.
121-130: Guard against missing Referer header and use proper 401 API.
req.headers.referer.includes('json')will throw if the Referer header is absent. Additionally,res.send(401, ...)is deprecated Express syntax.
159-189: Environment check uses lowercase 'live' instead of 'LIVE'.The code checks
env.NODE_ENV === 'live'(lowercase) while other parts of the codebase use'LIVE'(uppercase), causing the live branch to never execute.e2e/am-mock-api/src/app/routes.auth.js (3)
55-55: Remove password logging to avoid exposing secrets.This console.log prints a clear-text password from the test configuration, which should not be logged even in test environments.
449-475: Guard against missing Referer header in accounts route.
new URL(req.get('Referer'))will throw if the Referer header is absent or invalid across all three branches (SAMLFailure, SAMLTest, and default).
585-586: Fix duplicated word in error message.The error message contains "missing missing" - remove the duplicate word.
e2e/am-mock-api/src/app/responses.js (4)
51-58: Remove debug log and guard against invalid URL input.
console.log(url)adds noise, andnew URL(url)will throw ifurlis falsy or malformed. Add defensive URL parsing.
70-86: Guard against invalid URL in createTreeStepUpUrl.
new URL(url)can throw for falsy or invalid input. Apply the same defensive pattern as suggested forcreateTxnStepUpUrl.
88-99: Guard against invalid URL in createTxnStepUpHeader.
new URL(url)is unguarded and will throw for falsy or malformed input.
101-112: Guard against invalid URL in createTreeStepUpHeader.
new URL(url)will throw ifurlis falsy or malformed. Add try/catch or validate input before parsing.
🧹 Nitpick comments (2)
e2e/am-mock-api/.eslintrc.json (1)
1-30: Simplify the ESLint configuration structure.The configuration has some unconventional patterns that could be simplified:
- Line 3: The top-level
ignorePatterns: ["!**/*"](negation pattern) is confusing and contradicts the override's explicit ignore patterns. This pattern typically isn't used at the top level.- Lines 17–28: The three overrides with empty rule sets (
"rules": {}) for different file patterns are redundant and don't enforce any rules.Since this is a test/dev mock API project, a permissive linting baseline is appropriate. However, the configuration could be clearer and more maintainable by removing unnecessary overrides and the confusing negation pattern.
Here's a simplified alternative:
{ "extends": ["../../.eslintrc.json"], "overrides": [ { "files": ["**/*"], "ignorePatterns": [ "node_modules", "*.md", "LICENSE", ".babelrc", ".env*", ".bin", "dist", ".eslintignore" ] } ] }Alternatively, if no additional rules or ignores are needed beyond the parent config, you could simplify to:
{ "extends": ["../../.eslintrc.json"] }e2e/journey-app/server-configs.ts (1)
10-21: Good approach addressing the previous feedback.The addition of the separate
tenantconfiguration preserves the original remote test tenant while allowingbasicto use the local mock API. This addresses cerebrl's concern about replacing the shared test tenant.Consider renaming these configurations for better clarity:
basic→mock(orlocal) andtenant→remote(or keeptenant). This would make the purpose of each configuration immediately clear to developers.Example refactor:
export const serverConfigs: Record<string, JourneyClientConfig> = { - basic: { + mock: { serverConfig: { baseUrl: 'http://localhost:9443/am', }, realmPath: 'root', }, - tenant: { + remote: { serverConfig: { baseUrl: 'https://openam-sdks.forgeblocks.com/am', }, realmPath: 'alpha', }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
.node-version(1 hunks)e2e/am-mock-api/.babelrc(1 hunks)e2e/am-mock-api/.eslintrc.json(1 hunks)e2e/am-mock-api/package.json(1 hunks)e2e/am-mock-api/project.json(1 hunks)e2e/am-mock-api/src/app/app.auth.js(1 hunks)e2e/am-mock-api/src/app/constants.js(1 hunks)e2e/am-mock-api/src/app/env.config.js(1 hunks)e2e/am-mock-api/src/app/response.registration.js(1 hunks)e2e/am-mock-api/src/app/responses.js(1 hunks)e2e/am-mock-api/src/app/routes.auth.js(1 hunks)e2e/am-mock-api/src/app/routes.resource.js(1 hunks)e2e/am-mock-api/src/app/wait.js(1 hunks)e2e/am-mock-api/src/environments/environment.prod.ts(1 hunks)e2e/am-mock-api/src/environments/environment.ts(1 hunks)e2e/am-mock-api/src/index.js(1 hunks)e2e/am-mock-api/tsconfig.app.json(1 hunks)e2e/am-mock-api/tsconfig.json(1 hunks)e2e/am-mock-api/tsconfig.spec.json(1 hunks)e2e/davinci-suites/src/phone-number-field.test.ts(1 hunks)e2e/journey-app/main.ts(3 hunks)e2e/journey-app/server-configs.ts(1 hunks)e2e/journey-suites/playwright.config.ts(1 hunks)e2e/journey-suites/src/choice-confirm-poll.test.ts(2 hunks)e2e/journey-suites/src/custom-paths.test.ts(1 hunks)e2e/journey-suites/src/device-profile.test.ts(1 hunks)e2e/journey-suites/src/email-suspend.test.ts(1 hunks)e2e/journey-suites/src/login.test.ts(1 hunks)e2e/journey-suites/src/no-session.test.ts(2 hunks)e2e/journey-suites/src/otp-register.test.ts(1 hunks)e2e/journey-suites/src/protect.test.ts(1 hunks)e2e/journey-suites/src/registration.test.ts(2 hunks)e2e/journey-suites/src/request-middleware.test.ts(2 hunks)e2e/journey-suites/src/utils/demo-user.ts(1 hunks)package.json(2 hunks)packages/journey-client/src/lib/client.store.test.ts(1 hunks)packages/journey-client/src/lib/client.store.ts(2 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/am-mock-api/tsconfig.app.json
🚧 Files skipped from review as they are similar to previous changes (14)
- e2e/journey-suites/src/choice-confirm-poll.test.ts
- e2e/journey-suites/src/email-suspend.test.ts
- e2e/am-mock-api/src/environments/environment.ts
- e2e/journey-suites/src/login.test.ts
- packages/journey-client/src/lib/client.store.ts
- e2e/journey-suites/src/no-session.test.ts
- packages/journey-client/src/lib/client.store.test.ts
- e2e/am-mock-api/package.json
- e2e/am-mock-api/src/index.js
- e2e/am-mock-api/tsconfig.json
- e2e/journey-app/main.ts
- e2e/am-mock-api/project.json
- .node-version
- e2e/am-mock-api/src/environments/environment.prod.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: cerebrl
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: e2e/journey-app/main.ts:83-87
Timestamp: 2025-10-27T17:55:33.855Z
Learning: In the e2e/journey-app and similar e2e test applications in this repository, XSS hardening and similar production security practices are not required since these are exclusively test/development applications.
📚 Learning: 2025-10-28T14:44:26.883Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 427
File: .github/workflows/ci-fork.yml:50-56
Timestamp: 2025-10-28T14:44:26.883Z
Learning: Nx CLI accepts multiple targets with the short `-t` flag as space-separated tokens (e.g., `nx affected -t lint test build`) and also supports comma-separated values with the long form `--targets=` (e.g., `nx affected --targets=lint,test,build`). Both syntaxes are valid.
Applied to files:
package.json
🧬 Code graph analysis (11)
e2e/journey-suites/src/request-middleware.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/journey-suites/src/utils/demo-user.ts (1)
e2e/am-mock-api/src/app/routes.auth.js (1)
username(119-119)
e2e/journey-suites/src/device-profile.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/am-mock-api/src/app/routes.auth.js (4)
e2e/am-mock-api/src/app/env.config.js (4)
USERS(71-71)USERS(71-71)AM_URL(64-64)AM_URL(64-64)e2e/am-mock-api/src/app/constants.js (2)
authPaths(11-66)authPaths(11-66)e2e/am-mock-api/src/app/wait.js (1)
wait(13-15)e2e/am-mock-api/src/app/responses.js (36)
initialPlatformLogin(236-264)initialPlatformLogin(236-264)authSuccess(45-49)authSuccess(45-49)nameCallback(166-176)nameCallback(166-176)MetadataMarketPlaceInitialize(1205-1250)MetadataMarketPlaceInitialize(1205-1250)initialBasicLogin(198-215)initialBasicLogin(198-215)recaptchaEnterpriseCallback(1310-1349)recaptchaEnterpriseCallback(1310-1349)authFail(40-43)authFail(40-43)MetadataMarketPlacePingOneEvaluation(1252-1308)MetadataMarketPlacePingOneEvaluation(1252-1308)otpQRCodeCallbacks(500-584)otpQRCodeCallbacks(500-584)redirectCallbackFailureSaml(622-639)redirectCallbackFailureSaml(622-639)redirectCallbackSaml(604-621)redirectCallbackSaml(604-621)redirectCallback(586-603)redirectCallback(586-603)noSessionSuccess(424-424)noSessionSuccess(424-424)oauthTokens(13-20)oauthTokens(13-20)oauthTokensExpiringSoon(22-29)oauthTokensExpiringSoon(22-29)oauthTokensExpired(31-38)oauthTokensExpired(31-38)redirectUrl(57-57)redirectUrl(75-75)userInfo(665-671)userInfo(665-671)
e2e/journey-suites/src/custom-paths.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/am-mock-api/src/app/app.auth.js (2)
e2e/am-mock-api/src/app/routes.resource.js (2)
response(47-55)response(70-70)e2e/am-mock-api/src/app/env.config.js (4)
AM_URL(64-64)AM_URL(64-64)REALM_PATH(68-68)REALM_PATH(68-68)
e2e/journey-suites/src/registration.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/journey-suites/src/protect.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
navigate(54-56)
e2e/am-mock-api/src/app/env.config.js (1)
e2e/am-mock-api/src/app/responses.js (6)
realm(96-96)realm(109-109)amUrl(55-55)amUrl(73-73)amUrl(91-91)amUrl(104-104)
e2e/am-mock-api/src/app/routes.resource.js (5)
e2e/am-mock-api/src/app/env.config.js (8)
FORGEOPS(67-67)FORGEOPS(67-67)AM_PORT(61-61)AM_PORT(61-61)REALM_PATH(68-68)REALM_PATH(68-68)AM_URL(64-64)AM_URL(64-64)e2e/am-mock-api/src/app/app.auth.js (3)
response(18-24)session(14-14)session(14-14)e2e/am-mock-api/src/app/wait.js (1)
wait(13-15)e2e/am-mock-api/src/app/routes.auth.js (2)
baz(57-59)baz(57-59)e2e/am-mock-api/src/app/responses.js (12)
authByTxnResponse(124-132)authByTxnResponse(124-132)createTxnStepUpHeader(88-99)createTxnStepUpHeader(88-99)createTxnStepUpUrl(51-68)createTxnStepUpUrl(51-68)createTreeStepUpHeader(101-112)createTreeStepUpHeader(101-112)createTreeStepUpUrl(70-86)createTreeStepUpUrl(70-86)authByTreeResponse(114-122)authByTreeResponse(114-122)
e2e/am-mock-api/src/app/responses.js (2)
e2e/am-mock-api/src/app/routes.auth.js (5)
url(547-547)redirectUrl(453-455)redirectUrl(461-463)redirectUrl(470-472)redirectUrl(513-513)e2e/am-mock-api/src/app/env.config.js (6)
amUrl(49-49)AM_URL(64-64)AM_URL(64-64)RESOURCE_URL(69-69)RESOURCE_URL(69-69)realm(36-36)
🪛 Gitleaks (8.29.0)
e2e/am-mock-api/src/app/routes.auth.js
[high] 194-194: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (15)
e2e/davinci-suites/src/phone-number-field.test.ts (1)
109-111: Verify that includingtoPass()is intentional and clarify whether line 66 needs the same retry logic.The
toPass()pattern is established in the codebase (also used inotp-register.test.tsline 29) for handling flaky E2E tests. However, your previous review indicated this was debugging code to be removed. If this retry logic is now intentional for the SMS/Voice MFA check, clarify whether the EMAIL MFA assertion on line 66 should also be wrapped withtoPass()for consistency, or if the simpler test flow justifies the difference.e2e/am-mock-api/.babelrc (1)
1-3: LGTM! Standard Nx Babel configuration.The Babel preset and useBuiltIns configuration are appropriate for the new e2e mock API project.
e2e/am-mock-api/tsconfig.spec.json (1)
1-9: LGTM! Standard TypeScript test configuration.The test-specific compiler options and include patterns are appropriate for the e2e mock API test suite.
package.json (1)
37-37: LGTM! Appropriate updates for the new mock API.The watch script refinement to
vite:watch-depsand the addition of@nx/expressdevDependency align well with the Express-based mock API infrastructure being introduced.Also applies to: 64-64
e2e/journey-suites/src/utils/demo-user.ts (1)
7-8: LGTM! Test credentials aligned with mock API.The updated credentials (
sdkuser/password) match the test user configuration in the mock API (e2e/am-mock-api/src/app/env.config.jsline 41). The simplified password is appropriate for the e2e test environment.e2e/am-mock-api/src/app/constants.js (1)
11-66: LGTM! Well-organized endpoint configuration.The authPaths structure clearly organizes mock endpoints by type. This static configuration is appropriate for driving the mock API routing logic.
e2e/journey-suites/src/registration.test.ts (1)
14-14: LGTM! URL routing updated consistently.The navigation and assertion updates from
journey=totree=parameter withclientId=tenantare consistent with the broader routing changes across the journey test suite.Also applies to: 26-26
e2e/journey-suites/src/request-middleware.test.ts (1)
14-14: LGTM! Query parameter migration is consistent.The navigation URL and corresponding assertion have been properly updated from
journey=Logintotree=Login, maintaining consistency throughout the test.Also applies to: 36-36
e2e/journey-suites/src/protect.test.ts (1)
14-14: LGTM! Parameter migration is correct.The navigation URL has been correctly updated from
journey=TEST_Protecttotree=TEST_Protect.e2e/journey-suites/src/otp-register.test.ts (1)
14-14: LGTM! Parameter migration with clientId addition is correct.The navigation URL has been properly updated from
journey=TEST_OTPRegistrationtotree=TEST_OTPRegistration&clientId=tenant.tsconfig.json (1)
78-81: LGTM! Project reference properly added.The new am-mock-api project has been correctly added to the TypeScript workspace references.
e2e/journey-suites/src/custom-paths.test.ts (1)
15-15: LGTM! Parameter migration with clientId addition is correct.The navigation URL has been properly updated from
journey=Logintotree=Login&clientId=tenant. Note that the test remains skipped pending custom paths support in the mock API.e2e/journey-suites/src/device-profile.test.ts (1)
14-14: LGTM! Parameter migration is correct.The navigation URL has been correctly updated from
journey=TEST_DeviceProfiletotree=TEST_DeviceProfile.e2e/journey-suites/playwright.config.ts (1)
40-46: LGTM! Mock API server properly configured.The am-mock-api server is correctly configured on port 9443, resolving the previous port conflict issue. The configuration properly mirrors the journey-app setup with appropriate settings for HTTPS and server reuse.
e2e/am-mock-api/src/app/response.registration.js (1)
11-224: LGTM! Well-structured registration flow mock.The mock registration response provides comprehensive callback coverage for e2e testing, including username validation, attribute inputs, password policies, KBA questions, and T&C acceptance.
| test('Test happy paths on test page', async ({ page }) => { | ||
| const { clickButton, navigate } = asyncEvents(page); | ||
| await navigate('/?journey=TEST_LoginWithMiscCallbacks'); | ||
| await navigate('/?tree=TEST_LoginWithMiscCallbacks&clientId=tenant'); |
There was a problem hiding this comment.
Is there a reason we changed all of these over to tree from journey? I'd like to retire the use of "tree".
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I left two comments, but they aren't blocking.
| await expect( | ||
| async () => await expect(page.getByText('SMS/Voice MFA Registered')).toBeVisible(), | ||
| ).toPass(); |
There was a problem hiding this comment.
It seems this wasn't removed. Can you remove it?
ryanbas21
force-pushed
the
migrate-am-mock-api
branch
from
466a530 to
afda1da
Compare
ryanbas21
force-pushed
the
migrate-am-mock-api
branch
from
afda1da to
5dd2420
Compare
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4460
Description
migrate old mock api to repo
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores