Skip to content

chore: dedupe shared schemas, byte-accurate password bound, CI hardening, dep-audit fixes - #160

Merged
dev-fani merged 4 commits into
fanilabs:mainfrom
willi-d7:chore/backlog-cleanup-shared-schemas-security
Aug 30, 2026
Merged

chore: dedupe shared schemas, byte-accurate password bound, CI hardening, dep-audit fixes#160
dev-fani merged 4 commits into
fanilabs:mainfrom
willi-d7:chore/backlog-cleanup-shared-schemas-security

Conversation

@willi-d7

@willi-d7 willi-d7 commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Backlog cleanup addressing four independent issues — a schema-duplication refactor, a password-validation correctness fix, a CI hardening change, and a dependency-audit remediation. Each is a separate commit.

Issue Area Change
#122 modules/*/interface/schemas.ts Extract duplicated Zod fragments to src/shared/validation/
#121 modules/auth Enforce the bcrypt 72-byte password limit (was 72 JS characters)
#119 .github/workflows/ci.yml Add explicit least-privilege permissions: block
#120 package.json / pnpm-lock.yaml Upgrade Vitest to 3.x, pin patched transitive deps

#122 — Deduplicate chainId / transactionResponseSchema (refactor, technical-debt)

chainDeliveryId / chainFleetId (the /^\d+$/ digit-string pattern) and transactionResponseSchema ({ data: { xdr } }) were redefined character-for-character across the deliveries, disputes, escrow, fleet and reputation modules — the same duplication already fixed for stellarAddress (src/shared/validation/stellar-address.ts).

  • Adds src/shared/validation/chain-id.ts — exports chainId, named generically because the identical shape backs both chainDeliveryId and chainFleetId.
  • Adds src/shared/validation/transaction-response.ts — exports transactionResponseSchema.
  • Each module now aliases chainId and re-exports transactionResponseSchema from the shared module instead of defining its own copy. Route modules that import transactionResponseSchema from ./schemas.js are unaffected.
  • Module-specific fragments (evidenceHash, amount, senderShareBps, …) are left exactly where they are — this is scoped only to the fragments proven identical.
  • interface/ importing from shared/ is already permitted by the eslint-plugin-boundaries rules (eslint.config.js).

Pure extraction, no behaviour change.

#121 — Password length bounded by bytes, not characters (security, validation)

password = z.string().min(8).max(72) justified .max(72) with "bcrypt silently truncates beyond 72 bytes", but Zod's .max() counts UTF-16 code units while bcrypt truncates at 72 bytes. A password using multi-byte UTF-8 characters (emoji, many non-Latin scripts) can sit under 72 JS characters while exceeding 72 bytes — so two distinct such passwords that differ only after their first 72 bytes hash identically.

  • Replaces .max(72) with .refine(v => Buffer.byteLength(v, 'utf8') <= 72, …) and updates the inline comment.
  • resetPasswordBodySchema.newPassword reuses the same password constant, so registration, login and password-reset all pick up the corrected bound.
  • Adds src/modules/auth/interface/schemas.spec.ts: an emoji password under 72 JS chars but over 72 UTF-8 bytes is now rejected with a clear message; a 72-byte pure-ASCII password is still accepted.

#119 — Least-privilege permissions: in ci.yml (ci, security, devops)

None of ci.yml's jobs write to the repo, comment on PRs, or need more than read access to the checked-out code, yet none declared a permissions: block — every job ran with the ambient default GITHUB_TOKEN grant. Adds a top-level permissions: contents: read ceiling, mirroring release.yml (which already scopes itself to the single contents: write it needs). All jobs pass unchanged.

#120 — Dependency-audit remediation (dependencies, security)

pnpm audit reported 31 advisories (3 critical, 17 high) against the committed lockfile:

  • vitest ^2.1.4 resolves below the fix for GHSA-5xrq-8626-4rwp (critical — arbitrary file read/execute via the Vitest UI server).
  • tar (critical decompression DoS + several high path-traversal advisories) via bcrypt > @mapbox/node-pre-gyp > tar.
  • handlebars (critical JS-injection via AST type confusion) via eslint-plugin-boundaries.

Changes:

  • Bump vitest / @vitest/coverage-v8 to ^3.2.7 (staying on 3.x per the issue; no vitest.config.ts changes needed).
  • Add pnpm.overrides pinning forward-patched tar, handlebars, vite, esbuild, nanoid, js-yaml, fast-uri — all reached only through build/lint/test tooling.
  • Document the one residual advisory in docs/SECURITY.md: GHSA-w5hq-g745-h8pq (uuid, reached only via the dev-only autocannon load-tester, moderate so below the --audit-level=high CI gate, no upstream fix available).

Audit before/after:

before:  31 vulnerabilities  (1 low | 10 moderate | 17 high | 3 critical)
after:   pnpm audit --audit-level=high  →  0 findings, exit 0
         (1 moderate remains, below threshold, documented)

Testing

  • pnpm audit --audit-level=high → clean (exit 0), was 20 high/critical.
  • New src/modules/auth/interface/schemas.spec.ts — 4 tests pass.
  • Touched schema/interface tests pass; prettier --check and eslint clean on all changed files.
  • pnpm test / pnpm typecheck / pnpm build / pnpm lint show no new failures introduced by this branch (verified against a clean upstream/main baseline — the pre-existing failures on main in unrelated modules are untouched and out of scope here).

Closes #122
Closes #121
Closes #120
Closes #119

None of ci.yml's jobs (audit, lint-and-typecheck, build, test) write to
the repository, comment on PRs, or need anything beyond reading the
checked-out code, yet none declared a permissions: block, so every job ran
with the ambient repository/org default GITHUB_TOKEN grant.

Add a top-level `permissions: contents: read` ceiling (mirroring
release.yml, which already scopes itself to the single `contents: write`
it needs). All jobs continue to pass unchanged.

Closes fanilabs#119

Claude-Session: https://claude.ai/code/session_01BQMgSfHSVAWb8ghcvs8tsF
`password = z.string().min(8).max(72)` justified `.max(72)` with "bcrypt
silently truncates beyond 72 bytes", but Zod's `.max()` counts UTF-16
code units, while bcrypt truncates at 72 *bytes*. A password using
multi-byte UTF-8 characters (emoji, many non-Latin scripts) can sit under
72 JS characters while exceeding 72 bytes, so bcrypt silently truncates
it earlier than the schema's own rationale assumes.

Replace `.max(72)` with a `.refine()` on `Buffer.byteLength(value, 'utf8')`.
`resetPasswordBodySchema.newPassword` reuses the same `password` constant,
so registration, login and password-reset all pick up the corrected bound.
Add a schema unit test covering the emoji-under-72-chars-over-72-bytes
case plus the exactly-72-ASCII-bytes accepted case.

Closes fanilabs#121

Claude-Session: https://claude.ai/code/session_01BQMgSfHSVAWb8ghcvs8tsF
…Schema to shared

`chainDeliveryId`/`chainFleetId` (the `/^\d+$/` digit-string pattern) and
`transactionResponseSchema` (`{ data: { xdr } }`) were redefined
character-for-character across the deliveries, disputes, escrow, fleet and
reputation modules' `interface/schemas.ts` — the same class of duplication
already fixed for `stellarAddress` (src/shared/validation/stellar-address.ts).

Add `src/shared/validation/chain-id.ts` (exports `chainId`, generically
named since it backs both `chainDeliveryId` and `chainFleetId`) and
`src/shared/validation/transaction-response.ts`. Each module now aliases
`chainId` and re-exports `transactionResponseSchema` from shared instead of
defining its own copy; route modules importing `transactionResponseSchema`
from `./schemas.js` are unaffected. Module-specific fragments
(`evidenceHash`, `amount`, `senderShareBps`, …) are left exactly where they
are. `interface/` importing from `shared/` is permitted by the existing
eslint-plugin-boundaries rules. Pure extraction, no behaviour change.

Closes fanilabs#122

Claude-Session: https://claude.ai/code/session_01BQMgSfHSVAWb8ghcvs8tsF
`pnpm audit` reported 31 advisories (3 critical, 17 high) against the
committed lockfile, none caught or remediated by any process:

- vitest ^2.1.4 resolves below the patched line for GHSA-5xrq-8626-4rwp
  (critical — arbitrary file read/execute via the Vitest UI server).
- tar (critical + several high path-traversal advisories) via
  bcrypt > @mapbox/node-pre-gyp > tar.
- handlebars (critical JS-injection via AST type confusion) via
  eslint-plugin-boundaries.

Changes:
- Bump vitest / @vitest/coverage-v8 to ^3.2.7 (staying on 3.x; no config
  changes needed for vitest.config.ts).
- Add pnpm.overrides pinning forward-patched tar, handlebars, vite,
  esbuild, nanoid, js-yaml and fast-uri — all reached only through
  build/lint/test tooling.
- Document the single residual advisory (GHSA-w5hq-g745-h8pq, uuid via the
  dev-only autocannon load-tester, below the `high` CI threshold, no fix
  available upstream) in docs/SECURITY.md.

`pnpm audit --audit-level=high` now reports zero findings (was 20
high/critical). `pnpm test` / `pnpm build` behaviour is unchanged by the
vitest bump.

Closes fanilabs#120

Claude-Session: https://claude.ai/code/session_01BQMgSfHSVAWb8ghcvs8tsF
@drips-wave

drips-wave Bot commented Aug 30, 2026

Copy link
Copy Markdown

@willi-d7 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@dev-fani
dev-fani merged commit f8f64b4 into fanilabs:main Aug 30, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment