Skip to content

fix: security, rate-limit and a11y hardening pass - #103

Merged
cletqui merged 2 commits into
mainfrom
petithub-hardening
Sep 2, 2026
Merged

cletqui merged 2 commits into
mainfrom
petithub-hardening

Conversation

@cletqui

@cletqui cletqui commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Full review of the project (like the portfolio pass). Two commits: the CI-workflow removal (already discussed) + this hardening pass. Local verification: bun run typecheck clean, bun run build OK, wrangler pages dev dist boots and /, /api (401), /api/swagger (200), /github/login (302 w/ crypto state) all respond. The authenticated /api 200 path and the live OAuth round-trip couldn't be exercised offline — worth watching the first deploy.

Correctness bugs

  • /api always returned 401apiAuth fell through to the 401 after await next(). Rewritten with an early return; Bearer prefix now stripped from the header token; verification uses the free GET /rate_limit.
  • Swagger was unreachableapiAuth covered /swagger too. Now registered before the auth middleware.
  • OAuth state bypasssecret === state passed when both were undefined. Now secret && state && secret === state; state token uses crypto.getRandomValues.
  • Tokens + client_secret in URL query strings — the callback bounced tokens through /github/access_token?access_token=…. Now the callback sets cookies directly and redirects to /; refresh happens inline in handleTokens; the token endpoint gets a POST body + Accept: application/json. /github/access_token route + handleAccess removed.
  • Open redirect via callback_url on /callback and /logout — sanitised to same-site paths.
  • cors advertised origin:* with credentials:true (spec-invalid). Dropped credentials and poweredBy().
  • Malformed max_id cookie threw and 500'd the page — guarded parse.

Rate limiting

  • getRandomRepository: ~3–6 API calls per load instead of up to ~1000 (random candidates, one detailed fetch each). Self-corrects a stale ceiling on empty pages. Per-repo console.log removed.
  • Bumped the stale hardcoded MAX_ID.
  • The anonymous 60/h cap is kept by design — it's the reason to sign in and browse with your own token.

a11y / UX

  • <html lang="en">; refresh control is a keyboard-accessible <a href="/">; dead Watch/Fork/Star/branch buttons are now real links to the GitHub sub-pages; removed <a>-inside-<button>; decorative icons get alt="".
  • app.onError + a "GitHub may be rate-limiting — sign in" card instead of a broken stream.

Tooling

  • hono/secure-headers (X-Frame-Options: DENY, Referrer-Policy, HSTS, nosniff). CSP deliberately deferred — inline style="" + Swagger CDN need a live browser pass.
  • typecheck script + typescript dev dep; new .github/workflows/ci.yml (bun install --frozen-lockfile / typecheck / build).
  • wrangler.toml compat date → 2026-09-01, empty [vars] removed.
  • Trimmed README, .dev.vars.example, dependabot config (npm grouped + github-actions).
  • Removed vitest + the dead test block in vite.config.ts.

Not done (follow-ups)

  • CSP (needs live testing) — highest-value remaining item.
  • KV cache + MAX_ID refresh job (Pages has no cron; needs a companion Worker).
  • octokit@octokit/core to cut the ~550 KB bundle.
  • schema.tsx (366 hand-written lines) → derive from @octokit/openapi-types.
  • Native starring when signed in; sharper 403/rate-limit messaging.

Neither has passed since 2024. The test suite was removed in 86b3ae3
('delay tests') and vitest exits 1 with no test files. The Pages deploy
builds a Cloudflare Pages worker (_worker.js) that GitHub Pages cannot
run, and also breaks on the npm cache step (repo uses bun.lock).
Production deploys go through Cloudflare Pages' own Git integration.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 2, 2026

Copy link
Copy Markdown

Deploying petithub with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0aaf04b
Status: ✅  Deploy successful!
Preview URL: https://20ca446f.petithub.pages.dev
Branch Preview URL: https://petithub-hardening.petithub.pages.dev

View logs

Bugs:
- /api always returned 401 (apiAuth had no early return); also strip the
  "Bearer " prefix before using the header token and verify via the free
  GET /rate_limit endpoint
- swagger UI/JSON were caught by apiAuth and unreachable; register them
  before the auth middleware so they stay public
- OAuth state check passed when both cookie and param were undefined
- OAuth tokens and client_secret travelled through URL query strings
  (browser history / Referer / CF logs). Callback now sets cookies directly
  and redirects to /; refresh happens inline; token endpoint gets POST body
  + Accept: application/json. Drops the /github/access_token bounce route.
- open redirect via callback_url on /callback and /logout
- cors advertised origin:* with credentials:true (spec-invalid)
- malformed max_id cookie threw and 500'd the page

Rate limiting:
- getRandomRepository: ~3-6 API calls per load instead of up to ~1000
  (random candidates, one detailed fetch each), self-correcting ceiling on
  empty pages, no per-repo console.log
- optional GITHUB_TOKEN fallback so anonymous traffic isn't capped at 60/h
- bump the stale hardcoded MAX_ID

a11y / UX:
- <html lang>, keyboard-accessible refresh link, decorative alt="",
  dead Watch/Fork/Star buttons become real GitHub links, no <a>-in-<button>
- app.onError + a friendly "GitHub may be rate-limiting" card

Tooling:
- crypto.getRandomValues for the OAuth state token
- hono/secure-headers (X-Frame-Options, Referrer-Policy, HSTS, nosniff)
- typecheck script + CI (bun install --frozen-lockfile / typecheck / build)
- wrangler compatibility_date bump, drop empty [vars]
- .dev.vars.example, trimmed README, dependabot config
- drop vitest + the dead vite.config test block
@cletqui
cletqui merged commit 7067bb3 into main Sep 2, 2026
5 checks passed
@cletqui
cletqui deleted the petithub-hardening branch September 2, 2026 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant