Closed
Conversation
Adds apps/mock-saml-idp, a multi-tenant SAML 2.0 Identity Provider mock
mirroring apps/mock-oauth-server. Each tenant has its own RSA keypair
and self-signed cert generated at startup, so one mock service can back
many SamlConnection rows in tests and exercise per-connection isolation.
Uses samlify deliberately because the upcoming backend SAML wrapper will
use @node-saml/node-saml. Different libraries on each side means a bug
in either library's signature canonicalization surfaces as a test
failure instead of being masked by both sides agreeing.
Endpoints:
- GET /idp/:tenant/metadata IdP metadata XML
- GET /idp/:tenant/sso AuthnRequest receiver, renders login form
- POST /idp/:tenant/login builds and auto-POSTs signed assertion
- POST /idp/:tenant/test-controls queues misbehaviors (bad-signature,
expired, wrong-audience, replay, etc.)
- GET /idp introspection
Also adds @node-saml/node-saml to apps/backend deps for the upcoming
backend SAML protocol wrapper.
Three smaller pieces that unlock e2e testing: - .github/workflows/e2e-api-tests.yaml: starts mock-saml-idp on port 8115 alongside mock-oauth-server, with /idp as the readiness probe. Root package.json adds start:mock-saml-idp script and includes the mock in dev:basic. - apps/e2e/tests/snapshot-serializer.ts: strips SAMLRequest / SAMLResponse / RelayState query+form params, adds stack-saml-inner- to keyed cookie name prefixes (so the per-AuthnRequest CSRF cookie doesn't reroll snapshots), and adds regex replacements for SAML xs:ID identifiers and IssueInstant/NotBefore/NotOnOrAfter timestamps. - apps/backend/src/lib/seed-dummy-data.ts: STACK_SEED_ENABLE_SAML=true pre-creates acme + globex SAML connections on the dummy project, fetching the IdP metadata from the running mock at seed time so the seeded cert matches what the mock generated at startup. The mock regenerates keys per restart, so re-seed if you restart it. Mock URL configurable via STACK_MOCK_SAML_URL (default localhost:8115).
Deep dot-keys like `auth.saml.connections.X.field` get dropped by config normalization with onDotIntoNonObject=ignore when the parent record entry doesn't yet exist. Match the existing convention from auth.oauth.providers and write the whole connection entry as a single value. (Bug surfaced when running the SAML e2e tests against a live backend in a separate PR. Applied here so the seed function works on its own without requiring downstream PRs.)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis pull request introduces a new mock SAML 2.0 Identity Provider service with multi-tenant support. It includes a standalone Express-based IDP server, updates the E2E workflow to launch it with health checks, extends snapshot serialization to redact SAML-specific data, and integrates the service into development tooling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant MockSAMLIdP as Mock SAML IdP
participant TestSuite as E2E Test Suite
Client->>MockSAMLIdP: GET /idp/:tenant/sso?SAMLRequest=...
MockSAMLIdP->>MockSAMLIdP: Decode SAMLRequest
MockSAMLIdP-->>Client: Render login form (HTML)
Client->>MockSAMLIdP: POST /idp/:tenant/login (credentials)
MockSAMLIdP->>MockSAMLIdP: Check for queued misbehavior
alt Misbehavior Active
MockSAMLIdP->>MockSAMLIdP: Apply assertion modifications (e.g., expired)
end
MockSAMLIdP->>MockSAMLIdP: Generate signed SAML Response
MockSAMLIdP-->>Client: POST form with SAML Response
Client->>TestSuite: Submit SAML Response
TestSuite->>TestSuite: Verify assertion (timestamp, audience, etc.)
Client->>MockSAMLIdP: GET /idp/:tenant/test-controls (enqueue misbehavior)
MockSAMLIdP->>MockSAMLIdP: Queue next assertion modification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
This was referenced Apr 29, 2026
…ml-idp mock-saml-idp originally depended on body-parser for the parser middleware, but switched to using express.urlencoded()/express.json() directly. The package.json dep was removed but the lockfile entry remained, breaking 'pnpm install --frozen-lockfile' in CI.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/src/lib/seed-dummy-data.ts (1)
2072-2134:⚠️ Potential issue | 🟠 MajorAvoid parallel environment override writes for the same project/branch.
seedSamlConnections(projectId)and the directoverrideEnvironmentConfigOverridecall both run in parallel withinPromise.allat lines 2127–2134. Both perform read-modify-write operations on the same environment config override. The implementation (line 491) already has a TODO acknowledging this: "put this in a serializable transaction (or a single SQL query) to prevent race conditions". When SAML is enabled, concurrent reads of the environment config can each see the stale state, apply their respective changes, and the second write will clobber the first—losing either the SAML config or thepayments.testModeflag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/seed-dummy-data.ts` around lines 2072 - 2134, Concurrent environment writes in the Promise.all block can race: calls to seedSamlConnections(projectId) and overrideEnvironmentConfigOverride both perform read-modify-write on the same environment override; instead serialize them by removing seedSamlConnections(projectId) from the parallel array and await it before (or after) calling overrideEnvironmentConfigOverride, or merge the SAML-related changes into the single overrideEnvironmentConfigOverride call so only one read-modify-write occurs; update the Promise.all usage around Promise.all([...overrideBranchConfigOverride(...), /* remove seedSamlConnections from here */ overrideEnvironmentConfigOverride(...), ...]) and call await seedSamlConnections(projectId) in sequence to prevent clobbering.
🧹 Nitpick comments (2)
apps/mock-saml-idp/src/index.ts (1)
88-94: Encode tenant slugs when generating mock IdP URLs.Slugs from
STACK_MOCK_SAML_TENANTSare directly interpolated into URLs. If a slug contains reserved URL characters like/,?, or#, the metadata and SSO URLs will be malformed and tenant routing will fail. Per coding guidelines, useencodeURIComponent()to encode the path segment.♻️ Suggested change
function entityIdFor(slug: string): string { - return `http://localhost:${port}/idp/${slug}/metadata`; + const encodedSlug = encodeURIComponent(slug); + return `http://localhost:${port}/idp/${encodedSlug}/metadata`; } function ssoUrlFor(slug: string): string { - return `http://localhost:${port}/idp/${slug}/sso`; + const encodedSlug = encodeURIComponent(slug); + return `http://localhost:${port}/idp/${encodedSlug}/sso`; }Also applies to line 432.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mock-saml-idp/src/index.ts` around lines 88 - 94, The entityIdFor and ssoUrlFor functions currently interpolate raw slug values into URLs, which breaks when slugs contain reserved URL characters; update both functions (and the similar occurrence around the symbol referenced at line ~432) to encode the slug using encodeURIComponent() before inserting it into the path so metadata and SSO URLs are always valid and safe for routing.apps/backend/src/lib/seed-dummy-data.ts (1)
1986-2002: ⚡ Quick winRemove the cast by typing
overlaywith the target parameter type.The
as Parameters<...>cast bypasses type safety and is avoidable here.Suggested change
- const overlay: Record<string, unknown> = {}; + const overlay: Parameters<typeof overrideEnvironmentConfigOverride>[0]['environmentConfigOverrideOverride'] = {}; @@ await overrideEnvironmentConfigOverride({ projectId, branchId: DEFAULT_BRANCH_ID, - environmentConfigOverrideOverride: overlay as Parameters<typeof overrideEnvironmentConfigOverride>[0]["environmentConfigOverrideOverride"], + environmentConfigOverrideOverride: overlay, });As per coding guidelines, "Do NOT use
as/any/type casts or anything else like that to bypass the type system."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/seed-dummy-data.ts` around lines 1986 - 2002, The overlay object is being cast with a broad as-cast when passed to overrideEnvironmentConfigOverride; instead declare overlay with the exact parameter type so you don’t bypass the type system. Replace the current const overlay: Record<string, unknown> = {} with a declaration using Parameters<typeof overrideEnvironmentConfigOverride>[0]["environmentConfigOverrideOverride"] as the type (e.g., const overlay: Parameters<typeof overrideEnvironmentConfigOverride>[0]["environmentConfigOverrideOverride"] = {}), keep building properties from fetched (f.slug, f.displayName, etc.) and then call overrideEnvironmentConfigOverride(projectId, DEFAULT_BRANCH_ID, { environmentConfigOverrideOverride: overlay }) without the as cast. Ensure the property shapes you assign match that parameter type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/lib/seed-dummy-data.ts`:
- Around line 1958-1961: The fetch to `${mockUrl}/idp/${t.slug}/metadata` can
hang and uses an unescaped slug; update the call in seed-dummy-data (where
`mockUrl`, `t.slug` and `fetch` are used) to build the URL safely with
encodeURIComponent(t.slug) (or urlString`` helper) and wrap the fetch with an
AbortController timeout: create an AbortController, pass controller.signal to
fetch, set a timer to controller.abort() after a short bound (e.g. 5s), and
clear the timer after the response; ensure the existing error handling still
throws when res.ok is false and that aborted fetches surface as errors.
In `@apps/e2e/tests/snapshot-serializer.ts`:
- Around line 116-121: The snapshots still include POST-bound SAML hidden inputs
because only stripUrlQueryParams is removing those keys from URLs; update the
snapshot serializer to also redact form input values named "SAMLRequest",
"SAMLResponse", and "RelayState" when serializing HTML forms: extend the
existing HTML/form sanitization logic (where stripUrlQueryParams is used) to
detect <input> elements (hidden or otherwise) with those name attributes and
replace their value with a stable placeholder, and apply the same change to the
other similar block referenced around the second occurrence (the lines analogous
to 134-140) so both URL and form payloads are sanitized consistently.
In `@apps/mock-saml-idp/package.json`:
- Around line 14-24: The lockfile wasn't updated to reflect the new workspace
manifest, leaving the old specifier for body-parser@^1.20.3 in pnpm-lock.yaml
and breaking CI; run a root workspace install (pnpm install or pnpm -w install)
so pnpm regenerates pnpm-lock.yaml with the correct specifiers for the new
mock-saml-idp package, verify pnpm-lock.yaml now reflects the updated
dependencies, and commit the updated pnpm-lock.yaml alongside the package.json
change.
In `@apps/mock-saml-idp/src/index.ts`:
- Around line 417-423: The handler currently casts req.body to Misbehavior and
assigns it to t.nextMisbehavior without validating fields, which lets invalid
payloads through; update the /test-controls handler in index.ts to explicitly
validate that req.body.kind is one of the allowed Misbehavior discriminants
(e.g., the union of valid string kinds), then perform per-kind validation of
required fields for that variant (reject null or missing fields), avoid using
`as`/any casts, and only assign a properly narrowed/validated object to
t.nextMisbehavior; on validation failure return res.status(400).json({ error:
"..." }) with a clear message.
---
Outside diff comments:
In `@apps/backend/src/lib/seed-dummy-data.ts`:
- Around line 2072-2134: Concurrent environment writes in the Promise.all block
can race: calls to seedSamlConnections(projectId) and
overrideEnvironmentConfigOverride both perform read-modify-write on the same
environment override; instead serialize them by removing
seedSamlConnections(projectId) from the parallel array and await it before (or
after) calling overrideEnvironmentConfigOverride, or merge the SAML-related
changes into the single overrideEnvironmentConfigOverride call so only one
read-modify-write occurs; update the Promise.all usage around
Promise.all([...overrideBranchConfigOverride(...), /* remove seedSamlConnections
from here */ overrideEnvironmentConfigOverride(...), ...]) and call await
seedSamlConnections(projectId) in sequence to prevent clobbering.
---
Nitpick comments:
In `@apps/backend/src/lib/seed-dummy-data.ts`:
- Around line 1986-2002: The overlay object is being cast with a broad as-cast
when passed to overrideEnvironmentConfigOverride; instead declare overlay with
the exact parameter type so you don’t bypass the type system. Replace the
current const overlay: Record<string, unknown> = {} with a declaration using
Parameters<typeof
overrideEnvironmentConfigOverride>[0]["environmentConfigOverrideOverride"] as
the type (e.g., const overlay: Parameters<typeof
overrideEnvironmentConfigOverride>[0]["environmentConfigOverrideOverride"] =
{}), keep building properties from fetched (f.slug, f.displayName, etc.) and
then call overrideEnvironmentConfigOverride(projectId, DEFAULT_BRANCH_ID, {
environmentConfigOverrideOverride: overlay }) without the as cast. Ensure the
property shapes you assign match that parameter type.
In `@apps/mock-saml-idp/src/index.ts`:
- Around line 88-94: The entityIdFor and ssoUrlFor functions currently
interpolate raw slug values into URLs, which breaks when slugs contain reserved
URL characters; update both functions (and the similar occurrence around the
symbol referenced at line ~432) to encode the slug using encodeURIComponent()
before inserting it into the path so metadata and SSO URLs are always valid and
safe for routing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e34fa459-9761-47db-ab1d-53af2b88916a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/e2e-api-tests.yamlapps/backend/package.jsonapps/backend/src/lib/seed-dummy-data.tsapps/e2e/tests/snapshot-serializer.tsapps/mock-saml-idp/.eslintrc.jsapps/mock-saml-idp/package.jsonapps/mock-saml-idp/src/index.tsapps/mock-saml-idp/tsconfig.jsonpackage.json
The pattern \`/_[a-zA-Z][a-zA-Z0-9_.-]{8,}/g\` matched any SCREAMING_SNAKE_CASE
identifier with an underscore followed by 9+ chars — e.g.
\`STRIPE_ACCOUNT_INFO_NOT_FOUND\` became \`STRIPE<stripped SAML id>\`,
breaking unrelated Stripe / payments / OAuth e2e snapshots.
The regex isn't load-bearing today: no current SAML test snapshots a
random AuthnRequest / Response / Assertion ID. Cookie-name SAML IDs
are already covered by \`keyedCookieNamePrefixes\` ("stack-saml-inner-"),
URL path segments only carry the deterministic connection_id, and no
test snapshots raw SAML XML. If a future test ever does, follow the
precedent of the timestamp strip on the next line and anchor the
replacement to specific XML attributes (\`ID="..."\`, \`InResponseTo="..."\`)
rather than matching loose \`_<chars>\` strings everywhere.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/e2e/tests/snapshot-serializer.ts (1)
116-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPOST-bound SAML form payloads are still not redacted.
stripUrlQueryParamsonly applies inside URL parsing (Line 201), so hidden form inputs likeSAMLResponse/RelayStateremain dynamic and can still reroll snapshots.Suggested fix
const stringRegexReplacements = [ [/(\/integrations\/(neon|custom)\/oauth\/idp\/(interaction|auth)\/)[a-zA-Z0-9_-]+/gi, "$1<stripped $3 UID>"], [new RegExp(`localhost\:${getPortPrefix()}`, "gi"), "localhost:<$$NEXT_PUBLIC_STACK_PORT_PREFIX>"], [new RegExp(`localhost\%3A${getPortPrefix()}`, "gi"), "localhost%3A%3C%24NEXT_PUBLIC_STACK_PORT_PREFIX%3E"], [/(Timeout exceeded: elapsed )[0-9.]+( ms)/gi, "$1<stripped time>$2"], + [/(name="(?:SAMLRequest|SAMLResponse|RelayState)"\s+value=")[^"]*(")/g, '$1<stripped SAML form value>$2'], // SAML XML timestamps (e.g. IssueInstant, NotBefore, NotOnOrAfter). [/(IssueInstant|NotBefore|NotOnOrAfter)="[^"]+"/g, '$1="<stripped SAML timestamp>"'], ] as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/snapshot-serializer.ts` around lines 116 - 121, The serializer currently redacts SAML params only from URLs via stripUrlQueryParams, leaving POST-bound hidden form fields like SAMLResponse and RelayState dynamic; update the snapshot serializer to also detect and replace values of hidden form inputs (names "SAMLRequest", "SAMLResponse", "RelayState") during HTML serialization so snapshots are stable. Locate the HTML form handling/serialization logic in this file (where stripUrlQueryParams is defined/used) and add a step that parses form input elements (or runs a regex over <input ... type="hidden" name="...">) to replace their value attributes with a fixed placeholder, applying the same redaction list already used for URLs. Ensure the change targets those exact parameter names so other inputs remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/e2e/tests/snapshot-serializer.ts`:
- Around line 116-121: The serializer currently redacts SAML params only from
URLs via stripUrlQueryParams, leaving POST-bound hidden form fields like
SAMLResponse and RelayState dynamic; update the snapshot serializer to also
detect and replace values of hidden form inputs (names "SAMLRequest",
"SAMLResponse", "RelayState") during HTML serialization so snapshots are stable.
Locate the HTML form handling/serialization logic in this file (where
stripUrlQueryParams is defined/used) and add a step that parses form input
elements (or runs a regex over <input ... type="hidden" name="...">) to replace
their value attributes with a fixed placeholder, applying the same redaction
list already used for URLs. Ensure the change targets those exact parameter
names so other inputs remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 405857f8-f34a-440a-9a44-bf90befa0e6d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
apps/e2e/tests/snapshot-serializer.ts
Two cleanups in seed-dummy-data, both flagged on PR #1395: - The parallel `Promise.all` block ran `seedSamlConnections(projectId)` alongside another `overrideEnvironmentConfigOverride` write on the same project (`payments.testMode`). Both are read-modify-write, so concurrent reads of the env config can each see the stale state and the second write clobbers the first — the existing TODO at `config.ts:491` already documents the underlying race. Sequence the SAML seed after the parallel block to avoid the race until the override is wrapped in a serializable txn. - Type the SAML overlay with the target parameter type directly instead of using an `as Parameters<...>` cast, per project style.
`${keyedCookieNamePrefixes}` interpolated the entire array, which was
harmless when the array had one entry but produced
"stack-oauth-inner-,stack-saml-inner-" once a second prefix was added,
breaking every existing OAuth set-cookie snapshot.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/e2e/tests/snapshot-serializer.ts (1)
116-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPOST-bound SAML form values are still not redacted.
stripUrlQueryParamsonly helps when SAML fields appear in URLs. The mock IdP auto-post page carriesSAMLRequest/SAMLResponse/RelayStatein hidden form inputs, so snapshots can still churn.Suggested patch
const stringRegexReplacements = [ [/(\/integrations\/(neon|custom)\/oauth\/idp\/(interaction|auth)\/)[a-zA-Z0-9_-]+/gi, "$1<stripped $3 UID>"], [new RegExp(`localhost\:${getPortPrefix()}`, "gi"), "localhost:<$$NEXT_PUBLIC_STACK_PORT_PREFIX>"], [new RegExp(`localhost\%3A${getPortPrefix()}`, "gi"), "localhost%3A%3C%24NEXT_PUBLIC_STACK_PORT_PREFIX%3E"], [/(Timeout exceeded: elapsed )[0-9.]+( ms)/gi, "$1<stripped time>$2"], + [/(name="(?:SAMLRequest|SAMLResponse|RelayState)"\s+value=")[^"]*(")/g, '$1<stripped SAML form value>$2'], [/(IssueInstant|NotBefore|NotOnOrAfter)="[^"]+"/g, '$1="<stripped SAML timestamp>"'], ] as const;Also applies to: 134-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/snapshot-serializer.ts` around lines 116 - 121, The SAML form hidden inputs (names "SAMLRequest", "SAMLResponse", "RelayState") are not being redacted because stripUrlQueryParams only handles URLs; update the snapshot serializer that handles HTML output (the same code that currently calls stripUrlQueryParams) to also scan form inputs and replace value attributes for inputs whose name matches "SAMLRequest" | "SAMLResponse" | "RelayState" with a stable placeholder (same placeholder used for URL params), handling single/double quotes and variations (hidden/text/textarea), and apply the same change for the other occurrence referenced around lines 134-135 so POST-bound SAML values are consistently redacted from snapshots.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/e2e/tests/snapshot-serializer.ts`:
- Around line 116-121: The SAML form hidden inputs (names "SAMLRequest",
"SAMLResponse", "RelayState") are not being redacted because stripUrlQueryParams
only handles URLs; update the snapshot serializer that handles HTML output (the
same code that currently calls stripUrlQueryParams) to also scan form inputs and
replace value attributes for inputs whose name matches "SAMLRequest" |
"SAMLResponse" | "RelayState" with a stable placeholder (same placeholder used
for URL params), handling single/double quotes and variations
(hidden/text/textarea), and apply the same change for the other occurrence
referenced around lines 134-135 so POST-bound SAML values are consistently
redacted from snapshots.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4efbfe4e-a8de-4328-b6fc-896d7ea762f8
📒 Files selected for processing (1)
apps/e2e/tests/snapshot-serializer.ts
Collaborator
|
@greptile-ai review |
Contributor
Greptile SummaryThis PR introduces a standalone mock SAML 2.0 Identity Provider ( Confidence Score: 5/5Safe to merge — all findings are P2 style/quality issues with no impact on test correctness or production systems. No P0 or P1 defects found. The two P2 notes (wrong @types/express major version, stale tsconfig template settings) don't affect runtime behavior since typecheck reportedly passes. The snapshot-serializer cookie fix is a genuine improvement. apps/mock-saml-idp/package.json (@types/express version) and apps/mock-saml-idp/tsconfig.json (Next.js-derived lib/jsx settings). Important Files Changed
Sequence DiagramsequenceDiagram
participant SP as Service Provider (Backend)
participant IdP as Mock SAML IdP (:8142)
participant Browser
SP->>Browser: Redirect to /idp/:tenant/sso?SAMLRequest=...
Browser->>IdP: GET /idp/:tenant/sso?SAMLRequest=&RelayState=
IdP->>IdP: parseAuthnRequestRedirect (inflate + extract fields)
IdP->>Browser: Render login form (email input, hidden SAMLRequest)
opt E2E test misbehavior
Browser->>IdP: POST /idp/:tenant/test-controls {kind: bad-signature|expired|...}
IdP->>IdP: Set nextMisbehavior for tenant
end
Browser->>IdP: POST /idp/:tenant/login (email, SAMLRequest, RelayState)
IdP->>IdP: consumeNextMisbehavior → buildAssertion → renderLoginResponseXml
IdP->>IdP: samlify.createLoginResponse (signs + base64-encodes)
IdP->>Browser: Render auto-POST form (SAMLResponse, RelayState, ACS URL)
Browser->>SP: POST ACS URL (SAMLResponse=..., RelayState=...)
SP->>SP: Validate assertion (via @node-saml/node-saml)
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/mock-saml-idp/package.json:120
**`@types/express` v5 types used with Express v4 runtime**
`express` is pinned to `^4.21.2` (Express 4), but `@types/express` is `^5.0.0` (Express 5 types). The two major versions have different API signatures — for example, Express 5's `req.query` and async error-handling differ from v4. Using v5 type declarations against a v4 runtime means TypeScript may accept code that behaves differently at runtime, or flag valid v4 patterns as errors.
```suggestion
"@types/express": "^4.17.21",
```
### Issue 2 of 3
apps/mock-saml-idp/tsconfig.json:645-651
**`dom` lib and `jsx` setting inappropriate for a Node.js Express server**
This `tsconfig.json` is copied from a Next.js template. `"lib": ["dom", "dom.iterable", "esnext"]` exposes browser-global types (`window`, `document`, etc.) to a Node.js server, meaning TypeScript won't catch accidental use of browser-only APIs. `"jsx": "preserve"` is irrelevant in a plain Express service with no JSX. Consider using `"lib": ["esnext"]` and removing the `"jsx"` field entirely.
### Issue 3 of 3
apps/mock-saml-idp/src/index.ts:421-430
**User-supplied email embedded verbatim into SAML XML without escaping**
`user.email` (submitted by the browser via the login form) is inserted directly into the XML response template via string replacement. An email containing XML meta-characters such as `<`, `>`, `"`, or `&` (e.g. `alice@test.com&foo=bar`) will produce malformed XML that samlify signs but the SP parser may reject with an opaque parse error. For a test mock the input is controlled, but it makes failure modes hard to diagnose. Consider HTML-encoding the value before substitution.
Reviews (2): Last reviewed commit: "chore(backend): defer SAML seed + node-s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
mantrakp04
requested changes
Apr 30, 2026
- Move @types/express and @types/node-forge to devDependencies — they're compile-time only. - Drop the next.js ESLint extend (this is a plain Express service, not Next). Rename .eslintrc.js to .eslintrc.cjs to match the convention used by every other workspace package. - Add --ext .tsx,.ts to the lint script (required when only the defaults config is used, since the bare typescript-eslint parser doesn't pick up .ts/.tsx by default; matches apps/e2e and packages/stack-shared).
The mock SAML IdP previously used port suffix 15, which is also bound by
examples/supabase. Under \`pnpm dev:basic\` / \`dev:full\` whichever started
second failed to bind. Suffixes 01–41 are otherwise spoken for; 42 is the
first free slot before the LocalStack reservation at 50–99.
- Default port → \`\${prefix}42\`
- \`pnpm kms\` cleanup list grows to include 42
- e2e CI health-check URL updated to \`http://localhost:8142/idp\`
- Add a dev launchpad tile so the SAML mock is discoverable next to the
OAuth mock
Two related changes in apps/mock-saml-idp/src/index.ts: 1. Replay misbehavior now re-emits the original RelayState alongside the cached SAMLResponse. Previously buildAssertion returned the cached SAMLResponse but the login handler still rendered the form with the *current* request's RelayState, which tests "old response + fresh state" rather than a true replay. AssertionResult now carries relayState so the handler uses whatever the assertion path returned. 2. Split the 100-line buildAssertion into focused helpers: consumeNextMisbehavior, resolveSigningTenant, buildAssertionFields, renderLoginResponseXml, cacheReplayableResponse. Same behavior; the replay short-circuit and the cache update are now obvious at a glance. Also updates the file header to clarify that the @node-saml/node-saml dependency it pairs with lands in the stacked backend PR — this PR ships the mock alone.
Removes seedSamlConnections (and its STACK_SEED_ENABLE_SAML callsite) plus the @node-saml/node-saml dependency from this PR. Both depend on config.auth.saml schema entries that don't exist on this branch yet — the seed wrote overrides that were silently dropped during config normalization, and node-saml had no consumer here. They land together in the stacked backend PR alongside the schema and the SAML protocol wrapper that actually imports node-saml.
mantrakp04
approved these changes
May 1, 2026
Collaborator
Author
|
closing for now, handling this post launch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of 3 stacked PRs adding SAML 2.0 SSO. Stacked above: [#PR_BACKEND] (backend feature) → [#PR_CLIENT] (client + tests).
What this PR adds
apps/mock-saml-idp/— standalone Bun/Express service that acts as a multi-tenant SAML 2.0 Identity Provider for e2e tests and local dev. Listens on port 8115. Each tenant has its own RSA keypair + self-signed cert generated at startup, so one mock service can back many SamlConnection rows.Why a separate service (and not just a stub in the test file)
The mock uses `samlify` deliberately because the upcoming backend SAML wrapper uses `@node-saml/node-saml`. Different libraries on each side mean a bug in either library's signature canonicalization surfaces as a test failure rather than canceling out (the backend would otherwise be its own test oracle, which is no oracle at all).
Endpoints
Test plan
The mock IdP is independently useful — it can back SAML testing for any future feature, not just the SSO work in the next PR.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores