security: hardening pass across 7 packages#21
Merged
davidcrowe merged 4 commits intomainfrom Apr 14, 2026
Merged
Conversation
The ESM hotfix (type: module) was published to npm as 0.1.1 but the repo package.json was never updated to match. This commit brings the repo source of truth in line with what npm already serves.
Review uncovered 8 findings ranging from critical to medium. All are
addressed in this commit. Where the fix changes behavior visible to
consumers, the package version is bumped as a minor/major signal so
downstream projects do not auto-upgrade without opting in.
Findings and mitigations:
1. [CRITICAL] SSRF in @gatewaystack/proxyabl-core
The existing assertUrlSafe was synchronous and checked hostname strings
only — it self-documented that DNS resolution was the caller's
responsibility, which meant an allowlisted hostname resolving to a
private IP (e.g. 169.254.169.254 cloud metadata, 127.0.0.1 loopback)
bypassed the check entirely. Rewritten as an async function that
resolves every A/AAAA record via dns.lookup and rejects if any
resolved address is private. Added IPv4-mapped-IPv6 detection
(::ffff:127.0.0.1 and its compressed hex form ::ffff:7f00:1) so an
attacker cannot reach loopback by wrapping it in IPv6 syntax. Strips
URL brackets from IPv6 hostnames before checking. Retains all
existing private-range checks. Residual TOCTOU between lookup and
fetch is documented in source + README.
Version: proxyabl-core 0.0.4 → 0.1.0 (breaking: sync → async).
proxyabl 0.0.11 → 0.0.12 (dep bump, own API unchanged).
2. [CRITICAL] Default webhook secret + timing-unsafe compare
(explicabl/src/webhooks/auth0LogWebhook.ts)
LOG_WEBHOOK_SECRET defaulted to "dev-change-me" (hardcoded string in
public source). Comparison used `===` which short-circuits on first
mismatch. The webhook promotes DCR clients via Auth0 Management API,
so this is an authorization-relevant surface. Removed the default,
lazy-read env var in handler so module-load never throws, and switched
to crypto.timingSafeEqual with explicit length check + Bearer prefix
strip. Returns 503 not_configured if the secret is missing, to avoid
silently accepting the default.
3. [HIGH] Health endpoint leaks identity infra + amplifies outbound
(explicabl/src/health.ts)
/health/auth0 was unauthenticated, returned issuer/audience/JWKS URLs
verbatim, and fetched Auth0 Management API tokens on every call. Any
internet caller could hammer it to exhaust Auth0 Management rate
limits or inflate billing. Now gated behind HEALTH_ADMIN_SECRET
(timing-safe compare, 404 when unset or auth fails so the endpoint
is not discoverable by scans). Error fields narrowed to generic
codes (unreachable, bad_status_NNN) so raw upstream errors no
longer leak.
Version: explicabl 0.0.6 → 0.0.7.
4. [HIGH] Rate-limit/budget bypass via poisoned numeric input
(limitabl-core/src/budgetTracker.ts, agentGuard.ts)
record() and check() accepted unconstrained numeric costs. Negative
cost subtracted from the running sum (refund bypass); NaN poisoned
the sum permanently (NaN > maxSpend is false, cap silently
disabled); ±Infinity pinned the sum and caused user lockout. These
values realistically flow in from upstream usage data (e.g. an LLM
response's token count multiplied by a pricing table). Added
sanitize.ts with sanitizeCost() that clamps non-finite / negative
values to 0 and logs a warning. Applied at all three entry points:
budgetTracker.check, budgetTracker.record, agentGuard.recordToolCall.
Clamp-not-throw chosen for production safety so a buggy caller does
not crash the request path.
Version: limitabl-core 0.1.0 → 0.1.1.
5. [HIGH] X-Forwarded-For trusted without trust proxy
(limitabl/src/index.ts, explicabl/src/webhooks/auth0LogWebhook.ts)
Rate-limit key derivation read the leftmost X-Forwarded-For entry
directly. Any caller can forge that value, so an attacker could (a)
rotate the spoofed IP per request to avoid rate limits entirely, (b)
forge a victim's IP to exhaust their bucket, or (c) inflate the
in-memory key map to slow OOM. Removed the direct read; rely on
Express's req.ip with the operator-configured trust proxy setting.
Applied the same fix to the webhook's own rate-limit key function.
README documents the trust proxy requirement.
Version: limitabl 0.1.0 → 0.2.0 (behavior change).
6. [HIGH] PII detection bypass via zero-width characters
(transformabl-core/src/detect.ts, new normalize.ts)
detectPii scanned raw bytes only. An attacker could split PII with
zero-width characters (e.g. `john\u200B@example.com`) — the regex
breaks, nothing is detected, the LLM downstream ignores the ZW char
and sees the original PII. Compliance-relying callers lost their
redaction guarantee. Added stripInvisible() that removes U+200B-200D,
U+2060, U+FEFF from scan input and builds a position map back to
original-text offsets so redactPii still covers the entire obfuscated
span including the invisible chars themselves. Match.value preserves
the original form so mask-mode output is correctly sized. Fast path
returns reference-equal string with null map when no invisible chars
are present (zero overhead for normal content). NFKC normalization +
base64/hex decode-and-rescan left as follow-ups.
Version: transformabl-core 0.2.0 → 0.3.0; transformabl 0.1.0 → 0.2.0
with dep bump to transformabl-core ^0.3.0.
7. [MEDIUM] validatabl() middleware silently allows when misconfigured
(validatabl/src/middleware.ts)
The underlying decision() primitive returns allowed: true when
called with no checks configured — defensible as a neutral building
block. But the middleware wrapper that inherits this behavior is
marketed as deny-by-default, and an operator who calls
validatabl({}) silently gets open access. Factory now throws at
mount time if requiredPermissions, policies, and inputSchema are
all empty/missing, with a clear error naming the three options.
decision() itself is unchanged (preserves existing tests); its
docstring gained a SECURITY note explaining the empty-config
semantics and pointing at the middleware as the enforcing layer.
Version: validatabl 0.1.0 → 0.2.0 (behavior change);
validatabl-core 0.1.0 → 0.1.1 (docstring only).
8. [MEDIUM] Demo app middleware order bug
(apps/gateway-server/src/app.ts)
The /protected pipeline mounted `limitabl, identifiabl, transformabl`
while the adjacent comment described the correct order
`identifiabl, limitabl, transformabl`. With the code's order, the
per-identity rate limiter runs before identity is established —
falling back to IP keying, which means authenticated users sharing a
NAT/proxy IP with unauthenticated traffic share a rate-limit bucket.
Swapped the middlewares; no package publish needed (reference app
only). Anyone reading the repo as a composition example now sees
the correct order.
README updates for proxyabl-core, limitabl, validatabl, explicabl to
document the new APIs, env-var requirements, and behavior changes.
All 159 tests pass on this branch. Full monorepo build is clean.
Residual items (NFKC/homoglyph/base64 PII detection, TOCTOU DNS socket
pinning, defense-in-depth SSRF on proxyabl/src/router.ts, demo-app
body-parser depth + global error handler) are tracked as follow-ups.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Addresses CodeQL js/loop-bound-injection on the normalize.ts character walk. Inputs larger than MAX_NORMALIZE_LENGTH (1M chars) bypass the ZW-strip pass and return unchanged rather than truncating. PII detection still runs on the full input — callers enforce size limits upstream.
…ss-rate-limit dep Addresses CodeQL js/missing-rate-limiting on /health/auth0: even with timing-safe secret compare, the handler needs per-caller throughput limits to prevent brute-force of HEALTH_ADMIN_SECRET. 20 req/min matches the operator-polling use case and makes brute force of a reasonable- entropy secret infeasible. Also declares express-rate-limit as an explicabl dependency — the sibling auth0 webhook already imported it but the package only had express in its manifest (worked locally via hoisting, would fail a clean install).
4 tasks
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
2 participants
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.
Summary
Security review of the
@gatewaystack/*packages uncovered 8 findings from critical to medium. This branch lands the fixes in two commits: a tinychorethat syncsidentifiabl-core/package.jsonwith the 0.1.1 version already published on npm, and a largersecuritycommit with the hardening work.All changes are either (a) deletions of unsafe behavior, (b) new gates (timing-safe compare, env-var requirement, mount-time check), or (c) hardening of detection/validation surface. No product features have been added to the open-source packages.
Findings
proxyabl-coreexplicabl(webhook)LOG_WEBHOOK_SECREThad a hardcoded default and the comparison was not timing-safe. Default removed, env var now required,crypto.timingSafeEqualused.explicabl(health)/health/auth0was unauthenticated, returned identity-provider config verbatim, and amplified outbound requests to upstream management APIs. Now gated behindHEALTH_ADMIN_SECRET; 404 when unset.limitabl-coresanitizeCosthelper, clamp-and-warn at three entry points.limitabl+explicablwebhookreq.ip— operator must configuretrust proxy.transformabl-corestripInvisiblehelper with position-preserving remap.validatabldecision()primitive is unchanged.apps/gateway-serverCommit
50bde20contains the detailed per-finding write-up with class-of-bug, affected code paths, and mitigation rationale. Intentional disclosure of exploit primitives lives there (and, separately, in GitHub Security Advisories — see below) rather than in this summary.Related GitHub security posture
This PR intentionally does not bundle dependency updates, but reviewers should know:
express-rate-limitIPv4-mapped IPv6 bypass) closes the same class of bug this PR fixes in our own code. The dependabot PR chore(deps): bump express-rate-limit from 8.2.1 to 8.2.2 #17 that resolves it should be merged as a follow-up.path-to-regexpReDoS, runtime, HIGH) — merge PR chore(deps): bump path-to-regexp from 0.1.12 to 0.1.13 #19 alongside.qs, runtime, LOW) — PR chore(deps): bump qs from 6.14.1 to 6.14.2 #14.apps/gateway-server/src/app.ts, missing rate limiting) and #47 (proxyabl/src/tool-gateway.ts) — not addressed here, suggested follow-up.1922fadon this branch.Disclosure
Formal GitHub Security Advisories (with CVEs) should be filed for findings #1, #2, #3, and #5 after merge + publish. Without GHSAs, downstream consumers on old versions have no automated Dependabot signal to upgrade. Draft URL:
https://github.com/davidcrowe/GatewayStack/security/advisories/new.Follow-ups (not blocking this PR)
proxyabl/src/router.tsproxyHandlerTest plan
npm run build— full monorepo tsc build cleannpx vitest run— 159/159 tests pass (+ new regression tests inproxyabl-core/__tests__/security.test.tsfor DNS rebinding, IPv4-mapped IPv6, andtransformabl-core/__tests__/detect.test.tsfor ZW-char bypass)gitleakspre-commit clean on all three commitsassertUrlSafeis now called withawaitat every call site (only one internal caller:proxyabl-core/src/execute.ts)@gatewaystack/limitabl 0.2.0release notes mention thetrust proxyrequirement loudly enoughHEALTH_ADMIN_SECRETandLOG_WEBHOOK_SECRETare set in any downstream deployments before upgrading@gatewaystack/explicablRelease sequencing (after merge)
Publish order matters because of transitive deps:
limitabl-core0.1.1validatabl-core0.1.1transformabl-core0.3.0proxyabl-core0.1.0limitabl0.2.0,validatabl0.2.0,transformabl0.2.0,proxyabl0.0.12explicabl0.0.7