Repository navigation
refactor: consolidate admin/public API helpers, fix a public error-leak - #1
Merged
Merged
Conversation
… IPs
Q-3 from the Aug 2026 quality audit: handleIncomingWebhook still built
audit-log IPs from raw X-Forwarded-For / X-Real-IP headers, the same
spoofable pattern already fixed elsewhere in the codebase. Applies the
same clientIp(..., { trustForwardedFor }) policy so audit attribution
is consistent everywhere.
Q-2 from the Aug 2026 quality audit: three independent copies of the constant-time byte comparison existed — auth/passwords.ts, auth/csrf.ts, and an inline loop in public-api.ts's webhook token check. Copies of security-critical comparison code are a drift risk. New src/admin/timing-safe.ts exports the single implementation; all three call sites now share it. No behavior change.
…c APIs Q-5 from the Aug 2026 quality audit: public-api.ts and routes/api/_utils.ts each carried their own json() responder, typed error classes, and serverError() mapper with slightly diverging behavior (pretty-printing, charset). Error-handling policy must evolve in lockstep. New src/admin/http.ts is the single implementation; _utils.ts re-exports so all existing route imports keep working. The public API's responses switch from pretty-printed to compact JSON — equivalent for machine consumers.
Q-7 from the Aug 2026 quality audit: ad-hoc console calls used inconsistent prefixes — some carried [dune]/[dune/authz], others were unprefixed with leading indentation. All five unprefixed sites (users bootstrap warnings, plugin/theme install notices) now carry a [dune/users], [dune/plugins], or [dune/themes] tag so log aggregation can filter by subsystem.
Q-6 from the Aug 2026 quality audit: _layout.tsx was 762 lines, ~530 of which were the embedded stylesheet string inside adminCss(). Moving it to routes/admin-css.ts cuts the layout component to ~230 lines and gives the CSS its own reviewable unit. No output change — same string, same call sites.
Follow-up to cd30264: the admin manifest generator treats every file under routes/ as a route module, so routes/admin-css.ts was emitted as a bogus /admin-css route and broke type-checking of manifest.gen.ts. Moved to src/admin/admin-css.ts and regenerated the manifest.
Q-1 from the Aug 2026 quality audit: handleFormSubmission and
handleContactSubmission duplicated ~150 lines each of the same pipeline:
rate-limit → body-size cap → JSON/form parsing → field collapse →
upload sanitization/storage → notification fan-out. A fix applied to one
copy could silently miss the other, which had already happened once.
Extracted shared helpers, keeping each handler's unique validation and
redirect logic inline:
- guardAndParseSubmission() — rate limit, body-size cap, body parsing,
multi-value collapse
- storeUploads() — filename sanitization, extension allowlist,
size/file-count caps, storage layout
- dispatchNotifications() — global email/webhook config with optional
per-form destination overrides
No behavior change; the full unit suite passes.
The consolidated serverError() (Q-5) maps ValidationError/NotFoundError
to 400/404 including the error's own message, which is safe for the
admin API (authenticated) but not public-api.ts (unauthenticated) — the
original public-api.ts deliberately never did this ('Never reflect
internal error strings on the public, unauthenticated API'). Adds
publicServerError(), a fixed-500-always mapper matching that original
posture, and switches public-api.ts to use it. Regression tests confirm
neither typed errors nor plain Error messages leak through the public
path while the admin path's existing message-inclusion is unaffected.
zumbrunn
force-pushed
the
refactor/admin-api-consolidation
branch
from
August 27, 2026 07:09
d721a45 to
ba45e83
Compare
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
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
A round of code-quality cleanup for the admin/public API surface: dedup shared logic that had drifted into multiple copies, extract a couple of oversized files, and make logging more consistent. No intended functional change except one deliberate fix (see below).
handleFormSubmission/handleContactSubmissionduplicated ~150 lines each of rate-limit → body-size cap → parse → upload-storage → notify. Extracted intoguardAndParseSubmission(),storeUploads(),dispatchNotifications().timingSafeEqual— three independent copies of the same constant-time byte comparison (passwords, CSRF, webhook tokens) consolidated intosrc/admin/timing-safe.ts.json()/serverError()— the admin API and public API each carried their own JSON responder and error mapper with diverging behavior.adminCss()extracted out of_layout.tsx(762 → ~230 lines) into its own file.[dune/<area>]log prefixes across previously-unprefixed console output.clientIp()for webhook audit IPs — matches the trusted-proxy-aware policy already used elsewhere, instead of trusting raw forwarded headers.What I found and fixed in review
Two real issues turned up while reviewing the original branch, both fixed here:
A commit removing
minimumDependencyAgefromdeno.jsonwas dropped entirely. Its stated reason — a local warning that the field is "only valid in a workspace root deno.json" — only fires because this repo happens to be nested in a shared local dev workspace. I checked the actual GitHub Actions publish logs: the warning never appears there. Removing the field would have silently reintroduced a real, previously-hit bug (deno publishfailing right after a fresh@dune/corerelease, since there'd be no age-gate exclusion for it).The consolidated
serverError()leaked error messages to the unauthenticated public API. The originalpublic-api.tsdeliberately always returned a generic 500 with no internal detail ("Never reflect internal error strings on the public, unauthenticated API" per its own comment). The sharedserverError()mapsValidationError/NotFoundErrorto 400/404 including the thrown error's own message — correct for the authenticated admin API, but a latent leak for the public one (dormant today since nothing inpublic-api.tsthrows those types yet, but the next handler that does would silently start leaking). AddedpublicServerError(), a fixed-500-always mapper matching the original posture, switchedpublic-api.tsto use it, and added regression tests (tests/admin/http_test.ts) locking in the distinction.Testing
Full test suite passes in isolation against the real published
@dune/core: 226/226.deno task check(fmt/lint/type-check) clean.🤖 Generated with Claude Code