fix: post-0.2.0 audit remediation — Express 4 crash, CLI env/flag contract, a11y, packaging guards#6
Merged
Merged
Conversation
peerDependencies and the README claim express >=4, but only express@5 was
ever exercised. Alias express4 (npm:express@^4) as a devDependency and run
the same conformance behaviors against both majors; only the catch-all
route syntax differs ('/{*path}' on 5, '*' on 4).
…ecting Express 4 does not route a rejected middleware promise to error handlers, so the rethrow on a non-413 body-read failure became an unhandled rejection and killed the process: one unauthenticated client opening a login POST and dropping the socket mid-body crashed the whole server. Wrap the body-read/ handle path in try/catch and hand every failure to next(error), the error contract on both majors; the BodyTooLargeError 413 branch is unchanged. Regression test drives both express majors with a raw-socket login POST that under-delivers Content-Length and then destroys the connection, asserting the error reaches next(error), the adapter promise never rejects, and the server keeps gating afterwards.
createGate only normalized sessionSeconds against NaN/Infinity, so a fractional value (e.g. one computed by division) minted a token whose stringified expiry fails verifyToken's Number.isSafeInteger check: login succeeds, the redirect fires, and the visitor bounces back to the login page forever. Floor the value, matching the nowSeconds convention and the integer Max-Age the cookie spec expects.
- Tie the failed-login notice to the input with aria-describedby and aria-invalid: role=alert on a server-rendered page never announces, and autofocus drops screen-reader users into the empty field with no audible failure indication. - Dark-mode error text #f87171 (6.40:1 on the card, was 3.67:1) and input border #71717a in both schemes (4.83:1 light / 3.67:1 dark, was 1.48:1 / 1.70:1) to clear WCAG AA and 1.4.11 non-text contrast. - Declare color-scheme (meta + :root) so UA surfaces follow the dark theme, and add a 100dvh min-height with 100vh fallback so the card centers under mobile dynamic toolbars.
…dCookie - Move the self-contained HTML section (renderDefaultLoginPage, renderNotConfiguredPage, documentShell, pageStyles, escapeHtml) into internal src/login-page.ts, bringing core.ts under the 500-line cap. escapeHtml is re-exported from core so the package-root import keeps working; tsup entries unchanged (the module lands in shared chunks, verified via build + smoke-dist). - Replace the two 'as number' index assertions with checked iteration that compiles under noUncheckedIndexedAccess; the constant-time comparison still walks the full length with no early exit. - Declare ImportMeta.env optional so the compiler enforces the runtime guard astro.ts already performs outside Vite builds. - Add readCookie unit tests pinning the contract every adapter depends on (absent cookie, name-prefix non-collision, valueless parts, whitespace, verbatim values).
Pure move of TARGETS and SNIPPETS so cli.ts stays inside the 500-line file budget as its validation logic grows.
Read and write env files the way dotenv (Next, Astro dev, wrangler) does: honor 'export ' prefixes, strip unquoted inline comments, expand \n only inside double quotes, and quote written values containing #, quotes, or edge whitespace so both parsers read back the same password. init no longer regenerates a secret or appends last-wins duplicates when the file uses export lines. Argument handling now matches the documented strict-flag guarantee: single-dash typos and stray positionals are hard errors, boolean flags reject unrecognized values (--insecure-cookie=yes), and an explicitly passed --env-file that does not exist throws instead of silently starting fail-closed (help notes Node's own --env-file pre-scan). Also: require an http(s) --origin with a friendly error, print the OS-assigned port for --port 0, say so when init skips .gitignore outside a git root, and drop the unreachable KNOWN_FLAGS help entry.
…client-drop teardown The proxy's most common real-world failure paths shipped untested: the 502 side of the catch handler, the !upstream.body early return (HEAD, 204), and the no-reply teardown when the client vanishes mid-upload. Extract the listen/proxy/loginCookie/rawRequest harness into a shared fixture so the new failure-mode file stays a focused module.
…eader RFC 7230 6.1 requires an intermediary to remove every header the Connection header names, not just the static hop-by-hop set, so `Connection: x-foo` no longer smuggles x-foo to the origin (or, on the way back, to the client). Defense in depth: undici re-frames the forwarded request, so no smuggle was demonstrated.
…minator deployments The proxy always overwrote X-Forwarded-* from the connecting socket, so behind the README's own recommended TLS terminator the origin was told every request arrived over plain http from the terminator's loopback — downgrading Secure-cookie logic and per-IP banning. With trustProxy the front hop's X-Forwarded-* pass through (peer appended to the For chain). Default stays the safe overwrite, and the docs now say the proxy reports proto=http unless the flag is set.
…constants The eight adapters each hand-wrote the same options type and SITEPASS_* credential block (already drifted into two bypass-normalization forms), and the bypass header, 64 KiB login cap, and request-target split were declared per file. One copy of each now lives in the internal shared modules: - web.ts: AdapterGateOptions, createGateFromEnv (owns the env var names and empty-bypass normalization), BYPASS_HEADER; toResponse is no longer exported (gateWebRequest is its only caller) - node-body.ts: splitRequestTarget and firstHeaderValue for the Node-side consumers, so the proxy's gate decision and forward target cannot diverge from each other or from Express - proxy.ts: the 10 MiB forward cap is renamed DEFAULT_MAX_FORWARD_BYTES so the shared DEFAULT_MAX_BODY_BYTES name keeps one meaning; stripCookie now matches via core's readCookie, so the strip rule is definitionally the accept rule - cloudflare.ts: the env cast becomes a checked assignment Per-adapter option type names stay as public aliases; the emitted d.ts surface is equivalent (verified via build + dist inspection).
…ed types
The unannotated export widened to { path: string }, so a consumer writing
the natural typed form `export const config: Config = netlifyConfig` in a
Deno-checked edge function got a type error: Netlify's Config types path as
a /${string} template literal. Annotating locally keeps the literal without
taking the @netlify/edge-functions dependency.
…edup proxy fixtures
Pins behavior that no test exercised:
- hono: c.env bindings win over process.env (the Workers path, where
process.env does not exist)
- netlify: an absent Netlify global fails closed with the 503 page instead
of a ReferenceError, and config.path keeps its /${string} literal type
- node-body: a mid-read socket 'error' rejects with that error, and an
error after end still resolves (the last uncovered function in the suite)
- adapter conformance: HEAD and OPTIONS without a cookie get the 401 login
result, documenting the CORS-preflight consequence
- core: a non-ASCII password with form-special characters round-trips
through the form-decode -> TextEncoder -> HMAC pipeline end-to-end
proxy.test.ts also moves its repeated origin+proxy setup into gatedProxy /
headerCapturingProxy fixtures so each test body shows only what it asserts.
…r compatibility_date Caret ranges don't float across 0.x minors, so ^0.1.0 could never resolve to the 0.2.0 security release. Also bump devDependencies to current majors (typescript ^6, vite ^8, plugin-react ^6) and add a minimal wrangler.jsonc so pages dev stops floating runtime semantics to today's date.
…in vite dev is ungated The middleware is two lines, not three; the README never mentioned the built-in logout endpoint; and npm run dev serves the SPA without the Pages Function, which deserves an explicit warning in a gating example.
GateResult, Gate/handle, GateOptions, GateRequest (+method), ProxyOptions (origin, port, and the proxy-specific maxBodyBytes meaning), and each adapter's options alias now carry hover docs; netlify's config gets a /** */ doc so the re-export instruction surfaces in editors. Also corrects the segment-matching example in core's publicPaths comment: /apixyz never demonstrated the rule, /api/webhooksxyz does.
…in .env.example - The proxy section no longer implies maxBodyBytes is the 64 KiB login cap: on the proxy it caps the forwarded body (10 MiB default). - The CLI reference and printHelp both list --help | --version (-h / -v) and note Node's own --env-file pre-scan, which fires for the installed bin too, not just node dist/cli.js (verified on Node 22). - The publicPaths example now demonstrates segment matching: /api/webhooks covers /api/webhooks/stripe but not /api/webhooksxyz (verified against dist; /apixyz proved nothing a prefix match would not). Test 7 pins the cited example. - .env.example lists the optional SITEPASS_BYPASS_TOKEN every adapter and the proxy CLI read.
Security (Express 4 unhandled-rejection crash, proxy Connection-named hop-by-hop headers), Fixed (fractional sessionSeconds, login-page a11y, dotenv-compatible env files, strict CLI args, netlify config.path type, README/docs corrections, example pins), Added (trustProxy / --trust-proxy).
Strict positional parsing ran before the help-command check, so `sitepass help init` exited 1 with a confusing flag error; help's positionals are now dropped before parsing (--version still wins). A value flag given bare (`--env-file` with nothing after it) parsed as boolean true and collapsed to the implicit default, silently ignoring explicit user intent — it is now a hard error for every value flag.
…trictness A pre-0.2.1 env file holding an unquoted value with '#' in it now reads truncated at the '#' (the value dotenv-following frameworks were already reading) — say so, with the quote-it remedy, instead of leaving the consequence for existing files implicit.
The local joiner and node-body's firstHeaderValue had near-identical names with opposite duplicate semantics (join per RFC 9110 5.3 vs first-wins), and nothing but the name stops a future swap — make the difference self-documenting.
…ugh the exports map
…s require('url') shim
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (37)
📝 WalkthroughWalkthroughThis PR refactors all adapters to use shared ChangesAdapter consolidation and web abstraction
Proxy hardening and CLI refactoring
Test infrastructure and coverage expansion
Build configuration, workflows, and documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
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.
Context
A full multi-agent audit of the codebase and the published npm artifact ran after the 0.2.0 release (68 adversarially-verified findings: 1 high / 10 medium / 32 low / 25 suggestions). This branch fixes every defect-tier finding plus the zero-risk suggestions. The API-design suggestions (programmatic mint/verify, multi-password, cookie
Domain, etc.) are deliberately not included — they deserve their own discussion.Security
POSTand dropping the socket killed the server as an unhandled rejection. Errors now go tonext(error)(correct on both majors), and the adapter conformance suite runs against both express@4 and express@5.Connectionheader are stripped in both directions (RFC 7230 §6.1) on top of the static hop-by-hop list.Fixed (highlights)
sessionSecondsminted tokens that never validated → silent permanent login loop. Now floored.aria-describedby/aria-invalid(server-renderedrole="alert"is never announced), WCAG AA contrast in both schemes,color-scheme,100dvh.exportprefixes, quotes, inline comments) and values are quoted on write — so the passwordinitwrites is the password the framework reads, andinitnever regenerates an existing secret. Strict argument handling: positionals, single-dash typos, bad boolean values, bare value flags, and missing--env-fileare all hard errors;--originis validated at startup;--port 0reports the real bound port.^0.2.0(a^0.1range can never resolve to 0.2.x), pins a wranglercompatibility_date, and documents/__gate/logout.publicPathsexample, and.env.examplecorrected against actual behavior; JSDoc on the whole public type surface.Added
trustProxyoption /--trust-proxyflag for TLS-terminator deployments (off by default).Packaging / supply chain
smoke-distnow packs a real tarball, installs it into a temp consumer, and imports all 10 subpaths through the exports map in both ESM and CJS.npm publish ./sitepass-*.tgz) and pins the npm upgrade instead ofnpm@latest.node:-prefixed builtin imports; the Astro CJS build sheds a deadimport.metabranch and itsrequire('url')shim.noImplicitReturnson;ignoreDeprecationsremoved from the main typecheck and scoped to tsup's dts build (which injects a deprecatedbaseUrl).Internal quality
src/login-page.ts(all source files now ≤ 500 lines), avoidable type assertions removed, adapter options/env wiring/gate constants single-sourced across all 8 adapters, shared proxy test fixtures.Upgrade notes (in CHANGELOG)
#in a pre-existing env file now reads truncated at the#— the value frameworks were already reading; quote it to keep the#.These behavior changes likely warrant a 0.3.0 rather than 0.2.1, per the 0.2.0 precedent.
Verification
Summary by CodeRabbit
New Features
--trust-proxyflag for deployments behind TLS-terminating proxies--bypass-tokenflag support for token-based authenticationBug Fixes
Documentation