feat(e2e): contractor domain — scenarios + spec rewrites

[jeffredodd]

Summary

• Add contractor-onboarding and contractor-payment scenarios
• Rewrite contractor spec files to use scenario fixture
• Move to e2e/tests/contractor/ directory

Videos

canary-03-payment-create-submit-PASSED.webm

canary-02-onboarding-business-PASSED.webm

canary-01-onboarding-individual-PASSED.webm

Base: PR #1829 (e2e/infrastructure)

Notion tasks: #74-079

Made with Cursor

[jeffredodd]

Changes below this comment are for the API upgrade to 11-15-2025

[boostsecurity-io-ai]

⚠️  1 New Suppressed Finding

The latest commit contains 1 new suppressed finding.

Findings
CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG) Original Rule ID: rules_lgpl_javascript_crypto_rule-node-insecure-random-generator Remediation: Replace all uses of crypto.pseudoRandomBytes() and Math.random() withcrypto.randomBytes() for any security-sensitive random number generation.The crypto.randomBytes() function uses the operating system'scryptographically secure random number generator (e.g., /dev/urandom on Linux,CryptGenRandom on Windows). For generating random integers within a specificrange, use crypto.randomInt(min, max) which is available in Node.js 14.10.0and later. When generating tokens or identifiers, consider usingcrypto.randomUUID() for RFC 4122 compliant UUIDs, or encode random bytesusing base64url encoding with randomBytes(32).toString('base64url') for URL-safe tokens. Note that crypto.pseudoRandomBytes() was deprecated in Node.jsv6.0.0 and should never be used in modern applications. The product uses a Pseudo-Random Number Generator (PRNG) in a security context, but the PRNG's algorithm is not cryptographically strong. The application uses cryptographically weak random number generators such as crypto.pseudoRandomBytes() or Math.random() for security-sensitive operations. The crypto.pseudoRandomBytes() function uses a non-cryptographic PRNG that may be...
📘 Learn More embedded-react-sdk/e2e/utils/contractorFlowDrivers.ts Line 67 in b338223 const ein = `${Math.floor(Math.random() * 89 + 10)}-${Math.floor(Math.random() * 8999999 + 1000000)}` // noboost

Scanner: boostsecurity - Semgrep

[jeffredodd]

@cursoragent can we investigate? Is this issue caused by a specific change in this branch?

⚠️  1 New Suppressed Finding

The latest commit contains 1 new suppressed finding.

Findings
CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG)
Original Rule ID: rules_lgpl_javascript_crypto_rule-node-insecure-random-generator
Remediation: Replace all uses of crypto.pseudoRandomBytes() and Math.random() withcrypto.randomBytes() for any security-sensitive random number generation.The crypto.randomBytes() function uses the operating system'scryptographically secure random number generator (e.g., /dev/urandom on Linux,CryptGenRandom on Windows). For generating random integers within a specificrange, use crypto.randomInt(min, max) which is available in Node.js 14.10.0and later. When generating tokens or identifiers, consider usingcrypto.randomUUID() for RFC 4122 compliant UUIDs, or encode random bytesusing base64url encoding with randomBytes(32).toString('base64url') for URL-safe tokens. Note that crypto.pseudoRandomBytes() was deprecated in Node.jsv6.0.0 and should never be used in modern applications.
The product uses a Pseudo-Random Number Generator (PRNG) in a security context, but the PRNG's algorithm is not cryptographically strong.

The application uses cryptographically weak random number generators such as
crypto.pseudoRandomBytes() or Math.random() for security-sensitive
operations. The crypto.pseudoRandomBytes() function uses a non-cryptographic
PRNG that may be...
 📘 Learn More

embedded-react-sdk/e2e/utils/contractorFlowDrivers.ts

Line 67 in b338223

const ein = `${Math.floor(Math.random() * 89 + 10)}-${Math.floor(Math.random() * 8999999 + 1000000)}` // noboost

Scanner: boostsecurity - Semgrep

[cursor]

Investigation Complete

I investigated the security finding and here's what I found:

Yes, this issue was caused by changes in this branch. The file e2e/utils/contractorFlowDrivers.ts was added in commit b3382233 as part of this PR, and it used Math.random() to generate test EINs (Employer Identification Numbers).

What I Fixed

I replaced all instances of Math.random() with Node's cryptographically secure crypto.randomInt() function:

1. Created reusable helpers — Added generateUniqueEIN() to e2e/utils/helpers.ts (similar to the existing generateUniqueSSN() function)
2. Updated both helpers — Converted generateUniqueSSN() to also use crypto.randomInt() for consistency
3. Replaced all usages — Updated three files that were generating EINs inline:
   • e2e/utils/contractorFlowDrivers.ts (line 67, flagged by scanner)
   • e2e/tests/contractor/contractor-business-create-lifecycle.spec.ts
   • e2e/tests/company-onboarding.spec.ts

Why This Matters

While this is E2E test code (not production), using cryptographically secure random number generation is the right approach and eliminates the security scanner warning. The crypto.randomInt() function uses the OS's secure random number generator, satisfying the CWE-338 security requirement without needing suppression comments.

The changes have been committed and pushed to the e2e/contractor-domain branch.

  

[boostsecurity-io-ai]

🚀 1 New Security Fix

You just committed 1 security fix. 😎 Keep up the great work!

🎯 Take a look at what findings you fixed.

Findings
CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG) Original Rule ID: rules_lgpl_javascript_crypto_rule-node-insecure-random-generator The product uses a Pseudo-Random Number Generator (PRNG) in a security context, but the PRNG's algorithm is not cryptographically strong. The application uses cryptographically weak random number generators such as crypto.pseudoRandomBytes() or Math.random() for security-sensitive operations. The crypto.pseudoRandomBytes() function uses a non-cryptographic PRNG that may be...
📘 Learn More embedded-react-sdk/e2e/utils/contractorFlowDrivers.ts Line 67 in b338223 const ein = `${Math.floor(Math.random() * 89 + 10)}-${Math.floor(Math.random() * 8999999 + 1000000)}` // noboost

Scanner: boostsecurity - Semgrep

[gusto-fresh-eyes]

Fresh Eyes Review

Found 6 issues in this PR.

PR Description Issues

• Major | [fresh_eyes]: description-check: Missing Approach: No explanation of design decisions — why scenario fixtures over other patterns, why the canary/lifecycle separation, why the specific scenario groupings (e.g. why contractor-mixed-roster is shared across non-mutating specs while contractor-onboarding is used by mutating specs).
• Major | [fresh_eyes]: description-check: Missing Problem/Why: The description lists what was done (add scenarios, rewrite specs, move directory) but doesn't explain why. What motivated these changes — test reliability, speed, parallelization, or maintainability? The commit message on HEAD mentions 'consolidate contractor scenarios to speed up provisioning' which would be useful context in the description.
• Major | [fresh_eyes]: description-check: Significant undocumented changes: The summary mentions only 'contractor-onboarding and contractor-payment scenarios' but the PR also adds contractor-mixed-roster.json and full-flow-canary.json scenarios, a 342-line contractorFlowDrivers.ts utility, helpers.ts additions (generateUniqueEIN, nextBusinessDay, crypto.randomInt refactor), canary specs (3 files), 5 new lifecycle specs, and a change to company-onboarding.spec.ts. The description understates the scope.

Info (1)

• description-check: PR size: 20 files changed with well over 500 lines of meaningful additions. Consider whether the canary specs + flow drivers could be split from the scenario definitions + spec rewrites.

Download findings.json — drag the file into Claude or use /add to propose fixes

---

^{Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews}

[jeffredodd]

@cursoragent Are all the comments in this PR up to date and resolved?

[cursor]

Summary: PR Comments Review

I've reviewed and addressed all code-related comments in the PR. Here's the status:

✅ Resolved Issues

1. Security Finding (CWE-338) - Fixed in commit 7cb9adde

   • Replaced Math.random() with crypto.randomInt() in all test ID generation
   • Created reusable generateUniqueEIN() helper function
   • Updated generateUniqueSSN() to use secure random generation
   • Affected files: helpers.ts, contractorFlowDrivers.ts, contractor-business-create-lifecycle.spec.ts, company-onboarding.spec.ts

2. Unnecessary Dynamic Imports - Fixed in commit bc135e44

   • Replaced await import('../utils/helpers') with static imports
   • Added generateUniqueEIN to the top-level import statements
   • Affected files: contractor-business-create-lifecycle.spec.ts, company-onboarding.spec.ts

ℹ️ Remaining Comments (Non-blocking)

The Fresh Eyes bot has feedback about the PR description (not code):

• Incomplete scenario listing in summary
• Missing problem/why context
• Undocumented significant changes
• PR size note

These are documentation suggestions for the PR description. Would you like me to update the PR description to address this feedback, or are you satisfied with the current description?

All code issues have been resolved and pushed to the branch.

  

[jeffredodd]

@cursoragent can you resolve conflicts here and make sure we're rebased off main?

[cursor]

Perfect! I've successfully rebased the e2e/contractor-domain branch onto the latest main. Here's what was done:

Rebase Summary

Strategy: Instead of a full rebase (which caused many conflicts due to commits already in main), I cherry-picked only the contractor-domain-specific commits onto the latest main.

Result:

• ✅ Branch is now based on latest main (commit 83932d20)
• ✅ All conflicts resolved (package.json, helpers.ts)
• ✅ Branch contains 13 contractor-domain commits
• ✅ Force-pushed to remote successfully

Commits included (in order):

1. ci(e2e): discover matrix domains from e2e/tests subfolders
2. feat(e2e): contractor domain — scenarios + spec rewrites
3. fix(e2e): stabilize contractor scenario provisioning in CI
4. feat(e2e): expand contractor domain with business and mixed roster scenarios
5. fix(e2e): assert real terminal states in contractor lifecycle tests
6. feat(e2e): cover contractor onboarding state-machine branches
7. feat(e2e): cover contractor payment submit validation terminal state
8. feat(e2e): cover contractor payment create-to-summary full lifecycle
9. feat(e2e): cover contractor edit re-entry from list lifecycle
10. fix(e2e): anchor fillDate spinbutton matchers to /^month|^day|^year/
11. feat(e2e): add contractor canary suite covering all 3 contractor-domain flow paths
12. fix(e2e): replace Math.random() with crypto.randomInt() for test ID generation
13. fix(e2e): replace dynamic imports with static imports for generateUniqueEIN
14. fix(e2e): format company-onboarding imports

The branch is now clean and ready for review!

  

[serikjensen]

From claude, with a grain of salt:

1. Inconsistent field-visibility guards in flow drivers

fillIndividualProfile (line 881) and fillBusinessProfile (line 900) in contractorFlowDrivers.ts fill SSN and EIN unconditionally, but the parallel lifecycle specs guard both fields with isVisible().catch(() => false). If a flow variant ever skips those fields, the canary will throw while the lifecycle spec silently passes.

// Guard consistently with the lifecycle specs:
const ssnField = page.getByLabel(/social security number/i)
if (await ssnField.isVisible({ timeout: 5_000 }).catch(() => false)) {
  await ssnField.fill(generateUniqueSSN())
}
// Same pattern for EIN in fillBusinessProfile

2. Silent pass on empty-contractor state hides provisioning failures

In contractor-payment-create-full-lifecycle.spec.ts (line 671) and contractor-payment-submit-lifecycle.spec.ts (line 754), when the "no contractors available" heading is visible the test does a redundant expect then returns — making the test appear green when the scenario didn't provision contractors. A CI provisioning failure would go undetected.

// Surface the failure explicitly instead:
if (await empty.isVisible().catch(() => false)) {
  test.skip(true, 'No contractors provisioned — skipping payment flow')
}

---

Posted by Claude on behalf of Steve

[serikjensen]

Helpers in this file make me a little worried about how extensible this is going to be. Will every domain have a similar list of drivers?

[jeffredodd]

yeah thats a fair point. I think each domain has its own unique drivers currently. We might be able to create repeatable patterns here but right now I think AI's metric is just to get through the test and meet the need and this is what some QAs do. I don't love it either and I think we can iterate here.

[jeffredodd]

@cursoragent Can you investigate the feedback and see if we should address Steve's cursor feedback?

[cursor]

Investigation Complete ✅

I investigated Steve's feedback and both concerns are valid. I've implemented fixes for both issues.

Issue 1: Inconsistent Field Visibility Guards

Problem: The flow driver functions (fillIndividualProfile and fillBusinessProfile) filled SSN/EIN fields unconditionally, while the lifecycle specs guarded them with isVisible().catch(() => false). This meant:

• Canary tests (using drivers) would fail if fields became optional
• Lifecycle tests would silently pass

Fix Applied: Added visibility guards to both functions in contractorFlowDrivers.ts:

const ssnField = page.getByLabel(/social security number/i)
if (await ssnField.isVisible({ timeout: 5_000 }).catch(() => false)) {
  await ssnField.fill(generateUniqueSSN())
}

Issue 2: Silent Pass on Empty Contractor State ⚠️ More Critical

Problem: Both payment lifecycle specs checked for "no contractors available" and did:

if (await empty.isVisible().catch(() => false)) {
  await expect(empty).toBeVisible()  // redundant assertion
  return  // test passes green
}

This masked provisioning failures — tests appeared successful when the scenario didn't provision contractors properly.

Fix Applied: Replaced with test.skip() to surface provisioning issues:

if (await empty.isVisible().catch(() => false)) {
  test.skip(true, 'No contractors provisioned — skipping payment flow')
}

Now CI will show these as skipped (yellow) rather than passed (green), alerting us to provisioning problems.

Changes Committed

• Commit d5e4b9c3: "fix(e2e): add field guards to flow drivers and skip on provisioning failure"
• Files modified:
  • e2e/utils/contractorFlowDrivers.ts
  • e2e/tests/contractor/contractor-payment-submit-lifecycle.spec.ts
  • e2e/tests/contractor/contractor-payment-create-full-lifecycle.spec.ts

The branch has been pushed and is ready for review.

  

[gusto-fresh-eyes]

🟡 Minor | code-duplication

Payment setup sequence duplicated with contractor-payment-submit-lifecycle.spec.ts (lines 19-41 are nearly identical: goto, click new payment, empty-state skip, fill date). The PR already introduces shared helpers for this exact flow in contractorFlowDrivers.ts (landOnPaymentList, startNewPayment, setPaymentDate) but the lifecycle specs inline the logic instead of reusing them. The inlined version also uses a naive +7 day offset that may hit holidays, while the driver uses the more robust nextBusinessDay helper.

---

^{Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews}

[gusto-fresh-eyes]

🟠 Major | logic-errors

Payment date uses naive new Date() + 7 days which can land on a weekend or US holiday, causing the backend to reject it. The canary driver (contractorFlowDrivers.ts setPaymentDate) explicitly uses nextBusinessDay(new Date(), 14) to avoid this exact failure mode — its comment states: 'The demo backend rejects e.g. Memorial Day with "Mon, May 25th is not a business day"'. This spec goes all the way to submission, so a non-business date will cause intermittent CI failures (~28% chance of hitting a weekend alone).

---

^{Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews}

[gusto-fresh-eyes]

🟡 Minor | test-coverage

nextBusinessDay() contains non-trivial date logic (weekend detection, US federal holiday exclusion, UTC date advancement) with no unit test. A bug in the holiday set or the advancement loop would surface only as a flaky CI failure on a specific calendar date. Consider adding a unit test covering: offset landing on a weekend, offset landing on a holiday, and consecutive non-business days.

---

^{Please 👍🏽 👎🏽 if you found this useful. Generated by Fresh Eyes Reviewer. Get help in #ai-code-reviews}