Adding portless and refactoring mock service packages#185
Open
Adding portless and refactoring mock service packages#185
Conversation
Fixes #168
… interface exports for knip
…t.config.ts and shared config, update test description in RequireAuth tests
…h tests and adjust biome version in lockfile to fix pipeline
…and refactor render methods to utilize args
…fig to simplify includes
… vitest type checking for test files without polluting production output with test files and dev dependencies - Updated sidebars.ts to use ES module syntax for exporting sidebars. - Modified vitest.config.ts files across multiple packages to disable type checking. - Added tsconfig.vitest.json files to various packages and apps to extend base TypeScript configurations for Vitest. - Enhanced test files to use Mock types for better type safety and clarity. - Adjusted test implementations to ensure proper async handling and error propagation. - Updated turbo.json to streamline build dependencies and inputs.
…omImportMetaUrl utility and updating related imports according to sourcery suggestions
… for optional fields handling according to sourcery suggestion
…uration with dynamic API port
… config and Playwright support
…ases; add snyk exceptions with justification
… test processes; upgrade turbo to version 2.9.2
…nd script clarity chore: update sonar-project.properties to reflect new mock service package names chore: modify turbo.json to include a new dev task configuration
Contributor
Reviewer's GuideRefactors local mock services into reusable seedwork packages and dedicated apps, adopts portless-based HTTPS local development with stable ownercommunity.localhost subdomains, and hardens auth UX/flows while updating configs, scripts, and docs accordingly. Sequence diagram for RequireAuth and mock OIDC login via portlesssequenceDiagram
actor User
participant Browser
participant RequireAuth
participant OIDCClient
participant MockAuthServer as Mock_OIDC_Server
User->>Browser: Navigate to protected route
Browser->>RequireAuth: Render component
RequireAuth->>RequireAuth: useAuth() state check
alt NotAuthenticated and notHandlingCallback and forceLogin
RequireAuth->>Browser: sessionStorage.setItem("redirectTo")
RequireAuth->>OIDCClient: signinRedirect()
RequireAuth-->>Browser: Show "Redirecting to sign in..." view
OIDCClient->>MockAuthServer: GET https://mock-auth.ownercommunity.localhost/authorize
MockAuthServer-->>OIDCClient: 302 redirect with code,state to https://ownercommunity.localhost/auth-redirect
OIDCClient-->>Browser: Navigate to /auth-redirect with code,state
end
Browser->>RequireAuth: Re-render on /auth-redirect
RequireAuth->>RequireAuth: hasAuthParams() == true
RequireAuth-->>Browser: Show loading view (includes auth callback state)
OIDCClient->>MockAuthServer: POST /token (exchange code)
MockAuthServer-->>OIDCClient: id_token, access_token, refresh_token
OIDCClient-->>RequireAuth: auth.isAuthenticated = true
alt AuthenticationSuccess
RequireAuth-->>Browser: Render children (protected UI)
else AuthenticationError
RequireAuth-->>Browser: Render error view with auth.error.message
end
Class diagram for new OAuth2 and MongoDB mock seedwork and appsclassDiagram
class MockOAuth2UserProfile {
<<interface>>
+string email
+string given_name
+string family_name
+string sub
+string tid
}
class MockOAuth2ServerConfig {
<<interface>>
+number port
+string baseUrl
+string host
+Set~string~ allowedRedirectUris
+string allowedRedirectUri
+Map~string,string~ redirectUriToAudience
+getUserProfile() MockOAuth2UserProfile
}
class startMockOAuth2Server {
+startMockOAuth2Server(config MockOAuth2ServerConfig) Promise~void~
}
class MongoMemoryReplicaSetConfig {
<<interface>>
+number port
+string dbName
+string replSetName
+string binaryVersion
}
class startMongoMemoryReplicaSet {
+startMongoMemoryReplicaSet(config MongoMemoryReplicaSetConfig) Promise~MongoMemoryReplSet~
}
class ServerOAuth2MockApp {
<<module @apps/server-oauth2-mock>>
+setupEnvironment() void
+main() void
}
class ServerMongoMemoryMockApp {
<<module @apps/server-mongodb-memory-mock>>
+setupEnvironment() void
+main() void
}
class RequireAuthProps {
<<interface>>
+boolean forceLogin
+ReactNode children
}
class RequireAuth {
<<React.FC>>
+RequireAuth(props RequireAuthProps) ReactElement
}
MockOAuth2ServerConfig --> MockOAuth2UserProfile : uses
startMockOAuth2Server --> MockOAuth2ServerConfig : parameter
startMongoMemoryReplicaSet --> MongoMemoryReplicaSetConfig : parameter
ServerOAuth2MockApp --> MockOAuth2ServerConfig : builds
ServerOAuth2MockApp --> startMockOAuth2Server : calls
ServerMongoMemoryMockApp --> MongoMemoryReplicaSetConfig : builds
ServerMongoMemoryMockApp --> startMongoMemoryReplicaSet : calls
RequireAuth --> RequireAuthProps : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…ript formatting in package.json
Member
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- In
packages/cellix/server-oauth2-mock-seedwork/src/index.ts, the README states CORS is locked down to local origins, but the implementation usesAccess-Control-Allow-Origin: *; consider aligning the code with the documented restriction or updating the docs to match the actual behavior. - In
apps/api/start-dev.mjs, theNODE_EXTRA_CA_CERTSpath assumes a POSIX-styleHOMEdirectory and a fixed~/.portless/ca.pemlocation; you may want to defensively handle missingHOMEor non-Unix environments and surface a clearer error if the CA file is absent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `packages/cellix/server-oauth2-mock-seedwork/src/index.ts`, the README states CORS is locked down to local origins, but the implementation uses `Access-Control-Allow-Origin: *`; consider aligning the code with the documented restriction or updating the docs to match the actual behavior.
- In `apps/api/start-dev.mjs`, the `NODE_EXTRA_CA_CERTS` path assumes a POSIX-style `HOME` directory and a fixed `~/.portless/ca.pem` location; you may want to defensively handle missing `HOME` or non-Unix environments and surface a clearer error if the CA file is absent.
## Individual Comments
### Comment 1
<location path="packages/cellix/ui-core/src/components/molecules/require-auth/index.tsx" line_range="63-67" />
<code_context>
+ </Row>
+ );
+ }
+ if (props.forceLogin === false) {
return <Navigate to="/" />;
- } else {
- redirectUser();
- return;
}
+
+ return (
+ <Row
+ justify={'center'}
</code_context>
<issue_to_address>
**issue (bug_risk):** Unauthenticated + forceLogin undefined now leads to a spinner but never triggers sign-in
With this refactor, `signinRedirect()` is only called in the `useEffect` when `props.forceLogin === true`. When `forceLogin` is `false`, you correctly navigate to `/`. But when `forceLogin` is `undefined` (the common case), an unauthenticated user hits the final `return` that renders the “Redirecting to sign in...” spinner without ever calling `signinRedirect()`, so no redirect occurs. You could either treat `undefined` as `true` in the effect condition (e.g. `props.forceLogin !== false`) or call `signinRedirect()` explicitly in the final unauthenticated branch when `forceLogin` isn’t `false`.
</issue_to_address>
### Comment 2
<location path="packages/cellix/server-oauth2-mock-seedwork/src/index.ts" line_range="154-163" />
<code_context>
- });
-
- // Token endpoint
- app.post('/token', async (req, res) => {
- // biome-ignore lint:useLiteralKeys
- const email = process.env['Email'];
</code_context>
<issue_to_address>
**suggestion:** Refresh token handling in /token is inconsistent with the `buildTokenResponse` API
In `/token` you reject `grant_type === 'refresh_token'` with `invalid_grant`, so refresh flows are effectively unsupported. Yet `buildTokenResponse` takes an `existingRefreshToken`, and `/token` passes `refresh_token` into it, even though that value is never meaningfully used. To avoid a misleading API, consider either implementing proper refresh token support (validate the refresh token and use it in `buildTokenResponse`) or removing the `existingRefreshToken` parameter and corresponding argument from `/token` so the behavior matches the intended capabilities of the mock server.
</issue_to_address>
### Comment 3
<location path="apps/api/start-dev.mjs" line_range="10-13" />
<code_context>
+ process.exit(1);
+}
+
+const childEnv = {
+ ...process.env,
+ NODE_EXTRA_CA_CERTS: `${process.env.HOME}/.portless/ca.pem`,
+ NODE_OPTIONS: '--use-system-ca',
+};
+
</code_context>
<issue_to_address>
**suggestion:** Overwriting NODE_OPTIONS may clobber caller-provided Node flags
`childEnv` currently overwrites any existing `NODE_OPTIONS`, dropping flags like `--inspect` or memory limits that may be set by the caller. To preserve upstream configuration while still enabling system CAs, append instead of replace, e.g.
```ts
const childEnv = {
...process.env,
NODE_EXTRA_CA_CERTS: `${process.env.HOME}/.portless/ca.pem`,
NODE_OPTIONS: `${process.env.NODE_OPTIONS ?? ''} --use-system-ca`.trim(),
};
```
</issue_to_address>
### Comment 4
<location path="apps/server-oauth2-mock/package.json" line_range="12-13" />
<code_context>
+ "lint": "biome lint",
+ "format": "biome format --write",
+ "start": "node dist/index.js",
+ "dev": "portless mock-auth.ownercommunity.localhost node dist/index.js"
+ },
+ "dependencies": {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Dev script assumes a built dist, which can be surprising for local development
`dev` runs `node dist/index.js` via Portless but never ensures `dist` exists or is current. If someone runs `pnpm dev` before a build, they’ll likely get a runtime error. Consider either making `dev` run `build` first (e.g. `"dev": "pnpm run build && portless ..."`) or using a TS runner/watch build (e.g. `tsx`) to keep the dev loop smooth.
```suggestion
"start": "node dist/index.js",
"dev": "pnpm run build && portless mock-auth.ownercommunity.localhost node dist/index.js"
```
</issue_to_address>
### Comment 5
<location path="apps/server-mongodb-memory-mock/package.json" line_range="13" />
<code_context>
+ "lint": "biome lint",
+ "format": "biome format --write",
+ "start": "node dist/index.js",
+ "dev": "node dist/index.js"
+ },
+ "dependencies": {
</code_context>
<issue_to_address>
**suggestion:** Mongo memory mock dev script also relies on prebuilt JS without enforcing a build step
Because `dev` runs `node dist/index.js` without a preceding build or watch, it’s easy to run stale or missing output. Consider either chaining a build (or incremental build) before `node`, or running the TypeScript sources directly via a tool like `tsx` in watch mode to support iterative development.
Suggested implementation:
```
"start": "node dist/index.js",
"dev": "tsx watch src/index.ts"
```
```
"@cellix/config-typescript": "workspace:*",
"rimraf": "catalog:",
"typescript": "catalog:",
"tsx": "catalog:"
```
If the entrypoint is not `src/index.ts`, update the `dev` script path accordingly (e.g. `src/main.ts` or similar). Also ensure your workspace catalog defines a version for `tsx`; if not, replace `"catalog:"` with an explicit version (e.g. `"^4.19.0"`).
</issue_to_address>
### Comment 6
<location path="apps/server-oauth2-mock/README.md" line_range="3" />
<code_context>
+# Mock OAuth2 Server
+
+A lightweight OpenID Connect compliant mock OAuth2 server for local development and testing. This server simulates OAuth2/OIDC flows without requiring external authentication infrastructure.
+
+## Quick Start
</code_context>
<issue_to_address>
**nitpick (typo):** Consider hyphenating "OpenID Connect–compliant" for grammatical correctness.
Here, it should read “A lightweight OpenID Connect–compliant mock OAuth2 server…” so that “OpenID Connect” correctly modifies “compliant.”
```suggestion
A lightweight OpenID Connect–compliant mock OAuth2 server for local development and testing. This server simulates OAuth2/OIDC flows without requiring external authentication infrastructure.
```
</issue_to_address>
### Comment 7
<location path="apps/server-oauth2-mock/README.md" line_range="43" />
<code_context>
+| `FAMILY_NAME` | `User` | User family name in tokens |
+| `SUB` | *auto-generated* | User subject ID (optional - see below) |
+| `TID` | `test-tenant-id` | Tenant ID in tokens |
+| `PORT` | `4000` | Server port (dev mode only) |
+| `PORTLESS_URL` | *not set* | Portless proxy URL (set automatically in dev mode) |
+
</code_context>
<issue_to_address>
**issue:** Clarify when the `PORT` variable is used; "dev mode only" appears misleading.
The earlier "Production/Direct Mode" section states that `PORT` configures the server on `http://localhost:4000`. Labeling it as "dev mode only" here is inconsistent and may confuse readers. Please rename or adjust the description to clarify whether `PORT` applies to both dev and production modes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+63
to
+67
| if (props.forceLogin === false) { | ||
| return <Navigate to="/" />; | ||
| } else { | ||
| redirectUser(); | ||
| return; | ||
| } | ||
|
|
||
| return ( |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Unauthenticated + forceLogin undefined now leads to a spinner but never triggers sign-in
With this refactor, signinRedirect() is only called in the useEffect when props.forceLogin === true. When forceLogin is false, you correctly navigate to /. But when forceLogin is undefined (the common case), an unauthenticated user hits the final return that renders the “Redirecting to sign in...” spinner without ever calling signinRedirect(), so no redirect occurs. You could either treat undefined as true in the effect condition (e.g. props.forceLogin !== false) or call signinRedirect() explicitly in the final unauthenticated branch when forceLogin isn’t false.
Comment on lines
+154
to
+163
| app.post('/token', async (req, res) => { | ||
| const { grant_type, refresh_token, tid, code } = req.body as { | ||
| grant_type?: string; | ||
| refresh_token?: string; | ||
| tid?: string; | ||
| code?: string; | ||
| }; | ||
|
|
||
| if (grant_type === 'refresh_token') { | ||
| res.status(400).json({ error: 'invalid_grant' }); |
Contributor
There was a problem hiding this comment.
suggestion: Refresh token handling in /token is inconsistent with the buildTokenResponse API
In /token you reject grant_type === 'refresh_token' with invalid_grant, so refresh flows are effectively unsupported. Yet buildTokenResponse takes an existingRefreshToken, and /token passes refresh_token into it, even though that value is never meaningfully used. To avoid a misleading API, consider either implementing proper refresh token support (validate the refresh token and use it in buildTokenResponse) or removing the existingRefreshToken parameter and corresponding argument from /token so the behavior matches the intended capabilities of the mock server.
Comment on lines
+10
to
+13
| const childEnv = { | ||
| ...process.env, | ||
| NODE_EXTRA_CA_CERTS: `${process.env.HOME}/.portless/ca.pem`, | ||
| NODE_OPTIONS: '--use-system-ca', |
Contributor
There was a problem hiding this comment.
suggestion: Overwriting NODE_OPTIONS may clobber caller-provided Node flags
childEnv currently overwrites any existing NODE_OPTIONS, dropping flags like --inspect or memory limits that may be set by the caller. To preserve upstream configuration while still enabling system CAs, append instead of replace, e.g.
const childEnv = {
...process.env,
NODE_EXTRA_CA_CERTS: `${process.env.HOME}/.portless/ca.pem`,
NODE_OPTIONS: `${process.env.NODE_OPTIONS ?? ''} --use-system-ca`.trim(),
};
Comment on lines
+12
to
+13
| "start": "node dist/index.js", | ||
| "dev": "portless mock-auth.ownercommunity.localhost node dist/index.js" |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Dev script assumes a built dist, which can be surprising for local development
dev runs node dist/index.js via Portless but never ensures dist exists or is current. If someone runs pnpm dev before a build, they’ll likely get a runtime error. Consider either making dev run build first (e.g. "dev": "pnpm run build && portless ...") or using a TS runner/watch build (e.g. tsx) to keep the dev loop smooth.
Suggested change
| "start": "node dist/index.js", | |
| "dev": "portless mock-auth.ownercommunity.localhost node dist/index.js" | |
| "start": "node dist/index.js", | |
| "dev": "pnpm run build && portless mock-auth.ownercommunity.localhost node dist/index.js" |
| "lint": "biome lint", | ||
| "format": "biome format --write", | ||
| "start": "node dist/index.js", | ||
| "dev": "node dist/index.js" |
Contributor
There was a problem hiding this comment.
suggestion: Mongo memory mock dev script also relies on prebuilt JS without enforcing a build step
Because dev runs node dist/index.js without a preceding build or watch, it’s easy to run stale or missing output. Consider either chaining a build (or incremental build) before node, or running the TypeScript sources directly via a tool like tsx in watch mode to support iterative development.
Suggested implementation:
"start": "node dist/index.js",
"dev": "tsx watch src/index.ts"
"@cellix/config-typescript": "workspace:*",
"rimraf": "catalog:",
"typescript": "catalog:",
"tsx": "catalog:"
If the entrypoint is not src/index.ts, update the dev script path accordingly (e.g. src/main.ts or similar). Also ensure your workspace catalog defines a version for tsx; if not, replace "catalog:" with an explicit version (e.g. "^4.19.0").
| @@ -0,0 +1,191 @@ | |||
| # Mock OAuth2 Server | |||
|
|
|||
| A lightweight OpenID Connect compliant mock OAuth2 server for local development and testing. This server simulates OAuth2/OIDC flows without requiring external authentication infrastructure. | |||
Contributor
There was a problem hiding this comment.
nitpick (typo): Consider hyphenating "OpenID Connect–compliant" for grammatical correctness.
Here, it should read “A lightweight OpenID Connect–compliant mock OAuth2 server…” so that “OpenID Connect” correctly modifies “compliant.”
Suggested change
| A lightweight OpenID Connect compliant mock OAuth2 server for local development and testing. This server simulates OAuth2/OIDC flows without requiring external authentication infrastructure. | |
| A lightweight OpenID Connect–compliant mock OAuth2 server for local development and testing. This server simulates OAuth2/OIDC flows without requiring external authentication infrastructure. |
| | `FAMILY_NAME` | `User` | User family name in tokens | | ||
| | `SUB` | *auto-generated* | User subject ID (optional - see below) | | ||
| | `TID` | `test-tenant-id` | Tenant ID in tokens | | ||
| | `PORT` | `4000` | Server port (dev mode only) | |
Contributor
There was a problem hiding this comment.
issue: Clarify when the PORT variable is used; "dev mode only" appears misleading.
The earlier "Production/Direct Mode" section states that PORT configures the server on http://localhost:4000. Labeling it as "dev mode only" here is inconsistent and may confuse readers. Please rename or adjust the description to clarify whether PORT applies to both dev and production modes.
gidich
approved these changes
Apr 2, 2026
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
start-dev.mjsfor the API, theNODE_EXTRA_CA_CERTSpath is built fromprocess.env.HOME, which may not exist or behave as expected on non-Unix platforms; consider resolving the CA file usingos.homedir()or a more robust cross-platform approach. - The OAuth2 mock seedwork README claims CORS is "locked down to local origins", but the implementation sets
Access-Control-Allow-Originto*; either tighten the CORS logic to match the description or update the docs to reflect the current behavior. - In the mock OAuth2 token endpoint,
grant_type === 'refresh_token'is explicitly rejected even thoughbuildTokenResponsecarries through an existingrefresh_token; if refresh tokens are intentionally unsupported, consider making that explicit in the config/README or remove the unused parameter handling to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `start-dev.mjs` for the API, the `NODE_EXTRA_CA_CERTS` path is built from `process.env.HOME`, which may not exist or behave as expected on non-Unix platforms; consider resolving the CA file using `os.homedir()` or a more robust cross-platform approach.
- The OAuth2 mock seedwork README claims CORS is "locked down to local origins", but the implementation sets `Access-Control-Allow-Origin` to `*`; either tighten the CORS logic to match the description or update the docs to reflect the current behavior.
- In the mock OAuth2 token endpoint, `grant_type === 'refresh_token'` is explicitly rejected even though `buildTokenResponse` carries through an existing `refresh_token`; if refresh tokens are intentionally unsupported, consider making that explicit in the config/README or remove the unused parameter handling to avoid confusion.
## Individual Comments
### Comment 1
<location path="packages/cellix/ui-core/src/components/molecules/require-auth/index.tsx" line_range="63-68" />
<code_context>
+ </Row>
+ );
+ }
+ if (props.forceLogin === false) {
return <Navigate to="/" />;
- } else {
- redirectUser();
- return;
}
+
+ return (
+ <Row
+ justify={'center'}
+ style={{ height: '100vh', alignItems: 'center' }}
</code_context>
<issue_to_address>
**issue (bug_risk):** Non-`forceLogin` flows no longer trigger a redirect, leaving users stuck on the spinner.
Previously, when `auth.isAuthenticated` was false and there was no error, we always ended up calling `redirectUser()` for non-`forceLogin` flows. With this change:
- The auto-redirect `useEffect` now only runs when `props.forceLogin === true`.
- The final `return` only renders the "Redirecting to sign in..." spinner and never calls `signinRedirect`.
- For `props.forceLogin === undefined` (or any non-`false` value other than `true`), users will see the spinner but never be redirected.
If you want to preserve the previous default behavior, we should either:
- Call `auth.signinRedirect()` when `forceLogin` is not explicitly `false`, or
- Relax the `useEffect` condition to treat `forceLogin !== false` as eligible for auto sign-in.
Otherwise, all call sites should be updated to pass an explicit `forceLogin` to avoid flows that hang on the spinner.
</issue_to_address>
### Comment 2
<location path="packages/cellix/server-oauth2-mock-seedwork/src/index.ts" line_range="223-225" />
<code_context>
+
+ app.get('/authorize', (req, res) => {
+ const { state, redirect_uri } = req.query as { state?: string; redirect_uri?: string };
+ const requestedRedirectUri = redirect_uri ?? config.allowedRedirectUri;
+ const normalizedRequestedRedirectUri = normalizeUrl(requestedRedirectUri);
+ const isAllowed = [...config.allowedRedirectUris].some((value) => normalizeUrl(value) === normalizedRequestedRedirectUri) || normalizeUrl(config.allowedRedirectUri) === normalizedRequestedRedirectUri;
+ if (!isAllowed) {
+ res.status(400).send('Invalid redirect_uri');
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Redirect URI normalization is used for validation but not for later audience lookup, which can cause subtle mismatches.
The `/authorize` handler normalizes the redirect URI for validation, but the `/token` handler later decodes and uses the raw `redirect_uri` when looking up `config.allowedRedirectUris` / `redirectUriToAudience`. A URI that passes validation via normalization may then miss in `redirectUriToAudience` and fall back to the default audience. To keep `aud` consistent, consider normalizing both the stored keys in `allowedRedirectUris` / `redirectUriToAudience` and the decoded redirect URI before lookup.
Suggested implementation:
```typescript
const requestedRedirectUri = redirect_uri ?? config.allowedRedirectUri;
const normalizedRequestedRedirectUri = normalizeUrl(requestedRedirectUri);
const isAllowed =
[...config.allowedRedirectUris].some(
(value) => normalizeUrl(value) === normalizedRequestedRedirectUri,
) || normalizeUrl(config.allowedRedirectUri) === normalizedRequestedRedirectUri;
if (!isAllowed) {
res.status(400).send('Invalid redirect_uri');
return;
}
try {
// Use the normalized redirect URI when embedding into the auth code so that
// the `/token` handler can later decode and perform audience lookup on a
// normalized value consistent with validation.
const code = `mock-auth-code-${Buffer.from(normalizedRequestedRedirectUri).toString('base64')}`;
const redirectUrl = new URL(requestedRedirectUri);
```
To fully implement your suggestion and avoid subtle mismatches between `/authorize` and `/token`, you will also need to:
1. Normalize keys in configuration:
- Wherever `config.allowedRedirectUris` and `config.redirectUriToAudience` are constructed (or immediately after loading config), create normalized variants, for example:
- `normalizedAllowedRedirectUris = new Set([...config.allowedRedirectUris].map(normalizeUrl))`
- `normalizedRedirectUriToAudience = new Map([...config.redirectUriToAudience].map(([uri, aud]) => [normalizeUrl(uri), aud]))`
- Use these normalized collections for validation and audience lookup instead of the raw ones.
2. Normalize the decoded redirect URI in the `/token` handler:
- In the `/token` route, after decoding the redirect URI from the auth code (or reading from `req.body.redirect_uri`), call `normalizeUrl(decodedRedirectUri)` and use that normalized value when:
- Checking membership against `normalizedAllowedRedirectUris`
- Looking up the audience from `normalizedRedirectUriToAudience`
3. Ensure that `/authorize` and `/token` use the same normalization function:
- Confirm both handlers import and use the same `normalizeUrl` implementation so that the normalization is consistent across validation and lookup.
</issue_to_address>
### Comment 3
<location path="apps/api/start-dev.mjs" line_range="10-13" />
<code_context>
+ process.exit(1);
+}
+
+const childEnv = {
+ ...process.env,
+ NODE_EXTRA_CA_CERTS: `${process.env.HOME}/.portless/ca.pem`,
+ NODE_OPTIONS: '--use-system-ca',
+};
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Deriving the Portless CA path from `HOME` may not be robust across environments.
This builds the CA path as `${process.env.HOME}/.portless/ca.pem`. In some environments (e.g., certain Windows setups or CI runners), `HOME` may be unset or the directory may live under a different base var (like `USERPROFILE`), which would break `NODE_EXTRA_CA_CERTS` and cause TLS failures.
Consider using `os.homedir()` instead of `process.env.HOME`, and/or supporting an override env such as `PORTLESS_CA_PATH` with the current path as a fallback.
Suggested implementation:
```javascript
import { spawn } from 'node:child_process';
import os from 'node:os';
import path from 'node:path';
```
```javascript
const portlessCaPath =
process.env.PORTLESS_CA_PATH ??
path.join(os.homedir(), '.portless', 'ca.pem');
const childEnv = {
...process.env,
NODE_EXTRA_CA_CERTS: portlessCaPath,
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+63
to
+68
| if (props.forceLogin === false) { | ||
| return <Navigate to="/" />; | ||
| } else { | ||
| redirectUser(); | ||
| return; | ||
| } | ||
|
|
||
| return ( | ||
| <Row |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Non-forceLogin flows no longer trigger a redirect, leaving users stuck on the spinner.
Previously, when auth.isAuthenticated was false and there was no error, we always ended up calling redirectUser() for non-forceLogin flows. With this change:
- The auto-redirect
useEffectnow only runs whenprops.forceLogin === true. - The final
returnonly renders the "Redirecting to sign in..." spinner and never callssigninRedirect. - For
props.forceLogin === undefined(or any non-falsevalue other thantrue), users will see the spinner but never be redirected.
If you want to preserve the previous default behavior, we should either:
- Call
auth.signinRedirect()whenforceLoginis not explicitlyfalse, or - Relax the
useEffectcondition to treatforceLogin !== falseas eligible for auto sign-in.
Otherwise, all call sites should be updated to pass an explicit forceLogin to avoid flows that hang on the spinner.
Comment on lines
+223
to
+225
| const requestedRedirectUri = redirect_uri ?? config.allowedRedirectUri; | ||
| const normalizedRequestedRedirectUri = normalizeUrl(requestedRedirectUri); | ||
| const isAllowed = [...config.allowedRedirectUris].some((value) => normalizeUrl(value) === normalizedRequestedRedirectUri) || normalizeUrl(config.allowedRedirectUri) === normalizedRequestedRedirectUri; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Redirect URI normalization is used for validation but not for later audience lookup, which can cause subtle mismatches.
The /authorize handler normalizes the redirect URI for validation, but the /token handler later decodes and uses the raw redirect_uri when looking up config.allowedRedirectUris / redirectUriToAudience. A URI that passes validation via normalization may then miss in redirectUriToAudience and fall back to the default audience. To keep aud consistent, consider normalizing both the stored keys in allowedRedirectUris / redirectUriToAudience and the decoded redirect URI before lookup.
Suggested implementation:
const requestedRedirectUri = redirect_uri ?? config.allowedRedirectUri;
const normalizedRequestedRedirectUri = normalizeUrl(requestedRedirectUri);
const isAllowed =
[...config.allowedRedirectUris].some(
(value) => normalizeUrl(value) === normalizedRequestedRedirectUri,
) || normalizeUrl(config.allowedRedirectUri) === normalizedRequestedRedirectUri;
if (!isAllowed) {
res.status(400).send('Invalid redirect_uri');
return;
}
try {
// Use the normalized redirect URI when embedding into the auth code so that
// the `/token` handler can later decode and perform audience lookup on a
// normalized value consistent with validation.
const code = `mock-auth-code-${Buffer.from(normalizedRequestedRedirectUri).toString('base64')}`;
const redirectUrl = new URL(requestedRedirectUri);To fully implement your suggestion and avoid subtle mismatches between /authorize and /token, you will also need to:
-
Normalize keys in configuration:
- Wherever
config.allowedRedirectUrisandconfig.redirectUriToAudienceare constructed (or immediately after loading config), create normalized variants, for example:normalizedAllowedRedirectUris = new Set([...config.allowedRedirectUris].map(normalizeUrl))normalizedRedirectUriToAudience = new Map([...config.redirectUriToAudience].map(([uri, aud]) => [normalizeUrl(uri), aud]))
- Use these normalized collections for validation and audience lookup instead of the raw ones.
- Wherever
-
Normalize the decoded redirect URI in the
/tokenhandler:- In the
/tokenroute, after decoding the redirect URI from the auth code (or reading fromreq.body.redirect_uri), callnormalizeUrl(decodedRedirectUri)and use that normalized value when:- Checking membership against
normalizedAllowedRedirectUris - Looking up the audience from
normalizedRedirectUriToAudience
- Checking membership against
- In the
-
Ensure that
/authorizeand/tokenuse the same normalization function:- Confirm both handlers import and use the same
normalizeUrlimplementation so that the normalization is consistent across validation and lookup.
- Confirm both handlers import and use the same
Comment on lines
+10
to
+13
| const childEnv = { | ||
| ...process.env, | ||
| NODE_EXTRA_CA_CERTS: `${process.env.HOME}/.portless/ca.pem`, | ||
| NODE_OPTIONS: '--use-system-ca', |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Deriving the Portless CA path from HOME may not be robust across environments.
This builds the CA path as ${process.env.HOME}/.portless/ca.pem. In some environments (e.g., certain Windows setups or CI runners), HOME may be unset or the directory may live under a different base var (like USERPROFILE), which would break NODE_EXTRA_CA_CERTS and cause TLS failures.
Consider using os.homedir() instead of process.env.HOME, and/or supporting an override env such as PORTLESS_CA_PATH with the current path as a fallback.
Suggested implementation:
import { spawn } from 'node:child_process';
import os from 'node:os';
import path from 'node:path';const portlessCaPath =
process.env.PORTLESS_CA_PATH ??
path.join(os.homedir(), '.portless', 'ca.pem');
const childEnv = {
...process.env,
NODE_EXTRA_CA_CERTS: portlessCaPath,
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.
Summary by Sourcery
Adopt portless-based HTTPS local development URLs, refactor mock services into seedwork + app-level servers, and improve frontend auth handling and docs to match the new dev workflow.
New Features:
Enhancements:
Documentation: