Introduce custom *.localhost subdomains w/ HTTPS #334
+1,906
−762
Conversation
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
…ent/message mock servers
Contributor
Reviewer's GuideImplements HTTPS-enabled *.sharethrift.localhost subdomains for UI, API, docs, and mock services using mkcert-generated wildcard certificates, a custom HTTPS proxy for Azure Functions, and HTTPS-aware dev/server configs with HTTP fallbacks for CI. Sequence diagram for HTTPS proxying from data-access.sharethrift.localhost to Azure FunctionssequenceDiagram
actor Developer
participant Browser
participant HTTPSProxy as HTTPSProxy_data_access
participant AzureFuncs as AzureFunctions_Http
Developer->>Browser: Open https://data-access.sharethrift.localhost:7072/api/...
Browser->>HTTPSProxy: TLS handshake using sharethrift.localhost cert
HTTPSProxy-->>Browser: TLS session established
Browser->>HTTPSProxy: HTTPS request GET /api/resource
HTTPSProxy->>HTTPSProxy: Validate Host header == data-access.sharethrift.localhost
HTTPSProxy->>AzureFuncs: HTTP request GET /api/resource
AzureFuncs-->>HTTPSProxy: HTTP response (status, headers, body)
HTTPSProxy-->>Browser: HTTPS response (proxied status, headers, body)
Browser-->>Developer: Render API response in UI or tools
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…uction compatibility
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jason-t-hankins
changed the title
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 10 other issues, and left some high level feedback:
Security issues:
- Identified a Private Key, which may compromise cryptographic security and sensitive data encryption. (link)
General comments:
- The certificate path resolution logic (and hostnames/ports) is duplicated across multiple services (mock-payment, mock-oauth2, mock-messaging, docs, ui, proxy); consider extracting a shared helper or central config so future changes to cert locations or domains only need to be made in one place.
- In scripts/setup-local-certs.js, CERT_DIR is used but never created; ensure the directory exists (e.g., via fs.mkdirSync with { recursive: true }) before chdir and file operations to avoid runtime errors on a fresh clone.
- The checkCommand helper uses
command -vfor all platforms, which will fail on Windows shells; you may want to branch to use an appropriate check (e.g.,whereon win32) or rely on a more portable detection mechanism for mkcert and package managers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The certificate path resolution logic (and hostnames/ports) is duplicated across multiple services (mock-payment, mock-oauth2, mock-messaging, docs, ui, proxy); consider extracting a shared helper or central config so future changes to cert locations or domains only need to be made in one place.
- In scripts/setup-local-certs.js, CERT_DIR is used but never created; ensure the directory exists (e.g., via fs.mkdirSync with { recursive: true }) before chdir and file operations to avoid runtime errors on a fresh clone.
- The checkCommand helper uses `command -v` for all platforms, which will fail on Windows shells; you may want to branch to use an appropriate check (e.g., `where` on win32) or rely on a more portable detection mechanism for mkcert and package managers.
## Individual Comments
### Comment 1
<location> `packages/cellix/mock-oauth2-server/src/index.ts:310` </location>
<code_context>
+ const BASE_URL = 'https://mock-auth.sharethrift.localhost:4000';
</code_context>
<issue_to_address>
**issue (bug_risk):** Issuer and discovery URLs always use HTTPS even when the server falls back to plain HTTP, which can cause validation mismatches.
In the HTTP fallback path (no certs), the server listens on `http://localhost:${port}` but still advertises `https://mock-auth.sharethrift.localhost:${port}` as the issuer and discovery URL. Any client that validates `iss`/audience or uses the discovery document in that mode can fail due to this mismatch. Please make `BASE_URL` conditional on whether HTTPS is enabled, or otherwise ensure the HTTP fallback is reachable at the advertised HTTPS host so tokens and discovery metadata remain consistent with the running server.
</issue_to_address>
### Comment 2
<location> `scripts/setup-local-certs.js:46` </location>
<code_context>
+ return;
+ }
+
+ console.log(' Setting up local HTTPS certificates...\n');
+
+ // Check if mkcert is installed
</code_context>
<issue_to_address>
**issue (bug_risk):** Script assumes POSIX tooling and will behave poorly or fail on Windows (and possibly when .certs does not exist).
Specifically:
1) `checkCommand` uses `command -v`, which is POSIX-only. On Windows this will throw even if `choco`/`scoop` are installed, so `checkCommand('choco')` and `checkCommand('scoop')` always return `false` and force the manual install path.
2) The `ls *.localhost+4*.pem 2>/dev/null || true` pattern is also POSIX-only and will fail in Windows shells.
3) `process.chdir(CERT_DIR)` is called without ensuring `CERT_DIR` exists, so a fresh clone without `.certs` will throw before `mkcert` runs.
To make this portable and more robust, consider:
- Ensuring `CERT_DIR` exists, e.g. `fs.mkdirSync(CERT_DIR, { recursive: true })`, before `chdir`.
- Replacing `command -v` with a cross‑platform check (e.g. attempt to spawn the command and handle ENOENT, or use `where` on Windows).
- Replacing the `ls` + redirection with `fs.readdirSync`/`fs.promises.readdir` in Node.
</issue_to_address>
### Comment 3
<location> `scripts/setup-local-certs.js:101-110` </location>
<code_context>
+ // Generate wildcard certificate
</code_context>
<issue_to_address>
**suggestion:** Wildcard certificate generation and renaming logic is brittle (hard-coded filename pattern and assumptions about mkcert output).
This logic relies on mkcert always emitting `*.localhost+4*.pem` files and on detecting the key via a `-key.pem` substring, so any change in mkcert’s filename scheme (host count, ordering, suffixes) could break this without obvious failure.
Consider either:
- Programmatically distinguishing key vs cert (e.g., by content or a more robust pattern), or
- Using mkcert’s `-key-file` / `-cert-file` flags to write directly to `sharethrift.localhost-key.pem` and `sharethrift.localhost.pem`, avoiding `ls` and renames entirely.
Suggested implementation:
```javascript
// Generate wildcard certificate with deterministic key/cert filenames
console.log(' Generating wildcard certificate for *.sharethrift.localhost...');
process.chdir(CERT_DIR);
exec('mkcert -key-file sharethrift.localhost-key.pem -cert-file sharethrift.localhost.pem "*.sharethrift.localhost" "sharethrift.localhost" localhost 127.0.0.1 ::1');
```
```javascript
```
You will also need to:
1. Remove or update any subsequent code that:
- Uses `generatedFiles`, or
- Renames files matching `*.localhost+4*.pem`.
2. Replace those usages with direct references to:
- `sharethrift.localhost.pem` for the certificate, and
- `sharethrift.localhost-key.pem` for the private key,
assuming these files live in `CERT_DIR` (the current working directory after `process.chdir(CERT_DIR)`).
</issue_to_address>
### Comment 4
<location> `package.json:15-16` </location>
<code_context>
"build": "turbo run build",
"test": "turbo run test",
"lint": "turbo run lint",
+ "setup:certs": "node scripts/setup-local-certs.js",
+ "predev": "pnpm run setup:certs && nohup node local-https-proxy.js > /dev/null 2>&1 &",
"dev": "pnpm run build && turbo run azurite gen:watch start --parallel",
"start": "turbo run build && concurrently pnpm:start:* --kill-others-on-fail --workspace=@sthrift/api",
</code_context>
<issue_to_address>
**issue (bug_risk):** The `predev` script can spawn multiple background proxy processes and has no cleanup strategy.
Because `predev` uses `nohup ... &`, every `pnpm run dev` will try to start another `local-https-proxy.js`. If one is already bound to 7072, the new process will error with `EADDRINUSE`, and you'll accumulate orphaned background proxies.
To avoid this, either make the proxy startup idempotent (e.g., detect an existing listener and exit quietly), move proxy startup into an explicit one-off script, or have `local-https-proxy.js` manage a PID file so you can check/stop an existing process before starting a new one.
</issue_to_address>
### Comment 5
<location> `local-https-proxy.js:16-25` </location>
<code_context>
+const server = https.createServer({
</code_context>
<issue_to_address>
**suggestion:** Proxy server assumes cert files exist and does not handle startup errors, which can make failures opaque.
Because the cert/key are read synchronously and `server.listen` is called without error handling, missing/invalid certs or a port conflict will cause opaque failures.
Please:
- Check that `certPath`/`keyPath` exist and log a clear error if they don’t.
- Attach a `server.on('error', ...)` handler to surface `EADDRINUSE` and other startup errors before exiting.
</issue_to_address>
### Comment 6
<location> `apps/docs/docs/technical-overview/localhost-subdomain-setup.md:9-18` </location>
<code_context>
+Sharethrift uses a subdomain-based architecture in local development that mirrors our production environment. This approach uses `*.sharethrift.localhost` domains with HTTPS to create a realistic development experience and support seamless transitions between local, containerized, and cloud deployments.
</code_context>
<issue_to_address>
**suggestion (typo):** Use a consistent capitalization for the product name (Sharethrift/ShareThrift).
In this doc the product name appears with both `Sharethrift` and `ShareThrift` (e.g., intro vs HTTPS proxy section). Please choose a single canonical form and use it consistently throughout to avoid confusion.
Suggested implementation:
```
Sharethrift
```
1. Apply this same replacement (`ShareThrift` → `Sharethrift`) for every occurrence in the entire file, not just the snippet shown.
2. If other docs in the docs/technical-overview section also mix the two forms, consider running a project-wide search and standardizing on `Sharethrift` there as well for consistency.
</issue_to_address>
### Comment 7
<location> `apps/docs/docs/technical-overview/localhost-subdomain-setup.md:40-49` </location>
<code_context>
+## Architecture: HTTPS Proxy for Azure Functions
</code_context>
<issue_to_address>
**issue:** Clarify whether the recommended approach is a custom HTTPS proxy or Azure Functions built-in HTTPS.
The doc first describes using a custom Node.js HTTPS proxy because the `--cert` flag is broken and Azure Functions generates its own `CN=localhost` cert, but later the “Azure Functions HTTPS Configuration” section recommends `func start --useHttps` and says built-in HTTPS removes the need for a proxy. This reads as conflicting guidance. Please clarify which option is the primary, supported approach, and if both are valid, briefly document when to use the custom proxy versus built-in HTTPS so readers aren’t left unsure which path to follow.
</issue_to_address>
### Comment 8
<location> `apps/docs/docs/technical-overview/localhost-subdomain-setup.md:20` </location>
<code_context>
+- **Documentation**: `https://docs.sharethrift.localhost:3002`
</code_context>
<issue_to_address>
**issue (typo):** Align the documented Docs port (3002 vs 3001) for consistency.
The "Application Subdomains" section uses `https://docs.sharethrift.localhost:3002`, but the nginx config and "Single Dev Command" section both reference docs on port 3001. Please standardize on the correct port throughout the docs and update these examples accordingly to avoid confusion for developers running the docs locally.
```suggestion
- **Documentation**: `https://docs.sharethrift.localhost:3001`
```
</issue_to_address>
### Comment 9
<location> `packages/cellix/mock-payment-server/src/index.ts:1342` </location>
<code_context>
-app.listen(port, () => {
- console.log(`Payment Mock Server listening on port ${port}`);
-});
+const startServer = (portToTry: number, attempt = 0): void => {
+ // From dist/src/index.js, go up 5 levels to workspace root
+ // dist/src -> dist -> mock-payment-server -> cellix -> packages -> workspace root
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new startServer logic by extracting TLS setup and retryable listen behavior into helpers so this function only orchestrates HTTP vs HTTPS startup.
You can reduce the complexity without changing behavior by factoring out the reusable pieces and decoupling “how to listen” from “how to retry” and “how to get TLS options”.
### 1. Extract HTTPS options + cert detection
This keeps `startServer` focused on choosing transport, not doing path math and I/O on every retry:
```ts
const getHttpsOptionsOrNull = (): https.ServerOptions | null => {
// From dist/src/index.js, go up 5 levels to workspace root
const workspaceRoot = path.join(__dirname, '../../../../..');
const certKeyPath = path.join(workspaceRoot, '.certs/sharethrift.localhost-key.pem');
const certPath = path.join(workspaceRoot, '.certs/sharethrift.localhost.pem');
if (!fs.existsSync(certKeyPath) || !fs.existsSync(certPath)) {
return null;
}
return {
key: fs.readFileSync(certKeyPath),
cert: fs.readFileSync(certPath),
};
};
```
### 2. Extract generic listen-with-retry helper
This removes duplication between HTTP and HTTPS branches and keeps retry logic in one place:
```ts
type ListenFn = (port: number) => import('http').Server;
const listenWithRetry = (
listen: ListenFn,
portToTry: number,
attempt = 0,
maxAttempts = 5,
): void => {
const server = listen(portToTry);
server.on('error', (error: NodeJS.ErrnoException) => {
if (error.code === 'EADDRINUSE' && attempt < maxAttempts) {
const nextPort = portToTry + 1;
console.warn(
`Port ${portToTry} in use. Retrying mock-payment-server on ${nextPort}...`,
);
listenWithRetry(listen, nextPort, attempt + 1, maxAttempts);
return;
}
console.error('Failed to start mock-payment-server', error);
});
};
```
### 3. Simplify `startServer` to orchestration only
Now `startServer` just selects HTTP vs HTTPS and delegates:
```ts
const startServer = (portToTry: number): void => {
const httpsOptions = getHttpsOptionsOrNull();
if (httpsOptions) {
listenWithRetry(
(port) =>
https.createServer(httpsOptions, app).listen(port, HOST, () => {
console.log(`Mock Payment Server listening on https://${HOST}:${port}`);
}),
portToTry,
);
} else {
listenWithRetry(
(port) =>
app.listen(port, () => {
console.log(
`Mock Payment Server listening on http://localhost:${port} (no certs found)`,
);
}),
portToTry,
);
}
};
startServer(DEFAULT_PORT);
```
This keeps all existing behavior (HTTPS when certs exist, HTTP fallback, port retry with increment) but removes duplicated error handling, separates TLS detection from startup, and makes the control flow easier to follow.
</issue_to_address>
### Comment 10
<location> `packages/sthrift/mock-messaging-server/src/index.ts:77` </location>
<code_context>
- const server = app.listen(port, () => {
- console.log(`Mock Twilio Server listening on port ${port}`);
+
+ // From package directory, go up 3 levels to workspace root
+ // packages/sthrift/mock-messaging-server -> ../../..
+ const workspaceRoot = path.join(process.cwd(), '../../../');
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new HTTPS/HTTP startup logic to extract cert handling and share the common listen/seeding behavior between branches while avoiding unnecessary server creation.
You can reduce the new complexity while keeping all behavior by:
1. Extracting the cert resolution/FS logic into a helper.
2. Sharing the “listen + logging + seeding + resolve” callback between HTTP and HTTPS.
3. Using `app.listen` for the HTTP fallback (no need for `http.createServer` there).
This keeps HTTPS support and the HTTP fallback intact but removes duplication and splits concerns.
### 1. Factor out HTTPS options / cert logic
```ts
function getHttpsOptionsOrNull(): https.ServerOptions | null {
// From package directory, go up 3 levels to workspace root
// packages/sthrift/mock-messaging-server -> ../../..
const workspaceRoot = path.join(process.cwd(), '../../../');
const certKeyPath = path.join(workspaceRoot, '.certs/sharethrift.localhost-key.pem');
const certPath = path.join(workspaceRoot, '.certs/sharethrift.localhost.pem');
if (!fs.existsSync(certKeyPath) || !fs.existsSync(certPath)) {
return null;
}
return {
key: fs.readFileSync(certKeyPath),
cert: fs.readFileSync(certPath),
};
}
```
### 2. Share the startup callback and simplify branching
```ts
export function startServer(port = 10000, seedData = false): Promise<Server> {
return new Promise((resolve) => {
const app = createApp();
const httpsOptions = getHttpsOptionsOrNull();
const onListen = (protocol: 'http' | 'https', host: string, note?: string) => () => {
const extra = note ? ` ${note}` : '';
console.log(`Mock Messaging Server listening on ${protocol}://${host}:${port}${extra}`);
if (seedData) {
seedMockData();
} else {
console.log('Starting with empty data store (set seedData=true to seed)');
}
};
if (httpsOptions) {
const host = 'mock-messaging.sharethrift.localhost';
const server = https.createServer(httpsOptions, app).listen(
port,
host,
onListen('https', host)
);
resolve(server);
} else {
// Fallback to HTTP when certs don't exist (CI/CD)
const host = 'localhost';
const server = app.listen(
port,
onListen('http', host, '(no certs found)')
);
resolve(server);
}
});
}
```
This keeps:
- HTTPS with custom host when certs exist.
- HTTP fallback with `(no certs found)` note.
- Seeding behavior identical in both branches.
But it:
- Removes duplicated seeding/logging blocks.
- Moves filesystem and path logic out of `startServer`.
- Avoids unnecessary `http.createServer` usage in the HTTP case.
</issue_to_address>
### Comment 11
<location> `packages/sthrift/mock-messaging-server/src/index.test.ts:9-36` </location>
<code_context>
-----BEGIN PRIVATE KEY-----
MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDFSYieiqhnLDww
8PNRgxYqntovNdGLVrqFvBT7yNQjTMrdAx6WD4Zq66/m6FYhENHU+KRV1k5HK32z
HcIjte5+Vo1OjonXaZFmiwHfxwfK6dLcuzKJiswi4S/kr8AmXyN1clusYcQfS3JJ
ytl4leeuZC0dZOUWrFKVfOdYRnGx+74MzNQjjemhooiso8p4EjoOv6XrLap7CJrr
KTz40/UcXSdYpK2VCuqIsxB6+GHOoklDsngyFpS4XUJxo60ANvR23NUU1gfdFyXo
JqqOWvCDIYu+INSxxSdRcdXFHGqd5O5E1E5cfbxzjwD9CLcYutueG3TOQ7eX2pZ+
B/VRRZXvAgMBAAECggEADP7cBf2CPFGYoWytynfFFQejvapg5DnRTY/nHfLLrouX
bVmQQThClXZy/tXdHo56E/pQ8OqN+3gs7oiOSkXosN/IrR0MjWkBz+xmMjRtGfuv
DSUzngXLjpygyAKfuKdZsZnX2hISljElFSFMk/nj6gPlSONlh8nQItK4doMlaCDc
Vr/yuzyvmNoGKICYDdWE1elXlhVqaxapQgTSfVocJCiQcuSbo7zocbxUStUdv+Vf
8bIb4hJmQnFx1/KOx3PAhzjlwzGPDCaU8Iu7ZGfWwVFBk4EDt2KmO0lcL6Mf0kDj
1OZ/71b/HVrwm0oWel3udHrcLWVy0ttxONg7KTy9iQKBgQD2nY7JfWHlr2bVwts1
t3aWOcVKjYVWZOqQA4cj1OQ6qzZweD35Comt+nMbBYP85LAkIU2RuN4WW8eoF3TT
stE+R2hryBJSlZvLf6gqBuXMBKziKneQ2oxufqSs7Gfh4ZRn2loilx9RFU+nnykj
pJINF1KRXU9P9xWyBC0rXHz7UwKBgQDMy3AA2oJtDdSTKAEe5aMDR7a5kan1iBaM
2d7pCZJNBEsHjq6LuMk3auuSmGruT+9tPVA6uo2R8KzU6viLvJ0KRmD2MTrjQRlx
WZHloNMQnnovBFs2akPVeCe+Gxpxw5D6xpxHS79ocQre1qCNrLUuc0GVzT+Vr7Jy
AlZe5CBDdQKBgCBar17z2iIeDunAdK3pcd9nmOCdDH83Iiber54d0gt4Zt3+iHLh
1QpEL1G+OYC2J/21eM3DaS+ZvunomeUkMptvFt39E5pOEnByQQeye/d2LZBGbgVN
BtuANe6R3VcfFy9A0E51cSBcIWUmVmlcr+STyLtVeJBPJEOElMB5k9QNAoGBAMcl
5arO0hoeYuMe+cgD4vtjl2rpHW9ogRVDIKNY7W37iMK2m0G/1dwQGziZAxLvpJKE
FyqlEtoiQs9iF4Q7TDMbsVp3ER7AxW8WfOvh8p2snoBuKgzGVWSWzX7ueE68sJVM
4dfq7x0vIDUGXWan1iBvcA2uY/C5xDtdzoPELyOpAoGBAIxe6FdBNySitlAQp5OT
xixJoyBv+uHeg09uQ222qVSXU1TCkBp0Id6Fn0iBdg1GDCj0Gq08Mb3Q9P8tpMmp
z0PNSIe/ZK8Om8Rc6942eIx0e7rgN3fAbTrmP6l3Gl46DNtlb8JH+GOmtICut05M
BEg7ggPvvZ7wHxBYPFeY9/cK
-----END PRIVATE KEY-----
</code_context>
<issue_to_address>
**security (private-key):** Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }); | ||
| } else { | ||
| // Fallback to HTTP when certs don't exist (CI/CD) | ||
| app.listen(port, () => { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Issuer and discovery URLs always use HTTPS even when the server falls back to plain HTTP, which can cause validation mismatches.
In the HTTP fallback path (no certs), the server listens on http://localhost:${port} but still advertises https://mock-auth.sharethrift.localhost:${port} as the issuer and discovery URL. Any client that validates iss/audience or uses the discovery document in that mode can fail due to this mismatch. Please make BASE_URL conditional on whether HTTPS is enabled, or otherwise ensure the HTTP fallback is reachable at the advertised HTTPS host so tokens and discovery metadata remain consistent with the running server.
| return; | ||
| } | ||
|
|
||
| console.log(' Setting up local HTTPS certificates...\n'); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Script assumes POSIX tooling and will behave poorly or fail on Windows (and possibly when .certs does not exist).
Specifically:
checkCommandusescommand -v, which is POSIX-only. On Windows this will throw even ifchoco/scoopare installed, socheckCommand('choco')andcheckCommand('scoop')always returnfalseand force the manual install path.- The
ls *.localhost+4*.pem 2>/dev/null || truepattern is also POSIX-only and will fail in Windows shells. process.chdir(CERT_DIR)is called without ensuringCERT_DIRexists, so a fresh clone without.certswill throw beforemkcertruns.
To make this portable and more robust, consider:
- Ensuring
CERT_DIRexists, e.g.fs.mkdirSync(CERT_DIR, { recursive: true }), beforechdir. - Replacing
command -vwith a cross‑platform check (e.g. attempt to spawn the command and handle ENOENT, or usewhereon Windows). - Replacing the
ls+ redirection withfs.readdirSync/fs.promises.readdirin Node.
Comment on lines
+101
to
+110
| // Generate wildcard certificate | ||
| console.log(' Generating wildcard certificate for *.sharethrift.localhost...'); | ||
| process.chdir(CERT_DIR); | ||
| exec('mkcert "*.sharethrift.localhost" "sharethrift.localhost" localhost 127.0.0.1 ::1'); | ||
|
|
||
| // Rename files to standard names | ||
| const generatedFiles = execSync('ls *.localhost+4*.pem 2>/dev/null || true', { | ||
| encoding: 'utf-8', | ||
| cwd: CERT_DIR | ||
| }).trim().split('\n').filter(Boolean); |
Contributor
There was a problem hiding this comment.
suggestion: Wildcard certificate generation and renaming logic is brittle (hard-coded filename pattern and assumptions about mkcert output).
This logic relies on mkcert always emitting *.localhost+4*.pem files and on detecting the key via a -key.pem substring, so any change in mkcert’s filename scheme (host count, ordering, suffixes) could break this without obvious failure.
Consider either:
- Programmatically distinguishing key vs cert (e.g., by content or a more robust pattern), or
- Using mkcert’s
-key-file/-cert-fileflags to write directly tosharethrift.localhost-key.pemandsharethrift.localhost.pem, avoidinglsand renames entirely.
Suggested implementation:
// Generate wildcard certificate with deterministic key/cert filenames
console.log(' Generating wildcard certificate for *.sharethrift.localhost...');
process.chdir(CERT_DIR);
exec('mkcert -key-file sharethrift.localhost-key.pem -cert-file sharethrift.localhost.pem "*.sharethrift.localhost" "sharethrift.localhost" localhost 127.0.0.1 ::1');You will also need to:
- Remove or update any subsequent code that:
- Uses
generatedFiles, or - Renames files matching
*.localhost+4*.pem.
- Uses
- Replace those usages with direct references to:
sharethrift.localhost.pemfor the certificate, andsharethrift.localhost-key.pemfor the private key,
assuming these files live inCERT_DIR(the current working directory afterprocess.chdir(CERT_DIR)).
Comment on lines
+15
to
+16
| "setup:certs": "node scripts/setup-local-certs.js", | ||
| "predev": "pnpm run setup:certs && nohup node local-https-proxy.js > /dev/null 2>&1 &", |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The predev script can spawn multiple background proxy processes and has no cleanup strategy.
Because predev uses nohup ... &, every pnpm run dev will try to start another local-https-proxy.js. If one is already bound to 7072, the new process will error with EADDRINUSE, and you'll accumulate orphaned background proxies.
To avoid this, either make the proxy startup idempotent (e.g., detect an existing listener and exit quietly), move proxy startup into an explicit one-off script, or have local-https-proxy.js manage a PID file so you can check/stop an existing process before starting a new one.
Comment on lines
+16
to
+25
| const server = https.createServer({ | ||
| cert: fs.readFileSync(certPath), | ||
| key: fs.readFileSync(keyPath), | ||
| }, (req, res) => { | ||
| // Validate hostname | ||
| const host = req.headers.host?.split(':')[0]; | ||
| if (host !== 'data-access.sharethrift.localhost') { | ||
| res.writeHead(404); | ||
| res.end('Not Found'); | ||
| return; |
Contributor
There was a problem hiding this comment.
suggestion: Proxy server assumes cert files exist and does not handle startup errors, which can make failures opaque.
Because the cert/key are read synchronously and server.listen is called without error handling, missing/invalid certs or a port conflict will cause opaque failures.
Please:
- Check that
certPath/keyPathexist and log a clear error if they don’t. - Attach a
server.on('error', ...)handler to surfaceEADDRINUSEand other startup errors before exiting.
Comment on lines
+40
to
+49
| ## Architecture: HTTPS Proxy for Azure Functions | ||
|
|
||
| The ShareThrift local development setup uses a **custom Node.js HTTPS proxy** to provide HTTPS access to Azure Functions instead of Azure Functions' built-in `--useHttps` flag. | ||
|
|
||
| ### Why Not Use Azure Functions `--cert` Flag? | ||
|
|
||
| Azure Functions Core Tools v4.2.2 has a **broken `--cert` flag** that ignores custom PFX certificates: | ||
|
|
||
| ```bash | ||
| # This doesn't work - Azure Functions ignores the cert and generates its own |
Contributor
There was a problem hiding this comment.
issue: Clarify whether the recommended approach is a custom HTTPS proxy or Azure Functions built-in HTTPS.
The doc first describes using a custom Node.js HTTPS proxy because the --cert flag is broken and Azure Functions generates its own CN=localhost cert, but later the “Azure Functions HTTPS Configuration” section recommends func start --useHttps and says built-in HTTPS removes the need for a proxy. This reads as conflicting guidance. Please clarify which option is the primary, supported approach, and if both are valid, briefly document when to use the custom proxy versus built-in HTTPS so readers aren’t left unsure which path to follow.
| - **Frontend UI**: `https://sharethrift.localhost:3000` | ||
| - **Backend API (via HTTPS proxy)**: `https://data-access.sharethrift.localhost:7072` | ||
| - **Backend API (Azure Functions HTTP)**: `http://localhost:7071` | ||
| - **Documentation**: `https://docs.sharethrift.localhost:3002` |
Contributor
There was a problem hiding this comment.
issue (typo): Align the documented Docs port (3002 vs 3001) for consistency.
The "Application Subdomains" section uses https://docs.sharethrift.localhost:3002, but the nginx config and "Single Dev Command" section both reference docs on port 3001. Please standardize on the correct port throughout the docs and update these examples accordingly to avoid confusion for developers running the docs locally.
Suggested change
| - **Documentation**: `https://docs.sharethrift.localhost:3002` | |
| - **Documentation**: `https://docs.sharethrift.localhost:3001` |
Comment on lines
+9
to
+36
| let app: Application; | ||
|
|
||
| beforeAll(() => { | ||
| app = createApp(); | ||
| }); | ||
|
|
||
| it('should create an Express application', () => { | ||
| expect(app).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should respond to root endpoint with server info', async () => { | ||
| const response = await request(app).get('/'); | ||
| expect(response.status).toBe(200); | ||
| expect(response.body).toHaveProperty('service', 'Mock Twilio Server'); | ||
| expect(response.body).toHaveProperty('status', 'running'); | ||
| }); | ||
|
|
||
| it('should return 404 for unknown routes', async () => { | ||
| const response = await request(app).get('/unknown-route'); | ||
| expect(response.status).toBe(404); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Server Lifecycle', () => { | ||
| it('should start server successfully', async () => { | ||
| const server = await startServer(10001, false); | ||
| expect(server).toBeDefined(); | ||
| await stopServer(server); |
Contributor
There was a problem hiding this comment.
security (private-key): Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
Source: gitleaks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces HTTPS-enabled *.sharethrift.localhost subdomains for local development to mirror production domain structure. The changes include automated certificate generation via mkcert, HTTPS support for mock services, and an HTTPS proxy for Azure Functions.
Changes:
- Added mkcert-based certificate setup script with platform detection
- Implemented HTTPS proxy for Azure Functions backend API
- Updated mock services (payment, messaging, auth) to support HTTPS with subdomain routing and HTTP fallback
- Upgraded dependencies including
react-router-dom,axios, and@types/node - Updated OAuth configuration with HTTPS redirect URIs
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/setup-local-certs.js |
New automated certificate setup using mkcert with cross-platform support |
local-https-proxy.js |
New HTTPS proxy to provide TLS termination for Azure Functions |
packages/cellix/mock-payment-server/src/index.ts |
Added HTTPS support with certificate detection and HTTP fallback |
packages/cellix/mock-oauth2-server/src/index.ts |
Updated for HTTPS with new redirect URIs and subdomain support |
packages/sthrift/mock-messaging-server/src/index.ts |
Added HTTPS server creation with certificate loading |
apps/ui-sharethrift/vite.config.ts |
Configured HTTPS dev server with conditional certificate loading |
apps/ui-sharethrift/.env |
Updated OAuth and API endpoints to HTTPS subdomains |
apps/docs/docusaurus.config.ts |
Added custom HTTPS plugin for Docusaurus dev server |
package.json |
Added cert setup scripts and dependency overrides |
pnpm-lock.yaml |
Updated lock file with new dependency versions |
sonar-project.properties |
Added exclusions for new mock service packages |
knip.json |
Added ignore pattern for proxy script |
.snyk |
Added exclusions for local development tools |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| console.log(` ${CERT_DIR}\n`); | ||
| console.log(' Your local domains are now trusted for HTTPS:'); | ||
| console.log(' • https://sharethrift.localhost:3000 (UI)'); | ||
| console.log(' • https://data-access.sharethrift.localhost:8443 (API)'); |
There was a problem hiding this comment.
The port numbers mentioned in the script output don't match the actual ports being used:
Line 125 claims API is on port 8443, but the HTTPS proxy in local-https-proxy.js listens on port 7072 (line 10), and the Azure Functions backend runs on 7071. The correct output should be:
https://data-access.sharethrift.localhost:7072(proxy)- Backend HTTP:
http://localhost:7071
This misleading output will confuse developers about which URLs to use.
Suggested change
| console.log(' • https://data-access.sharethrift.localhost:8443 (API)'); | |
| console.log(' • https://data-access.sharethrift.localhost:7072 (API HTTPS proxy)'); | |
| console.log(' • http://localhost:7071 (API backend HTTP)'); |
Comment on lines
+1342
to
+1345
| const startServer = (portToTry: number, attempt = 0): void => { | ||
| // From dist/src/index.js, go up 5 levels to workspace root | ||
| // dist/src -> dist -> mock-payment-server -> cellix -> packages -> workspace root | ||
| const workspaceRoot = path.join(__dirname, '../../../../..'); |
There was a problem hiding this comment.
The certificate path resolution in startServer uses a relative path that depends on the current working directory at runtime. The comment states "From dist/src/index.js, go up 5 levels" but the actual path uses path.join(__dirname, '../../../../..') which goes up only 5 levels from __dirname, and __dirname points to the compiled location in dist/src/. This is fragile because:
- If the build output structure changes, this breaks
- If the script is run from a different location, it may fail
- The path calculation depends on assumptions about the build directory structure
Consider using a more robust approach like finding the workspace root by searching for a marker file (e.g., package.json with "workspaces" field) or using an environment variable for the workspace root.
Suggested change
| const startServer = (portToTry: number, attempt = 0): void => { | |
| // From dist/src/index.js, go up 5 levels to workspace root | |
| // dist/src -> dist -> mock-payment-server -> cellix -> packages -> workspace root | |
| const workspaceRoot = path.join(__dirname, '../../../../..'); | |
| const resolveWorkspaceRootForCerts = (): string => { | |
| // Prefer an explicit workspace root if provided | |
| const envRoot = process.env.WORKSPACE_ROOT; | |
| if (envRoot && path.isAbsolute(envRoot)) { | |
| return envRoot; | |
| } | |
| // Walk up from this file's directory looking for either: | |
| // - a ".certs" directory, or | |
| // - a "package.json" file (as a generic project root marker) | |
| let currentDir = __dirname; | |
| for (let i = 0; i < 10; i += 1) { | |
| const certsDir = path.join(currentDir, '.certs'); | |
| const packageJsonPath = path.join(currentDir, 'package.json'); | |
| if (fs.existsSync(certsDir) || fs.existsSync(packageJsonPath)) { | |
| return currentDir; | |
| } | |
| const parentDir = path.dirname(currentDir); | |
| if (parentDir === currentDir) { | |
| break; | |
| } | |
| currentDir = parentDir; | |
| } | |
| // Fallback: use this file's directory if no better root is found | |
| return __dirname; | |
| }; | |
| const startServer = (portToTry: number, attempt = 0): void => { | |
| // Resolve workspace root in a way that does not depend on build output structure | |
| const workspaceRoot = resolveWorkspaceRootForCerts(); |
Comment on lines
+1360
to
+1371
| server.on('error', (error: NodeJS.ErrnoException) => { | ||
| if (error.code === 'EADDRINUSE' && attempt < 5) { | ||
| const nextPort = portToTry + 1; | ||
| console.warn( | ||
| `Port ${portToTry} in use. Retrying mock-payment-server on ${nextPort}...`, | ||
| ); | ||
| startServer(nextPort, attempt + 1); | ||
| return; | ||
| } | ||
|
|
||
| console.error('Failed to start mock-payment-server', error); | ||
| }); |
There was a problem hiding this comment.
The error handler for server startup attempts to retry on port conflicts, but there's a logical issue: when server.on('error') fires and we call startServer() recursively, the original server instance remains bound (or attempting to bind) to the port. This can lead to:
- Multiple server instances competing for ports
- The original server never being cleaned up
- Potential memory leaks from abandoned server instances
The server should be explicitly closed before retrying with a different port, or the retry logic should be handled before creating the server instance.
Comment on lines
+1378
to
+1389
| server.on('error', (error: NodeJS.ErrnoException) => { | ||
| if (error.code === 'EADDRINUSE' && attempt < 5) { | ||
| const nextPort = portToTry + 1; | ||
| console.warn( | ||
| `Port ${portToTry} in use. Retrying mock-payment-server on ${nextPort}...`, | ||
| ); | ||
| startServer(nextPort, attempt + 1); | ||
| return; | ||
| } | ||
|
|
||
| console.error('Failed to start mock-payment-server', error); | ||
| }); |
There was a problem hiding this comment.
Similar to the HTTPS server error handler above, this HTTP fallback error handler has the same retry logic issue where the server instance isn't closed before attempting to start a new one on a different port. This creates the same problems: multiple server instances, resource leaks, and port conflicts.
Comment on lines
+76
to
+82
|
|
||
| // From package directory, go up 3 levels to workspace root | ||
| // packages/sthrift/mock-messaging-server -> ../../.. | ||
| const workspaceRoot = path.join(process.cwd(), '../../../'); | ||
| const certKeyPath = path.join(workspaceRoot, '.certs/sharethrift.localhost-key.pem'); | ||
| const certPath = path.join(workspaceRoot, '.certs/sharethrift.localhost.pem'); | ||
| const hasCerts = fs.existsSync(certKeyPath) && fs.existsSync(certPath); |
There was a problem hiding this comment.
The certificate path resolution uses path.join(process.cwd(), '../../../') to find the workspace root. This assumes that process.cwd() is always the package directory (e.g., packages/sthrift/mock-messaging-server), but this isn't guaranteed:
- If the process is started from the workspace root, this will resolve incorrectly
- Different execution contexts (npm scripts, turbo, direct node execution) may have different cwd values
- The path '../../../' depends on the directory structure not changing
Consider using __dirname or import.meta.url to calculate the path relative to the source file location, not the current working directory.
Comment on lines
+287
to
+288
| // When run via turbo/pnpm, cwd is the package directory, so we go up 3 levels to reach project root | ||
| const workspaceRoot = path.join(process.cwd(), '../../..'); |
There was a problem hiding this comment.
The certificate path resolution uses path.join(process.cwd(), '../../..') which assumes the script is always run from the package directory. This is problematic because:
process.cwd()varies depending on where the script is invoked from- If run from workspace root, this will look in the wrong location
- Different npm script runners may set different cwd values
Since the file already imports and uses import.meta.url at the top to define __dirname, consider using that to calculate certificate paths relative to the source file location instead of relying on process.cwd().
Suggested change
| // When run via turbo/pnpm, cwd is the package directory, so we go up 3 levels to reach project root | |
| const workspaceRoot = path.join(process.cwd(), '../../..'); | |
| // Use __dirname (from import.meta.url) instead of process.cwd() to avoid cwd-dependence | |
| const workspaceRoot = path.join(__dirname, '../../..'); |
Comment on lines
+16
to
17
| "predev": "pnpm run setup:certs && nohup node local-https-proxy.js > /dev/null 2>&1 &", | ||
| "dev": "pnpm run build && turbo run azurite gen:watch start --parallel", |
There was a problem hiding this comment.
The predev script uses nohup node local-https-proxy.js > /dev/null 2>&1 & which:
- Is Unix/Linux-specific and won't work on Windows
- Starts a background process that may not be cleaned up properly when
pnpm run devexits - Has no mechanism to stop the proxy when development is finished
- The redirect to
/dev/nullsuppresses all output, making debugging difficult
Consider:
- Using a cross-platform process manager like
concurrently(already in use) orpm2 - Adding a corresponding cleanup script
- Providing visible output for developer awareness
- Using the
--workspacefilter pattern for consistency with other scripts
Suggested change
| "predev": "pnpm run setup:certs && nohup node local-https-proxy.js > /dev/null 2>&1 &", | |
| "dev": "pnpm run build && turbo run azurite gen:watch start --parallel", | |
| "predev": "pnpm run setup:certs", | |
| "dev": "pnpm run build && concurrently -n proxy,dev \"node local-https-proxy.js\" \"turbo run azurite gen:watch start --parallel\"", |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Customized localhost subdomains now reflect the production domains instead of an IP or a localhost with a port number. mkcert is used to create and manage local certificates for https.
Applications
Mock Services
closes #328
Summary by Sourcery
Introduce HTTPS-enabled custom *.sharethrift.localhost domains for the UI, API proxy, and mock services to better mirror production environments in local development.
New Features:
Enhancements:
Build:
Summary by Sourcery
Introduce HTTPS-enabled *.sharethrift.localhost subdomains across the UI, docs, backend proxy, and mock services to better mirror production in local development while keeping CI/CD on HTTP fallbacks.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores: