Extract bounded feature modules - #72
Merged
Merged
Conversation
Move cohesive HTTP, bot-output, and thread-formatting behavior behind narrow interfaces and add advisory architecture budgets without changing runtime contracts. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Joseph Yaksich <gitcommit90@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR adds module architecture guidance, budgets, and reporting. It extracts client thread formatters, server HTTP utilities, and bot output helpers into separate modules. New phase-6 tests validate the extracted modules and architecture report. ChangesModular architecture boundaries
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/http.ts`:
- Around line 44-68: Update body to reject immediately when received exceeds
limit, while ensuring the promise rejects only once and the request stream
continues draining so the connection is handled cleanly. For declared content
lengths over the limit, reject immediately and keep consuming the request rather
than returning without attaching stream handlers; preserve PayloadTooLargeError
details for both declared and streamed oversized bodies.
- Around line 72-75: Update the jbody function to validate that the parsed JSON
result is an object before returning it. After JSON.parse succeeds, check
whether the result is actually a Record (object type), and return an empty
record instead if the parsed value is null, an array, or a scalar type. This
ensures the function always returns a valid record matching its declared return
type, preventing downstream property access errors like b.username from throwing
on non-object JSON inputs.
- Around line 78-80: Update requestAddress to trust CF-Connecting-IP only when
req.socket.remoteAddress belongs to the configured trusted proxy addresses;
otherwise use the socket’s remote address. Add or reuse the trusted-proxy
configuration for this validation, preserving the existing format check for
accepted forwarded values.
- Around line 26-28: Update the headers in the json() helper’s res.writeHead()
call to include cache-control: no-store alongside the existing content type and
security headers, ensuring authenticated JSON responses are not cached.
🪄 Autofix
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 Plus
Run ID: d8484b97-5144-4727-acda-b43249104f9a
📒 Files selected for processing (14)
AGENTS.mdconfig/module-budgets.jsondocs/module-map.mdpackage.jsonscripts/module-architecture-report.mjsscripts/run-test-suite.mjssrc/client/app.tssrc/client/thread-formatters.tssrc/server/bot-output.tssrc/server/bots.tssrc/server/http.tssrc/server/index.tstest/phase6-modules.mjstest/workspace-interactions.mjs
Comment on lines
+26
to
+28
| export function json(res: ServerResponse, code: number, responseBody: unknown): void { | ||
| res.writeHead(code, { "content-type": "application/json", ...SECURITY_HEADERS }); | ||
| res.end(JSON.stringify(responseBody)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# First, inspect the HTTP module to see what SECURITY_HEADERS contains
cat -n src/server/http.tsRepository: gitcommit90/1Helm
Length of output: 5600
🏁 Script executed:
# Search for SECURITY_HEADERS definition and any cache-control headers
rg "SECURITY_HEADERS|cache-control|Cache-Control" -i src/server/Repository: gitcommit90/1Helm
Length of output: 1916
🏁 Script executed:
# Check if there's any middleware or other code that adds cache headers
rg "cache-control|Cache-Control|writeHead" -i src/server/ -A 2 -B 2Repository: gitcommit90/1Helm
Length of output: 6947
🏁 Script executed:
# Verify the /api/me endpoint and confirm it uses json()
rg "/api/me|api/me" src/server/index.ts -A 5 -B 5Repository: gitcommit90/1Helm
Length of output: 4120
Sensitive Data Exposure (CWE-525): Use of Web Browser Cache Containing Sensitive Information
Reachability: External
Reachability path
● Entry
src/server/index.ts:466
applyMobileCors
│
▼
● Sink
src/server/http.ts
Add cache-control: no-store to the json() helper.
The json() function serves authenticated endpoints such as /api/me that return user-specific data. Without cache-control: no-store, browsers cache these responses at the profile level. When the same browser profile authenticates as a different account, the cached data can be reused, exposing the previous user's identity.
Add cache-control: no-store to the header object passed to res.writeHead().
Proposed fix
export function json(res: ServerResponse, code: number, responseBody: unknown): void {
- res.writeHead(code, { "content-type": "application/json", ...SECURITY_HEADERS });
+ res.writeHead(code, {
+ "content-type": "application/json",
+ "cache-control": "no-store",
+ ...SECURITY_HEADERS,
+ });
res.end(JSON.stringify(responseBody));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function json(res: ServerResponse, code: number, responseBody: unknown): void { | |
| res.writeHead(code, { "content-type": "application/json", ...SECURITY_HEADERS }); | |
| res.end(JSON.stringify(responseBody)); | |
| export function json(res: ServerResponse, code: number, responseBody: unknown): void { | |
| res.writeHead(code, { | |
| "content-type": "application/json", | |
| "cache-control": "no-store", | |
| ...SECURITY_HEADERS, | |
| }); | |
| res.end(JSON.stringify(responseBody)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/http.ts` around lines 26 - 28, Update the headers in the json()
helper’s res.writeHead() call to include cache-control: no-store alongside the
existing content type and security headers, ensuring authenticated JSON
responses are not cached.
Comment on lines
+44
to
+68
| export function body(req: IncomingMessage, limit = JSON_BODY_LIMIT): Promise<Buffer> { | ||
| return new Promise((resolve, reject) => { | ||
| const declared = Number(req.headers["content-length"] || 0); | ||
| if (declared > limit) { | ||
| const error = new Error(`Request exceeds the ${Math.floor(limit / 1024 / 1024)} MB limit.`); | ||
| error.name = "PayloadTooLargeError"; | ||
| reject(error); | ||
| return; | ||
| } | ||
| const chunks: Buffer[] = []; | ||
| let received = 0; | ||
| let oversized = false; | ||
| req.on("data", (chunk: Buffer) => { | ||
| received += chunk.length; | ||
| if (received > limit) { oversized = true; chunks.length = 0; } | ||
| else if (!oversized) chunks.push(chunk); | ||
| }); | ||
| req.on("end", () => { | ||
| if (oversized) { | ||
| const error = new Error(`Request exceeds the ${Math.floor(limit / 1024 / 1024)} MB limit.`); | ||
| error.name = "PayloadTooLargeError"; | ||
| reject(error); | ||
| } else resolve(Buffer.concat(chunks)); | ||
| }); | ||
| req.on("error", reject); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject an oversized streamed body when the limit is exceeded.
Line 58 marks the request as oversized, but Lines 61-66 wait for end before rejecting. A client can exceed the limit with chunked input and keep the request open. The route then cannot return its 413 response.
Reject once at the threshold. Drain declared-oversized requests after rejection.
Proposed fix
export function body(req: IncomingMessage, limit = JSON_BODY_LIMIT): Promise<Buffer> {
return new Promise((resolve, reject) => {
+ const payloadTooLarge = () => {
+ const error = new Error(`Request exceeds the ${Math.floor(limit / 1024 / 1024)} MB limit.`);
+ error.name = "PayloadTooLargeError";
+ return error;
+ };
const declared = Number(req.headers["content-length"] || 0);
if (declared > limit) {
- const error = new Error(`Request exceeds the ${Math.floor(limit / 1024 / 1024)} MB limit.`);
- error.name = "PayloadTooLargeError";
- reject(error);
+ req.resume();
+ reject(payloadTooLarge());
return;
}
@@
req.on("data", (chunk: Buffer) => {
received += chunk.length;
- if (received > limit) { oversized = true; chunks.length = 0; }
+ if (received > limit && !oversized) {
+ oversized = true;
+ chunks.length = 0;
+ reject(payloadTooLarge());
+ }
else if (!oversized) chunks.push(chunk);
});
req.on("end", () => {
- if (oversized) {
- const error = new Error(`Request exceeds the ${Math.floor(limit / 1024 / 1024)} MB limit.`);
- error.name = "PayloadTooLargeError";
- reject(error);
- } else resolve(Buffer.concat(chunks));
+ if (!oversized) resolve(Buffer.concat(chunks));
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function body(req: IncomingMessage, limit = JSON_BODY_LIMIT): Promise<Buffer> { | |
| return new Promise((resolve, reject) => { | |
| const declared = Number(req.headers["content-length"] || 0); | |
| if (declared > limit) { | |
| const error = new Error(`Request exceeds the ${Math.floor(limit / 1024 / 1024)} MB limit.`); | |
| error.name = "PayloadTooLargeError"; | |
| reject(error); | |
| return; | |
| } | |
| const chunks: Buffer[] = []; | |
| let received = 0; | |
| let oversized = false; | |
| req.on("data", (chunk: Buffer) => { | |
| received += chunk.length; | |
| if (received > limit) { oversized = true; chunks.length = 0; } | |
| else if (!oversized) chunks.push(chunk); | |
| }); | |
| req.on("end", () => { | |
| if (oversized) { | |
| const error = new Error(`Request exceeds the ${Math.floor(limit / 1024 / 1024)} MB limit.`); | |
| error.name = "PayloadTooLargeError"; | |
| reject(error); | |
| } else resolve(Buffer.concat(chunks)); | |
| }); | |
| req.on("error", reject); | |
| export function body(req: IncomingMessage, limit = JSON_BODY_LIMIT): Promise<Buffer> { | |
| return new Promise((resolve, reject) => { | |
| const payloadTooLarge = () => { | |
| const error = new Error(`Request exceeds the ${Math.floor(limit / 1024 / 1024)} MB limit.`); | |
| error.name = "PayloadTooLargeError"; | |
| return error; | |
| }; | |
| const declared = Number(req.headers["content-length"] || 0); | |
| if (declared > limit) { | |
| req.resume(); | |
| reject(payloadTooLarge()); | |
| return; | |
| } | |
| const chunks: Buffer[] = []; | |
| let received = 0; | |
| let oversized = false; | |
| req.on("data", (chunk: Buffer) => { | |
| received += chunk.length; | |
| if (received > limit && !oversized) { | |
| oversized = true; | |
| chunks.length = 0; | |
| reject(payloadTooLarge()); | |
| } | |
| else if (!oversized) chunks.push(chunk); | |
| }); | |
| req.on("end", () => { | |
| if (!oversized) resolve(Buffer.concat(chunks)); | |
| }); | |
| req.on("error", reject); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/http.ts` around lines 44 - 68, Update body to reject immediately
when received exceeds limit, while ensuring the promise rejects only once and
the request stream continues draining so the connection is handled cleanly. For
declared content lengths over the limit, reject immediately and keep consuming
the request rather than returning without attaching stream handlers; preserve
PayloadTooLargeError details for both declared and streamed oversized bodies.
Comment on lines
+72
to
+75
| export async function jbody(req: IncomingMessage): Promise<Record<string, unknown>> { | ||
| const raw = await body(req); | ||
| try { return JSON.parse(raw.toString() || "{}"); } | ||
| catch { return {}; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize valid non-object JSON input.
JSON.parse can return null or a scalar. The declared return type hides this condition. Routes then access fields such as b.username, which throws for null and returns a 500 response.
Return an empty record for non-object JSON roots.
Proposed fix
export async function jbody(req: IncomingMessage): Promise<Record<string, unknown>> {
const raw = await body(req);
- try { return JSON.parse(raw.toString() || "{}"); }
+ try {
+ const parsed: unknown = JSON.parse(raw.toString() || "{}");
+ return parsed && typeof parsed === "object" && !Array.isArray(parsed)
+ ? parsed as Record<string, unknown>
+ : {};
+ }
catch { return {}; }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function jbody(req: IncomingMessage): Promise<Record<string, unknown>> { | |
| const raw = await body(req); | |
| try { return JSON.parse(raw.toString() || "{}"); } | |
| catch { return {}; } | |
| export async function jbody(req: IncomingMessage): Promise<Record<string, unknown>> { | |
| const raw = await body(req); | |
| try { | |
| const parsed: unknown = JSON.parse(raw.toString() || "{}"); | |
| return parsed && typeof parsed === "object" && !Array.isArray(parsed) | |
| ? parsed as Record<string, unknown> | |
| : {}; | |
| } | |
| catch { return {}; } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/http.ts` around lines 72 - 75, Update the jbody function to
validate that the parsed JSON result is an object before returning it. After
JSON.parse succeeds, check whether the result is actually a Record (object
type), and return an empty record instead if the parsed value is null, an array,
or a scalar type. This ensures the function always returns a valid record
matching its declared return type, preventing downstream property access errors
like b.username from throwing on non-object JSON inputs.
Comment on lines
+78
to
+80
| export function requestAddress(req: IncomingMessage): string { | ||
| const forwarded = String(req.headers["cf-connecting-ip"] || "").trim(); | ||
| return /^[a-f0-9:.]{3,64}$/i.test(forwarded) ? forwarded : String(req.socket.remoteAddress || "unknown"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 --glob '*.{ts,js,mjs,json,yml,yaml}' \
'cf-connecting-ip|requestAddress\(|createServer\(|\.listen\(|reverse proxy|cloudflare' .Repository: gitcommit90/1Helm
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/server/http.ts ---'
sed -n '1,110p' src/server/http.ts
printf '%s\n' '--- src/server/index.ts server startup and imports ---'
sed -n '1,90p' src/server/index.ts
sed -n '450,575p' src/server/index.ts
sed -n '2185,2210p' src/server/index.ts
printf '%s\n' '--- address and rate-limit references ---'
rg -n -C 4 'requestAddress|rateLimited|HOST|PORT|server\.listen' src/server/index.ts src/server/http.ts
printf '%s\n' '--- deployment files mentioning the application port or proxy ---'
fd -t f . deploy config | sort | while read -r file; do
if rg -qi '8130|1helm|proxy|cloudflared|listen|localhost|127\.0\.0\.1|0\.0\.0\.0' "$file"; then
printf '\n### %s\n' "$file"
rg -n -C 3 -i '8130|1helm|proxy|cloudflared|listen|localhost|127\.0\.0\.1|0\.0\.0\.0' "$file"
fi
doneRepository: gitcommit90/1Helm
Length of output: 30229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- runtime and service configuration ---'
rg -n -C 4 --glob '!node_modules/**' --glob '!dist/**' \
'HELM_HOST|Environment=PORT|Environment=HELM|ExecStart=.*server|src/server/index|8123|reverse proxy|proxy_pass|cloudflared.*8123|127\.0\.0\.1:8123|0\.0\.0\.0' \
.github deploy scripts site package.json README.md docs container 2>/dev/null || true
printf '%s\n' '--- relevant deployment and installer files ---'
fd -t f . deploy scripts site docs container | sort | while read -r file; do
if rg -q 'HELM_HOST|8123|reverse proxy|proxy_pass|cloudflared|server/index|node .*server|PORT=' "$file"; then
printf '\n### %s\n' "$file"
rg -n -C 5 'HELM_HOST|8123|reverse proxy|proxy_pass|cloudflared|server/index|node .*server|PORT=' "$file"
fi
done
printf '%s\n' '--- Cloudflare connector target ---'
rg -n -C 6 'startTunnelConnector|localhost|127\.0\.0\.1|PORT|service|ingress|tunnel' \
src/server/cloudflare.ts src/server/connectors.ts cloudflare deploy 2>/dev/null || trueRepository: gitcommit90/1Helm
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
function requestAddress(req) {
const forwarded = String(req.headers["cf-connecting-ip"] || "").trim();
return /^[a-f0-9:.]{3,64}$/i.test(forwarded)
? forwarded
: String(req.socket.remoteAddress || "unknown");
}
const socket = { remoteAddress: "10.0.0.5" };
const headers = ["192.0.2.1", "198.51.100.7", "2001:db8::1", "not-an-ip"];
for (const value of headers) {
console.log(JSON.stringify({ header: value, key: requestAddress({ headers: { "cf-connecting-ip": value }, socket }) }));
}
console.log(JSON.stringify({
directListener: "0.0.0.0",
forwardedHeaderTrustedByCode: true,
attackerCanChooseDistinctRateLimitKeys: requestAddress({
headers: { "cf-connecting-ip": "192.0.2.1" }, socket
}) !== requestAddress({
headers: { "cf-connecting-ip": "198.51.100.7" }, socket
})
}));
JS
printf '%s\n' '--- production unit binding ---'
sed -n '68,88p' site/public/install-linux-units.sh
printf '%s\n' '--- Cloudflare tunnel target configuration ---'
sed -n '1,18p' deploy/config-1helm-site.yml.exampleRepository: gitcommit90/1Helm
Length of output: 1662
Spoofed Client Identity Used For Rate Limiting (CWE-345)
Reachability: External · Exploitability: Moderate
Reachability path
● Entry
src/server/index.ts:466
applyMobileCors
│
▼
● Sink
src/server/http.ts
Trust CF-Connecting-IP only from a trusted proxy.
The production service binds to 0.0.0.0, so a directly reachable client can submit different valid CF-Connecting-IP values and bypass access and login rate limits. Restrict port 8123 to the proxy, or accept this header only from trusted proxy addresses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/http.ts` around lines 78 - 80, Update requestAddress to trust
CF-Connecting-IP only when req.socket.remoteAddress belongs to the configured
trusted proxy addresses; otherwise use the socket’s remote address. Add or reuse
the trusted-proxy configuration for this validation, preserving the existing
format check for accepted forwarded values.
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
1 participant
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.
Outcome
Phase 6 reduces future change risk through three bounded, behavior-preserving extractions:
app.ts;server/index.ts;server/bots.ts.It also adds a concise module map and nonblocking architecture-size/cycle report for future agents. No framework, schema, API, UX, installer, or runtime behavior changes.
Verification
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests