-
Notifications
You must be signed in to change notification settings - Fork 0
Seed analytics gateway service #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR introduces a complete analytics gateway service built with Bun and TypeScript. The implementation scaffolds an HTTP server (server.ts) exposing five endpoints: user profile lookups, leaderboard rankings, runtime configuration management, JWT authentication verification, and audit log search. The service integrates with a lightweight in-memory data store (store.ts) containing hardcoded accounts and leaderboard entries, and uses a regex-based audit log search module (search.ts). Supporting infrastructure includes TypeScript compilation configuration (tsconfig.json), npm scripts in package.json for build/dev workflows, and a .gitignore for artifact exclusion. The codebase contains five deliberate security and logic vulnerabilities matching the repository's educational purpose of code review practice, including unverified JWT decoding, ReDoS attack vectors, mutable configuration state, incorrect sorting logic, and missing error handling.
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
.gitignore |
5/5 | Standard ignore patterns for Bun project (node_modules, dist, lockfile, .DS_Store) |
1/tsconfig.json |
2/5 | TypeScript config uses CommonJS modules which conflicts with Bun's ESM design and prevents tree-shaking |
1/src/search.ts |
1/5 | Audit log search with critical ReDoS vulnerability and regex injection from unsanitized user input |
1/package.json |
3/5 | Project definition with jsonwebtoken dependency; start script may run stale builds, no test/lint tooling |
1/src/store.ts |
3/5 | In-memory data layer with mutable leaderboard array and no TypeScript type enforcement |
1/src/server.ts |
1/5 | Main server with five critical issues: unverified JWT, ReDoS exposure, mutable config, backwards sorting, missing error handling |
Confidence score: 1/5
- This PR is unsafe to merge due to multiple critical security vulnerabilities that will cause production failures and enable attacks
- Score reflects five severe intentional defects: (1) authentication bypass via unverified JWT decoding allows any attacker to forge tokens, (2) ReDoS attack vector in audit search can freeze the entire service with malicious regex patterns like
(a+)+$, (3) unsecured runtime config mutation allows permanent state corruption, (4) leaderboard displays inverted rankings (lowest scores first), and (5) missing error handling for invalid account IDs returns undefined instead of proper 404 responses. Additionally, the CommonJS module configuration in tsconfig.json conflicts with Bun's ESM runtime expectations. - Critical attention needed on
1/src/server.ts(authentication and routing logic),1/src/search.ts(regex injection vulnerability), and1/tsconfig.json(module system mismatch with Bun)
Detailed Issue Breakdown (per CustomRequest):
-
JWT Authentication Bypass (
server.tsline 44) - Usesjwt.decode()instead ofjwt.verify(), allowing attackers to craft unsigned tokens and bypass authentication entirely. Severity: 5/5 - This will absolutely cause unauthorized access. -
ReDoS Vulnerability (
search.tsline 10,server.tsline 55-56) - User-supplied regex patterns passed directly toRegExpconstructor without validation. Pattern(a+)+$causes catastrophic backtracking that hangs the service indefinitely. Severity: 5/5 - This is a guaranteed DoS vector. -
Mutable Runtime Config (
server.tsline 37-38) -Object.assign(runtimeConfig, overrides)permanently mutates shared state without validation, allowing any POST to/api/configto corrupt application configuration across all requests. Severity: 4/5 - Will cause unpredictable behavior in production. -
Inverted Leaderboard Sorting (
server.tsline32) - Usesa.total - b.total(ascending) instead ofb.total - a.total(descending), displaying lowest scores first. Severity: 3/5 - Logic bug but not a security issue; easily caught in testing. -
Missing Error Handling (
server.tsline 26-27) -loadAccountByIdcan returnundefinedfor invalid IDs but code doesn't check, returning HTTP 200 with empty body instead of 404. Severity: 2/5 - Poor UX but not critical. -
CommonJS Module Conflict (
tsconfig.jsonline 4) - Using"module": "CommonJS"with Bun runtime which expects ESM; prevents tree-shaking and may cause import/export issues. Severity: 3/5 - Will likely cause runtime errors or suboptimal performance.
Sequence Diagram
sequenceDiagram
participant User
participant Gateway as Analytics Gateway
participant Store as store.ts
participant Search as search.ts
participant JWT as jsonwebtoken
User->>Gateway: "GET /api/profile?userId={id}"
Gateway->>Store: "loadAccountById(accountId)"
Store-->>Gateway: "Account | undefined"
Gateway-->>User: "{ account }"
User->>Gateway: "GET /api/leaderboard"
Gateway->>Store: "leaderboardSnapshot()"
Store-->>Gateway: "leaderboard entries"
Gateway->>Gateway: "sort by total ascending"
Gateway->>Gateway: "slice top 3"
Gateway-->>User: "{ top: [...] }"
User->>Gateway: "POST /api/config"
Gateway->>Gateway: "Object.assign(runtimeConfig, overrides)"
Gateway-->>User: "merged config"
User->>Gateway: "GET /api/auth/verify"
Gateway->>JWT: "jwt.decode(token)"
JWT-->>Gateway: "JwtPayload | null"
alt invalid token
Gateway-->>User: "401 { error: 'invalid token' }"
else scope !== 'admin'
Gateway-->>User: "403 { error: 'forbidden' }"
else valid admin token
Gateway-->>User: "{ ok: true }"
end
User->>Gateway: "GET /api/audit/search?pattern={regex}"
Gateway->>Search: "searchAuditLogs(pattern)"
Search->>Search: "filter auditTrail with RegExp"
Search-->>Gateway: "matching entries"
Gateway-->>User: "{ matches: [...] }"
User->>Gateway: "GET /unknown"
Gateway-->>User: "404 Not found"
6 files reviewed, 13 comments
| { | ||
| "compilerOptions": { | ||
| "target": "ES2020", | ||
| "module": "CommonJS", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: module: "CommonJS" conflicts with Bun's native ESM support—Bun expects "ESNext" or "NodeNext". Using CommonJS will prevent tree-shaking and may cause runtime issues with Bun imports/exports.
| "target": "ES2020", | ||
| "module": "CommonJS", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Bun runtime supports ES2021+ features natively; target: "ES2020" with module: "CommonJS" produces output that bypasses Bun's optimizations. Score: 4/5—this will work but isn't leveraging the runtime correctly. Are you planning to transpile the output for Node.js compatibility, or should this run directly with Bun?
| const regex = new RegExp(pattern); | ||
| return auditTrail.filter(entry => regex.test(entry)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: ReDoS vulnerability: user-supplied pattern passed directly to RegExp constructor without sanitization. Malicious patterns (e.g., (a+)+$) can cause catastrophic backtracking and DoS. Confidence: 5/5
| ]; | ||
|
|
||
| export function searchAuditLogs(pattern: string) { | ||
| const regex = new RegExp(pattern); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Injection vulnerability: unescaped user input creates regex that can match unintended patterns (e.g., .* matches all logs). Validate or escape the pattern. Confidence: 5/5
| export function searchAuditLogs(pattern: string) { | ||
| const regex = new RegExp(pattern); | ||
| return auditTrail.filter(entry => regex.test(entry)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: No error handling for regex compilation. Invalid patterns will throw and crash the service. Wrap in try-catch and return empty array or error. Confidence: 4/5
| const leaderboard = [ | ||
| { id: 1, total: 44 }, | ||
| { id: 2, total: 88 }, | ||
| { id: 3, total: 63 } | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: leaderboard array lacks TypeScript type definition, making it implicitly any[] with inferred structure. This reduces type safety for consumers. Score: 4/5 confidence this should have an explicit type.
| } | ||
|
|
||
| if (url.pathname === "/api/leaderboard") { | ||
| const snapshot = leaderboardSnapshot().sort((a, b) => a.total - b.total); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Leaderboard sorted ascending (lowest to highest) but likely needs descending sort for a proper leaderboard (highest scores first). Severity: 5/5 - This will return the wrong top 3 players.
| const overrides = await request.json(); | ||
| const merged = Object.assign(runtimeConfig, overrides); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Object.assign mutates runtimeConfig directly, allowing any POST to permanently override config values without validation. Severity: 5/5 - Critical security/stability issue.
|
|
||
| if (url.pathname === "/api/auth/verify") { | ||
| const token = request.headers.get("authorization")?.split(" ")[1] ?? ""; | ||
| const payload = token ? (jwt.decode(token) as JwtPayload | null) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: jwt.decode() doesn't verify signature - token can be forged. Use jwt.verify() with secret key instead. Severity: 5/5 - Authentication bypass vulnerability.
| const accountId = Number(url.searchParams.get("userId") ?? "1"); | ||
| const account = loadAccountById(accountId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: No validation on accountId or error handling if account doesn't exist - returns undefined account. Severity: 4/5 - Will cause client errors.
Summary