Audit remediation: security hardening, power-user options, packaging & docs#2
Merged
Conversation
…ry footguns - reject literal dot-segments and backslash segments before prefix matching: the reverse proxy and Express adapters feed the raw request target into the gate, so /assets/../secret matched an /assets prefix verbatim while resolving elsewhere at the origin (the encoded form was already rejected in 0.1.1) - treat a '/' entry as an exact match on the root path: trailing-slash normalization left an empty base whose prefix check matched every path, silently un-gating the whole site; empty entries are now ignored - handle the login POST before publicPaths matching, so an entry covering loginPath can no longer make login impossible
…handler, options) signature The Targets table marked SvelteKit as gating static output, but hooks.server.ts only runs for server-rendered requests: prerendered pages and /_app client assets bypass it, the same boundary the README already documents for Astro. The Configuration section also implied every adapter takes gate(options), which mints a broken gate on Bun where the handler comes first.
…etadata dist/proxy.* already shipped in the tarball but no './proxy' entry existed in the exports map, so startProxy (which accepts the full gate options) was unreachable dead weight. Also add sideEffects: false (publint's one suggestion; every module is a pure factory) and ship CHANGELOG.md — 0.1.1 is an 'upgrading recommended' security release consumers couldn't read from the installed package.
…version match Tests import src/, so the 10 dual-format entry points shipped without ever being executed — a tsup or exports-map regression would publish green. scripts/smoke-dist.mjs imports every dist entry in both formats (tolerating only host-resolved specifiers: $env/dynamic/private, next/server), asserts the expected export, and runs the CLI. The publish workflow now refuses a tag that doesn't match package.json and builds + smokes the exact artifact before npm publish; the trigger is tightened to v[0-9]*.
…oken, cookieSecure, hooks
- tokens now sign expiry + a password digest, so rotating SITEPASS_PASSWORD
(the obvious lever when the shared password leaks) invalidates every
outstanding session instead of leaving cookies valid for up to 7 days;
existing sessions get one forced re-login on upgrade
- GET/POST {loginPath}/logout clears the cookie and redirects to /
- bypassToken option + x-sitepass-bypass header let CI, E2E runs, and uptime
monitors through without scripting the login form (constant-time compare,
same shape as the password check)
- cookieSecure: false escape hatch for plain-HTTP LAN deployments, which
previously failed as a silent login loop (default unchanged: Secure on)
- secrets shorter than 16 chars now count as unconfigured and fail closed
instead of minting tokens with a brute-forceable HMAC key
- onAuthFailure observer for fail2ban-style logging; renderLoginPage escape
hatch for custom/localized login pages
- JSDoc on password/secret/brand, readonly publicPaths and Gate fields
- regression tests: malformed tokens, CR/LF next injection, all of the above
…everywhere, type-boundary fixes All 7 web adapters now delegate to an internal src/web.ts (gateWebRequest / toResponse), deleting the ~20-line GateRequest/GateResult translation that was copied per adapter — and with it the per-adapter isLoginPost re-implementation. The helper reads the login body incrementally with a 64 KiB cap, so bun and hono (self-hosted runtimes with no platform cap) get the same 413 fail-closed behavior express already had; every adapter now accepts maxBodyBytes. The byte-identical body readers in proxy.ts/express.ts moved to src/node-body.ts. Both internal modules are bundled into shared chunks, not dist entries. Type-boundary fixes, verified against consumer annotations: - cloudflare: env typed as named optional unknown fields instead of an index signature, so 'PagesFunction<Env>' annotations compile; non-string bindings count as unset (fail closed) instead of flowing into the gate - bun: gate() is generic over the handler's rest args, so '(req, server)' websocket-upgrade handlers keep their signature and receive 'server' - netlify: the Netlify global is typeof-guarded — outside the Edge runtime the gate fails closed (503) instead of throwing ReferenceError - every adapter exports a named XxxGateOptions type and its context interface - express: returns express's own RequestHandler type; reads x-sitepass-bypass Tests: per-adapter blocks replaced by a conformance runner (each adapter supplies an ~10-line driver); conformance now also asserts the 413 body cap, logout, and bypass-token behavior on all 7 adapters; cloudflare gains adapter-level fail-closed tests; shared credentials moved to test/fixtures.
… the forward path The proxy's own session token no longer leaks to the origin (anything logging request headers there would capture a replayable credential); other cookies forward unchanged, and the Cookie header is dropped when the gate cookie was the only one. The origin now receives X-Forwarded-For/-Proto/-Host, so its logs, rate limiting, and absolute-URL generation see the real client. Also passes the bypass header through to the gate and types handle() with the exported Gate interface. New forward-path tests: byte-for-byte body forwarding, host pinning + hop-by-hop stripping + cookie stripping + X-Forwarded-* assertions via an origin header echo, the unauthenticated login-body 413 cap, and a rewritten mid-body reset test that actually exercises the streaming pipeline catch (headers flushed and first chunk read before the upstream is destroyed).
…strict flags, unit tests - 'sitepass --help' and '-h' print usage and exit 0 (previously 'Unknown command' + exit 1); '--version'/'-v' print the package version (injected by tsup at build time); 'sitepass init --help' prints usage instead of starting an interactive init - the proxy accepts the gate options the README promises for every deployment mode: --public-paths, --login-path, --cookie-name, --session-seconds, --bypass-token (or SITEPASS_BYPASS_TOKEN), and --insecure-cookie for plain-HTTP LAN origins; --port is validated instead of silently defaulting - unknown flags are a hard error naming the known set, so a typo'd --public-paths can never be silently ignored - --env-file redirects both init's write target and the proxy's env source; .env loading no longer depends on process.loadEnvFile (Node >=20.12 only, silently absent on 20.0-20.11) — a dependency-free parser replaces it - startup warns when either SITEPASS_PASSWORD or SITEPASS_SECRET is missing, naming the variable (previously only a missing secret warned) - the entrypoint runs only when executed as a script (realpath-aware is-main guard), making the helpers importable: parseFlags, upsertEnv (including the 0600-permission regression test for the 0.1.1 fix), ensureGitignored, readEnvValue, loadDotenv now have unit tests; upsertEnv no longer leaves a blank line when appending to a newline-terminated file - every adapter reads SITEPASS_BYPASS_TOKEN from its environment, so the bypass token needs no code change to enable - tsconfig gains exactOptionalPropertyTypes (optionals widened with '| undefined' so consumer assignments stay compatible); vitest alias uses fileURLToPath so the suite resolves on Windows checkouts; coverage floors raised to 90/79/93/93 to match actual coverage (94.8/84.5/97.5/97.3)
…dabot; issue/PR templates - PR branches run CI once (pull_request) instead of twice (push duplicated every PR run, confirmed in the run history); superseded pushes cancel in-flight runs; every job gets timeout-minutes: 10 instead of GitHub's 360-minute default - the quality job pins Node 22 via setup-node — the coverage gate previously ran on whatever Node shipped with the rotating ubuntu-latest image, and v8 coverage output shifts between Node majors - dependabot keeps the SHA-pinned actions fresh (it updates pin + version comment together) and groups weekly dev-dependency bumps; without it the pins rot silently - bug-report template asks the first triage question (which adapter?) up front and routes security reports to private reporting; PR template points at the CONTRIBUTING guardrails - package.json: keywords now cover all shipped adapters (express, hono, bun) plus password-protection/staging; @types/express added as an optional peer so TS consumers learn dist/express.d.ts needs it instead of hitting TS2307
…ed changelog README gains badges, a Netlify quickstart (the required config re-export was documented nowhere user-facing), a CLI reference, a core-API section for custom adapters, framework version requirements in the Targets table, and sections for the bypass token, logout, renderLoginPage, and onAuthFailure. The Configuration block now lists every real option including maxBodyBytes. The localhost TLS guidance is corrected (wrangler pages dev serves plain HTTP; Chrome/Firefox exempt localhost from the Secure requirement, Safari doesn't). SECURITY.md gains an email fallback for the reporting channel and a rotation section documenting that password or secret rotation revokes all sessions. CONTRIBUTING's adapter contract reflects the shared web/node-body plumbing and the conformance-driver test pattern. CHANGELOG documents everything on this branch including the two deliberate behavior changes, and 0.1.0 gets its date and link. escapeHtml is exported for renderLoginPage implementers.
…ode10 resolution attw reported 'node10: resolution failed' for every subpath: consumers on TypeScript <= 5.x with moduleResolution 'node' (still the default for plain module:commonjs tsconfigs there) got TS2307 for sitepass/express et al, even though Node itself resolves them via the exports map at runtime. attw is now fully green across node10/node16-cjs/node16-esm/bundler for all 11 entries.
Security:
- isPublicPath: reject encoded backslash (%5c), stray/double percent (%25),
and path-parameter (';') segments too — the prior pass caught literal
../backslash and %2e/%2f but missed /assets/..%5c..%5csecret and double
encoding. Broadened the encoded-traversal regex and the literal-segment scan.
- proxy: X-Forwarded-For/-Proto/-Host are now set authoritatively — inbound
client values are stripped first, so a client can no longer spoof its IP or
inject x-forwarded-host (cache poisoning / host-header injection) to the
origin. The previous version appended to client XFF and deferred proto/host
to the client, which for a front-most gate proxy is exactly backwards.
- proxy: the x-sitepass-bypass credential is stripped before forwarding,
alongside the gate cookie — it was leaking to origin logs.
- proxy: the login POST body is capped at 64 KiB independently of the (10 MiB)
forward-body limit, so an unauthenticated login POST can't buffer to 10 MiB.
Correctness:
- core: a NaN/Infinity sessionSeconds falls back to the default instead of
minting a 'NaN'-expiry token that never validates (silent login loop); the
CLI rejects a non-numeric/non-positive --session-seconds like it does --port.
- cloudflare: the PagesContext.env fix was still a weak type (all-optional),
so 'PagesFunction<Env> = gate()' still failed for a bindings-only Env. env is
now typed 'object' (any binding interface satisfies it) and cast internally;
verified compiling against @cloudflare/workers-types.
- core: logout responds to GET/POST only, so it no longer shadows a consumer
route on other verbs and narrows the forced-logout surface.
- cli: 'sitepass init' warns when it keeps an existing sub-16-char secret
(which now fails closed); --insecure-cookie=true is honored (was silently
ignored — only the bare flag worked).
- proxy: ProxyOptions.maxBodyBytes gains '| undefined' to match the rest of the
surface under exactOptionalPropertyTypes.
Polish:
- module-header comments on core.ts/proxy.ts converted to plain // so the
emitted d.ts attaches docs to createGate/startProxy, not GateOptions/ProxyOptions.
- docs: publint now runs in publish.yml (not just CI); CONTRIBUTING's adapter
snippet has the required await; CHANGELOG corrected (proxy 64 KiB login cap,
typesVersions bullet, authoritative X-Forwarded) and flags this as a 0.2.0
release; CONTRIBUTING build line notes the CLI is ESM-only.
Tests: encoded-backslash/double-encoding/path-param traversal, logout method
restriction, NaN sessionSeconds fallback, bypass-header stripping, X-Forwarded
spoof rejection, flagEnabled. 98 tests; coverage 95.0/85.1/97.5/97.3.
|
Warning Review limit reached
More reviews will be available in 37 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR introduces significant hardening to the sitepass authentication library. The core API now supports bypass tokens, session logout, password rotation invalidation via token-embedded password tags, auth failure observers, custom login page rendering, and stricter path traversal validation. All framework adapters are refactored to delegate request handling to a shared ChangesSitepass v0.1 hardening and adapter consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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 unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/publish.yml (1)
22-24:⚠️ Potential issue | 🟠 MajorHarden publish workflow by disabling checkout credential persistence and Bun executable caching.
actions/checkout@v4defaultspersist-credentials: true, persisting auth material in the runner’s git config for the job.oven-sh/setup-bun@v2defaultsno-cache: false, enabling caching/reuse of the downloaded Bun executable across workflow runs.Suggested patch
- - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + persist-credentials: false + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + no-cache: true🤖 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 @.github/workflows/publish.yml around lines 22 - 24, Update the workflow steps that call actions/checkout@v4 and oven-sh/setup-bun@v2: for the actions/checkout step (identified by uses: actions/checkout@v4) add with: persist-credentials: false to prevent persisting runner git credentials, and for the oven-sh/setup-bun step (uses: oven-sh/setup-bun@v2) add with: no-cache: true to disable reuse/caching of the Bun executable across runs; leave other steps (e.g., actions/setup-node) unchanged.src/express.ts (1)
47-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the connection after rejecting an oversized login body.
readRawBody()pauses the request once the cap is hit. Sending a plain413here withoutConnection: closeor an explicit socket teardown leaves unread bytes on a keep-alive connection, so a client can keep that socket occupied by continuing the oversized upload. Mirror the proxy path and close the connection after flushing the413.🤖 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/express.ts` around lines 47 - 55, In the BodyTooLargeError branch inside the isLoginPost handling, ensure the connection is closed after sending the 413: set the response Connection: close header (e.g., via res.set('Connection', 'close') or res.setHeader) before calling res.status(413).type('text/plain').send('Payload too large'), and teardown the socket once the response is flushed (attach a finish listener to destroy the request/response socket, e.g., res.on('finish', () => req.socket.destroy())). This change should be applied around the readRawBody error handling where BodyTooLargeError is caught so unread bytes can't occupy a keep-alive connection.
🤖 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 @.github/ISSUE_TEMPLATE/bug_report.yml:
- Around line 38-42: Update the SECURITY.md link in the issue template so it
resolves correctly from .github/ISSUE_TEMPLATE: replace the current
../blob/main/SECURITY.md with a working path (either the repository-root
relative ../../SECURITY.md or an absolute URL like
https://github.com/PeterM45/sitepass/blob/main/SECURITY.md) in the markdown
block that starts with "**Security issues:** please do not file them here — use
the private reporting channel described in [SECURITY.md]"; after changing the
link, create a test issue using the template to verify the link opens the
SECURITY.md page.
In @.github/workflows/ci.yml:
- Around line 28-29: Update the GitHub Actions steps that use actions/checkout
and oven-sh/setup-bun: for the actions/checkout steps (identified by uses:
actions/checkout@...), add a with block setting persist-credentials: false to
avoid leaving auth/SSH credentials on the runner; for the oven-sh/setup-bun
steps (identified by uses: oven-sh/setup-bun@...), add a with block setting
no-cache: true to disable Bun executable caching. Ensure you apply these changes
to both occurrences of each action in the workflow so the runner and cache
hardening are consistent.
In `@CHANGELOG.md`:
- Around line 147-148: Update the inconsistent link target for the 0.1.0 entry
so it matches the 0.1.1 pattern: replace the npm URL used in the [0.1.0]
reference with the GitHub release tag URL (e.g., change
"https://www.npmjs.com/package/sitepass/v/0.1.0" to
"https://github.com/PeterM45/sitepass/releases/tag/v0.1.0") so both [0.1.1] and
[0.1.0] point to GitHub release tags.
In `@README.md`:
- Around line 206-217: The fenced code block under "CLI reference" that shows
the sitepass commands (the block containing "sitepass init [--target
<name>]..." and "sitepass proxy --origin <url>...") needs a language identifier
to satisfy linters; update the opening triple backticks to include a language
such as text or sh (e.g., change ``` to ```text) so the CLI usage block is
annotated while keeping the content unchanged.
In `@src/cli.ts`:
- Around line 99-109: runInit() and the helpers readEnvValue()/upsertEnv() are
out of sync with loadDotenv(): they only match literal "KEY=" lines and don't
trim whitespace or strip surrounding quotes, causing duplicate keys and wrong
length checks; change readEnvValue and upsertEnv to reuse the same parsing logic
as loadDotenv() (or call a shared parser) so they accept optional whitespace
around '=' and strip matching single/double quotes from values, ensure
generateSecret()/secret assignment and the short-secret length check use the
parsed value (not the raw line), and apply the same fix to the other occurrences
noted (around the other blocks referenced).
- Around line 183-200: startProxy currently returns immediately after calling
server.listen, causing the CLI to log success before the socket is bound and
leaving no error handler; change start-up to capture and return the created
server, attach an 'error' handler to the server to handle bind failures, and
wait for the 'listening' event before printing the banner so you can read the
actual bound port from server.address() (handles ephemeral port 0 correctly) —
update the call site around startProxy/startProxy(...) to use the returned
server (or a Promise that resolves on 'listening') and emit the console.log only
after the server emits 'listening' using server.address() for the port, and
ensure errors are handled to avoid uncaught exceptions.
In `@src/core.ts`:
- Around line 199-205: The current call to options.onAuthFailure(request) passes
the full GateRequest (including urlencoded form body and cookies) which may leak
secrets; change the call to pass a redacted/filtered object or change the
callback type to accept non-secret metadata only (e.g., { method, url/path,
headersExceptCookies, ip, timestamp, userAgent, maybe usernameOnlyIfNeeded })
instead of the raw GateRequest; locate the failure path around
isCorrectPassword(...) and replace the request argument with a constructed
redactedRequest that explicitly omits the form body and session/cookie fields
before invoking options.onAuthFailure, or update the onAuthFailure signature and
callers accordingly so no raw credentials are forwarded.
- Around line 298-305: The current encoded-segment guard
(/%(2[ef]|5c|25)/i.test(path)) does not reject encoded semicolons, allowing
inputs like "/assets/..%3b/secret" to bypass the later literal-segment checks;
update the early reject logic to also detect "%3B" (encoded ';') so these cases
are rejected before any publicPaths prefix matching. Modify the regex/test that
checks the path (the /%(...)/i test on the path variable) to include the 3B hex
code for semicolon and keep this check executed before the publicPaths/prefix
logic and the segment loop that examines '.' '..' backslashes and ';'.
In `@src/node-body.ts`:
- Around line 13-37: Add handlers for 'aborted' and 'close' on the incoming
request stream (in the function that currently registers req.on('data'...),
req.on('end'...), req.on('error'...)) that reject the promise when the client
disconnects mid-upload (guard the rejection with !req.complete to avoid false
positives). When resolving or rejecting (in the size > limit branch, end, error,
aborted or close handlers) ensure you mark the read as done and remove all
listeners (data, end, error, aborted, close) to avoid leaks; keep the existing
BodyTooLargeError path and reuse the same done/limit logic and req.pause
behavior but also clean up listeners there. Apply the same changes to the second
reader block referenced around lines 42-63 so both reader implementations handle
aborted/close and perform listener cleanup.
In `@src/proxy.ts`:
- Around line 27-34: CLIENT_CONTROLLED currently strips only x-forwarded-* but
leaves the RFC7239 Forwarded header and rebuilds x-forwarded-host from an
unvalidated Host, allowing spoofing; update CLIENT_CONTROLLED to also include
the 'forwarded' header and stop reconstructing forwarded host metadata from the
raw Host header. Instead, only synthesize or set
x-forwarded-host/x-forwarded-proto from a trusted configuration value (e.g.,
publicHost or allowedHostnames) or an explicit allowlist check; if no trusted
public-host is configured, drop forwarded host metadata entirely. Ensure the
code paths that previously rebuilt or trusted Host (search for uses of
CLIENT_CONTROLLED, any Host-to-x-forwarded-host logic, and the block that
handles forwarded headers) are changed to strip 'Forwarded' and
validate/synthesize using the trusted config.
---
Outside diff comments:
In @.github/workflows/publish.yml:
- Around line 22-24: Update the workflow steps that call actions/checkout@v4 and
oven-sh/setup-bun@v2: for the actions/checkout step (identified by uses:
actions/checkout@v4) add with: persist-credentials: false to prevent persisting
runner git credentials, and for the oven-sh/setup-bun step (uses:
oven-sh/setup-bun@v2) add with: no-cache: true to disable reuse/caching of the
Bun executable across runs; leave other steps (e.g., actions/setup-node)
unchanged.
In `@src/express.ts`:
- Around line 47-55: In the BodyTooLargeError branch inside the isLoginPost
handling, ensure the connection is closed after sending the 413: set the
response Connection: close header (e.g., via res.set('Connection', 'close') or
res.setHeader) before calling res.status(413).type('text/plain').send('Payload
too large'), and teardown the socket once the response is flushed (attach a
finish listener to destroy the request/response socket, e.g., res.on('finish',
() => req.socket.destroy())). This change should be applied around the
readRawBody error handling where BodyTooLargeError is caught so unread bytes
can't occupy a keep-alive connection.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8aa57a55-5c7e-4157-a53b-aec497d01193
📒 Files selected for processing (33)
.github/ISSUE_TEMPLATE/bug_report.yml.github/PULL_REQUEST_TEMPLATE.md.github/dependabot.yml.github/workflows/ci.yml.github/workflows/publish.ymlCHANGELOG.mdCONTRIBUTING.mdREADME.mdSECURITY.mdpackage.jsonscripts/smoke-dist.mjssrc/astro.tssrc/bun.tssrc/cli.tssrc/cloudflare.tssrc/core.tssrc/express.tssrc/hono.tssrc/netlify.tssrc/next.tssrc/node-body.tssrc/proxy.tssrc/sveltekit.tssrc/web.tstest/adapters.test.tstest/cli.test.tstest/cloudflare.test.tstest/core.test.tstest/fixtures/credentials.tstest/proxy.test.tstsconfig.jsontsup.config.tsvitest.config.ts
Comment on lines
28
to
29
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||
| - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 |
There was a problem hiding this comment.
Harden CI: disable persisted checkout credentials and Bun executable cache
- Lines 28 & 60 (
actions/checkout): addwith: persist-credentials: falseto avoid leaving auth/SSH credentials on the runner. - Lines 29 & 61 (
oven-sh/setup-bun): addwith: no-cache: trueto disable Bun executable caching.
Suggested patch
- - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
- - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2
+ - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
+ with:
+ persist-credentials: false
+ - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2
+ with:
+ no-cache: true
...
- - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
- - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2
+ - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
+ with:
+ persist-credentials: false
+ - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2
+ with:
+ no-cache: true📝 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
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | |
| - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 | |
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | |
| with: | |
| persist-credentials: false | |
| - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 | |
| with: | |
| no-cache: true |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 28-28: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 29-29: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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 @.github/workflows/ci.yml around lines 28 - 29, Update the GitHub Actions
steps that use actions/checkout and oven-sh/setup-bun: for the actions/checkout
steps (identified by uses: actions/checkout@...), add a with block setting
persist-credentials: false to avoid leaving auth/SSH credentials on the runner;
for the oven-sh/setup-bun steps (identified by uses: oven-sh/setup-bun@...), add
a with block setting no-cache: true to disable Bun executable caching. Ensure
you apply these changes to both occurrences of each action in the workflow so
the runner and cache hardening are consistent.
Comment on lines
147
to
+148
| [0.1.1]: https://github.com/PeterM45/sitepass/releases/tag/v0.1.1 | ||
| [0.1.0]: https://www.npmjs.com/package/sitepass/v/0.1.0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Inconsistent version link targets.
The 0.1.1 link points to a GitHub release tag, while 0.1.0 points to the npm package page. For consistency and to follow common changelog conventions, both should point to GitHub release tags (e.g., https://github.com/PeterM45/sitepass/releases/tag/v0.1.0) or to GitHub compare URLs showing the diff.
📝 Suggested fix for consistency
-[0.1.0]: https://www.npmjs.com/package/sitepass/v/0.1.0
+[0.1.0]: https://github.com/PeterM45/sitepass/releases/tag/v0.1.0📝 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
| [0.1.1]: https://github.com/PeterM45/sitepass/releases/tag/v0.1.1 | |
| [0.1.0]: https://www.npmjs.com/package/sitepass/v/0.1.0 | |
| [0.1.1]: https://github.com/PeterM45/sitepass/releases/tag/v0.1.1 | |
| [0.1.0]: https://github.com/PeterM45/sitepass/releases/tag/v0.1.0 |
🤖 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 `@CHANGELOG.md` around lines 147 - 148, Update the inconsistent link target for
the 0.1.0 entry so it matches the 0.1.1 pattern: replace the npm URL used in the
[0.1.0] reference with the GitHub release tag URL (e.g., change
"https://www.npmjs.com/package/sitepass/v/0.1.0" to
"https://github.com/PeterM45/sitepass/releases/tag/v0.1.0") so both [0.1.1] and
[0.1.0] point to GitHub release tags.
Comment on lines
+27
to
+34
| // Headers the client must not control: the gate's own bypass credential, and the | ||
| // forwarded-request metadata, which a front-most proxy sets authoritatively. | ||
| const CLIENT_CONTROLLED = new Set([ | ||
| 'x-sitepass-bypass', | ||
| 'x-forwarded-for', | ||
| 'x-forwarded-proto', | ||
| 'x-forwarded-host', | ||
| ]) |
There was a problem hiding this comment.
The forwarded host/proxy metadata is still client-spoofable.
This only strips the x-forwarded-* family. A client can still send RFC 7239 Forwarded, and x-forwarded-host is rebuilt from the unvalidated Host header, so downstream code that trusts forwarded host/scheme/IP can still be poisoned. Either drop forwarded host metadata unless you have a trusted public-host config, or synthesize it from an allowlisted hostname and strip Forwarded alongside the legacy headers.
Also applies to: 153-171
🤖 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/proxy.ts` around lines 27 - 34, CLIENT_CONTROLLED currently strips only
x-forwarded-* but leaves the RFC7239 Forwarded header and rebuilds
x-forwarded-host from an unvalidated Host, allowing spoofing; update
CLIENT_CONTROLLED to also include the 'forwarded' header and stop reconstructing
forwarded host metadata from the raw Host header. Instead, only synthesize or
set x-forwarded-host/x-forwarded-proto from a trusted configuration value (e.g.,
publicHost or allowedHostnames) or an explicit allowlist check; if no trusted
public-host is configured, drop forwarded host metadata entirely. Ensure the
code paths that previously rebuilt or trusted Host (search for uses of
CLIENT_CONTROLLED, any Host-to-x-forwarded-host logic, and the block that
handles forwarded headers) are changed to strip 'Forwarded' and
validate/synthesize using the trusted config.
Security/correctness:
- core: onAuthFailure now receives a redacted { method, path } view instead of
the full GateRequest — the hook is the natural place for logging, and the
request carried the urlencoded login body (cleartext password) and the
session cookie. New exported AuthFailure type.
- core: isPublicPath also rejects encoded semicolon (%3b) — an origin decoding
it to ';' could reach the same '..;' path-parameter traversal the literal
guard blocks.
- node-body: readers reject on 'aborted'/'close' arriving without 'error' (a
client dropping mid-upload), which previously left the promise pending and
hung the Express/proxy request. Both readers mark done on 'end' so a trailing
'close' is a clean no-op. New test/node-body.test.ts.
- proxy: drops the RFC 7239 Forwarded header too (was only stripping the
X-Forwarded-* family), so a client can't poison a downstream that trusts it.
- cli: proxy announces success only on 'listening' and surfaces a bind error
(e.g. EADDRINUSE) instead of printing a false banner then crashing.
- cli: readEnvValue/upsertEnv/loadDotenv share one tolerant parser, so init
recognizes a hand-edited 'KEY = "value"' line instead of regenerating the
secret (and upsert updates it in place rather than appending a duplicate).
Polish:
- ci: persist-credentials: false on every checkout (don't leave repo creds on
the runner).
- docs: issue-template SECURITY link is now absolute; README CLI block tagged
'text'; README onAuthFailure example uses the redacted payload.
- test/proxy: the hop-by-hop/Forwarded stripping assertions now actually send
proxy-authorization and Forwarded, so they can't pass vacuously.
Declined: CodeRabbit wanted CHANGELOG's 0.1.0 to link a release tag, but no
v0.1.0 git tag exists, so the npm link is the honest target.
105 tests; coverage 95.4/85.5/98.8/97.7; publint clean; attw 0 problems.
Owner
Author
|
Thanks for the review — addressed in df563b5. Summary of dispositions: Fixed (security / correctness):
Fixed (polish):
Declined:
All green locally: 105 tests, coverage 95.4% stmts / 85.5% branch, |
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.
Implements the fixes from the two multi-agent audits of the package and codebase. Recommend releasing as
0.2.0, not0.1.2— two deliberate breaking behavior changes (below). Local gate is fully green: strict typecheck (withexactOptionalPropertyTypes), Biome, 98 tests (was 43), coverage 95.0% stmts / 85.1% branch, dist smoke test,publintclean, andattwreporting 0 problems across all 11 entry points.Breaking changes (why 0.2.0)
SITEPASS_PASSWORDactually revokes outstanding sessions. Existing cookies force one re-login.sitepass initwrote is unaffected.Security
publicPaths: reject literal and encoded (%5c,%25,%2e,%2f) and path-parameter (;) traversal;/is an exact root match (was un-gating the whole site); the login POST is handled beforepublicPaths(an entry coveringloginPathno longer breaks login). The encoded-only case was the 0.1.1 fix; this closes the literal/proxy/Express route.x-sitepass-bypasscredential before forwarding; setsX-Forwarded-For/-Proto/-Hostauthoritatively (client-spoofed values stripped); caps the unauthenticated login body at 64 KiB.Features (power-user QoL)
SITEPASS_BYPASS_TOKEN/bypassToken+x-sitepass-bypassheader, constant-time).<loginPath>/logout(GET/POST),renderLoginPagehook (custom/localized pages,escapeHtmlexported),onAuthFailurehook,cookieSecure: falsefor plain-HTTP LAN.sitepass/proxyis now an importable export; the proxy CLI gained--public-paths,--login-path,--cookie-name,--session-seconds,--bypass-token,--insecure-cookie,--env-file.--help/--versionbehave (exit 0),init --helpprints usage, strict unknown-flag rejection, dependency-free.envloader (no longer silently no-ops on Node 20.0–20.11).Types & packaging
PagesFunction<Env> = gate()now compiles (verified against@cloudflare/workers-types); Bun adapter is generic over the handler's rest args (serverfor websocket upgrades); Netlify fails closed instead of throwing outside the Edge runtime.publicPathsacceptsreadonly string[];typesVersionsmap fixes node10 type resolution;sideEffects: false;@types/expressoptional peer;./proxyexport;CHANGELOG.mdships in the tarball.Internals & CI
src/web.ts; Node body readers shared viasrc/node-body.ts.publintbefore merge and publish; tag-vs-version publish guard; concurrency groups; job timeouts; pinned Node for coverage; Dependabot; issue/PR templates.Manual follow-ups (not in this PR)
package.jsonto0.2.0and tag at release time (the publish workflow enforces tag == version).Full details in the CHANGELOG
[Unreleased]section.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores