Repository navigation
refactor(middleware): convert protect, headers, and missing middlewares to TypeScript - #615
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates several middleware modules (headers, missing, and protect) and their corresponding unit tests from JavaScript to TypeScript, introducing robust type definitions and expanding test coverage. The review feedback highlights a few key improvement opportunities: adding a defensive check in headers.ts to prevent a potential runtime crash if a header key is missing, simplifying the object mapping logic in normalizedConfigHeaders to reduce redundancy, and replacing the deprecated url.parse API with the modern URL constructor.
joehan
left a comment
joehan
left a comment
There was a problem hiding this comment.
Code Review: PR #615 (refactor(middleware): convert protect, headers, and missing middlewares to TypeScript)
Strengths & LGTM Aspects
- Consolidates three key middlewares into TypeScript with clean structure.
- Adopts WHATWG
URLfor modern URL parsing. - Expands unit test coverage for headers and missing handlers.
Key Findings to Address
- Security-header bypass via backslash authority confusion (
headers.ts): Passing raw URLs tonew URL(normalizedUrl, "http://localhost")beforeslashPathallowsGET /�dmin/secret.jsto parse"admin"as the hostname and"/secret.js"as the pathname. Headers configured for/admin/**(CSP, X-Frame-Options, Cache-Control: no-store) are completely bypassed, while downstream middlewares serve/admin/secret.jswith200 OK. - Fail-open authentication bypass in
protect.ts: Changingspec.protect || config.protecttospec.protect ?? req.superstatic?.protectcauses{ protect: "" }to evaluate to"", callingnext()and bypassing authentication entirely. - Server crash and password leak in
protect.ts: Colon-less credentials crash the server synchronously on the first request. Alsocredentials.split(":")truncates passwords containing:and leaks the password suffix in cleartext inside theWWW-Authenticateheader. connect().use()failsTS2769understrictFunctionTypes: Typing handler parameters asSuperstaticRequestrather thanIncomingMessage & Partial<SuperstaticRequest>breaksconnect().use(missing())andconnect().use(protect()).entry.globis unslashed:entry.sourceis normalized withslashPath, butentry.globis left unslashed.
| provider?: unknown; | ||
| } = {}, | ||
| ): ( | ||
| req: SuperstaticRequest, |
There was a problem hiding this comment.
🟡 [P2 Typing] SuperstaticRequest breaks connect().use(missing()) under strictFunctionTypes
Requiring req: SuperstaticRequest, res: SuperstaticResponse causes connect().use(missing()) to fail compilation with TS2769: No overload matches this call because connect.IncomingMessage is not assignable to SuperstaticRequest.
Typing parameters as connect.IncomingMessage & Partial<SuperstaticRequest> and ServerResponse & Partial<SuperstaticResponse> allows seamless usage with Connect.
There was a problem hiding this comment.
Addressed in 019e85a: typed middleware handler parameters across headers.ts, missing.ts, and protect.ts as req: connect.IncomingMessage & { superstatic?: Configuration }, res: ServerResponse & { superstatic?: Responder }, next: connect.NextFunction, and verified direct connect().use(...) calls without casts.
Converts src/utils/patterns.js to TypeScript with typed pattern matching utilities and adds unit tests in test/unit/utils/patterns.spec.ts.
b93843e to
9b1084a
Compare
…es to TypeScript
Converts src/middleware/protect.js, src/middleware/headers.js, src/middleware/missing.js, and their unit specs to TypeScript (src/middleware/protect.ts, src/middleware/headers.ts, src/middleware/missing.ts, test/unit/middleware/headers.spec.ts, and test/unit/middleware/missing.spec.ts).
- Reuses Configuration, Header, Matcher, SuperstaticRequest, and SuperstaticResponse types and patterns.configMatcher from src/utils/patterns.ts.
- Defaults spec = {} across protect, headers, and missing factories and preserves CommonJS middleware exports for unconverted JS callers.
- Guard headerToSet?.key before calling toLowerCase() when filtering with allowedHeaders - Simplify normalizedConfigHeaders object construction with conditional source spread - Replace deprecated url.parse with WHATWG URL in extractPathname with double-slash and invalid-URL handling
… and missing
- Parse only absolute URIs via URL() in headers extractPathname so backslash paths and decoded non-ASCII error paths normalize consistently via slashPath
- Normalize entry.glob alongside entry.source in normalizedConfigHeaders
- Fall back to req.superstatic.protect when spec.protect is empty string and split credentials on the first colon only, returning 401 when missing a colon
- Type middleware handlers with connect.IncomingMessage & { superstatic?: Configuration } and ServerResponse & { superstatic?: Responder } across headers, missing, and protect
- Add unit tests covering backslash security headers, non-ASCII paths, unslashed globs, Connect direct mounting, and protect middleware edge cases
03b6579 to
019e85a
Compare
Converts
src/middleware/protect.js,src/middleware/headers.js,src/middleware/missing.js, and their unit specs to TypeScript (src/middleware/protect.ts,src/middleware/headers.ts,src/middleware/missing.ts,test/unit/middleware/headers.spec.ts, andtest/unit/middleware/missing.spec.ts).Configuration,Header,Matcher,SuperstaticRequest, andSuperstaticResponsetypes andpatterns.configMatcherfromsrc/utils/patterns.ts.spec = {}acrossprotect,headers, andmissingfactories and preserves CommonJS middleware exports for unconverted JS callers.