Skip to content

Repo hardening and security review - #6

Merged
zerotrustdavid merged 5 commits into
mainfrom
repo-hardening-review
Aug 21, 2026
Merged

Repo hardening and security review#6
zerotrustdavid merged 5 commits into
mainfrom
repo-hardening-review

Conversation

@zerotrustdavid

@zerotrustdavid zerotrustdavid commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Hardening pass across dependencies, HTTP headers, secret scanning and repository governance. Five commits, each independently verified.

The finding that matters

A Web3Forms access key was hardcoded as a test fixture in lib/feedback/web3forms.test.ts and committed to this public repository.

gitleaks 8.21.2 scanned all 20 commits across every ref and exited clean. It missed it. Default rules do not match bare UUIDs, because a UUID carries too little entropy to trip a generic detector, and a UUID is exactly the credential shape this project uses. A targeted search found it.

What this key is, stated accurately: Web3Forms publishes it as a public value. It is prefixed NEXT_PUBLIC_, compiled into the client bundle by design, and readable by anyone viewing source on the deployed site. The vendor's own dashboard labels it "a public key" that is safe to use in client side code, and the API treats it interchangeably with the form identifier. Submissions are additionally restricted to this site's domain.

Its presence in git history therefore adds little beyond what the deployed site already exposes, which is the reasoning behind leaving history intact rather than force-pushing. Unreachable commits also stay retrievable by direct SHA URL without a support request, so a rewrite would have invalidated every clone and fork SHA while leaving the value fetchable.

Changes made here:

  • the fixture is now a documented all-zeros placeholder, so no real value is carried forward
  • .gitleaks.toml adds two UUID rules so this class of value cannot pass silently again

The remaining question of whether to replace the form itself is being evaluated separately, outside this PR.

One detail worth recording: the value was assigned to a constant called VALID, so the identifier-based rule does not match it. The bare-UUID backstop is what actually catches it, and that is why both rules exist.

This disclosure is deliberate. The repository is public to demonstrate engineering practice, and that includes how a mistake was found and handled.

Dependencies

npm audit reported 7 high severity advisories. Now zero.

Four cleared without version changes (undici, brace-expansion, js-yaml, nanoid). The rest needed next@16.3.2, a minor bump rather than a major one, which also resolved a vulnerable sharp nested under next.

Two declaration problems surfaced while auditing:

  • sharp is imported by scripts/generate-brand-assets.mjs but was never declared, resolving only as a transitive dependency of next. Now an explicit devDependency, which additionally lets npm dedupe next's nested copy onto the patched version.
  • @testing-library/react and @testing-library/jest-dom were declared but appear nowhere outside package.json, and no test renders a component. Removed.

Also removes five unreferenced create-next-app scaffold SVGs. Confirmed unreachable by static analysis and by crawling every route for 4xx responses, since a missing asset would not fail the build.

Security headers

next.config.ts sets no output: 'export', so this deploys as a Next server and headers() is emitted normally. vercel.json would have been the wrong mechanism.

Adds CSP, X-Frame-Options, X-Content-Type-Options, Referrer-Policy, Permissions-Policy and HSTS, and sets poweredByHeader: false.

HSTS carries max-age and includeSubDomains but not preload. Submitting to the browser preload list is effectively irreversible, and the header should not advertise an intent that has not been decided on.

The CSP is same-origin throughout except connect-src, which allows exactly one external host for the feedback form. script-src keeps 'unsafe-inline' deliberately: removing it needs a nonce, a nonce needs middleware, and middleware would force every route to render dynamically and cost the site its fully static profile. object-src, base-uri, form-action and frame-ancestors carry the weight instead.

Verified on a real served response from the production server, not a clean build: all six headers present, X-Powered-By absent, and a scripted browser submitted the feedback form successfully with zero CSP violations.

README and umbrella.ts

The README no longer narrates how the umbrella circular dependency is solved. It states that it is solved exactly and reconciles to the penny, without handing the approach to anyone who has not opened the code.

Nothing is lost. The full derivation, both branch equations, and the reason the result is exact rather than iterated were already documented in the docstring above solveGrossPay. Only the round-trip test rationale was missing there, so that was added.

Also adds a short note that the repository is public to be read and evaluated rather than reused, and corrects a stale test count from 138 to 155.

Governance

  • LICENSE.md states the all-rights-reserved position GitHub's terms already imply, so it reads as a decision rather than an oversight
  • SECURITY.md gives a private disclosure route, sets scope, and pre-empts the recurring non-finding that the publishable NEXT_PUBLIC_ key is visible in the bundle
  • CI, which the repository had none of. Runs lint, tests, palette contrast checks, production build, and a high-severity audit, plus secret scanning in a separate job

CI triggers on pull_request and never pull_request_target, holds contents: read, and uses no secrets. gitleaks is fetched by pinned version and verified against a pinned SHA256 rather than pulled in as a third-party action.

The secret scan covers the tree as merged rather than full history, because history holds one known finding and failing every future build on it would only train people to ignore the job.

Verification

npm run lint, 155 tests, npm run palette, npm run build and npm audit all pass. npm run brand still produces byte-identical assets on sharp 0.35. All 19 build entries accounted for: 11 HTML routes crawled with no 4xx or 5xx, 6 asset and metadata routes serving 200, /_not-found correctly 404ing, and /guides/[slug] being a parent row rather than a URL.

Outstanding, handled outside this PR

Repository settings could not be changed from the environment this work was done in: no gh CLI, and no repo-settings capability in the available tooling. Secret scanning, push protection, Dependabot alerts, branch protection and stale branch deletion are all being applied manually by the repository owner.

Worth stating plainly: neither secret scanning nor push protection would have caught this particular case. GitHub's scanners do not flag bare UUIDs either. They are worth enabling on their own merits, not as the control that solves this.

…rules

The Web3Forms access key used in production was hardcoded as a test
fixture in lib/feedback/web3forms.test.ts and committed to this public
repository. The tests only need a UUID-shaped string, so the fixture is
now a documented all-zeros placeholder.

Adds .gitleaks.toml, because gitleaks 8.21.2 scanned the full history and
exited clean while that key sat in it. The default rules do not match bare
UUIDs, since a UUID carries too little entropy to trip a generic detector,
and a UUID is the one credential shape this project uses.

Two rules. The first matches a UUID assigned to an identifier that names
itself a credential. The second is a backstop matching any bare UUID
literal, and it is the one that earns its place: the key that leaked was
assigned to a constant called VALID, which the first rule does not match.

Verified: the history scan now reports the key in both commits carrying
it, the working tree scans clean with the placeholder allowlisted, and the
full suite of 155 tests still passes.
npm audit reported 7 high severity advisories. All are now resolved and
the audit is clean.

Four were fixed by npm audit fix without version changes: undici,
brace-expansion, js-yaml and nanoid.

The remaining three (next, postcss, and a vulnerable sharp nested under
next) needed next 16.3.2. That is a minor bump rather than a major one, so
it was applied rather than deferred, and the exact pinning the project
already used has been kept instead of the caret range npm substituted.

Also corrects two declaration problems found while auditing:

sharp is imported by scripts/generate-brand-assets.mjs but was never
declared, and resolved only as a transitive dependency of next. It is now
an explicit devDependency, which additionally lets npm dedupe next's
nested copy onto the patched version.

@testing-library/react and @testing-library/jest-dom were declared but
appear nowhere outside package.json, and no test renders a component. Both
removed.

Removes five unreferenced create-next-app scaffold SVGs from public/.

Verified: lint, 155 tests, palette validation and the production build all
pass, npm run brand still produces byte-identical assets on sharp 0.35,
and npm audit reports zero vulnerabilities.
next.config.ts sets no output: 'export', so this deploys as a Next server
and headers() is emitted normally. A vercel.json headers block is not
needed and would be the wrong mechanism here.

Adds Content-Security-Policy, X-Frame-Options, X-Content-Type-Options,
Referrer-Policy, Permissions-Policy and Strict-Transport-Security, and
sets poweredByHeader to false.

The CSP is tight where it counts. Fonts are self hosted by next/font and
all imagery is local, so every directive is same origin except connect-src,
which allows exactly one external host for the feedback form. script-src
keeps 'unsafe-inline' deliberately: removing it needs a nonce, a nonce
needs middleware, and middleware would force every route to render
dynamically and cost the site its fully static profile. object-src,
base-uri, form-action and frame-ancestors carry the weight instead, and
'unsafe-eval' is development only.

Verified against a real served response from the production server rather
than a clean build: all six headers present, X-Powered-By absent, and a
scripted browser submitted the feedback form successfully with zero CSP
violations, confirming connect-src does not silently break it.
LICENSE.md states the position that GitHub's terms already imply for a
repository published without one: all rights reserved. The repository is
public so the code can be read and evaluated, and that is the only
permission granted. Naming it explicitly makes it a decision rather than
something that looks forgotten.

SECURITY.md gives anyone who finds a real problem somewhere to send it
other than a public issue, sets scope, and explains up front why the
publishable Web3Forms key in the client bundle is not itself a finding, so
that report does not arrive repeatedly.

Adds CI, which the repository had none of. A public repository carrying a
155 test suite with nothing running it on push is both a missing safety
net and a wasted signal. It runs lint, tests, the palette contrast checks,
the production build, and a high severity dependency audit, plus a secret
scan in a separate job.

The workflow triggers on pull_request and never pull_request_target, holds
contents: read, and uses no secrets, so a pull request from outside has
nothing to reach for. gitleaks is fetched by pinned version and verified
against a pinned SHA256 rather than pulled in as a third party action.

The secret scan covers the tree as merged rather than full history. History
holds one accepted finding, and failing every future build on it would only
train people to ignore the job. The command for scanning history manually
is in a comment beside the step.

Verified: every step passes locally, and the workflow parses as valid YAML.
@vercel

vercel Bot commented Aug 21, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
payreckon Ready Ready Preview Aug 21, 2026 3:08pm

Drops the preload token from Strict-Transport-Security. max-age and
includeSubDomains stay. Submitting a domain to the browser preload list is
effectively irreversible, and the header should not advertise an intent
that has not been decided on. Adding it back is a one line change.

Rewrites the umbrella paragraph in the README so it states that the
circular dependency between employer NI, the apprenticeship levy and
employer pension is solved exactly and reconciles to the penny, without
narrating how. The landing page no longer hands over the approach, or
signposts the file, to anyone who has not opened the code.

Nothing is lost. The full derivation, both branch equations, and the reason
the result is exact rather than iterated were already documented in the
docstring above solveGrossPay. Only the round trip test rationale was
missing there, so that is added, and it is the one thing the README used to
carry that the code did not.

Adds a two line note near the top of the README saying the repository is
public to be read and evaluated rather than reused, pointing at LICENSE.md.

Corrects the test count from 138 to 155, read from the suite rather than
copied forward.

Verified: lint, 155 tests, palette, production build and audit all pass. All
eleven routes crawl clean with no 4xx or 5xx, confirming nothing references
the removed scaffold SVGs by any dynamically built path.
@zerotrustdavid
zerotrustdavid merged commit cbb2f9c into main Aug 21, 2026
4 checks passed
@zerotrustdavid
zerotrustdavid deleted the repo-hardening-review branch August 27, 2026 06:35
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