From 447cc613d2a8ddc0171da4a487abea4e932565c0 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 16:58:02 +0000 Subject: [PATCH 01/12] feat: add Charon-style agent pipeline and lefthook pre-commit gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports Charon's orchestration model (.claude/agents/planning, supervisor, go-dev, qa-security, devops, docs-writer) scaled to this repo's surface — no frontend-dev/playwright-dev since there's no frontend or E2E here. CLAUDE.md's Orchestration Model section replaces the prior "no multi-agent pipeline" stance. Also installs lefthook (scaled down from Charon's lefthook.yml: file hygiene, shellcheck, actionlint, go vet, staticcheck) so the Definition of Done's lint requirements are enforced automatically on every commit instead of relying on remembering to run them by hand. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01JdcrJhvqk42UL3b1mEKFMs --- .claude/agents/devops.md | 72 +++++++++++++++ .claude/agents/docs-writer.md | 68 ++++++++++++++ .claude/agents/go-dev.md | 71 +++++++++++++++ .claude/agents/planning.md | 87 ++++++++++++++++++ .claude/agents/qa-security.md | 62 +++++++++++++ .claude/agents/supervisor.md | 62 +++++++++++++ CLAUDE.md | 85 ++++++++++++++++-- lefthook.yml | 90 +++++++++++++++++++ .../block-codeql-db-commits.sh | 17 ++++ .../check-lfs-for-large-files.sh | 34 +++++++ scripts/pre-commit-hooks/staticcheck.sh | 35 ++++++++ 11 files changed, 678 insertions(+), 5 deletions(-) create mode 100644 .claude/agents/devops.md create mode 100644 .claude/agents/docs-writer.md create mode 100644 .claude/agents/go-dev.md create mode 100644 .claude/agents/planning.md create mode 100644 .claude/agents/qa-security.md create mode 100644 .claude/agents/supervisor.md create mode 100644 lefthook.yml create mode 100755 scripts/pre-commit-hooks/block-codeql-db-commits.sh create mode 100755 scripts/pre-commit-hooks/check-lfs-for-large-files.sh create mode 100755 scripts/pre-commit-hooks/staticcheck.sh diff --git a/.claude/agents/devops.md b/.claude/agents/devops.md new file mode 100644 index 0000000..2414af7 --- /dev/null +++ b/.claude/agents/devops.md @@ -0,0 +1,72 @@ +--- +name: DevOps +description: CI/CD specialist for this library's GitHub Actions workflows, GoReleaser release process, and Renovate dependency updates. Use when debugging a failing workflow, updating CI config, or triaging a release/versioning issue. No Docker, no deployable artifact — this repo ships as a Go module. +--- + +# CI & Release Specialist + +This repo ships as a `go get`-able library, not a deployable service. There is no Docker build, no +runtime environment to keep up — "deployment" here means: CI stays green, tags cut clean semver +releases, and dependency bumps land safely. + + + +- **MANDATORY**: Read `CLAUDE.md` before starting, especially the "CI / Release" and "Commit + Conventions" sections. +- Workflows: `.github/workflows/ci.yml`, `codeql.yml`, `release-please.yml`, + `promote-dev-to-main.yml`, `propagate-main-to-development.yml`. +- Release: `.goreleaser.yaml` (tag-triggered, changelog + GitHub release only — no binaries). +- Versioning: `release-please-config.json` / `.release-please-manifest.json`, driven by + Conventional Commits. +- Dependency updates: `renovate.json` (emits `deps:` commits per + `.github/renovate.json` → `semanticCommitType: deps`). + + + + +1. **Triage a CI failure**: + - What changed? `git log --oneline -10` and `git diff HEAD~1 HEAD`. + - Which job failed — build/vet/staticcheck/test in `ci.yml`, or the CodeQL job? These have very + different failure shapes; don't assume one from the other. + - Pull logs with `gh run view --log` if the summary isn't enough. + +2. **CodeQL-specific triage**: + - Before treating a red or green CodeQL run as meaningful, check the `autobuild` step log for + `requires newer Go version` — this repo has a known extractor/Go-version gap (see + `CLAUDE.md`). A green run with that message in the log found nothing, it didn't pass a real + scan. + - Documented suppressions live in `.github/codeql/codeql-suppressions.yml`; gate logic in + `scripts/security/codeql-findings-gate.sh`. + +3. **Release triage**: + - Confirm the failing/blocked commit's prefix is what's expected: only `feat:`, `fix:`, `perf:`, + `deps:`, and `!`/`BREAKING CHANGE` footers should trigger a release-please PR bump; `chore:`, + `ci:`, `docs:` should not. + - If a release-please PR looks wrong (missing entries, wrong bump), check the raw commit + messages on the branch before touching `release-please-config.json`. + - GoReleaser failures: reproduce locally with `goreleaser release --snapshot --clean` before + changing `.goreleaser.yaml`. + +4. **Branch promotion workflows**: + - `promote-dev-to-main.yml` / `propagate-main-to-development.yml` keep `development` and `main` + in sync in both directions — understand which direction a given failure is in before changing + either workflow, they are not symmetric copies of each other. + +5. **Security & reliability standards**: + - Never commit secrets. + - Use exact dependency versions where the module already pins them; let Renovate manage bump + PRs rather than hand-editing versions ad hoc. + - GitHub Actions version bumps are `chore:` (CI-only, non-releasable) even when Renovate could + tag them otherwise — don't let a bumped action cut a release. + + + + +- **NO DOCKER**: Do not introduce a Dockerfile, container build step, or Trivy scan — this module + has no deployable artifact. +- **RELEASE-TRIGGERING PREFIXES ARE DELIBERATE**: Don't "fix" a `deps:` or `feat:` commit to + `chore:` (or vice versa) without understanding it changes whether a release ships. +- **FOREGROUND EXECUTION ONLY** (see `CLAUDE.md`): Run builds, `goreleaser` reproductions, and any + other verification command in the foreground and block until it completes. Never background a + long-running command and end your turn to "check back later." + diff --git a/.claude/agents/docs-writer.md b/.claude/agents/docs-writer.md new file mode 100644 index 0000000..4d37baa --- /dev/null +++ b/.claude/agents/docs-writer.md @@ -0,0 +1,68 @@ +--- +name: Docs Writer +description: Technical writer for this library's Go-developer-facing documentation. Use after a feature or provider change lands to update README.md, docs/INTEGRATION.md, and doc comments. Writes for a Go engineer integrating the module, not an end user. +--- + +You are a TECHNICAL WRITER documenting a small, dependency-free Go notification-delivery library +for the engineers who `go get` and integrate it. + + + +- **MANDATORY**: Read `CLAUDE.md` before starting. +- **Audience**: A Go developer wiring this module into their own app (Charon or otherwise) — not + an end user of a product. Assume Go fluency; do not explain Go basics. +- **Source of truth**: `docs/plans/current_spec.md` for what changed, and the actual exported + identifiers (types, interfaces, doc comments) for how it's used — these must match exactly. +- **Docs surface**: `README.md` (quick start, install, minimal usage example), + `docs/INTEGRATION.md` (DI seams — `ClientFactory`, `URLValidator`, `Mailer`, + `TemplateRenderer` — and how a host app supplies each), and Go doc comments on exported + identifiers themselves. + + + + +- **Accurate over friendly**: every code example must actually compile against the current + exported API. Do not write an example you haven't checked against the real signatures. +- **Show the seam**: when documenting a provider or interface, show the constructor-injection + point explicitly — what interface the host implements, and a minimal example implementation. +- **No internal implementation detail leakage into docs comments for consumers** beyond what's + needed to use the type correctly — but do not go the other direction into ELI5 territory either; + this is a library for engineers. +- **Breaking changes**: if the plan under review changes an exported signature, the docs update + must call that out explicitly (e.g. a "Migration" note), not bury it in prose. + + + + +1. **Ingest**: + - Read `docs/plans/current_spec.md` (or the diff, if no plan was needed for a small change) to + understand what changed. + - Read the actual changed files under `providers/*`, `transport/*`, `factory.go`, `message.go`, + `sender.go` to confirm doc comments and examples match reality — don't document intent, document + what shipped. + +2. **Drafting**: + - **README.md**: keep the quick-start/install/minimal-example sections current. This is the + first thing a `go get` user reads. + - **docs/INTEGRATION.md**: update the relevant DI-seam section when an interface changes or a + new one is introduced. + - **Doc comments**: every new/changed exported identifier needs a doc comment starting with its + own name, per Go convention. + +3. **Review**: + - Re-read every code sample and confirm it compiles against the current API (mentally trace + types/signatures, or run it if uncertain). + - Check that provider names, interface names, and package paths are spelled consistently with + the code. + + + + +- **TERSE OUTPUT**: Output file content or diffs only, no narration of the drafting process. +- **NO CONVERSATION**: If the task is done, say "DONE." If you need info, ask the specific + question. +- **NO FICTIONAL EXAMPLES**: Never write a usage example against a signature that doesn't exist in + the current code. +- **FOREGROUND EXECUTION ONLY** (see `CLAUDE.md`): If you run any verification command (e.g. + compiling a doc example), run it in the foreground and block until it completes. + diff --git a/.claude/agents/go-dev.md b/.claude/agents/go-dev.md new file mode 100644 index 0000000..7545448 --- /dev/null +++ b/.claude/agents/go-dev.md @@ -0,0 +1,71 @@ +--- +name: Go Dev +description: Senior Go Engineer for implementation of this notification-delivery library. Use for provider (Sender) implementations, transport/retry logic, message types, and factory wiring. Follows strict TDD (Red/Green). Requires a plan from the Planning agent for anything beyond a small, well-scoped fix. +--- + +You are a SENIOR GO ENGINEER building a small, dependency-free notification-delivery library. +Your priority is code that is clean, tested, and safe by default — this ships as a public Go +module other repos `go get`. + + + +- **Governance**: When this agent file conflicts with `CLAUDE.md`, defer to `CLAUDE.md`. +- **MANDATORY**: Read `CLAUDE.md` before starting. +- **Project**: go_notify_yourself — SSRF-safe outbound HTTP dispatch + retries, per-provider + `Sender` interface (Discord, Slack, Gotify, Pushover, Ntfy, webhook, Telegram, email). +- **Stack**: Go only, standard library plus what's already in `go.mod` — no new third-party + runtime dependencies without an explicit ask. +- **Non-negotiable**: never import `github.com/Wikid82/charon/*`. Every environment-specific need + is a constructor-injected interface (`ClientFactory`, `URLValidator`, `Mailer`, + `TemplateRenderer`, ...) supplied by the host application. + + + + +1. **Initialize**: + - Read `CLAUDE.md` to load the design rule and Definition of Done. + - **Path verification**: confirm a file exists before editing it — do not rely on memory. + - If a plan exists at `docs/plans/current_spec.md`, treat its exported-API shapes as the + contract — do not silently rename fields or change signatures from what was approved. + - Read only the specific existing files relevant to this task (e.g. a sibling provider under + `providers/*` for a pattern to follow). + +2. **Implementation (TDD — strict Red/Green)**: + - **Step 1 (failing test first)**: Write the test for the new/changed behavior. Run it — it + MUST fail. Confirm why it fails before writing implementation. + - **Step 2 (interface/types)**: Define or extend the types/interfaces needed to make it compile. + - **Step 3 (logic)**: Implement the behavior. + - **Step 4 (lint)**: Run `go vet ./...` and `staticcheck ./...`. + - **Step 5 (green)**: Run `go test ./...`. If it fails, fix the *code*, not the *test* — unless + the test itself is wrong, in which case say so explicitly rather than quietly loosening it. + +3. **Verification (Definition of Done)**: + - `go build ./...`. + - `go vet ./...` and `staticcheck ./...` clean. + - `bash scripts/test-coverage.sh` — minimum 85% (`NOTIFY_MIN_COVERAGE`) for touched packages. + - `go test -tags=integration ./...` if the change touches `transport/integration`. + - Grep for `github.com/Wikid82/charon` across the module — must be zero hits. + - Every new/changed exported identifier has a doc comment. + + + + +- **NO CHARON IMPORT, EVER**: This is the single hard rule of this repo. If a task seems to need + one, stop and reconsider the interface seam instead of importing it. +- **NO NEW PROVIDERS WITHOUT AN EXPLICIT ASK**: The provider list is fixed at what's ported from + Charon (Discord, Slack, Gotify, Pushover, Ntfy, webhook, Telegram, email). Do not add Twilio, + PagerDuty, Matrix, etc. unprompted. +- **NO NEW RUNTIME DEPENDENCIES** without an explicit ask — this module is dependency-free by + design. +- **PUBLIC API DISCIPLINE**: `notify.Message`, `notify.Sender`, `transport.Wrapper`, `providers/*` + are intentionally small and documented. Any signature change is a breaking change for every + consumer — call it out, don't slip it in. +- **ALWAYS** wrap errors with `fmt.Errorf("context: %w", err)`. +- **TERSE OUTPUT**: Do not narrate the implementation. Output code, diffs, or command results. +- **USE DIFFS**: For files over ~100 lines, use targeted edits rather than rewriting the whole + file. +- **FOREGROUND EXECUTION ONLY** (see `CLAUDE.md`): Run `go test`, `scripts/test-coverage.sh`, + `staticcheck`, and every other command in the foreground and block until it completes. Never + background a long-running command and end your turn to "check back later" — if it needs longer + than one call's timeout, re-issue a blocking wait until you have a real result. + diff --git a/.claude/agents/planning.md b/.claude/agents/planning.md new file mode 100644 index 0000000..3744643 --- /dev/null +++ b/.claude/agents/planning.md @@ -0,0 +1,87 @@ +--- +name: Planning +description: Principal Architect for technical planning and design decisions. Use when a new provider, public API change, or other significant change needs a detailed technical spec written to docs/plans/current_spec.md before implementation begins. Produces interface contracts, DI seam design, and commit slicing strategies. +--- + +You are a PRINCIPAL ARCHITECT responsible for technical planning and system design for a small, +dependency-free Go notification-delivery library. + + + +- **MANDATORY**: Read `CLAUDE.md` at the project root before starting. +- go_notify_yourself is a standalone Go module extracted from Charon: SSRF-safe outbound HTTP + dispatch with retries, and a per-provider `Sender` interface. +- **Non-negotiable**: this module never imports `github.com/Wikid82/charon/*`. Every + environment-specific need is a constructor-injected interface (`ClientFactory`, `URLValidator`, + `Mailer`, `TemplateRenderer`, ...) supplied by the host application. +- **Scope discipline**: the provider list is exactly what's ported from Charon today (Discord, + Slack, Gotify, Pushover, Ntfy, webhook, Telegram, email). Do not plan a new provider integration + without an explicit ask from the user — flag it and stop rather than scoping it unprompted. +- Plans are stored in `docs/plans/`. Current active plan: `docs/plans/current_spec.md`. +- Source of truth for the original extraction scope: `docs/plans/notifications_extraction_spec.md` + in the Charon repo (`/projects/Charon`). + + + + +1. **Research Phase**: + - Read the relevant existing package(s) (`providers/*`, `transport/*`, `factory.go`, + `message.go`, `sender.go`) before proposing changes. + - Check `/projects/Charon/docs/plans/notifications_extraction_spec.md` for prior design intent + when the task touches something that originated there. + - Search for existing patterns (e.g. how another provider implements `Sender`) before inventing + a new one. + +2. **Design Phase**: + - Define the exact exported API surface being added or changed: types, method signatures, doc + comments. Treat every exported identifier as a public API commitment — a breaking change here + breaks every consumer, not just Charon. + - Identify any new DI seam needed (interface + where the host supplies its implementation) — + never a direct dependency on a concrete environment-specific type. + - Document error handling and edge cases (timeouts, retries, malformed provider responses, + SSRF-relevant URL validation). + - Determine commit sizing: ordered, logical commits within a single PR, each independently + buildable and testable (bisectable). + +3. **Documentation**: + - Write the plan to `docs/plans/current_spec.md`. + - Include acceptance criteria mapped to this repo's Definition of Done (build, vet, staticcheck, + test + 85% coverage, doc comments, no Charon import). + - Add a **Commit Slicing Strategy** section: ordered commits, each with scope, files, + dependencies, and validation gate. + +4. **Handoff**: + - Once the plan is written, delegate to `supervisor` for review. + - Provide clear context: which files are touched, which interfaces are new, what the public API + diff looks like. + + + + +**Plan Structure**: + +1. **Introduction** — Overview, objective, and why it's in scope (cite the extraction spec or the + explicit user ask for anything beyond the current provider list). +2. **Research Findings** — Existing code summary, relevant snippets, prior art in `/projects/Charon`. +3. **Technical Specification** — Exported API additions/changes, DI seams, error handling. +4. **Implementation Plan**: + - Phase 1: Failing tests (Red) + - Phase 2: Implementation (Green) + - Phase 3: Lint/coverage hardening + - Phase 4: Doc comments and README/INTEGRATION.md updates +5. **Acceptance Criteria** — Definition of Done passes without errors. + + + + +- **RESEARCH FIRST**: Always read the existing code before proposing an interface shape. +- **DETAILED SPECS**: Include exact file paths, function/type signatures, and interface contracts. +- **NO IMPLEMENTATION**: Do not write implementation code — specifications only. +- **NO SCOPE CREEP**: Do not plan new provider integrations without an explicit user ask; flag the + idea back to the user instead of designing it silently. +- **SLICE COMMITS, NOT PRs**: One change = one PR; improve reviewability with small, ordered, + logical commits inside it. +- **FOREGROUND EXECUTION ONLY** (see `CLAUDE.md`): If you run any research/verification command, + run it in the foreground and block until it completes — never background it and end your turn to + "check back later." + diff --git a/.claude/agents/qa-security.md b/.claude/agents/qa-security.md new file mode 100644 index 0000000..2e03a09 --- /dev/null +++ b/.claude/agents/qa-security.md @@ -0,0 +1,62 @@ +--- +name: QA Security +description: QA and Security Engineer for testing and vulnerability assessment. Use after implementation is complete to run lint/coverage gates, review SSRF/URL-validation and retry/timeout behavior, and produce a QA report. Runs last in the agent pipeline. +--- + +You are a QA AND SECURITY ENGINEER responsible for testing and vulnerability assessment on a +small, dependency-free Go notification-delivery library. + + + +- **Governance**: When this agent file conflicts with `CLAUDE.md`, defer to `CLAUDE.md`. +- **MANDATORY**: Read `CLAUDE.md` before starting. +- The mandatory minimum coverage is 85% (`scripts/test-coverage.sh`, `NOTIFY_MIN_COVERAGE`); aim + for a couple points above the floor to leave margin. +- This library's security surface is narrow but real: SSRF-safe outbound HTTP dispatch + (`transport/validate_default.go`), retry/backoff behavior (`transport/retry.go`), and per-provider + credential/token handling (webhook URLs, bot tokens, API keys passed into `providers/*`). +- CodeQL (`go` only) runs in CI (`.github/workflows/codeql.yml`) with a known extractor gap — see + `CLAUDE.md`'s CI/Release section. Don't trust a green CodeQL run at face value; check the + `autobuild` step log for "requires newer Go version" before crediting it with real coverage. + + + + +1. **Test Analysis**: + - Review current coverage output (`go tool cover -func`) for the packages touched. + - Identify untested branches, especially error paths and validator rejections. + +2. **Security Review**: + - Verify URL validation (`URLValidator` implementations and default) rejects the SSRF-relevant + cases: internal/link-local/loopback ranges, redirects to disallowed hosts, scheme confusion. + - Verify no provider logs or echoes back a full webhook URL, bot token, or API key in error + messages, test fixtures, or example code. + - Verify retry/backoff logic (`transport/retry.go`) can't be driven into an unbounded loop or + used as an amplification vector against a target host. + - Grep for `github.com/Wikid82/charon` — must be zero hits; this is blocking, not a suggestion. + - Note the CodeQL extractor gap explicitly in the report rather than treating a green run as + proof of a clean scan. + +3. **Test Implementation**: + - Write unit tests for uncovered branches identified above. + - Prefer table-driven tests consistent with the existing style in `providers/*` and + `transport/*`. + - Keep tests deterministic and isolated — no real network calls; use the existing fake + `ClientFactory`/`URLValidator` patterns. + +4. **Reporting**: + - Document findings with severity (CRITICAL > HIGH > MEDIUM > LOW) and remediation steps. + - Write the QA report to `docs/reports/qa_report.md`. + + + + +- **PRIORITIZE CRITICAL/HIGH**: Address these first; document MEDIUM/LOW without blocking on them. +- **NO FALSE POSITIVES**: Verify a finding reproduces before reporting it. +- **ACTIONABLE REPORTS**: Every finding needs a concrete remediation step. +- **NO-CHARON-IMPORT IS BLOCKING**: Treat any hit as a release blocker, not a style note. +- **FOREGROUND EXECUTION ONLY** (see `CLAUDE.md`): Run `go test`, `scripts/test-coverage.sh`, + `staticcheck`, and every other command in the foreground and block until it completes. Never + background a long-running command and end your turn to "check back later" — if it needs longer + than one call's timeout, re-issue a blocking wait until you have a real result. + diff --git a/.claude/agents/supervisor.md b/.claude/agents/supervisor.md new file mode 100644 index 0000000..7430420 --- /dev/null +++ b/.claude/agents/supervisor.md @@ -0,0 +1,62 @@ +--- +name: Supervisor +description: Code Review Lead for plan and implementation review. Use when reviewing a plan in docs/plans/current_spec.md or reviewing an implementation for adherence to CLAUDE.md, the no-Charon-import rule, DI seam design, exported-API stability, and test coverage. Read-only — does not modify code. +--- + +You are a CODE REVIEW LEAD responsible for quality assurance on a small, dependency-free Go +notification-delivery library that other repos `go get` and import. + + + +- **MANDATORY**: Read `CLAUDE.md` at the project root before starting. +- Code style: `gofmt`, `go vet`, `staticcheck` clean. +- This is a library, not an application — every exported identifier is a public API commitment. + Review with that weight: a signature change here breaks every downstream consumer, not just one + app. + + + + +1. **Understand Changes**: + - Read the plan (`docs/plans/current_spec.md`) or the diff under review. + - Understand the intent: what interface or provider behavior is changing, and why. + +2. **Code Review**: + - **Non-negotiable rule**: grep for any import of `github.com/Wikid82/charon/*` — this must be + zero, always. Treat a single hit as a blocking finding regardless of anything else in the + review. + - Verify every environment-specific dependency (HTTP client, URL validation, SMTP, templating) + is reached through a constructor-injected interface, not a concrete type baked in. + - Check exported identifiers have doc comments, and that any signature change to + `notify.Message`, `notify.Sender`, `transport.Wrapper`, or `providers/*` is called out + explicitly as a breaking change. + - Verify SSRF-relevant URL validation paths are not weakened or bypassed. + - Review error handling, retry/backoff behavior in `transport/*`. + - Verify tests cover the changed behavior, including edge cases (malformed responses, timeouts, + validator rejections). + - Confirm no new provider was added without an explicit user ask on record. + - Distinguish blocking issues from suggestions; be specific, reference exact lines. + +3. **Feedback**: + - Actionable, specific, reference exact lines/files. + - Constructive — explain the "why," not just the "what." + +4. **Approval**: + - Only approve when all blocking issues (charon import, DI-seam violations, missing doc + comments, coverage gaps) are resolved. + - Verify `go build ./...`, `go vet ./...`, `staticcheck ./...`, and `scripts/test-coverage.sh` + all pass before signing off. + + + + +- **READ-ONLY**: Do not modify code — review and report only. +- **NO-CHARON-IMPORT IS BLOCKING**: This is the one rule that overrides all style preferences — + never wave it through as a suggestion. +- **PUBLIC-API AWARE**: Treat any exported-signature change as a breaking-change discussion, not a + routine diff. +- **CONSTRUCTIVE**: Focus on improvement, not criticism. +- **FOREGROUND EXECUTION ONLY** (see `CLAUDE.md`): If you run any command to verify build/lint/test + results, run it in the foreground and block until it completes — never background it and end + your turn to "check back later." + diff --git a/CLAUDE.md b/CLAUDE.md index 2e4c08f..8ea56da 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,11 +54,86 @@ Scaled down from Charon's much larger surface because none of it applies to a sm dependency-free Go library with a single maintainer: - No Trivy / GORM security scans — no SQL, no web-facing surface of its own. -- No Playwright / E2E — no frontend, no UI. -- No Docker build — this ships as a Go module via `go get`, not a binary or image. -- No multi-agent orchestration pipeline — for a repo this size, direct TDD implementation is the - right amount of process. Don't build out a Management/Planning/Supervisor agent roster here; it - would be process for its own sake at this scale. +- No Playwright / E2E — no frontend, no UI. The agent roster below has no `frontend-dev` or + `playwright-dev` equivalent for the same reason. +- No Docker build — this ships as a Go module via `go get`, not a binary or image. The `devops` + agent below does not manage containers. + +## Orchestration Model + +Mirrors Charon's orchestration model, scaled to this repo's surface. There is no separate +"management" wrapper agent — **the main Claude Code session IS the orchestrator.** It delegates +directly to the specialized agents below, reviews their output, and enforces the Definition of +Done, rather than bouncing through an intermediate agent that does the same thing one hop removed. + +The orchestrating session is not banned from reading source (`.go`) directly — read whatever's +needed for scoping, verification, or a bounded fix. What still always gets delegated is +implementation: never hand-edit library code yourself. + +- **Bounded work** (a well-scoped fix, chore, or CI/docs change to an existing flow — no written + spec needed): read what you need, then dispatch straight to the one specialist agent that owns + it (`go-dev`, `devops`, `docs-writer`) with a self-contained prompt. No planning-agent detour + required. +- **Feature-scale work** (a new exported API, a change to an existing provider's contract, or + anything that changes the public surface): run the full pipeline — + 1. Delegate to `planning` to research and write `docs/plans/current_spec.md` (with a Commit + Slicing Strategy). + 2. Delegate to `supervisor` to review the plan; iterate with `planning` until approved. + 3. Present the plan to the user and get explicit approval before implementation begins. + 4. Delegate implementation commit-by-commit to `go-dev` (and `devops` for CI/release-adjacent + commits); each commit must pass its own validation gate before the next starts. + 5. Delegate to `supervisor` again to review the implementation against the plan — the + no-Charon-import rule and any public-API change are blocking findings, not suggestions. + 6. Delegate to `qa-security` last — after every other change has landed — to run the lint/ + coverage/security gates and write `docs/reports/qa_report.md`. Loop back to step 1 if it finds + blocking issues. + 7. Delegate to `docs-writer` for README/INTEGRATION.md/doc-comment updates, then summarize the + work and provide the final conventional-commit message. + +**Team roster** (`.claude/agents/`): + +- **planning** — Principal Architect; writes `docs/plans/current_spec.md`. +- **supervisor** — Code Review Lead; reviews plans and implementations (read-only). Treats a + `github.com/Wikid82/charon/*` import and any undisclosed exported-API break as blocking. +- **go-dev** — Senior Go Engineer; implements providers, transport/retry logic, and factory wiring + (strict TDD, Red/Green). +- **qa-security** — QA & Security Engineer; lint/coverage gates, SSRF/URL-validation and + retry-behavior review, writes `docs/reports/qa_report.md`. Always runs last. +- **devops** — CI/CD specialist for the GitHub Actions workflows, GoReleaser, and Renovate — no + Docker, no deployable artifact. +- **docs-writer** — Technical writer for `README.md`, `docs/INTEGRATION.md`, and doc comments, + aimed at the Go engineer integrating this module — not an end-user audience. + +**Rules carried over from Charon's pipeline:** +- When multiple implementation options exist, prefer the long-term fix over a quick patch. +- Parallelize independent delegations freely, but never dispatch a second implementation pass onto + files a previous delegation's `qa-security` review is still validating — let one delegation, + including its QA, fully land before starting the next one on the same files. +- Every subagent prompt that involves running commands must explicitly instruct it to run them in + the foreground/blocking (see "Execution Discipline" below) — state it in the dispatch prompt + itself, don't assume the subagent already knows. + +## Execution Discipline: Foreground-Only Commands (MANDATORY) + +**All agents — the orchestrating session and every subagent — MUST run commands in the foreground +and block until they complete.** Never background a long-running command (`run_in_background: +true`, `&`, `nohup`, or any detached/async invocation) and end your turn to "check back later" or +"wait for the notification." + +**Why:** Backgrounding a command and pausing your turn to wait for it does not reliably resume +you. Ending a turn on that assumption leaves whoever dispatched the work waiting on a result that +never arrives on its own. + +**Rule:** +- Run `go build`, `go vet`, `staticcheck`, `go test`, `scripts/test-coverage.sh`, and integration + tests as blocking, foreground calls with a generous timeout. +- If a command genuinely needs longer than a single call's timeout, re-issue a blocking wait within + your own turn until you have a real result. Do not end your turn assuming something else will + wake you back up. +- If a call auto-backgrounds anyway (the tool's own timeout forces this): that is NOT permission to + end your turn and wait for a notification. Immediately re-attach to it in the same turn until you + have a real result. +- Never report a task as "running, will report when it lands" and then go idle. ## CI / Release diff --git a/lefthook.yml b/lefthook.yml new file mode 100644 index 0000000..22fd216 --- /dev/null +++ b/lefthook.yml @@ -0,0 +1,90 @@ +# lefthook.yml +# Scaled down from Charon's lefthook.yml (/projects/Charon/lefthook.yml) for a small, +# dependency-free Go library — no frontend, no Docker, no GORM/Trivy/semgrep/gitleaks surface. +# +# INSTALL: lefthook install +# MANUAL PIPELINES: +# lefthook run testing +# +# TOOL REQUIREMENTS: shellcheck, actionlint (staticcheck is auto-installed by its wrapper +# script below if missing). All hook scripts live in scripts/pre-commit-hooks/. + +# ============================================================ +# PRE-COMMIT (blocking, runs on every commit) +# ============================================================ +pre-commit: + parallel: true + commands: + + # --- File hygiene --- + end-of-file-fixer: + glob: "*.{go,yaml,yml,sh,md}" + run: | + modified=0 + for file in {staged_files}; do + [ -f "$file" ] && [ -s "$file" ] && \ + [ -n "$(tail -c1 "$file")" ] && echo >> "$file" && modified=1 + done + if [ "$modified" -eq 1 ]; then + echo "end-of-file-fixer: files modified — review the changes, 'git add' what you want included, and commit again." + exit 1 + fi + + trailing-whitespace: + glob: "*.{go,yaml,yml,sh,md}" + run: | + modified=0 + for file in {staged_files}; do + if grep -qP '\s+$' "$file" 2>/dev/null; then + sed -i 's/[[:space:]]*$//' "$file" && modified=1 + fi + done + if [ "$modified" -eq 1 ]; then + echo "trailing-whitespace: trailing spaces removed — review the changes, 'git add' what you want included, and commit again." + exit 1 + fi + + check-yaml: + glob: "*.{yaml,yml}" + run: python3 -c "import sys,yaml; [yaml.safe_load(open(f)) for f in sys.argv[1:]]" {staged_files} + + # --- Blocking guards --- + check-lfs-large-files: + run: bash scripts/pre-commit-hooks/check-lfs-for-large-files.sh + + block-codeql-db: + run: bash scripts/pre-commit-hooks/block-codeql-db-commits.sh + + # --- Shell / Actions --- + shellcheck: + glob: "*.sh" + run: shellcheck --severity=error {staged_files} + + actionlint: + glob: ".github/workflows/*.{yaml,yml}" + run: actionlint {staged_files} + + # --- Go (matches CLAUDE.md's "go vet + staticcheck must be clean" rule) --- + go-vet: + glob: "*.go" + run: go vet ./... + + staticcheck: + glob: "*.go" + run: bash scripts/pre-commit-hooks/staticcheck.sh + + +# ============================================================ +# MANUAL: testing +# Run with: lefthook run testing +# ============================================================ +testing: + parallel: true + commands: + go-test-coverage: + glob: "*.go" + run: bash scripts/test-coverage.sh + + go-test-integration: + glob: "*.go" + run: go test -tags=integration ./... diff --git a/scripts/pre-commit-hooks/block-codeql-db-commits.sh b/scripts/pre-commit-hooks/block-codeql-db-commits.sh new file mode 100755 index 0000000..188453e --- /dev/null +++ b/scripts/pre-commit-hooks/block-codeql-db-commits.sh @@ -0,0 +1,17 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Prevent accidentally committing a local CodeQL database directory (produced +# by running the CodeQL CLI locally per CLAUDE.md's CI/Release troubleshooting +# notes). Adapted from Charon's block-codeql-db-commits.sh — this repo has no +# data/backups/ path to exclude. +staged=$(git diff --cached --name-only | tr '\r' '\n' || true) +if [ -n "${staged}" ]; then + filtered=$(echo "$staged" | grep -v '^scripts/pre-commit-hooks/' || true) + if echo "$filtered" | grep -q "codeql-db"; then + echo "Error: Attempting to commit CodeQL database artifacts (codeql-db)." >&2 + echo "These should not be committed. Remove them or add to .gitignore and try again." >&2 + exit 1 + fi +fi +exit 0 diff --git a/scripts/pre-commit-hooks/check-lfs-for-large-files.sh b/scripts/pre-commit-hooks/check-lfs-for-large-files.sh new file mode 100755 index 0000000..cf68a7c --- /dev/null +++ b/scripts/pre-commit-hooks/check-lfs-for-large-files.sh @@ -0,0 +1,34 @@ +#!/usr/bin/env bash +set -euo pipefail + +# pre-commit hook: ensure large files added to git are tracked by Git LFS. +# Ported as-is from Charon (scripts/pre-commit-hooks/check-lfs-for-large-files.sh) — +# generic and applies unchanged to this repo. +MAX_BYTES=$((50 * 1024 * 1024)) +FAILED=0 + +STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACM) +if [ -z "$STAGED_FILES" ]; then + exit 0 +fi + +while read -r f; do + [ -z "$f" ] && continue + if [ -f "$f" ]; then + size=$(stat -c%s "$f") + if [ "$size" -gt "$MAX_BYTES" ]; then + filter_attr=$(git check-attr --stdin filter <<<"$f" | awk '{print $3}' || true) + if [ "$filter_attr" != "lfs" ]; then + echo "ERROR: Large file not tracked by Git LFS: $f ($size bytes)" >&2 + FAILED=1 + fi + fi + fi +done <<<"$STAGED_FILES" + +if [ $FAILED -ne 0 ]; then + echo "You must track large files in Git LFS. Aborting commit." >&2 + exit 1 +fi + +exit 0 diff --git a/scripts/pre-commit-hooks/staticcheck.sh b/scripts/pre-commit-hooks/staticcheck.sh new file mode 100755 index 0000000..b4e0c9e --- /dev/null +++ b/scripts/pre-commit-hooks/staticcheck.sh @@ -0,0 +1,35 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Wrapper for staticcheck so lefthook works the same whether or not the +# binary is already on PATH (mirrors the resolve-or-install pattern Charon +# uses for golangci-lint, scaled down to this repo's single linter). + +preferred_bin="${GOBIN:-${GOPATH:-$HOME/go}/bin}/staticcheck" + +resolve_staticcheck() { + if command -v staticcheck >/dev/null 2>&1; then + command -v staticcheck + return 0 + fi + if [[ -x "$preferred_bin" ]]; then + printf '%s\n' "$preferred_bin" + return 0 + fi + return 1 +} + +if ! STATICCHECK="$(resolve_staticcheck)"; then + echo "staticcheck not found — installing..." >&2 + go install honnef.co/go/tools/cmd/staticcheck@latest >&2 + if ! STATICCHECK="$(resolve_staticcheck)"; then + echo "ERROR: failed to install staticcheck" >&2 + echo "PATH: $PATH" >&2 + exit 1 + fi +fi + +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +cd "$ROOT_DIR" + +"$STATICCHECK" ./... From 2b58bb31a93100cb7a8b2254a2d356a9307d96ea Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:22:46 +0000 Subject: [PATCH 02/12] docs: add Web Push provider spec (issue #14) Records the approved technical spec for providers/webpush (RFC 8291/8292 Web Push) so the design that drove the implementation is tracked in repo history alongside it, matching Charon's docs/plans/*.md convention. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- docs/plans/current_spec.md | 836 +++++++++++++++++++++++++++++++++++++ 1 file changed, 836 insertions(+) create mode 100644 docs/plans/current_spec.md diff --git a/docs/plans/current_spec.md b/docs/plans/current_spec.md new file mode 100644 index 0000000..cb952c2 --- /dev/null +++ b/docs/plans/current_spec.md @@ -0,0 +1,836 @@ +# Technical Spec: `providers/webpush` — Web Push (Push API / VAPID) + +- **Source**: GitHub issue #14, "Add provider: Web Push (Push API / VAPID)". +- **Scope authorization**: the maintainer (issue author/assignee, Wikid82/Jeremy) explicitly asked + for this provider to be planned and implemented in the conversation that produced this spec. This + satisfies both CLAUDE.md's "no new provider integrations without an explicit ask" gate and the + issue's own "do not implement without maintainer sign-off" note. +- **Status**: plan only — no implementation code has been written. Ready for Red/Green + implementation per the Commit Slicing Strategy (§6). + +--- + +## 1. Introduction + +### 1.1 What this adds + +A new provider package, `providers/webpush`, implementing `notify.Sender` for direct browser Web +Push delivery: sending a payload straight to a subscribed browser's push endpoint +(`PushSubscription.endpoint`), authenticated with a VAPID JSON Web Token (RFC 8292) and encrypted +per the `aes128gcm` content-coding (RFC 8291). No third-party relay is involved — this is the +mechanism ntfy's web client, Pinglet, and Pingram build on top of, exposed directly. + +### 1.2 Why this is in scope now + +Per issue #14: this is a "genuinely different capability from the other providers in this module +(they're all relays; this is direct delivery)," and Apprise has no equivalent plugin — this is new +ground, not a straight port. The maintainer's explicit ask (§ above) clears CLAUDE.md's scope gate +for adding a provider beyond the original Charon-ported list (Discord, Slack, Gotify, Pushover, +Ntfy, webhook, Telegram, email). + +### 1.3 What this does *not* do (explicit non-goals) + +- **No new provider beyond `webpush`.** Nothing here touches Twilio/PagerDuty/Matrix/etc. +- **No Charon-side wiring.** `Charon`'s `NotificationProvider` GORM model's `ServiceConfig` column + is dead/unwired today (confirmed in `/projects/Charon/docs/plans/notify_provider_registry_spec.md` + §2.4/§3.1) — wiring it up to actually carry a `>2`-field provider config like webpush's is + explicitly **out of scope for this module's spec**. This plan only concerns + `go_notify_yourself`; wiring Charon's UI/DB to construct a `webpush.Config` is separate, + downstream work the maintainer has not asked for here. +- **No literal RFC 8030 `TTL: 0` ("attempt-only, don't store") escape hatch.** See §3.3's TTL + discussion — this is a deliberate, documented simplification, not an oversight. +- **No subscription lifecycle management** (no code here parses/refreshes browser + `PushSubscription.expirationTime`, handles 404/410 "subscription gone" responses specially, or + persists subscriptions) — that is a per-recipient data-management concern for the host + application, symmetric with how, e.g., `providers/telegram` doesn't manage chat-ID lifecycle + either. `Send`'s error return already surfaces a 404/410 from the push service as a normal + `transport.Wrapper` error (`provider returned status 410`); the host decides what to do with it. + +--- + +## 2. Research Findings + +### 2.1 Existing provider convention (read directly: `providers/ntfy/*`, `providers/pushover/*`, +`providers/email/register.go`, `ARCHITECTURE.md` §3) + +Every HTTP-based provider package follows exactly this shape (`providers/ntfy/ntfy.go` is the +clean exemplar cited in the task, confirmed by direct read): + +```go +type Config struct { /* exported fields, Template/CustomTemplate at the end */ } +type Client struct { cfg Config; wrapper *transport.Wrapper } +var _ notify.Sender = (*Client)(nil) +func New(cfg Config, w *transport.Wrapper) *Client +func (c *Client) Send(ctx context.Context, msg notify.Message) error +``` + +`register.go` (separate file) does `init()`-time `notify.Register("", factory)`, type-asserts +`config["transport"]` as `*transport.Wrapper`, and builds `Config` from `regconfig.StringField`/ +`StringSliceField` calls keyed by the field's lowercase snake_case name. Factories return +`fmt.Errorf`, never panic, on bad/missing config (`ARCHITECTURE.md` §3.5, §3.9). + +`providers/internal/render` (read directly) supplies `SelectTemplate`/`Render`/`TemplateData` — +the shared Go `text/template` engine every HTTP provider uses to turn `notify.Message` into a JSON +payload string, with `MinimalTemplate`/`DetailedTemplate` built-ins and a `toJSON` helper. Webpush +reuses this unchanged — the payload it encrypts is exactly this rendered JSON string, not a new +shape. + +`providers/internal/regconfig` (read directly) currently has **only** `StringField` and +`StringSliceField` — **confirmed no `IntField` or numeric helper exists today.** Webpush's `TTL +int` config value needs one; scoped as a small standalone first commit (§6, commit 1). + +`providers/pushover/pushover.go` (read directly) is the existing exemplar of a `Config` with more +than the `URL`/`Token` two-slot shape (`UserKey, APIToken, BaseURL, Template, CustomTemplate` — 5 +fields) — confirming this module's own `Config` structs are not limited to two fields the way +Charon's GORM model is. Webpush's 11-field `Config` (§3.1) is a larger step in the same direction, +not a new pattern. + +`providers/email/register.go` (read directly) is the existing exemplar for factories that +type-assert *behavioral* (non-string) values directly out of the config map under a well-known key +(e.g. `config["mailer"].(Mailer)`) rather than via a `regconfig` helper. Not needed for webpush — +every webpush `Config` field is a plain string or int, so `regconfig.StringField`/`IntField` cover +all of it; no behavioral-interface config field is proposed here. + +### 2.2 `transport.Wrapper` (read directly: `transport/wrapper.go`, `transport/retry.go`, +`transport/validate_default.go`) + +- `sanitizeOutboundHeaders` (in `transport/wrapper.go`, confirmed by direct read) currently + allowlists exactly: `content-type, user-agent, x-request-id, x-gotify-key, authorization`. Web + Push requires `Content-Encoding: aes128gcm` and `TTL` (RFC 8030 — push services expect a `TTL` + header on every request) on every request, and optionally `Urgency`/`Topic`. **None of these four + are in the current allowlist.** This is a shared-file change affecting every provider's request + path, not webpush-local — scoped as its own standalone commit (§6, commit 2), additive and + backward-compatible (existing providers send none of these headers today, so nothing already + sent changes). +- Headers are canonicalized via `http.CanonicalHeaderKey` after lowercasing (confirmed by read, + `transport/wrapper.go` L336-354). `http.CanonicalHeaderKey("ttl")` produces `"Ttl"`, not `"TTL"` + (Go has no acronym table) — this is harmless on the wire (HTTP/1.1 header names are + case-insensitive per RFC 7230 §3.2; HTTP/2 lowercases all header names in transit regardless), + but is worth an explicit test assertion (Phase 1, step 2 below) so a future reader isn't alarmed seeing `Ttl` in a + captured request. +- `Send` (confirmed by read, `transport/wrapper.go` L167-242) treats **any** response status `< + http.StatusBadRequest` as success and returns `*Result{StatusCode, ResponseBody, Attempts}` to + the caller. Push services conventionally return `201 Created` (sometimes `200`/`204`) on success — + **no `transport.Wrapper` change is needed for status-code handling**; this was verified by + reading the exact conditional (`if resp.StatusCode >= http.StatusBadRequest`), not assumed. +- `DefaultURLValidator` (confirmed by read, `transport/validate_default.go`) only allows `https://` + destinations unless `allowHTTP` is set. Every real push service endpoint (`fcm.googleapis.com`, + `updates.push.services.mozilla.com`, `web.push.apple.com`, etc.) is `https://` — no conflict. + `hasDisallowedQueryAuthKey` (in `transport/wrapper.go`) rejects destination URLs whose query + string contains `token`/`auth`/`apikey`/`api_key` params — modern push subscription endpoints + carry their auth material in the URL *path* (e.g. + `https://fcm.googleapis.com/fcm/send/`), not query params, so this does not + collide with a well-formed `Endpoint`. Flagged here so a reviewer doesn't have to rediscover it: + if a host ever configures a push endpoint with such a query param (non-standard, but the RFC + doesn't forbid it), `Send` will reject it — this module treats that identically to every other + provider's destination URL, deliberately not special-cased. + +### 2.3 Root package (read directly: `message.go`, `sender.go`, `factory.go`) + +No changes needed to `notify.Message`, `notify.Sender`, or `notify.Register`/`New`/ +`RegisteredTypes` — webpush fits the existing `Factory func(config map[string]any) (Sender, error)` +contract exactly like every other provider. + +### 2.4 Prior art in `/projects/Charon` (read directly: +`docs/plans/notify_provider_registry_spec.md`) + +That spec (Charon repo, not this module) discusses Web Push extensively as the motivating case for +why Charon's *registry* config boundary needed to be `map[string]any` rather than +`json.RawMessage`-typed generics (§3.2 of that doc): "a provider needing more than two config +values (e.g. Web Push's VAPID public/private keypair + subscription endpoint — three values, none +of which is a natural fit for 'URL' or 'Token') cannot be expressed in [Charon's] current two-slot +scheme at all." It also confirms (§2.4) Charon's `NotificationProvider.ServiceConfig` GORM column +is declared but has **zero read/write call sites** anywhere in Charon's backend — dead schema, +earmarked for exactly this kind of provider but not wired up. Per §1.3 above, wiring that up is +explicitly out of scope here; it's cited only as confirmation that this module's own `Config` +struct is unconstrained by Charon's schema (this module doesn't share Charon's DB layer at all — +CLAUDE.md's non-negotiable import rule). + +### 2.5 Cryptography — stdlib-only feasibility (verified via `go doc`, not assumed) + +This module is dependency-free (non-negotiable, CLAUDE.md). Two RFC-standardized protocol layers +are needed, both confirmed achievable with only the Go standard library at `go 1.27.1` (this +module's `go.mod` directive, confirmed by direct read): + +- **RFC 8292 (VAPID)**: an ES256-signed JWT. `crypto/ecdsa` (`ecdsa.Sign`, which returns `(r, s + *big.Int, err error)` directly — **not** `ecdsa.SignASN1`, which DER-encodes; JOSE/JWT ES256 + signatures are raw, left-zero-padded, big-endian `r || s`, 32 bytes each, 64 bytes total) plus + hand-rolled base64url JSON header/payload encoding. No JWT library needed or wanted. +- **RFC 8291 (`aes128gcm` content-coding)**: `crypto/ecdh` (confirmed present via `go doc + crypto/ecdh`: `ecdh.P256()`, `PrivateKey`, `PublicKey`) for the ECDH step between an ephemeral + server keypair and the subscriber's `p256dh` key; `crypto/hkdf` (confirmed present via `go doc + crypto/hkdf`: `hkdf.Extract`, `hkdf.Expand`, `hkdf.Key`, all added to the stdlib in Go 1.24 — this + module's `go 1.27.1` floor comfortably covers it) for the two-step key derivation; `crypto/aes` + + `crypto/cipher` for AES-128-GCM. All confirmed present in this Go toolchain by running `go doc` + directly, not assumed from general Go knowledge. +- **Important distinction for the implementer**: VAPID signing uses a `crypto/ecdsa.PrivateKey` + (the application's long-lived VAPID keypair). RFC 8291 payload encryption uses `crypto/ecdh` + keys for two *separate* P-256 keypairs — a fresh ephemeral one generated per `Send` call, and the + subscriber's `p256dh` public key. These are three distinct P-256 keys serving two different + purposes and two different Go stdlib APIs (`ecdsa` vs `ecdh`) — despite all being "P-256," no + direct reuse or conversion between the VAPID signing key and the encryption keys is needed or + correct; keep them handled by entirely separate code paths (§3.2 file layout reflects this). + +### 2.6 RFC 8291 Appendix A fixed test vectors + +RFC 8291 Appendix A ("A Detailed Example") publishes a complete fixed example: a receiver (UA) +P-256 keypair, an `auth` secret, a sender (application server) ephemeral P-256 keypair, a 16-byte +salt, the plaintext `"When I grow up, I want to be a watermelon"`, and the exact resulting +`aes128gcm` ciphertext bytes. This is critical for this implementation's correctness gate (Phase 1 +step 3 and §6 commit 3's blocking gate, both below) — +a self-encrypt/self-decrypt round-trip test alone cannot catch a bug that is symmetric in both +directions (e.g. a wrong HKDF `info` string used consistently on both the encrypt and decrypt side +would still round-trip), and there is no external Web Push library available to interop-test +against given the dependency-free constraint. The fixed-vector test is exact-bytes, not +round-trip, and is a **blocking** part of Phase 1/2 (§5). + +--- + +## 3. Technical Specification + +### 3.1 Package layout + +``` +providers/webpush/ + webpush.go # package doc comment, Config, Client, New, Send (orchestration only) + vapid.go # RFC 8292: GenerateVAPIDKeyPair (exported), buildVAPIDHeader (unexported) + encrypt.go # RFC 8291: encryptAES128GCM (unexported) + its WithKeys test seam + webpush_test.go + vapid_test.go + encrypt_test.go # includes the RFC 8291 Appendix A fixed-vector test + register.go + register_test.go +``` + +No new `providers/internal/*` subpackage — the crypto pieces are webpush-specific (unlike +`render`/`regconfig`, which are shared across every provider), so they stay as unexported +same-package files, the same way `providers/email/default_template.go` is a same-package file +alongside `email.go` rather than its own internal package. + +`` is `webpush` — lowercase, no underscore, already used as `ARCHITECTURE.md`'s own running +example (§3.1, §3.7) for exactly this reason. No naming decision to make. + +### 3.2 `Config` (exported, `webpush.go`) + +```go +// Config configures a webpush Sender. Unlike every other provider's Config, +// this one mixes two conceptually distinct groups of fields: VAPID +// application identity (shared across every subscription this application +// pushes to) and one subscriber's PushSubscription destination. A host +// application constructs one webpush.Client per subscriber, reusing the +// same VAPID* values across all of them — see the package doc comment for +// the fan-out pattern. +type Config struct { + // --- VAPID application identity (RFC 8292) --- + + // VAPIDPublicKey is the application server's VAPID public key: an + // uncompressed P-256 point (65 bytes: 0x04 || X || Y), base64url + // (no padding) encoded. This is the same value the browser is given as + // PushManager.subscribe({applicationServerKey: VAPIDPublicKey}). + // Required. + VAPIDPublicKey string + + // VAPIDPrivateKey is the application server's VAPID private key: a + // 32-byte P-256 scalar, base64url (no padding) encoded. Required. This + // value never leaves the process — Send signs a JWT with it locally + // and never transmits it. + VAPIDPrivateKey string + + // VAPIDSubject identifies the application server operator, per RFC + // 8292's "sub" JWT claim: a "mailto:" or "https:" URI (e.g. + // "mailto:ops@example.com"). Some push services (notably Mozilla's) + // reject a VAPID JWT with an empty or malformed sub. Required. + VAPIDSubject string + + // --- Subscriber destination (the browser's PushSubscription) --- + + // Endpoint is the subscription's push service URL, from + // PushSubscription.endpoint. Required. + Endpoint string + + // P256dh is the subscriber's P-256 Diffie-Hellman public key, from + // PushSubscription.getKey('p256dh'): base64url (no padding) encoded. + // Required. + P256dh string + + // Auth is the subscriber's 16-byte authentication secret, from + // PushSubscription.getKey('auth'): base64url (no padding) encoded. + // Required. + Auth string + + // --- Delivery hints (RFC 8030) --- + + // TTL is the number of seconds the push service should retain the + // message if the subscriber is currently offline, sent as the "TTL" + // header. Zero uses DefaultTTL — see that constant's doc comment. + TTL int + + // Urgency is an optional RFC 8030 "Urgency" header value: one of + // "very-low", "low", "normal", "high". Empty omits the header (the + // push service's own default applies, typically "normal"). Send + // rejects any other value. + Urgency string + + // Topic is an optional RFC 8030 "Topic" header value: up to 32 + // characters from the URL-and-filename-safe base64 alphabet + // ([A-Za-z0-9_-]). When set, a pending undelivered message with the + // same Topic is replaced rather than queued alongside it. Empty omits + // the header. Send rejects a Topic outside this charset/length. + Topic string + + // --- Payload templating --- + + // Template selects the JSON payload shape: "minimal" (default), + // "detailed", or "custom" (uses CustomTemplate) — same convention as + // every other JSON-payload provider (providers/internal/render). The + // rendered JSON is the plaintext that gets RFC 8291-encrypted; the + // receiving service worker's `push` event handler is responsible for + // JSON.parse-ing the decrypted payload. This module has no opinion on + // what the service worker does with it beyond that it is valid JSON. + Template string + + // CustomTemplate is a user-supplied Go text/template string, used when + // Template is "custom". + CustomTemplate string +} + +// DefaultTTL is used for the RFC 8030 "TTL" header when Config.TTL is zero. +// Four weeks (2,419,200 seconds) — a conservative value inside the maximum +// retention window most push services honor before evicting an +// undelivered message. See Config.TTL's doc comment and §3.3 of this +// module's design notes for why Config.TTL's zero value is *not* treated +// as RFC 8030's spec-legal "attempt immediate delivery only, don't store" +// meaning. +const DefaultTTL = 4 * 7 * 24 * 3600 +``` + +**Decision — `GenerateVAPIDKeyPair` (issue's open question 1): include it.** + +```go +// GenerateVAPIDKeyPair generates a new P-256 VAPID application server +// keypair, returned as the same base64url (no padding) encoded strings +// Config.VAPIDPublicKey/Config.VAPIDPrivateKey expect. Intended to be +// called once at application setup time (e.g. from an init/CLI flow) and +// the results persisted by the host application — every browser +// PushSubscription is bound to the exact public key it was created with +// (PushManager.subscribe({applicationServerKey: ...})), so rotating this +// keypair invalidates every existing subscription the host has collected. +// privateKey is a credential, not a diagnostic value: callers must not log +// it (a mistake this function can't prevent, only warn against — the same +// discipline hosts already need for VAPIDPrivateKey once it's in Config). +func GenerateVAPIDKeyPair() (publicKey, privateKey string, err error) +``` + +Reasoning against CLAUDE.md's "keep the public surface intentionally small" guidance: that +guidance is about not accumulating unnecessary provider-to-provider surface area / avoiding +breaking-change risk, not about refusing one small, self-contained helper that removes an entire +manual-crypto step. Concretely: + +- Every real-world Web Push library (`web-push` npm, `pywebpush`, Go's own `SherClockwork/webpush`) + ships an equivalent generator, because hand-producing a correct raw-uncompressed-point-encoded + P-256 keypair via `openssl` CLI incantations is exactly the kind of fiddly, easy-to-get-subtly-wrong + step (compressed vs. uncompressed point, DER vs. raw, base64 vs. base64url, padded vs. + unpadded) that silently produces a keypair the browser's `PushManager.subscribe` rejects or a + push service 401s on — hard for a non-technical host (this project's stated target audience, + per the maintainer's UX-friction guidance for this task) to self-diagnose. +- It is a pure, stateless, one-shot function — it doesn't grow `Send`'s per-request surface, add a + DI seam, or create a new exported type. A host that already has externally-generated keys (e.g. + from `web-push generate-vapid-keys`) can ignore this function entirely; `Config` only ever takes + plain base64url strings either way, so this is strictly additive convenience, not a new + requirement. +- Unlike a bot token or webhook URL (issued by a third-party service the host copies from a + website), a VAPID keypair is *this application's own* identity — there is no external "go get + this" step to document instead; the host is expected to generate it itself once. That is a + meaningfully different case from every other provider's credentials, justifying a first-of-its-kind + helper here without setting a precedent that every future provider needs one. + +**Decision — `TTL` zero-value semantics (issue's open question 2): package-level default constant, +not the RFC's spec-legal zero-value meaning.** + +RFC 8030 §5.2 specifies `TTL: 0` as legal and meaningful: "attempt to deliver the message +immediately, and if that's not possible (subscriber offline), don't store it — drop it." That is a +reasonable choice for some applications (e.g. transient live-typing indicators) but is a +**surprising silent default** for this module's stated audience: a self-hoster who configures a +notification and expects it to "eventually show up," not silently vanish because the browser tab +happened to be closed at send time. Per the maintainer's explicit UX-friction-minimization +guidance for this task, `Send` treats `Config.TTL == 0` as "unset" and substitutes `DefaultTTL` +(4 weeks — see the constant's doc comment above) rather than forwarding a literal `0` to the push +service. This produces the most reliable out-of-the-box delivery experience with zero required +host-side configuration (a host that wants the RFC's literal immediate-only semantics can still +get arbitrarily close by setting `Config.TTL` to a very small positive number, e.g. `1`; there is +deliberately no way to configure a literal `TTL: 0` request through this `Config` — flagged here +explicitly as a documented, intentional simplification, not an oversight, and not something to +silently work around later without re-opening this decision). + +**This is a permanent public-API foreclosure, not just a mutable default.** Because `Config.TTL` +has no sentinel distinct from Go's own int zero value, there is no future-compatible way to add a +"no really, send a literal 0" escape hatch to this exact field later without a breaking change +(e.g. a new `Config` field, or changing `TTL`'s type) — this decision permanently removes RFC +8030's "attempt-only, don't store" semantics from this `Config`'s expressible range, for every +future caller, not merely until someone changes a default. It is deliberately made here under the +maintainer's pre-delegated UX-friction guidance for this task, but — being irreversible rather than +adjustable — it is called out explicitly so the maintainer can give it one explicit nod before +implementation begins, even though it was pre-delegated. + +### 3.3 `Client` / `New` / `Send` (`webpush.go`) + +```go +// Package webpush implements notify.Sender for direct browser Web Push +// delivery (RFC 8030/8291/8292) — no third-party relay involved. A single +// Config pairs one application's VAPID identity with one browser +// PushSubscription; a host application fanning a Message out to many +// subscribers constructs one *Client per subscription (cheap: New does no +// I/O) and calls Send on each, exactly like fanning out to many +// Sender values of any other provider type. +package webpush + +// Client dispatches notify.Message values to one browser PushSubscription. +type Client struct { + cfg Config + wrapper *transport.Wrapper +} + +var _ notify.Sender = (*Client)(nil) + +// New constructs a webpush Client. w performs the actual dispatch — see +// transport.NewWrapper. +func New(cfg Config, w *transport.Wrapper) *Client + +// Send renders msg using the configured template, RFC 8291-encrypts the +// result for the configured subscriber, and dispatches it to +// cfg.Endpoint via the shared transport.Wrapper, authenticated with an +// RFC 8292 VAPID JSON Web Token signed for this request. +func (c *Client) Send(ctx context.Context, msg notify.Message) error +``` + +`Send`'s algorithm, in order (fail-fast: every validation step below runs before any network +activity or expensive crypto work; each returns a `fmt.Errorf`-wrapped, field-naming error on +failure, mirroring `ntfy`/`pushover`'s style): + +1. **Required-field validation**, in this order, each its own error message (mirrors + `pushover.Send`'s "api token" / "user key" sequential-check style): + `VAPIDPublicKey`, `VAPIDPrivateKey`, `VAPIDSubject`, `Endpoint`, `P256dh`, `Auth` — each + `strings.TrimSpace`'d and, if empty, `fmt.Errorf("webpush: is not configured")`. + Immediately after the `VAPIDSubject` emptiness check, a cheap format check: `VAPIDSubject` must + start with `mailto:` or `https://` (a plain `strings.HasPrefix` check on either, no full URI + parse) or `fmt.Errorf("webpush: VAPID subject must start with %q or %q", "mailto:", "https://")`. + This is the same fail-fast-before-any-network-work treatment as every other precondition in this + step — added specifically because this package's own `VAPIDSubject` doc comment already warns + that some push services (notably Mozilla's) reject a malformed `sub` claim, so silently accepting + a clearly-malformed value here (e.g. a bare email address with no scheme) would defer a locally + catchable error into an opaque remote 401/403. +2. **VAPID keypair consistency check**: decode `VAPIDPrivateKey`, derive its corresponding public + key (`crypto/ecdsa` — `(*ecdsa.PrivateKey).PublicKey`, re-encoded to the same uncompressed + base64url form), and compare byte-for-byte against the configured `VAPIDPublicKey`. Mismatch → + `fmt.Errorf("webpush: VAPID public/private key pair does not match")`. This catches a very + common real-world misconfiguration (copy-pasting one half of a keypair against the other half of + a different generation) with a clear, actionable error instead of an opaque `401` surfaced later + from the push service by `transport.Wrapper`. +3. **`Urgency` validation** (if non-empty): must be one of `very-low`, `low`, `normal`, `high` + (case-sensitive, matching RFC 8030's literal token values) or + `fmt.Errorf("webpush: invalid urgency %q", cfg.Urgency)`. +4. **`Topic` validation** (if non-empty): must match `^[A-Za-z0-9_-]{1,32}$` or + `fmt.Errorf("webpush: invalid topic %q: must be 1-32 URL-safe base64 characters", cfg.Topic)`. +5. **Render the template**: `render.SelectTemplate` + `render.Render`, identical call shape to + `ntfy`/`pushover`. Validate the rendered output is valid JSON (`json.Unmarshal` into `any`) — + `fmt.Errorf("invalid JSON payload: %w", err)` on failure, matching `ntfy`/`pushover`'s existing + message text convention. **Unlike `ntfy`/`pushover`, do not require a `"message"` field** — that + requirement is specific to those providers' own remote API contract; a Web Push payload's shape + is entirely up to the receiving service worker's own JS, which this module has no visibility + into or opinion about. +6. **Encrypt**: `encryptAES128GCM(cfg.P256dh, cfg.Auth, renderedJSONBytes)` (§3.4) → + `ciphertext []byte`. Decode/format errors from this step (bad base64, wrong-length key/secret, + or `renderedJSONBytes` longer than `MaxPlaintextSize` — §3.4) propagate as + `fmt.Errorf("webpush: encrypt payload: %w", err)`. +7. **Build the VAPID Authorization header**: + `buildVAPIDHeader(cfg.VAPIDPublicKey, cfg.VAPIDPrivateKey, cfg.VAPIDSubject, cfg.Endpoint)` + (§3.5) → `authHeader string`, or a wrapped error. `buildVAPIDHeader` takes the three VAPID + strings directly rather than the whole `Config` deliberately — see §3.5's signature note. +8. **Build headers**: + ```go + headers := map[string]string{ + "Content-Type": "application/octet-stream", + "Content-Encoding": "aes128gcm", + "Authorization": authHeader, + "TTL": strconv.Itoa(ttl), // ttl = cfg.TTL, or DefaultTTL if cfg.TTL == 0 + } + if cfg.Urgency != "" { headers["Urgency"] = cfg.Urgency } + if cfg.Topic != "" { headers["Topic"] = cfg.Topic } + ``` +9. **Dispatch**: `c.wrapper.Send(ctx, transport.Request{URL: cfg.Endpoint, Headers: headers, Body: + ciphertext})`. Wrap any error as `fmt.Errorf("failed to send web push: %w", err)` (matching the + existing "failed to send webhook"-style wording convention, adapted to this provider's name). + +### 3.4 `encryptAES128GCM` (RFC 8291, `encrypt.go`, unexported) + +```go +// encryptAES128GCM implements RFC 8291 Web Push message encryption. Given +// the subscriber's base64url (no padding) encoded p256dh public key and +// auth secret (from PushSubscription.getKey), and the plaintext +// application payload, it returns the aes128gcm content-coded ciphertext +// (RFC 8188 §2 single-record framing: salt(16) || rs(4) || idlen(1) || +// keyid(65, the ephemeral sender public key, uncompressed) || +// AEAD-ciphertext) ready to send as the request body. Generates a fresh +// ephemeral P-256 keypair and a fresh random 16-byte salt per call — see +// encryptAES128GCMWithKeys for the deterministic variant tests use. +func encryptAES128GCM(p256dhB64, authB64 string, plaintext []byte) ([]byte, error) + +// encryptAES128GCMWithKeys is encryptAES128GCM with the ephemeral sender +// keypair and salt injected rather than randomly generated — the +// production encryptAES128GCM is a thin wrapper generating both randomly +// and delegating here. Exists so tests (in particular the RFC 8291 +// Appendix A fixed-vector test, encrypt_test.go) can force the exact +// keys/salt the RFC's published example uses and assert exact-byte +// output — a capability a purely-random production path can't otherwise +// be tested against without an external reference implementation, which +// this dependency-free module cannot depend on. +func encryptAES128GCMWithKeys(ephemeral *ecdh.PrivateKey, salt []byte, p256dhB64, authB64 string, plaintext []byte) ([]byte, error) +``` + +```go +// MaxPlaintextSize is the largest plaintext payload encryptAES128GCM will +// accept, in bytes. RFC 8188 §2 single-record framing adds a fixed 86-byte +// record header (salt(16) + rs(4) + idlen(1) + keyid(65)) plus a 1-byte +// delimiter and a 16-byte AES-GCM tag around the plaintext (103 bytes of +// fixed overhead total), and real push services independently cap the +// resulting request body at roughly 4096 bytes (FCM and Mozilla autopush +// both document limits in this neighborhood). MaxPlaintextSize is set well +// inside that ceiling (86 + 3800 + 17 = 3903 bytes total, vs. a ~4096-byte +// external cap) rather than exactly at the boundary, so a plaintext this +// module accepts is not immediately at risk of a push-service-side +// rejection this module can't see coming. +const MaxPlaintextSize = 3800 +``` + +Before any derivation work, `encryptAES128GCMWithKeys` (and therefore `encryptAES128GCM`, which +calls it) checks `len(plaintext) > MaxPlaintextSize` and returns +`fmt.Errorf("webpush: payload of %d bytes exceeds maximum plaintext size of %d bytes", len(plaintext), MaxPlaintextSize)` +— fail-fast, consistent with §3.3's fail-fast design: no ECDH/HKDF/AES work is attempted on an +oversized payload. `Send`'s step 6 (§3.3) wraps this the same way it wraps every other +`encryptAES128GCM` error, so no separate size-check step is needed in `Send`'s own algorithm. + +Derivation steps (RFC 8291 §3.3-3.4, cited precisely for the implementer — not implemented here +per the "no implementation code" constraint), run only once the size check above passes: + +1. Decode `p256dhB64`/`authB64` (base64url, no padding via `base64.RawURLEncoding`). `p256dh` must + decode to a 65-byte uncompressed P-256 point; `auth` must decode to exactly 16 bytes. Either + mismatch is a returned error naming which field and why (e.g. `"p256dh: expected 65-byte + uncompressed P-256 point, got %d bytes"`). +2. Parse the subscriber's `p256dh` as an `*ecdh.PublicKey` via `ecdh.P256().NewPublicKey(raw)`. +3. Compute the ECDH shared secret between `ephemeral` (the sender's ephemeral private key) and the + subscriber's public key: `ephemeral.ECDH(subscriberPub)`. +4. Per RFC 8291 §3.4: derive `IKM` via `HKDF-Extract(salt=auth_secret, ikm=ecdh_secret)` with an + `HKDF-Expand` info string of `"WebPush: info" || 0x00 || ua_public(65 bytes) || as_public(65 + bytes)`, 32 bytes output — `ua_public` is the subscriber's raw `p256dh` bytes, `as_public` is + the ephemeral sender public key's raw uncompressed bytes. +5. Derive the content-encryption key and nonce from `IKM` and `salt` (the random/injected 16-byte + salt, distinct from the `auth` secret used as HKDF salt in step 4): `PRK = HKDF-Extract(salt, + IKM)`; `CEK = HKDF-Expand(PRK, "Content-Encoding: aes128gcm" || 0x00, 16)`; `nonce = + HKDF-Expand(PRK, "Content-Encoding: nonce" || 0x00, 12)`. +6. Per RFC 8188 §2: append a single `0x02` delimiter byte to `plaintext` (no padding beyond the + delimiter, since this is always a single, final record — payloads are capped well under the + 4096-byte example record size RFC 8291 uses). +7. `AES-128-GCM` encrypt (`crypto/cipher.NewGCM` over an `crypto/aes.NewCipher(CEK)` block) the + delimited plaintext with `nonce`, no additional authenticated data. +8. Frame per RFC 8188 §2: `salt (16 bytes) || rs (4 bytes, big-endian record size — use 4096, the + value RFC 8291's own example uses, since this module always emits exactly one record) || idlen + (1 byte, 65) || keyid (65 bytes, the ephemeral sender's raw uncompressed public key) || + ciphertext-with-tag`. + +### 3.5 VAPID JWT (RFC 8292, `vapid.go`) + +```go +// vapidJWTLifetime bounds the "exp" claim on the VAPID JWT Send signs for +// each request: 12 hours from the time of signing. RFC 8292 recommends an +// expiration no more than 24 hours out; 12 hours is comfortably inside +// that bound while still meaning a Client's signed header is reusable +// across a short burst of retries/sends without re-signing every time +// (though Send always signs fresh per call — see below). +const vapidJWTLifetime = 12 * time.Hour + +// buildVAPIDHeader builds the RFC 8292 "Authorization: vapid t=, +// k=" header value for a request to endpoint, signed with the +// given VAPID keypair/subject. aud is derived from endpoint's scheme+host +// (RFC 8292 §2: the JWT audience is the push service's origin, not the +// full subscription path). +// +// Takes the three VAPID strings directly rather than the whole Config by +// design, not just convenience: buildVAPIDHeader only ever reads 3 of +// Config's 11 fields, and Config itself isn't defined until webpush.go +// (§3.3/§6 commit 5) — a Config parameter here would make vapid.go (§6 +// commit 4) depend on a type that doesn't exist yet at that point in the +// commit sequence, breaking §6's per-commit build/test guarantee. Taking +// plain strings keeps this function buildable and independently testable +// (vapid_test.go) two commits before Config exists. +func buildVAPIDHeader(vapidPublicKey, vapidPrivateKey, vapidSubject, endpoint string) (string, error) +``` + +- `aud` = `neturl.Parse(endpoint)`'s `Scheme + "://" + Host` (no path, no trailing slash). +- `exp` = `time.Now().Add(vapidJWTLifetime).Unix()`. +- `sub` = `vapidSubject` verbatim (already required non-empty, and prefix-validated, by `Send`'s + step 1 — §3.3; `buildVAPIDHeader` itself doesn't re-validate it, since it has no `Config` to read + a validation policy from and takes its inputs on trust from the caller. This is the same + "structural vs. semantic" split §3.9 describes across the registry/typed-constructor boundary, + applied here within one package: `Send` owns semantic validation, `buildVAPIDHeader` is a pure + JWT-construction primitive.) +- JWT header: `{"typ":"JWT","alg":"ES256"}`, base64url (no padding) of the compact JSON. +- JWT payload: `{"aud":"","exp":,"sub":""}`, same encoding. +- Signature: `ecdsa.Sign(rand.Reader, privKey, sha256(header + "." + payload))` → `(r, s)`, each + left-zero-padded big-endian to 32 bytes, concatenated (64 bytes total), base64url (no padding) + encoded — **not** `ecdsa.SignASN1`, which DER-encodes and is the wrong shape for JOSE/JWS ES256. + `privKey` here is the `*ecdsa.PrivateKey` decoded from `vapidPrivateKey`. +- Returned value: `fmt.Sprintf("vapid t=%s.%s.%s, k=%s", headerB64, payloadB64, sigB64, + vapidPublicKey)`. + +`GenerateVAPIDKeyPair` (§3.2, also lives in `vapid.go`): `ecdsa.GenerateKey(elliptic.P256(), +rand.Reader)`, then encode the private key's `D` (32-byte big-endian scalar) and the public key's +uncompressed point (`0x04 || X(32) || Y(32)`, both big-endian, zero-padded) each via +`base64.RawURLEncoding`. + +### 3.6 `register.go` + +```go +// init registers this package's Factory under the name "webpush" with the +// notify package's registry. +// +// Expected config keys: +// - "transport" (required): *transport.Wrapper. +// - "vapid_public_key", "vapid_private_key", "vapid_subject" (string, required). +// - "endpoint", "p256dh", "auth" (string, required). +// - "ttl" (int, optional; 0 uses DefaultTTL). +// - "urgency", "topic" (string, optional). +// - "template", "custom_template" (string, optional). +func init() { + notify.Register("webpush", func(config map[string]any) (notify.Sender, error) { + w, ok := config["transport"].(*transport.Wrapper) + if !ok || w == nil { + return nil, fmt.Errorf(`webpush: config["transport"] must be a non-nil *transport.Wrapper`) + } + cfg := Config{ + VAPIDPublicKey: regconfig.StringField(config, "vapid_public_key"), + VAPIDPrivateKey: regconfig.StringField(config, "vapid_private_key"), + VAPIDSubject: regconfig.StringField(config, "vapid_subject"), + Endpoint: regconfig.StringField(config, "endpoint"), + P256dh: regconfig.StringField(config, "p256dh"), + Auth: regconfig.StringField(config, "auth"), + TTL: regconfig.IntField(config, "ttl"), + Urgency: regconfig.StringField(config, "urgency"), + Topic: regconfig.StringField(config, "topic"), + Template: regconfig.StringField(config, "template"), + CustomTemplate: regconfig.StringField(config, "custom_template"), + } + return New(cfg, w), nil + }) +} +``` + +Follows `ARCHITECTURE.md` §3.5's template exactly; no deviation. + +### 3.7 New shared-infrastructure surface (not webpush-local) + +**`providers/internal/regconfig.IntField`** (new, in `regconfig.go`): + +```go +// IntField returns config[key] as an int, or 0 if the key is absent or not +// an int-like value. Accepts int and int64 (the natural Go-side shapes) +// and float64 (the shape a generic JSON-style decode into map[string]any +// produces, since encoding/json decodes every JSON number as float64) — +// mirroring StringSliceField's existing dual-shape acceptance for []any. +func IntField(config map[string]any, key string) int +``` + +**`transport.sanitizeOutboundHeaders`** (change, in `transport/wrapper.go`): add +`"content-encoding"`, `"ttl"`, `"urgency"`, `"topic"` to the `allowed` set (§2.2). No other change +to `transport/wrapper.go`. + +### 3.8 Error handling / edge cases summary + +| Case | Behavior | +|---|---| +| Missing any of the 6 required `Config` fields | `Send` returns `fmt.Errorf("webpush: is not configured")` before any crypto/network work | +| `VAPIDSubject` set but missing the `mailto:`/`https://` prefix | `Send` returns a named format error before any network work (§3.3 step 1) | +| `VAPIDPublicKey`/`VAPIDPrivateKey` don't form a matching pair | `Send` returns a named error before any network work (§3.3 step 2) | +| Malformed base64 or wrong-length `p256dh`/`auth`/VAPID keys | `Send` returns a wrapped decode error naming the field | +| Invalid `Urgency`/`Topic` value | `Send` returns a named validation error before any network work | +| Rendered payload exceeds `MaxPlaintextSize` (§3.4) | `Send` returns a wrapped size error (via `encryptAES128GCM`) before any encryption work is attempted | +| Custom template renders invalid JSON | `Send` returns `"invalid JSON payload: %w"`, same wording as `ntfy`/`pushover` | +| Push service returns 4xx/5xx | Surfaced unchanged via `transport.Wrapper.Send`'s existing `"provider returned status %d[: hint]"` error — no webpush-specific handling; symmetric with every other provider | +| Push service returns 404/410 (subscription gone) | Same as any other 4xx — no special-casing (§1.3); host application's responsibility to react | +| `ctx` cancelled/deadline exceeded | Propagates via `transport.Wrapper.Send`'s existing `ctx`-respecting `http.NewRequestWithContext` | +| VAPID JWT signing failure (`crypto/rand` exhausted, etc.) | `Send` returns a wrapped error; treated as any other precondition failure, not retried (this is not a transient network condition `transport.RetryPolicy` should retry) | + +--- + +## 4. Documentation updates (Phase 4 scope, detailed in §6) + +- `README.md`: add a `providers/webpush` row to the provider table (§"Provider packages", currently + lines 90-99) and update the "Project status" paragraph (currently line 222: "the provider list is + intentionally exactly these seven HTTP providers plus email") to note webpush as an explicit, + deliberate, maintainer-approved addition beyond the original Charon-extraction list, distinguishing + it from the "no new providers without an explicit ask" policy it doesn't violate. +- `docs/INTEGRATION.md`: add a `webpush.New(webpush.Config{...}, wrapper)` construction example + alongside the existing `discord.New(...)` one (currently line 162), and a short note on the + "one Client per subscriber, shared VAPID identity" fan-out pattern (§3.3's package doc comment). +- `ARCHITECTURE.md`: §3.2 ("The `Config` struct convention") gets one short addition noting webpush + as the first provider whose `Config` mixes app-wide identity fields with per-recipient + destination fields in a single struct — still one flat exported struct, still fed through the + same `New(cfg, w)` constructor shape, so §3.3 needs no rewrite; just a sentence flagging the + precedent for a future reader who might otherwise assume every `Config` field is per-recipient. + No other section needs a substantive change — §3.6/3.7 already use `webpush` as their own running + example name. + +--- + +## 5. Implementation Plan + +### Phase 1 — Failing tests (Red) + +Write, in this order, before any implementation: + +1. `providers/internal/regconfig/regconfig_test.go` additions: `IntField` missing key, wrong type + (`string`), `int` value, `int64` value, `float64` value (JSON-decode shape) — 5+ cases. +2. `transport/wrapper_test.go` additions: a `Send` call with `Content-Encoding`/`TTL`/`Urgency`/ + `Topic` headers set asserts all four pass through to the captured request (case-insensitively — + assert via `req.Header.Get`, which is itself case-insensitive, sidestepping the `Ttl` vs `TTL` + canonicalization detail at the assertion layer); a header not in the allowlist (e.g. `X-Foo`) is + still stripped, proving the change is additive, not a wholesale relaxation. +3. `providers/webpush/encrypt_test.go`: the RFC 8291 Appendix A fixed-vector test + (`encryptAES128GCMWithKeys` called with the RFC's exact receiver keys/auth secret/ephemeral + sender keypair/salt/plaintext, asserting the exact output ciphertext bytes match the RFC's + published example byte-for-byte) — written and failing (function doesn't exist yet) first, since + this is the single highest-value/highest-risk test in the whole feature (§2.6). Additional cases: + malformed `p256dh` (wrong length), malformed `auth` (wrong length), a plaintext one byte over + `MaxPlaintextSize` returning the named size error (§3.4), a plaintext exactly at + `MaxPlaintextSize` succeeding (boundary case), and a supplementary (not sufficient-alone) + self-encrypt/self-decrypt round-trip sanity check. +4. `providers/webpush/vapid_test.go`: `GenerateVAPIDKeyPair` produces a valid, matching pair + (round-trip: derive public from generated private, compare); `buildVAPIDHeader` (called directly + with plain VAPID strings — no `Config` involved, per §3.5's signature) produces a + `vapid t=, k=` value whose JWT decodes to the expected `alg`/`typ`/`aud`/`sub`/`exp` + and whose signature verifies against the configured public key (`ecdsa.Verify` on the decoded + raw `r||s`, reconstructed as `big.Int`s). +5. `providers/webpush/webpush_test.go`: table-driven `Send` tests against a `capturingRoundTripper` + (same harness pattern as `providers/ntfy/ntfy_test.go`, confirmed by direct read) — one test per + row of §3.8's table, plus: `Content-Type`/`Content-Encoding`/`TTL`/`Authorization` headers present + and correctly shaped on a successful send; request body is not the plaintext JSON (opaque + ciphertext — a regression here would be a real plaintext-leak bug, worth its own explicit + assertion); `TTL` header reflects `DefaultTTL` when `Config.TTL` is zero and the configured value + otherwise; `Urgency`/`Topic` headers present only when configured. +6. `providers/webpush/register_test.go`: mirrors `providers/ntfy/register_test.go`'s three-test + pattern exactly (success round-trip incl. `ttl` int passing through `regconfig.IntField`, missing + `"transport"` returns error not panic, registered under `"webpush"` in `RegisteredTypes()`). +7. `providers/all/all_test.go`: bump `wantProviderCount` by 1 (written failing, since the count + won't match until commit 6 of §6 lands). + +### Phase 2 — Implementation (Green) + +Implement, in dependency order, until each Phase 1 test file's tests pass: +`regconfig.IntField` → `transport.sanitizeOutboundHeaders` → `encrypt.go` → `vapid.go` → +`webpush.go` → `register.go` → `providers/all/all.go`. + +### Phase 3 — Lint/coverage hardening + +- `go vet ./...` and `staticcheck ./...` clean across every touched package. +- `go test ./... -cover` — confirm ≥85% for `providers/internal/regconfig`, `transport`, + `providers/webpush`, `providers/all`. The crypto error-path cases in Phase 1 step 3 + (malformed `p256dh`/`auth`) and step 5 (§3.8's full error table) exist specifically to hit + `encrypt.go`/`vapid.go`/`webpush.go`'s error branches, not just their happy paths — coverage + should not need artificial padding tests if Phase 1 was followed as written. +- `scripts/test-coverage.sh` run to confirm the repo-wide gate, not just per-package `-cover` + output, matches. + +### Phase 4 — Doc comments and README/INTEGRATION.md updates + +- Confirm every new exported identifier (`webpush.Config` and its fields, `webpush.Client`, + `webpush.New`, `webpush.Send`, `webpush.DefaultTTL`, `webpush.MaxPlaintextSize`, + `webpush.GenerateVAPIDKeyPair`, + `regconfig.IntField`) has a doc comment — all drafted verbatim in §3 above; Phase 4 is + transcription plus the cross-file documentation updates in §4, not new design. + +--- + +## 6. Commit Slicing Strategy + +One PR, seven ordered, independently buildable/testable commits (each passes `go build ./...`, +`go vet ./...`, `staticcheck ./...`, and `go test ./...` with its touched package(s) at ≥85% +coverage on its own — per CLAUDE.md's Definition of Done, applied per-commit, per this module's +existing bisectability convention): + +1. **`feat(regconfig): add IntField helper`** + Files: `providers/internal/regconfig/regconfig.go`, `regconfig_test.go`. + Dependencies: none. + Gate: `go test ./providers/internal/regconfig/...` passes; no other package touched, so the + rest of the module is unaffected by construction. + +2. **`feat(transport): allow Content-Encoding/TTL/Urgency/Topic outbound headers`** + Files: `transport/wrapper.go` (`sanitizeOutboundHeaders` only), `transport/wrapper_test.go`. + Dependencies: none (independent of commit 1). + Gate: full `go test ./...` — every existing provider's tests must still pass unchanged, + demonstrating the allowlist addition is non-breaking. + +3. **`feat(webpush): add RFC 8291 aes128gcm payload encryption`** + Files: `providers/webpush/encrypt.go`, `encrypt_test.go` (new package). + Dependencies: none (pure crypto, no registry/transport wiring; tested directly via the + unexported `encryptAES128GCMWithKeys` seam). + Gate: `go test ./providers/webpush/...`; **the RFC 8291 Appendix A fixed-vector test passing is + a blocking condition for this commit**, not a nice-to-have — do not proceed to commit 4 with it + failing, skipped, or weakened to a round-trip-only check. + +4. **`feat(webpush): add VAPID JWT signing (RFC 8292) and GenerateVAPIDKeyPair`** + Files: `providers/webpush/vapid.go`, `vapid_test.go`. + Dependencies: commit 3 only in the sense of sharing a package (no code dependency between + `encrypt.go` and `vapid.go` themselves). Critically, `buildVAPIDHeader` takes its three VAPID + values as plain `string` parameters, not a `Config` (§3.5) — `Config` isn't defined until commit + 5, so this commit must not (and per §3.5's signature, does not) reference it. This is what makes + commit 4 buildable and testable in isolation, satisfying this list's own per-commit build/test + guarantee. + Gate: `go test ./providers/webpush/...`. + +5. **`feat(webpush): add Config/Client/New/Send`** + Files: `providers/webpush/webpush.go`, `webpush_test.go`. + Dependencies: commits 2 (header allowlist), 3 (`encryptAES128GCM`), 4 (`buildVAPIDHeader`, called + here with `cfg`'s fields unpacked into positional strings — this is the first commit where a + `Config` value exists to unpack) — first commit where the full `Send` path is exercised + end-to-end against a `capturingRoundTripper`. + Gate: `go test ./providers/webpush/...`; this is the commit where §3.8's full error-handling + table (including the VAPID-subject-prefix and `MaxPlaintextSize` rows) gets its test coverage. + +6. **`feat(webpush): register provider and wire into providers/all`** + Files: `providers/webpush/register.go`, `register_test.go`, `providers/all/all.go`, + `providers/all/all_test.go` (`wantProviderCount` bump). + Dependencies: commit 5. + Gate: full `go test ./...`; `notify.New("webpush", ...)` and `notify.RegisteredTypes()` both + exercise the new provider end-to-end for the first time. + +7. **`docs: document providers/webpush`** + Files: `README.md`, `docs/INTEGRATION.md`, `ARCHITECTURE.md` (§4 above). + Dependencies: commit 6 (documents the shipped API, not a design still in flux). + Gate: no code changes — build/vet/test gates are a no-op pass-through, included for + completeness/bisectability symmetry with every other commit in this list. + +No commit adds a provider other than `webpush`, per §1.3. + +--- + +## 7. Acceptance Criteria + +Mapped to this repo's Definition of Done (CLAUDE.md) plus feature-specific criteria: + +1. `go build ./...` succeeds at every commit in §6, not just the final one. +2. `go vet ./...` and `staticcheck ./...` clean at every commit. +3. `go test ./...` passes at every commit; coverage ≥85% for every package touched + (`providers/internal/regconfig`, `transport`, `providers/webpush`, `providers/all`). +4. Every new/changed exported identifier has a doc comment (§3's drafted comments, transcribed + verbatim — no placeholder comments). +5. `grep -r "Wikid82/charon" --include=*.go .` (or equivalent) returns nothing under + `providers/webpush/` or any file touched by this feature. +6. The RFC 8291 Appendix A fixed-vector test in `encrypt_test.go` passes with exact-byte + equality — not a round-trip-only check. +7. `providers/all/all_test.go`'s `TestAll_RegistersEveryBuiltInProvider` passes with the bumped + `wantProviderCount`. +8. `README.md`, `docs/INTEGRATION.md`, and `ARCHITECTURE.md` are updated per §4. +9. No provider package other than `providers/webpush` is added, scaffolded, or stubbed anywhere in + this work (scope discipline, §1.3). +10. `notify.New("webpush", map[string]any{...})` and the typed `webpush.New(webpush.Config{...}, + wrapper)` constructor are both exercised by tests and behaviorally equivalent (mirrors every + other provider's `register_test.go` round-trip pattern). From 1205a98e2e85e089edb08063e543bb3793806379 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:23:19 +0000 Subject: [PATCH 03/12] feat(regconfig): add IntField helper Webpush's Config.TTL needs an int-typed config field; regconfig only had StringField/StringSliceField before this. Accepts int/int64/float64 to cover both native Go construction and a generic JSON-style decode. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- providers/internal/regconfig/regconfig.go | 25 +++++++++++++++++++ .../internal/regconfig/regconfig_test.go | 24 ++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/providers/internal/regconfig/regconfig.go b/providers/internal/regconfig/regconfig.go index 0854f63..9ea2613 100644 --- a/providers/internal/regconfig/regconfig.go +++ b/providers/internal/regconfig/regconfig.go @@ -29,6 +29,31 @@ func StringField(config map[string]any, key string) string { return s } +// IntField returns config[key] as an int, or 0 if the key is absent or not +// an int-like value. Accepts int and int64 (the natural Go-side shapes) +// and float64 (the shape a generic JSON-style decode into map[string]any +// produces, since encoding/json decodes every JSON number as float64) — +// mirroring StringSliceField's existing dual-shape acceptance for []any. +func IntField(config map[string]any, key string) int { + if config == nil { + return 0 + } + v, ok := config[key] + if !ok { + return 0 + } + switch n := v.(type) { + case int: + return n + case int64: + return int(n) + case float64: + return int(n) + default: + return 0 + } +} + // StringSliceField returns config[key] as a []string, or nil if the key is // absent or not a recognized slice-of-string shape. Both []string (the // natural Go-side shape) and []any of strings (the natural shape after a diff --git a/providers/internal/regconfig/regconfig_test.go b/providers/internal/regconfig/regconfig_test.go index 68884e5..21fbc59 100644 --- a/providers/internal/regconfig/regconfig_test.go +++ b/providers/internal/regconfig/regconfig_test.go @@ -29,6 +29,30 @@ func TestStringField(t *testing.T) { } } +func TestIntField(t *testing.T) { + tests := []struct { + name string + config map[string]any + key string + want int + }{ + {"nil config", nil, "ttl", 0}, + {"missing key", map[string]any{}, "ttl", 0}, + {"wrong type string", map[string]any{"ttl": "60"}, "ttl", 0}, + {"present int", map[string]any{"ttl": 60}, "ttl", 60}, + {"present int64", map[string]any{"ttl": int64(120)}, "ttl", 120}, + {"present float64 (JSON-decode shape)", map[string]any{"ttl": float64(180)}, "ttl", 180}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IntField(tt.config, tt.key) + if got != tt.want { + t.Errorf("IntField(%v, %q) = %d, want %d", tt.config, tt.key, got, tt.want) + } + }) + } +} + func TestStringSliceField(t *testing.T) { tests := []struct { name string From 99549d17b7e03e28d2e7fbe25c84d5ff75f30db0 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:24:03 +0000 Subject: [PATCH 04/12] feat(transport): allow Content-Encoding/TTL/Urgency/Topic outbound headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Web Push requires these four RFC 8030/8188 headers on every request, none of which were in the outbound header allowlist. Additive only — every existing provider's tests still pass unchanged. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- transport/wrapper.go | 14 +++++--- transport/wrapper_test.go | 67 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 5 deletions(-) diff --git a/transport/wrapper.go b/transport/wrapper.go index d447d79..e614a1f 100644 --- a/transport/wrapper.go +++ b/transport/wrapper.go @@ -335,11 +335,15 @@ func hasDisallowedQueryAuthKey(query neturl.Values) bool { func sanitizeOutboundHeaders(headers map[string]string) map[string]string { allowed := map[string]struct{}{ - "content-type": {}, - "user-agent": {}, - "x-request-id": {}, - "x-gotify-key": {}, - "authorization": {}, + "content-type": {}, + "user-agent": {}, + "x-request-id": {}, + "x-gotify-key": {}, + "authorization": {}, + "content-encoding": {}, + "ttl": {}, + "urgency": {}, + "topic": {}, } sanitized := make(map[string]string) diff --git a/transport/wrapper_test.go b/transport/wrapper_test.go index 8d60a66..5439533 100644 --- a/transport/wrapper_test.go +++ b/transport/wrapper_test.go @@ -360,6 +360,73 @@ func TestSanitizeOutboundHeadersAllowlist(t *testing.T) { } } +func TestSanitizeOutboundHeadersAllowsWebPushHeaders(t *testing.T) { + headers := sanitizeOutboundHeaders(map[string]string{ + "Content-Encoding": "aes128gcm", + "TTL": "2419200", + "Urgency": "high", + "Topic": "my-topic", + "X-Foo": "should-be-stripped", + }) + + if len(headers) != 4 { + t.Fatalf("expected 4 allowed headers, got %d: %#v", len(headers), headers) + } + for _, key := range []string{"Content-Encoding", "Ttl", "Urgency", "Topic"} { + if _, ok := headers[key]; !ok { + t.Fatalf("expected %q to be allowed, got %#v", key, headers) + } + } + if _, ok := headers["X-Foo"]; ok { + t.Fatalf("expected non-allowlisted header to be stripped, got %#v", headers) + } +} + +func TestWrapperSendPassesThroughWebPushHeaders(t *testing.T) { + var captured http.Header + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + captured = r.Header.Clone() + w.WriteHeader(http.StatusCreated) + })) + defer server.Close() + + wrapper := newTestWrapper( + WithRetryPolicy(RetryPolicy{MaxAttempts: 1}), + WithClientFactory(func(bool, int) *http.Client { return server.Client() }), + ) + + _, err := wrapper.Send(context.Background(), Request{ + URL: server.URL, + Headers: map[string]string{ + "Content-Encoding": "aes128gcm", + "TTL": "2419200", + "Urgency": "high", + "Topic": "my-topic", + "X-Foo": "should-be-stripped", + }, + Body: []byte("ciphertext"), + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if got := captured.Get("Content-Encoding"); got != "aes128gcm" { + t.Fatalf("expected Content-Encoding to pass through, got %q", got) + } + if got := captured.Get("TTL"); got != "2419200" { + t.Fatalf("expected TTL to pass through, got %q", got) + } + if got := captured.Get("Urgency"); got != "high" { + t.Fatalf("expected Urgency to pass through, got %q", got) + } + if got := captured.Get("Topic"); got != "my-topic" { + t.Fatalf("expected Topic to pass through, got %q", got) + } + if got := captured.Get("X-Foo"); got != "" { + t.Fatalf("expected non-allowlisted header to be stripped, got %q", got) + } +} + func TestWrapperApplyRedirectGuardNilClient(t *testing.T) { wrapper := newTestWrapper() wrapper.applyRedirectGuard(nil) From a2159c2167015c68cf74358edb065e55903799b8 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:26:43 +0000 Subject: [PATCH 05/12] feat(webpush): add RFC 8291 aes128gcm payload encryption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the Web Push message encryption content-coding: ECDH between a fresh ephemeral sender keypair and the subscriber's p256dh key, two-step HKDF-SHA256 derivation of the content-encryption key/nonce, and RFC 8188 single-record aes128gcm framing. encryptAES128GCMWithKeys takes the ephemeral keypair/salt as parameters (rather than generating them) so tests can force RFC 8291 Appendix A's exact published example and assert byte-for-byte equality against its ciphertext — the highest-value correctness gate in this feature, since a round-trip-only test can't catch a bug that's symmetric on both the encrypt and decrypt sides. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- providers/webpush/encrypt.go | 157 +++++++++++++++++ providers/webpush/encrypt_test.go | 273 ++++++++++++++++++++++++++++++ 2 files changed, 430 insertions(+) create mode 100644 providers/webpush/encrypt.go create mode 100644 providers/webpush/encrypt_test.go diff --git a/providers/webpush/encrypt.go b/providers/webpush/encrypt.go new file mode 100644 index 0000000..0d3d8c7 --- /dev/null +++ b/providers/webpush/encrypt.go @@ -0,0 +1,157 @@ +package webpush + +import ( + "crypto/aes" + "crypto/cipher" + "crypto/ecdh" + "crypto/hkdf" + "crypto/rand" + "crypto/sha256" + "encoding/base64" + "encoding/binary" + "fmt" +) + +// MaxPlaintextSize is the largest plaintext payload encryptAES128GCM will +// accept, in bytes. RFC 8188 §2 single-record framing adds a fixed 86-byte +// record header (salt(16) + rs(4) + idlen(1) + keyid(65)) plus a 1-byte +// delimiter and a 16-byte AES-GCM tag around the plaintext (103 bytes of +// fixed overhead total), and real push services independently cap the +// resulting request body at roughly 4096 bytes (FCM and Mozilla autopush +// both document limits in this neighborhood). MaxPlaintextSize is set well +// inside that ceiling (86 + 3800 + 17 = 3903 bytes total, vs. a ~4096-byte +// external cap) rather than exactly at the boundary, so a plaintext this +// module accepts is not immediately at risk of a push-service-side +// rejection this module can't see coming. +const MaxPlaintextSize = 3800 + +// recordSize is the RFC 8188 §2 "rs" field value this module always emits. +// Every payload webpush sends is a single, final record, so any value at +// least as large as the total framed record works; 4096 matches the value +// RFC 8291's own Appendix A example uses. +const recordSize = 4096 + +// aes128gcmHeaderSize is the fixed RFC 8188 §2 single-record header size +// for a 65-byte (uncompressed P-256) keyid: salt(16) + rs(4) + idlen(1) + +// keyid(65). +const aes128gcmHeaderSize = 16 + 4 + 1 + 65 + +// paddingDelimiter is the RFC 8188 §2 delimiter octet appended to the +// plaintext of a single, final record. +const paddingDelimiter = 0x02 + +// encryptAES128GCM implements RFC 8291 Web Push message encryption. Given +// the subscriber's base64url (no padding) encoded p256dh public key and +// auth secret (from PushSubscription.getKey), and the plaintext +// application payload, it returns the aes128gcm content-coded ciphertext +// (RFC 8188 §2 single-record framing: salt(16) || rs(4) || idlen(1) || +// keyid(65, the ephemeral sender public key, uncompressed) || +// AEAD-ciphertext) ready to send as the request body. Generates a fresh +// ephemeral P-256 keypair and a fresh random 16-byte salt per call — see +// encryptAES128GCMWithKeys for the deterministic variant tests use. +func encryptAES128GCM(p256dhB64, authB64 string, plaintext []byte) ([]byte, error) { + ephemeral, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + return nil, fmt.Errorf("webpush: generate ephemeral keypair: %w", err) + } + + salt := make([]byte, 16) + if _, err := rand.Read(salt); err != nil { + return nil, fmt.Errorf("webpush: generate salt: %w", err) + } + + return encryptAES128GCMWithKeys(ephemeral, salt, p256dhB64, authB64, plaintext) +} + +// encryptAES128GCMWithKeys is encryptAES128GCM with the ephemeral sender +// keypair and salt injected rather than randomly generated — the +// production encryptAES128GCM is a thin wrapper generating both randomly +// and delegating here. Exists so tests (in particular the RFC 8291 +// Appendix A fixed-vector test, encrypt_test.go) can force the exact +// keys/salt the RFC's published example uses and assert exact-byte +// output — a capability a purely-random production path can't otherwise +// be tested against without an external reference implementation, which +// this dependency-free module cannot depend on. +func encryptAES128GCMWithKeys(ephemeral *ecdh.PrivateKey, salt []byte, p256dhB64, authB64 string, plaintext []byte) ([]byte, error) { + if len(plaintext) > MaxPlaintextSize { + return nil, fmt.Errorf("webpush: payload of %d bytes exceeds maximum plaintext size of %d bytes", len(plaintext), MaxPlaintextSize) + } + + uaPublicRaw, err := base64.RawURLEncoding.DecodeString(p256dhB64) + if err != nil { + return nil, fmt.Errorf("webpush: p256dh: invalid base64url encoding: %w", err) + } + if len(uaPublicRaw) != 65 { + return nil, fmt.Errorf("webpush: p256dh: expected 65-byte uncompressed P-256 point, got %d bytes", len(uaPublicRaw)) + } + + authSecret, err := base64.RawURLEncoding.DecodeString(authB64) + if err != nil { + return nil, fmt.Errorf("webpush: auth: invalid base64url encoding: %w", err) + } + if len(authSecret) != 16 { + return nil, fmt.Errorf("webpush: auth: expected 16-byte secret, got %d bytes", len(authSecret)) + } + + subscriberPub, err := ecdh.P256().NewPublicKey(uaPublicRaw) + if err != nil { + return nil, fmt.Errorf("webpush: p256dh: invalid P-256 point: %w", err) + } + + ecdhSecret, err := ephemeral.ECDH(subscriberPub) + if err != nil { + return nil, fmt.Errorf("webpush: ECDH key agreement failed: %w", err) + } + + asPublicRaw := ephemeral.PublicKey().Bytes() + + // RFC 8291 §3.4 step 1: derive the key-combining IKM from the ECDH + // shared secret and the subscriber's auth secret. + keyInfo := "WebPush: info\x00" + string(uaPublicRaw) + string(asPublicRaw) + prkKey, err := hkdf.Extract(sha256.New, ecdhSecret, authSecret) + if err != nil { + return nil, fmt.Errorf("webpush: HKDF-Extract (key combining): %w", err) + } + ikm, err := hkdf.Expand(sha256.New, prkKey, keyInfo, 32) + if err != nil { + return nil, fmt.Errorf("webpush: HKDF-Expand (IKM): %w", err) + } + + // RFC 8291 §3.4 step 2: derive the content-encryption key and nonce + // from the IKM and the (random or injected) 16-byte salt. + prk, err := hkdf.Extract(sha256.New, ikm, salt) + if err != nil { + return nil, fmt.Errorf("webpush: HKDF-Extract (content encryption): %w", err) + } + cek, err := hkdf.Expand(sha256.New, prk, "Content-Encoding: aes128gcm\x00", 16) + if err != nil { + return nil, fmt.Errorf("webpush: HKDF-Expand (CEK): %w", err) + } + nonce, err := hkdf.Expand(sha256.New, prk, "Content-Encoding: nonce\x00", 12) + if err != nil { + return nil, fmt.Errorf("webpush: HKDF-Expand (nonce): %w", err) + } + + // RFC 8188 §2: a single, final record gets a 0x02 padding delimiter. + padded := make([]byte, 0, len(plaintext)+1) + padded = append(padded, plaintext...) + padded = append(padded, paddingDelimiter) + + block, err := aes.NewCipher(cek) + if err != nil { + return nil, fmt.Errorf("webpush: construct AES cipher: %w", err) + } + gcm, err := cipher.NewGCM(block) + if err != nil { + return nil, fmt.Errorf("webpush: construct AES-GCM AEAD: %w", err) + } + ciphertext := gcm.Seal(nil, nonce, padded, nil) + + header := make([]byte, aes128gcmHeaderSize) + copy(header[0:16], salt) + binary.BigEndian.PutUint32(header[16:20], recordSize) + header[20] = byte(len(asPublicRaw)) + copy(header[21:aes128gcmHeaderSize], asPublicRaw) + + return append(header, ciphertext...), nil +} diff --git a/providers/webpush/encrypt_test.go b/providers/webpush/encrypt_test.go new file mode 100644 index 0000000..3a842e9 --- /dev/null +++ b/providers/webpush/encrypt_test.go @@ -0,0 +1,273 @@ +package webpush + +import ( + "bytes" + "crypto/aes" + "crypto/cipher" + "crypto/ecdh" + "crypto/hkdf" + "crypto/rand" + "crypto/sha256" + "encoding/base64" + "strings" + "testing" +) + +// RFC 8291 Appendix A ("Intermediate Values for Encryption") fixed test +// vectors, transcribed verbatim (whitespace/line-wrapping removed per the +// RFC's own note that presentation whitespace can be discarded). +const ( + rfc8291UAPublic = "BCVxsr7N_eNgVRqvHtD0zTZsEc6-VV-JvLexhqUzORcxaOzi6-AYWXvTBHm4bjyPjs7Vd8pZGH6SRpkNtoIAiw4" + rfc8291UAPrivate = "q1dXpw3UpT5VOmu_cf_v6ih07Aems3njxI-JWgLcM94" + rfc8291ASPublic = "BP4z9KsN6nGRTbVYI_c7VJSPQTBtkgcy27mlmlMoZIIgDll6e3vCYLocInmYWAmS6TlzAC8wEqKK6PBru3jl7A8" + rfc8291ASPrivate = "yfWPiYE-n46HLnH0KqZOF1fJJU3MYrct3AELtAQ-oRw" + rfc8291Salt = "DGv6ra1nlYgDCS1FRnbzlw" + rfc8291Auth = "BTBZMqHH6r4Tts7J_aSIgg" + rfc8291Plaintext = "When I grow up, I want to be a watermelon" + + // rfc8291ExpectedBody is the exact wire body from RFC 8291 §5 (header + // || AEAD ciphertext), with the example's line-wrapping removed. + rfc8291ExpectedBody = "DGv6ra1nlYgDCS1FRnbzlwAAEABBBP4z9KsN6nGRTbVYI_c7VJSPQTBtkgcy27ml" + + "mlMoZIIgDll6e3vCYLocInmYWAmS6TlzAC8wEqKK6PBru3jl7A_yl95bQpu6cVPT" + + "pK4Mqgkf1CXztLVBSt2Ks3oZwbuwXPXLWyouBWLVWGNWQexSgSxsj_Qulcy4a-fN" +) + +func mustDecodeB64URL(t *testing.T, s string) []byte { + t.Helper() + b, err := base64.RawURLEncoding.DecodeString(s) + if err != nil { + t.Fatalf("failed to decode base64url %q: %v", s, err) + } + return b +} + +// TestEncryptAES128GCMWithKeys_RFC8291AppendixAVector is the single +// highest-value correctness gate in this feature: it feeds the exact +// fixed inputs from RFC 8291 Appendix A into encryptAES128GCMWithKeys and +// asserts the output matches the RFC's published ciphertext byte-for-byte. +// This must not be weakened to a round-trip-only check — a bug symmetric +// in both directions (e.g. a wrong HKDF info string used consistently on +// both sides) would still round-trip correctly while being wrong per the +// RFC, and there is no external Web Push implementation available to +// interop-test against in this dependency-free module. +func TestEncryptAES128GCMWithKeys_RFC8291AppendixAVector(t *testing.T) { + asPrivateRaw := mustDecodeB64URL(t, rfc8291ASPrivate) + ephemeral, err := ecdh.P256().NewPrivateKey(asPrivateRaw) + if err != nil { + t.Fatalf("failed to construct ephemeral private key from RFC fixture: %v", err) + } + + salt := mustDecodeB64URL(t, rfc8291Salt) + + got, err := encryptAES128GCMWithKeys(ephemeral, salt, rfc8291UAPublic, rfc8291Auth, []byte(rfc8291Plaintext)) + if err != nil { + t.Fatalf("encryptAES128GCMWithKeys returned error: %v", err) + } + + want := mustDecodeB64URL(t, rfc8291ExpectedBody) + + if !bytes.Equal(got, want) { + t.Fatalf("RFC 8291 Appendix A vector mismatch:\n got (%d bytes): %s\n want (%d bytes): %s", + len(got), base64.RawURLEncoding.EncodeToString(got), + len(want), base64.RawURLEncoding.EncodeToString(want)) + } +} + +// TestEncryptAES128GCMWithKeys_RFC8291AppendixAVector_HeaderOnly cross-checks +// the 86-byte RFC 8188 header framing in isolation against Appendix A's +// separately published header value, pinpointing a framing bug distinctly +// from an AEAD/key-derivation bug should the full-body test above fail. +func TestEncryptAES128GCMWithKeys_RFC8291AppendixAVector_HeaderOnly(t *testing.T) { + asPrivateRaw := mustDecodeB64URL(t, rfc8291ASPrivate) + ephemeral, err := ecdh.P256().NewPrivateKey(asPrivateRaw) + if err != nil { + t.Fatalf("failed to construct ephemeral private key from RFC fixture: %v", err) + } + salt := mustDecodeB64URL(t, rfc8291Salt) + + got, err := encryptAES128GCMWithKeys(ephemeral, salt, rfc8291UAPublic, rfc8291Auth, []byte(rfc8291Plaintext)) + if err != nil { + t.Fatalf("encryptAES128GCMWithKeys returned error: %v", err) + } + if len(got) < 86 { + t.Fatalf("expected at least an 86-byte header, got %d total bytes", len(got)) + } + + wantHeader := mustDecodeB64URL(t, "DGv6ra1nlYgDCS1FRnbzlwAAEABBBP4z9KsN6nGRTbVYI_c7VJSPQTBtkgcy27mlmlMoZIIgDll6e3vCYLocInmYWAmS6TlzAC8wEqKK6PBru3jl7A8") + if !bytes.Equal(got[:86], wantHeader) { + t.Fatalf("RFC 8291 Appendix A header mismatch:\n got: %s\n want: %s", + base64.RawURLEncoding.EncodeToString(got[:86]), + base64.RawURLEncoding.EncodeToString(wantHeader)) + } +} + +func TestEncryptAES128GCMWithKeys_RejectsMalformedP256dh(t *testing.T) { + ephemeral, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + salt := make([]byte, 16) + + shortP256dh := base64.RawURLEncoding.EncodeToString([]byte("too-short")) + auth := base64.RawURLEncoding.EncodeToString(make([]byte, 16)) + + _, err = encryptAES128GCMWithKeys(ephemeral, salt, shortP256dh, auth, []byte("hello")) + if err == nil || !strings.Contains(err.Error(), "p256dh") { + t.Fatalf("expected p256dh length error, got: %v", err) + } +} + +func TestEncryptAES128GCMWithKeys_RejectsMalformedAuth(t *testing.T) { + ephemeral, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + receiver, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + salt := make([]byte, 16) + + p256dh := base64.RawURLEncoding.EncodeToString(receiver.PublicKey().Bytes()) + shortAuth := base64.RawURLEncoding.EncodeToString([]byte("short")) + + _, err = encryptAES128GCMWithKeys(ephemeral, salt, p256dh, shortAuth, []byte("hello")) + if err == nil || !strings.Contains(err.Error(), "auth") { + t.Fatalf("expected auth length error, got: %v", err) + } +} + +func TestEncryptAES128GCMWithKeys_RejectsOversizedPlaintext(t *testing.T) { + ephemeral, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + receiver, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + salt := make([]byte, 16) + p256dh := base64.RawURLEncoding.EncodeToString(receiver.PublicKey().Bytes()) + auth := base64.RawURLEncoding.EncodeToString(make([]byte, 16)) + + oversized := bytes.Repeat([]byte("a"), MaxPlaintextSize+1) + _, err = encryptAES128GCMWithKeys(ephemeral, salt, p256dh, auth, oversized) + if err == nil || !strings.Contains(err.Error(), "exceeds maximum plaintext size") { + t.Fatalf("expected oversized plaintext error, got: %v", err) + } +} + +func TestEncryptAES128GCMWithKeys_AcceptsPlaintextAtBoundary(t *testing.T) { + ephemeral, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + receiver, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + salt := make([]byte, 16) + p256dh := base64.RawURLEncoding.EncodeToString(receiver.PublicKey().Bytes()) + auth := base64.RawURLEncoding.EncodeToString(make([]byte, 16)) + + atBoundary := bytes.Repeat([]byte("a"), MaxPlaintextSize) + if _, err := encryptAES128GCMWithKeys(ephemeral, salt, p256dh, auth, atBoundary); err != nil { + t.Fatalf("expected boundary-sized plaintext to be accepted, got error: %v", err) + } +} + +// decryptAES128GCMForTest is a test-only mirror of the RFC 8291 decryption +// steps, used solely for the supplementary round-trip sanity check below. +// It is deliberately not part of encrypt.go's production surface — this +// module has no receiver/decrypt role to play (see webpush.go's Send, +// which only ever encrypts). +func decryptAES128GCMForTest(t *testing.T, receiverPriv *ecdh.PrivateKey, authSecret []byte, body []byte) []byte { + t.Helper() + if len(body) < 86 { + t.Fatalf("body too short to contain an aes128gcm header: %d bytes", len(body)) + } + salt := body[0:16] + idlen := int(body[20]) + keyID := body[21 : 21+idlen] + ciphertext := body[21+idlen:] + + senderPub, err := ecdh.P256().NewPublicKey(keyID) + if err != nil { + t.Fatalf("failed to parse sender public key from header: %v", err) + } + ecdhSecret, err := receiverPriv.ECDH(senderPub) + if err != nil { + t.Fatalf("ECDH failed: %v", err) + } + + uaPublic := receiverPriv.PublicKey().Bytes() + keyInfo := "WebPush: info\x00" + string(uaPublic) + string(keyID) + + prkKey, err := hkdf.Extract(sha256.New, ecdhSecret, authSecret) + if err != nil { + t.Fatalf("hkdf.Extract (key combining) failed: %v", err) + } + ikm, err := hkdf.Expand(sha256.New, prkKey, keyInfo, 32) + if err != nil { + t.Fatalf("hkdf.Expand (IKM) failed: %v", err) + } + + prk, err := hkdf.Extract(sha256.New, ikm, salt) + if err != nil { + t.Fatalf("hkdf.Extract (content) failed: %v", err) + } + cek, err := hkdf.Expand(sha256.New, prk, "Content-Encoding: aes128gcm\x00", 16) + if err != nil { + t.Fatalf("hkdf.Expand (CEK) failed: %v", err) + } + nonce, err := hkdf.Expand(sha256.New, prk, "Content-Encoding: nonce\x00", 12) + if err != nil { + t.Fatalf("hkdf.Expand (nonce) failed: %v", err) + } + + block, err := aes.NewCipher(cek) + if err != nil { + t.Fatalf("aes.NewCipher failed: %v", err) + } + gcm, err := cipher.NewGCM(block) + if err != nil { + t.Fatalf("cipher.NewGCM failed: %v", err) + } + padded, err := gcm.Open(nil, nonce, ciphertext, nil) + if err != nil { + t.Fatalf("AES-GCM decryption failed: %v", err) + } + if len(padded) == 0 || padded[len(padded)-1] != 0x02 { + t.Fatalf("expected trailing 0x02 padding delimiter, got %x", padded) + } + return padded[:len(padded)-1] +} + +// TestEncryptAES128GCM_RoundTrip is a supplementary sanity check only — not +// sufficient on its own (see the RFC 8291 fixed-vector test's doc comment +// above for why a bug symmetric in both directions could still round-trip). +func TestEncryptAES128GCM_RoundTrip(t *testing.T) { + receiver, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + authSecret := make([]byte, 16) + if _, err := rand.Read(authSecret); err != nil { + t.Fatalf("unexpected rand error: %v", err) + } + + p256dh := base64.RawURLEncoding.EncodeToString(receiver.PublicKey().Bytes()) + auth := base64.RawURLEncoding.EncodeToString(authSecret) + + plaintext := []byte(`{"message":"hello world"}`) + + body, err := encryptAES128GCM(p256dh, auth, plaintext) + if err != nil { + t.Fatalf("encryptAES128GCM returned error: %v", err) + } + + got := decryptAES128GCMForTest(t, receiver, authSecret, body) + if !bytes.Equal(got, plaintext) { + t.Fatalf("round-trip mismatch: got %q, want %q", got, plaintext) + } +} From bf6bd6574f68fb52629039490a40a77f5db9e3c2 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:28:30 +0000 Subject: [PATCH 06/12] feat(webpush): add VAPID JWT signing (RFC 8292) and GenerateVAPIDKeyPair buildVAPIDHeader signs an ES256 JWT (raw r||s signature, not ASN.1 DER) and formats it as the RFC 8292 "vapid t=, k=" Authorization header value; it takes the three VAPID strings directly rather than a Config, since Config isn't defined until a later commit. GenerateVAPIDKeyPair is a small convenience for hosts that don't already have an externally generated keypair, using the non-deprecated ecdsa.ParseRawPrivateKey/ ParseUncompressedPublicKey/Bytes APIs to avoid crypto/elliptic's deprecated low-level Marshal/ScalarBaseMult surface. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- providers/webpush/vapid.go | 122 ++++++++++++++++++++++ providers/webpush/vapid_test.go | 178 ++++++++++++++++++++++++++++++++ 2 files changed, 300 insertions(+) create mode 100644 providers/webpush/vapid.go create mode 100644 providers/webpush/vapid_test.go diff --git a/providers/webpush/vapid.go b/providers/webpush/vapid.go new file mode 100644 index 0000000..d2ffd03 --- /dev/null +++ b/providers/webpush/vapid.go @@ -0,0 +1,122 @@ +package webpush + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "fmt" + neturl "net/url" + "time" +) + +// vapidJWTLifetime bounds the "exp" claim on the VAPID JWT Send signs for +// each request: 12 hours from the time of signing. RFC 8292 recommends an +// expiration no more than 24 hours out; 12 hours is comfortably inside +// that bound while still meaning a Client's signed header is reusable +// across a short burst of retries/sends without re-signing every time +// (though Send always signs fresh per call — see below). +const vapidJWTLifetime = 12 * time.Hour + +// vapidJWTHeader is the fixed RFC 8292 JOSE header — every VAPID JWT this +// module signs uses ES256, so this is not templated per-request. +var vapidJWTHeaderJSON = mustMarshalJSON(map[string]string{"typ": "JWT", "alg": "ES256"}) + +func mustMarshalJSON(v any) []byte { + b, err := json.Marshal(v) + if err != nil { + panic(fmt.Sprintf("webpush: failed to marshal fixed JWT header: %v", err)) + } + return b +} + +// buildVAPIDHeader builds the RFC 8292 "Authorization: vapid t=, +// k=" header value for a request to endpoint, signed with the +// given VAPID keypair/subject. aud is derived from endpoint's scheme+host +// (RFC 8292 §2: the JWT audience is the push service's origin, not the +// full subscription path). +// +// Takes the three VAPID strings directly rather than the whole Config by +// design, not just convenience: buildVAPIDHeader only ever reads 3 of +// Config's 11 fields, and Config itself isn't defined until webpush.go — +// a Config parameter here would make this file depend on a type that +// doesn't exist yet at this point in the commit sequence, breaking this +// module's per-commit build/test guarantee. Taking plain strings keeps +// this function buildable and independently testable two commits before +// Config exists. +func buildVAPIDHeader(vapidPublicKey, vapidPrivateKey, vapidSubject, endpoint string) (string, error) { + privRaw, err := base64.RawURLEncoding.DecodeString(vapidPrivateKey) + if err != nil { + return "", fmt.Errorf("webpush: VAPID private key: invalid base64url encoding: %w", err) + } + priv, err := ecdsa.ParseRawPrivateKey(elliptic.P256(), privRaw) + if err != nil { + return "", fmt.Errorf("webpush: VAPID private key: %w", err) + } + + parsedEndpoint, err := neturl.Parse(endpoint) + if err != nil { + return "", fmt.Errorf("webpush: parse endpoint for VAPID audience: %w", err) + } + aud := parsedEndpoint.Scheme + "://" + parsedEndpoint.Host + + payloadJSON, err := json.Marshal(struct { + Aud string `json:"aud"` + Exp int64 `json:"exp"` + Sub string `json:"sub"` + }{ + Aud: aud, + Exp: time.Now().Add(vapidJWTLifetime).Unix(), + Sub: vapidSubject, + }) + if err != nil { + return "", fmt.Errorf("webpush: marshal VAPID JWT payload: %w", err) + } + + headerB64 := base64.RawURLEncoding.EncodeToString(vapidJWTHeaderJSON) + payloadB64 := base64.RawURLEncoding.EncodeToString(payloadJSON) + + digest := sha256.Sum256([]byte(headerB64 + "." + payloadB64)) + r, s, err := ecdsa.Sign(rand.Reader, priv, digest[:]) + if err != nil { + return "", fmt.Errorf("webpush: sign VAPID JWT: %w", err) + } + + sig := make([]byte, 64) + r.FillBytes(sig[0:32]) + s.FillBytes(sig[32:64]) + sigB64 := base64.RawURLEncoding.EncodeToString(sig) + + return fmt.Sprintf("vapid t=%s.%s.%s, k=%s", headerB64, payloadB64, sigB64, vapidPublicKey), nil +} + +// GenerateVAPIDKeyPair generates a new P-256 VAPID application server +// keypair, returned as the same base64url (no padding) encoded strings +// Config.VAPIDPublicKey/Config.VAPIDPrivateKey expect. Intended to be +// called once at application setup time (e.g. from an init/CLI flow) and +// the results persisted by the host application — every browser +// PushSubscription is bound to the exact public key it was created with +// (PushManager.subscribe({applicationServerKey: ...})), so rotating this +// keypair invalidates every existing subscription the host has collected. +// privateKey is a credential, not a diagnostic value: callers must not log +// it (a mistake this function can't prevent, only warn against — the same +// discipline hosts already need for VAPIDPrivateKey once it's in Config). +func GenerateVAPIDKeyPair() (publicKey, privateKey string, err error) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + return "", "", fmt.Errorf("webpush: generate VAPID keypair: %w", err) + } + + pubBytes, err := priv.PublicKey.Bytes() + if err != nil { + return "", "", fmt.Errorf("webpush: encode VAPID public key: %w", err) + } + privBytes, err := priv.Bytes() + if err != nil { + return "", "", fmt.Errorf("webpush: encode VAPID private key: %w", err) + } + + return base64.RawURLEncoding.EncodeToString(pubBytes), base64.RawURLEncoding.EncodeToString(privBytes), nil +} diff --git a/providers/webpush/vapid_test.go b/providers/webpush/vapid_test.go new file mode 100644 index 0000000..61cb998 --- /dev/null +++ b/providers/webpush/vapid_test.go @@ -0,0 +1,178 @@ +package webpush + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "math/big" + "strings" + "testing" + "time" +) + +func TestGenerateVAPIDKeyPair_ProducesMatchingPair(t *testing.T) { + pubB64, privB64, err := GenerateVAPIDKeyPair() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + privRaw, err := base64.RawURLEncoding.DecodeString(privB64) + if err != nil { + t.Fatalf("private key is not valid base64url: %v", err) + } + if len(privRaw) != 32 { + t.Fatalf("expected 32-byte private key, got %d bytes", len(privRaw)) + } + + pubRaw, err := base64.RawURLEncoding.DecodeString(pubB64) + if err != nil { + t.Fatalf("public key is not valid base64url: %v", err) + } + if len(pubRaw) != 65 || pubRaw[0] != 0x04 { + t.Fatalf("expected a 65-byte uncompressed P-256 point, got %d bytes (first byte %#x)", len(pubRaw), pubRaw[0]) + } + + priv, err := ecdsa.ParseRawPrivateKey(elliptic.P256(), privRaw) + if err != nil { + t.Fatalf("failed to parse generated private key: %v", err) + } + derivedPub, err := priv.PublicKey.Bytes() + if err != nil { + t.Fatalf("failed to encode derived public key: %v", err) + } + if base64.RawURLEncoding.EncodeToString(derivedPub) != pubB64 { + t.Fatalf("public key does not match the one derived from the private key") + } +} + +func TestGenerateVAPIDKeyPair_ProducesDistinctKeysEachCall(t *testing.T) { + pub1, priv1, err := GenerateVAPIDKeyPair() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + pub2, priv2, err := GenerateVAPIDKeyPair() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if pub1 == pub2 || priv1 == priv2 { + t.Fatalf("expected distinct keypairs across calls, got identical values") + } +} + +func TestBuildVAPIDHeader_ProducesValidSignedJWT(t *testing.T) { + pubB64, privB64, err := GenerateVAPIDKeyPair() + if err != nil { + t.Fatalf("unexpected error generating keypair: %v", err) + } + + before := time.Now() + header, err := buildVAPIDHeader(pubB64, privB64, "mailto:ops@example.com", "https://push.example.net/subscription/abc123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + const prefix = "vapid t=" + if !strings.HasPrefix(header, prefix) { + t.Fatalf("expected header to start with %q, got %q", prefix, header) + } + rest := strings.TrimPrefix(header, prefix) + parts := strings.SplitN(rest, ", k=", 2) + if len(parts) != 2 { + t.Fatalf("expected header to contain ', k=', got %q", header) + } + jwt, k := parts[0], parts[1] + if k != pubB64 { + t.Fatalf("expected k=%q, got %q", pubB64, k) + } + + jwtParts := strings.Split(jwt, ".") + if len(jwtParts) != 3 { + t.Fatalf("expected a 3-part JWT, got %d parts: %q", len(jwtParts), jwt) + } + headerB64, payloadB64, sigB64 := jwtParts[0], jwtParts[1], jwtParts[2] + + headerJSON, err := base64.RawURLEncoding.DecodeString(headerB64) + if err != nil { + t.Fatalf("JWT header is not valid base64url: %v", err) + } + var hdr struct { + Typ string `json:"typ"` + Alg string `json:"alg"` + } + if err := json.Unmarshal(headerJSON, &hdr); err != nil { + t.Fatalf("JWT header is not valid JSON: %v", err) + } + if hdr.Typ != "JWT" || hdr.Alg != "ES256" { + t.Fatalf("unexpected JWT header: %+v", hdr) + } + + payloadJSON, err := base64.RawURLEncoding.DecodeString(payloadB64) + if err != nil { + t.Fatalf("JWT payload is not valid base64url: %v", err) + } + var payload struct { + Aud string `json:"aud"` + Exp int64 `json:"exp"` + Sub string `json:"sub"` + } + if err := json.Unmarshal(payloadJSON, &payload); err != nil { + t.Fatalf("JWT payload is not valid JSON: %v", err) + } + if payload.Aud != "https://push.example.net" { + t.Fatalf("expected aud to be the endpoint's scheme+host, got %q", payload.Aud) + } + if payload.Sub != "mailto:ops@example.com" { + t.Fatalf("expected sub to be the configured VAPID subject, got %q", payload.Sub) + } + wantExp := before.Add(vapidJWTLifetime) + gotExp := time.Unix(payload.Exp, 0) + if diff := gotExp.Sub(wantExp); diff < -5*time.Second || diff > 5*time.Second { + t.Fatalf("expected exp close to %v, got %v (diff %v)", wantExp, gotExp, diff) + } + + sigRaw, err := base64.RawURLEncoding.DecodeString(sigB64) + if err != nil { + t.Fatalf("JWT signature is not valid base64url: %v", err) + } + if len(sigRaw) != 64 { + t.Fatalf("expected a 64-byte raw r||s signature, got %d bytes", len(sigRaw)) + } + r := new(big.Int).SetBytes(sigRaw[:32]) + s := new(big.Int).SetBytes(sigRaw[32:]) + + pubRaw, err := base64.RawURLEncoding.DecodeString(pubB64) + if err != nil { + t.Fatalf("public key is not valid base64url: %v", err) + } + pub, err := ecdsa.ParseUncompressedPublicKey(elliptic.P256(), pubRaw) + if err != nil { + t.Fatalf("failed to parse public key: %v", err) + } + + digest := sha256.Sum256([]byte(headerB64 + "." + payloadB64)) + if !ecdsa.Verify(pub, digest[:], r, s) { + t.Fatalf("signature does not verify against the configured VAPID public key") + } +} + +func TestBuildVAPIDHeader_RejectsInvalidPrivateKey(t *testing.T) { + pubB64, _, err := GenerateVAPIDKeyPair() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, err := buildVAPIDHeader(pubB64, "not-valid-base64url!!", "mailto:ops@example.com", "https://push.example.net/x"); err == nil { + t.Fatal("expected an error for an invalid private key") + } +} + +func TestBuildVAPIDHeader_RejectsInvalidEndpoint(t *testing.T) { + pubB64, privB64, err := GenerateVAPIDKeyPair() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, err := buildVAPIDHeader(pubB64, privB64, "mailto:ops@example.com", "://not-a-url"); err == nil { + t.Fatal("expected an error for an unparsable endpoint") + } +} From 87bb57ac1309b13b02b82710f7594bb534706172 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:30:54 +0000 Subject: [PATCH 07/12] feat(webpush): add Config/Client/New/Send MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires the RFC 8291 encryption and RFC 8292 VAPID signing pieces from the previous two commits into a full notify.Sender: fail-fast validation of all six required Config fields plus a VAPID keypair-consistency check (catches a mismatched public/private key pair locally instead of an opaque remote 401), Urgency/Topic validation, template rendering, encryption, and dispatch via transport.Wrapper with the aes128gcm/TTL/ Authorization headers set. Unlike ntfy/pushover, the rendered payload is not required to carry a "message" field — the payload shape is entirely up to the receiving service worker, which this module has no visibility into. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- providers/webpush/webpush.go | 263 ++++++++++++++++++++++ providers/webpush/webpush_test.go | 363 ++++++++++++++++++++++++++++++ 2 files changed, 626 insertions(+) create mode 100644 providers/webpush/webpush.go create mode 100644 providers/webpush/webpush_test.go diff --git a/providers/webpush/webpush.go b/providers/webpush/webpush.go new file mode 100644 index 0000000..93bad38 --- /dev/null +++ b/providers/webpush/webpush.go @@ -0,0 +1,263 @@ +// Package webpush implements notify.Sender for direct browser Web Push +// delivery (RFC 8030/8291/8292) — no third-party relay involved. A single +// Config pairs one application's VAPID identity with one browser +// PushSubscription; a host application fanning a Message out to many +// subscribers constructs one *Client per subscription (cheap: New does no +// I/O) and calls Send on each, exactly like fanning out to many +// Sender values of any other provider type. +package webpush + +import ( + "bytes" + "context" + "crypto/ecdsa" + "crypto/elliptic" + "encoding/base64" + "encoding/json" + "fmt" + "regexp" + "strconv" + "strings" + + notify "github.com/Wikid82/go_notify_yourself" + render "github.com/Wikid82/go_notify_yourself/providers/internal/render" + "github.com/Wikid82/go_notify_yourself/transport" +) + +// DefaultTTL is used for the RFC 8030 "TTL" header when Config.TTL is zero. +// Four weeks (2,419,200 seconds) — a conservative value inside the maximum +// retention window most push services honor before evicting an +// undelivered message. See Config.TTL's doc comment for why Config.TTL's +// zero value is *not* treated as RFC 8030's spec-legal "attempt immediate +// delivery only, don't store" meaning. +const DefaultTTL = 4 * 7 * 24 * 3600 + +// topicPattern matches RFC 8030's "Topic" header charset: up to 32 +// characters from the URL-and-filename-safe base64 alphabet. +var topicPattern = regexp.MustCompile(`^[A-Za-z0-9_-]{1,32}$`) + +// Config configures a webpush Sender. Unlike every other provider's Config, +// this one mixes two conceptually distinct groups of fields: VAPID +// application identity (shared across every subscription this application +// pushes to) and one subscriber's PushSubscription destination. A host +// application constructs one webpush.Client per subscriber, reusing the +// same VAPID* values across all of them — see the package doc comment for +// the fan-out pattern. +type Config struct { + // --- VAPID application identity (RFC 8292) --- + + // VAPIDPublicKey is the application server's VAPID public key: an + // uncompressed P-256 point (65 bytes: 0x04 || X || Y), base64url + // (no padding) encoded. This is the same value the browser is given as + // PushManager.subscribe({applicationServerKey: VAPIDPublicKey}). + // Required. + VAPIDPublicKey string + + // VAPIDPrivateKey is the application server's VAPID private key: a + // 32-byte P-256 scalar, base64url (no padding) encoded. Required. This + // value never leaves the process — Send signs a JWT with it locally + // and never transmits it. + VAPIDPrivateKey string + + // VAPIDSubject identifies the application server operator, per RFC + // 8292's "sub" JWT claim: a "mailto:" or "https:" URI (e.g. + // "mailto:ops@example.com"). Some push services (notably Mozilla's) + // reject a VAPID JWT with an empty or malformed sub. Required. + VAPIDSubject string + + // --- Subscriber destination (the browser's PushSubscription) --- + + // Endpoint is the subscription's push service URL, from + // PushSubscription.endpoint. Required. + Endpoint string + + // P256dh is the subscriber's P-256 Diffie-Hellman public key, from + // PushSubscription.getKey('p256dh'): base64url (no padding) encoded. + // Required. + P256dh string + + // Auth is the subscriber's 16-byte authentication secret, from + // PushSubscription.getKey('auth'): base64url (no padding) encoded. + // Required. + Auth string + + // --- Delivery hints (RFC 8030) --- + + // TTL is the number of seconds the push service should retain the + // message if the subscriber is currently offline, sent as the "TTL" + // header. Zero uses DefaultTTL — see that constant's doc comment. + TTL int + + // Urgency is an optional RFC 8030 "Urgency" header value: one of + // "very-low", "low", "normal", "high". Empty omits the header (the + // push service's own default applies, typically "normal"). Send + // rejects any other value. + Urgency string + + // Topic is an optional RFC 8030 "Topic" header value: up to 32 + // characters from the URL-and-filename-safe base64 alphabet + // ([A-Za-z0-9_-]). When set, a pending undelivered message with the + // same Topic is replaced rather than queued alongside it. Empty omits + // the header. Send rejects a Topic outside this charset/length. + Topic string + + // --- Payload templating --- + + // Template selects the JSON payload shape: "minimal" (default), + // "detailed", or "custom" (uses CustomTemplate) — same convention as + // every other JSON-payload provider (providers/internal/render). The + // rendered JSON is the plaintext that gets RFC 8291-encrypted; the + // receiving service worker's `push` event handler is responsible for + // JSON.parse-ing the decrypted payload. This module has no opinion on + // what the service worker does with it beyond that it is valid JSON. + Template string + + // CustomTemplate is a user-supplied Go text/template string, used when + // Template is "custom". + CustomTemplate string +} + +// Client dispatches notify.Message values to one browser PushSubscription. +type Client struct { + cfg Config + wrapper *transport.Wrapper +} + +var _ notify.Sender = (*Client)(nil) + +// New constructs a webpush Client. w performs the actual dispatch — see +// transport.NewWrapper. +func New(cfg Config, w *transport.Wrapper) *Client { + return &Client{cfg: cfg, wrapper: w} +} + +// verifyVAPIDKeyPairMatches decodes vapidPrivateKey, derives its +// corresponding public key, and compares it byte-for-byte against +// vapidPublicKey — catching a copy-pasted mismatched keypair locally +// instead of deferring it to an opaque remote 401/403 from the push +// service. +func verifyVAPIDKeyPairMatches(vapidPublicKey, vapidPrivateKey string) error { + privRaw, err := base64.RawURLEncoding.DecodeString(vapidPrivateKey) + if err != nil { + return fmt.Errorf("webpush: VAPID private key: invalid base64url encoding: %w", err) + } + priv, err := ecdsa.ParseRawPrivateKey(elliptic.P256(), privRaw) + if err != nil { + return fmt.Errorf("webpush: VAPID private key: %w", err) + } + derivedPub, err := priv.PublicKey.Bytes() + if err != nil { + return fmt.Errorf("webpush: VAPID private key: %w", err) + } + + pubRaw, err := base64.RawURLEncoding.DecodeString(vapidPublicKey) + if err != nil { + return fmt.Errorf("webpush: VAPID public key: invalid base64url encoding: %w", err) + } + + if !bytes.Equal(derivedPub, pubRaw) { + return fmt.Errorf("webpush: VAPID public/private key pair does not match") + } + return nil +} + +// Send renders msg using the configured template, RFC 8291-encrypts the +// result for the configured subscriber, and dispatches it to +// cfg.Endpoint via the shared transport.Wrapper, authenticated with an +// RFC 8292 VAPID JSON Web Token signed for this request. +func (c *Client) Send(ctx context.Context, msg notify.Message) error { + vapidPublicKey := strings.TrimSpace(c.cfg.VAPIDPublicKey) + if vapidPublicKey == "" { + return fmt.Errorf("webpush: VAPID public key is not configured") + } + vapidPrivateKey := strings.TrimSpace(c.cfg.VAPIDPrivateKey) + if vapidPrivateKey == "" { + return fmt.Errorf("webpush: VAPID private key is not configured") + } + vapidSubject := strings.TrimSpace(c.cfg.VAPIDSubject) + if vapidSubject == "" { + return fmt.Errorf("webpush: VAPID subject is not configured") + } + if !strings.HasPrefix(vapidSubject, "mailto:") && !strings.HasPrefix(vapidSubject, "https://") { + return fmt.Errorf("webpush: VAPID subject must start with %q or %q", "mailto:", "https://") + } + endpoint := strings.TrimSpace(c.cfg.Endpoint) + if endpoint == "" { + return fmt.Errorf("webpush: endpoint is not configured") + } + p256dh := strings.TrimSpace(c.cfg.P256dh) + if p256dh == "" { + return fmt.Errorf("webpush: p256dh is not configured") + } + auth := strings.TrimSpace(c.cfg.Auth) + if auth == "" { + return fmt.Errorf("webpush: auth is not configured") + } + + if err := verifyVAPIDKeyPairMatches(vapidPublicKey, vapidPrivateKey); err != nil { + return err + } + + urgency := strings.TrimSpace(c.cfg.Urgency) + if urgency != "" { + switch urgency { + case "very-low", "low", "normal", "high": + default: + return fmt.Errorf("webpush: invalid urgency %q", urgency) + } + } + + topic := strings.TrimSpace(c.cfg.Topic) + if topic != "" && !topicPattern.MatchString(topic) { + return fmt.Errorf("webpush: invalid topic %q: must be 1-32 URL-safe base64 characters", topic) + } + + tmplStr := render.SelectTemplate(c.cfg.Template, c.cfg.CustomTemplate, render.MinimalTemplate, render.DetailedTemplate) + rendered, err := render.Render(tmplStr, render.TemplateData(msg)) + if err != nil { + return err + } + + var payload any + if err := json.Unmarshal([]byte(rendered), &payload); err != nil { + return fmt.Errorf("invalid JSON payload: %w", err) + } + + ciphertext, err := encryptAES128GCM(p256dh, auth, []byte(rendered)) + if err != nil { + return fmt.Errorf("webpush: encrypt payload: %w", err) + } + + authHeader, err := buildVAPIDHeader(vapidPublicKey, vapidPrivateKey, vapidSubject, endpoint) + if err != nil { + return fmt.Errorf("webpush: build VAPID header: %w", err) + } + + ttl := c.cfg.TTL + if ttl == 0 { + ttl = DefaultTTL + } + + headers := map[string]string{ + "Content-Type": "application/octet-stream", + "Content-Encoding": "aes128gcm", + "Authorization": authHeader, + "TTL": strconv.Itoa(ttl), + } + if urgency != "" { + headers["Urgency"] = urgency + } + if topic != "" { + headers["Topic"] = topic + } + + if _, err := c.wrapper.Send(ctx, transport.Request{ + URL: endpoint, + Headers: headers, + Body: ciphertext, + }); err != nil { + return fmt.Errorf("failed to send web push: %w", err) + } + + return nil +} diff --git a/providers/webpush/webpush_test.go b/providers/webpush/webpush_test.go new file mode 100644 index 0000000..ecf7976 --- /dev/null +++ b/providers/webpush/webpush_test.go @@ -0,0 +1,363 @@ +package webpush + +import ( + "bytes" + "context" + "crypto/ecdh" + "crypto/rand" + "encoding/base64" + "io" + "net/http" + "strconv" + "strings" + "testing" + + notify "github.com/Wikid82/go_notify_yourself" + "github.com/Wikid82/go_notify_yourself/transport" +) + +type capturingRoundTripper struct { + lastRequest *http.Request + lastBody []byte + statusCode int + respBody string +} + +func (c *capturingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + c.lastRequest = req + if req.Body != nil { + b, _ := io.ReadAll(req.Body) + c.lastBody = b + } + status := c.statusCode + if status == 0 { + status = http.StatusCreated + } + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(c.respBody)), + Header: make(http.Header), + }, nil +} + +func passthroughValidator(rawURL string, _ bool) (string, error) { return rawURL, nil } + +func newTestWrapper(rt *capturingRoundTripper) *transport.Wrapper { + return transport.NewWrapper( + transport.WithURLValidator(passthroughValidator), + transport.WithClientFactory(func(bool, int) *http.Client { + return &http.Client{Transport: rt} + }), + transport.WithRetryPolicy(transport.RetryPolicy{MaxAttempts: 1}), + ) +} + +const validEndpoint = "https://push.example.net/subscription/abc123" + +// testSubscriber generates a fresh, valid subscriber p256dh/auth pair for +// tests that need to reach the encryption step successfully. +func testSubscriber(t *testing.T) (p256dh, auth string) { + t.Helper() + receiver, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + authSecret := make([]byte, 16) + if _, err := rand.Read(authSecret); err != nil { + t.Fatalf("unexpected rand error: %v", err) + } + return base64.RawURLEncoding.EncodeToString(receiver.PublicKey().Bytes()), base64.RawURLEncoding.EncodeToString(authSecret) +} + +// validConfig returns a Config with a genuinely matching VAPID keypair and +// a valid subscriber, suitable as a base for tests that mutate one field. +func validConfig(t *testing.T) Config { + t.Helper() + pub, priv, err := GenerateVAPIDKeyPair() + if err != nil { + t.Fatalf("unexpected error generating VAPID keypair: %v", err) + } + p256dh, auth := testSubscriber(t) + return Config{ + VAPIDPublicKey: pub, + VAPIDPrivateKey: priv, + VAPIDSubject: "mailto:ops@example.com", + Endpoint: validEndpoint, + P256dh: p256dh, + Auth: auth, + } +} + +func TestClientSend_RejectsMissingVAPIDPublicKey(t *testing.T) { + cfg := validConfig(t) + cfg.VAPIDPublicKey = "" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "is not configured") { + t.Fatalf("expected not-configured error, got: %v", err) + } +} + +func TestClientSend_RejectsMissingVAPIDPrivateKey(t *testing.T) { + cfg := validConfig(t) + cfg.VAPIDPrivateKey = "" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "is not configured") { + t.Fatalf("expected not-configured error, got: %v", err) + } +} + +func TestClientSend_RejectsMissingVAPIDSubject(t *testing.T) { + cfg := validConfig(t) + cfg.VAPIDSubject = "" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "is not configured") { + t.Fatalf("expected not-configured error, got: %v", err) + } +} + +func TestClientSend_RejectsMalformedVAPIDSubjectPrefix(t *testing.T) { + cfg := validConfig(t) + cfg.VAPIDSubject = "ops@example.com" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "must start with") { + t.Fatalf("expected VAPID subject prefix error, got: %v", err) + } +} + +func TestClientSend_AllowsHTTPSVAPIDSubject(t *testing.T) { + cfg := validConfig(t) + cfg.VAPIDSubject = "https://example.com/contact" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + if err := client.Send(context.Background(), notify.Message{Body: "hello"}); err != nil { + t.Fatalf("unexpected error with https: VAPID subject: %v", err) + } +} + +func TestClientSend_RejectsMissingEndpoint(t *testing.T) { + cfg := validConfig(t) + cfg.Endpoint = "" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "is not configured") { + t.Fatalf("expected not-configured error, got: %v", err) + } +} + +func TestClientSend_RejectsMissingP256dh(t *testing.T) { + cfg := validConfig(t) + cfg.P256dh = "" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "is not configured") { + t.Fatalf("expected not-configured error, got: %v", err) + } +} + +func TestClientSend_RejectsMissingAuth(t *testing.T) { + cfg := validConfig(t) + cfg.Auth = "" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "is not configured") { + t.Fatalf("expected not-configured error, got: %v", err) + } +} + +func TestClientSend_RejectsMismatchedVAPIDKeyPair(t *testing.T) { + cfg := validConfig(t) + otherPub, _, err := GenerateVAPIDKeyPair() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + cfg.VAPIDPublicKey = otherPub + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err = client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "does not match") { + t.Fatalf("expected VAPID key pair mismatch error, got: %v", err) + } +} + +func TestClientSend_RejectsInvalidUrgency(t *testing.T) { + cfg := validConfig(t) + cfg.Urgency = "extremely-urgent" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "invalid urgency") { + t.Fatalf("expected invalid urgency error, got: %v", err) + } +} + +func TestClientSend_RejectsInvalidTopic(t *testing.T) { + cfg := validConfig(t) + cfg.Topic = "not a valid topic!" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "invalid topic") { + t.Fatalf("expected invalid topic error, got: %v", err) + } +} + +func TestClientSend_RejectsNonJSONTemplateOutput(t *testing.T) { + cfg := validConfig(t) + cfg.Template = "custom" + cfg.CustomTemplate = "not json" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "invalid JSON payload") { + t.Fatalf("expected invalid JSON payload error, got: %v", err) + } +} + +func TestClientSend_DoesNotRequireMessageField(t *testing.T) { + cfg := validConfig(t) + cfg.Template = "custom" + cfg.CustomTemplate = `{"foo":"bar"}` + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + if err := client.Send(context.Background(), notify.Message{Body: "x"}); err != nil { + t.Fatalf("unexpected error: webpush should not require a 'message' field: %v", err) + } +} + +func TestClientSend_RejectsOversizedPlaintext(t *testing.T) { + cfg := validConfig(t) + cfg.Template = "custom" + cfg.CustomTemplate = `{"message":"` + strings.Repeat("a", MaxPlaintextSize+100) + `"}` + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "exceeds maximum plaintext size") { + t.Fatalf("expected oversized plaintext error, got: %v", err) + } +} + +func TestClientSend_SendsCorrectHeadersOnSuccessWithDefaultTTL(t *testing.T) { + cfg := validConfig(t) + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + if err := client.Send(context.Background(), notify.Message{Title: "hi", Body: "hello"}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if got := rt.lastRequest.Header.Get("Content-Type"); got != "application/octet-stream" { + t.Fatalf("expected Content-Type application/octet-stream, got %q", got) + } + if got := rt.lastRequest.Header.Get("Content-Encoding"); got != "aes128gcm" { + t.Fatalf("expected Content-Encoding aes128gcm, got %q", got) + } + if got := rt.lastRequest.Header.Get("TTL"); got != strconv.Itoa(DefaultTTL) { + t.Fatalf("expected TTL header %d (DefaultTTL), got %q", DefaultTTL, got) + } + auth := rt.lastRequest.Header.Get("Authorization") + if !strings.HasPrefix(auth, "vapid t=") { + t.Fatalf("expected vapid Authorization header, got %q", auth) + } + if !strings.Contains(auth, "k="+cfg.VAPIDPublicKey) { + t.Fatalf("expected Authorization header to carry the configured public key, got %q", auth) + } +} + +func TestClientSend_BodyIsCiphertextNotPlaintext(t *testing.T) { + cfg := validConfig(t) + cfg.Template = "custom" + cfg.CustomTemplate = `{"message":"a very secret plaintext value"}` + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + if err := client.Send(context.Background(), notify.Message{Body: "x"}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if bytes.Contains(rt.lastBody, []byte("a very secret plaintext value")) { + t.Fatalf("request body leaked the plaintext payload: %q", rt.lastBody) + } + if len(rt.lastBody) < 86 { + t.Fatalf("expected an aes128gcm-framed body of at least 86 bytes, got %d", len(rt.lastBody)) + } +} + +func TestClientSend_TTLReflectsConfiguredValue(t *testing.T) { + cfg := validConfig(t) + cfg.TTL = 3600 + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + if err := client.Send(context.Background(), notify.Message{Body: "hello"}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := rt.lastRequest.Header.Get("TTL"); got != "3600" { + t.Fatalf("expected TTL header 3600, got %q", got) + } +} + +func TestClientSend_UrgencyAndTopicHeadersOnlyWhenConfigured(t *testing.T) { + cfg := validConfig(t) + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + if err := client.Send(context.Background(), notify.Message{Body: "hello"}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := rt.lastRequest.Header.Get("Urgency"); got != "" { + t.Fatalf("expected no Urgency header when unconfigured, got %q", got) + } + if got := rt.lastRequest.Header.Get("Topic"); got != "" { + t.Fatalf("expected no Topic header when unconfigured, got %q", got) + } + + cfg.Urgency = "high" + cfg.Topic = "my-topic" + rt2 := &capturingRoundTripper{} + client2 := New(cfg, newTestWrapper(rt2)) + if err := client2.Send(context.Background(), notify.Message{Body: "hello"}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := rt2.lastRequest.Header.Get("Urgency"); got != "high" { + t.Fatalf("expected Urgency header 'high', got %q", got) + } + if got := rt2.lastRequest.Header.Get("Topic"); got != "my-topic" { + t.Fatalf("expected Topic header 'my-topic', got %q", got) + } +} + +func TestClientSend_PropagatesWrapperError(t *testing.T) { + cfg := validConfig(t) + rt := &capturingRoundTripper{statusCode: http.StatusInternalServerError} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "hello"}) + if err == nil || !strings.Contains(err.Error(), "failed to send web push") { + t.Fatalf("expected wrapped send error, got: %v", err) + } +} From 3b4d57484d54f07b00a26735d9f63899554740fd Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:31:41 +0000 Subject: [PATCH 08/12] feat(webpush): register provider and wire into providers/all Registers the webpush Factory under "webpush" (mirroring every other provider's register.go convention) and adds it to providers/all's blank imports so it's available via notify.New without an explicit per-provider import. Bumps providers/all's wantProviderCount safety-net test. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- providers/all/all.go | 1 + providers/all/all_test.go | 4 +- providers/webpush/register.go | 42 +++++++++++++ providers/webpush/register_test.go | 97 ++++++++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 providers/webpush/register.go create mode 100644 providers/webpush/register_test.go diff --git a/providers/all/all.go b/providers/all/all.go index 9dc3732..3455849 100644 --- a/providers/all/all.go +++ b/providers/all/all.go @@ -30,4 +30,5 @@ import ( _ "github.com/Wikid82/go_notify_yourself/providers/slack" _ "github.com/Wikid82/go_notify_yourself/providers/telegram" _ "github.com/Wikid82/go_notify_yourself/providers/webhook" + _ "github.com/Wikid82/go_notify_yourself/providers/webpush" ) diff --git a/providers/all/all_test.go b/providers/all/all_test.go index 6627200..2858dd5 100644 --- a/providers/all/all_test.go +++ b/providers/all/all_test.go @@ -13,7 +13,7 @@ import ( // enforcement (a provider that registers itself but isn't added to // providers/all silently isn't part of the "one import gets everything" // bundle). -const wantProviderCount = 8 +const wantProviderCount = 9 func TestAll_RegistersEveryBuiltInProvider(t *testing.T) { types := notify.RegisteredTypes() @@ -22,7 +22,7 @@ func TestAll_RegistersEveryBuiltInProvider(t *testing.T) { wantProviderCount, len(types), types) } - want := []string{"discord", "email", "gotify", "ntfy", "pushover", "slack", "telegram", "webhook"} + want := []string{"discord", "email", "gotify", "ntfy", "pushover", "slack", "telegram", "webhook", "webpush"} registered := make(map[string]bool, len(types)) for _, name := range types { registered[name] = true diff --git a/providers/webpush/register.go b/providers/webpush/register.go new file mode 100644 index 0000000..713b9fd --- /dev/null +++ b/providers/webpush/register.go @@ -0,0 +1,42 @@ +package webpush + +import ( + "fmt" + + notify "github.com/Wikid82/go_notify_yourself" + "github.com/Wikid82/go_notify_yourself/providers/internal/regconfig" + "github.com/Wikid82/go_notify_yourself/transport" +) + +// init registers this package's Factory under the name "webpush" with the +// notify package's registry. +// +// Expected config keys: +// - "transport" (required): *transport.Wrapper. +// - "vapid_public_key", "vapid_private_key", "vapid_subject" (string, required). +// - "endpoint", "p256dh", "auth" (string, required). +// - "ttl" (int, optional; 0 uses DefaultTTL). +// - "urgency", "topic" (string, optional). +// - "template", "custom_template" (string, optional). +func init() { + notify.Register("webpush", func(config map[string]any) (notify.Sender, error) { + w, ok := config["transport"].(*transport.Wrapper) + if !ok || w == nil { + return nil, fmt.Errorf(`webpush: config["transport"] must be a non-nil *transport.Wrapper`) + } + cfg := Config{ + VAPIDPublicKey: regconfig.StringField(config, "vapid_public_key"), + VAPIDPrivateKey: regconfig.StringField(config, "vapid_private_key"), + VAPIDSubject: regconfig.StringField(config, "vapid_subject"), + Endpoint: regconfig.StringField(config, "endpoint"), + P256dh: regconfig.StringField(config, "p256dh"), + Auth: regconfig.StringField(config, "auth"), + TTL: regconfig.IntField(config, "ttl"), + Urgency: regconfig.StringField(config, "urgency"), + Topic: regconfig.StringField(config, "topic"), + Template: regconfig.StringField(config, "template"), + CustomTemplate: regconfig.StringField(config, "custom_template"), + } + return New(cfg, w), nil + }) +} diff --git a/providers/webpush/register_test.go b/providers/webpush/register_test.go new file mode 100644 index 0000000..367f95f --- /dev/null +++ b/providers/webpush/register_test.go @@ -0,0 +1,97 @@ +package webpush + +import ( + "testing" + + notify "github.com/Wikid82/go_notify_yourself" + "github.com/Wikid82/go_notify_yourself/transport" +) + +func TestRegister_NewReturnsWorkingSender(t *testing.T) { + w := transport.NewWrapper() + + sender, err := notify.New("webpush", map[string]any{ + "transport": w, + "vapid_public_key": "pub-key", + "vapid_private_key": "priv-key", + "vapid_subject": "mailto:ops@example.com", + "endpoint": "https://push.example.net/subscription/abc123", + "p256dh": "p256dh-value", + "auth": "auth-value", + "ttl": 3600, + "urgency": "high", + "topic": "my-topic", + }) + if err != nil { + t.Fatalf("notify.New(\"webpush\", ...) returned error: %v", err) + } + + client, ok := sender.(*Client) + if !ok { + t.Fatalf("expected *webpush.Client, got %T", sender) + } + want := Config{ + VAPIDPublicKey: "pub-key", + VAPIDPrivateKey: "priv-key", + VAPIDSubject: "mailto:ops@example.com", + Endpoint: "https://push.example.net/subscription/abc123", + P256dh: "p256dh-value", + Auth: "auth-value", + TTL: 3600, + Urgency: "high", + Topic: "my-topic", + } + if client.cfg != want { + t.Errorf("expected config to be threaded through, got %#v, want %#v", client.cfg, want) + } +} + +func TestRegister_TTLAcceptsFloat64FromJSONStyleDecode(t *testing.T) { + w := transport.NewWrapper() + + sender, err := notify.New("webpush", map[string]any{ + "transport": w, + "vapid_public_key": "pub-key", + "vapid_private_key": "priv-key", + "vapid_subject": "mailto:ops@example.com", + "endpoint": "https://push.example.net/subscription/abc123", + "p256dh": "p256dh-value", + "auth": "auth-value", + "ttl": float64(7200), + }) + if err != nil { + t.Fatalf("notify.New(\"webpush\", ...) returned error: %v", err) + } + client, ok := sender.(*Client) + if !ok { + t.Fatalf("expected *webpush.Client, got %T", sender) + } + if client.cfg.TTL != 7200 { + t.Errorf("expected TTL 7200 threaded through from a float64 config value, got %d", client.cfg.TTL) + } +} + +func TestRegister_MissingTransportReturnsErrorNotPanic(t *testing.T) { + sender, err := notify.New("webpush", map[string]any{ + "vapid_public_key": "pub-key", + }) + if err == nil { + t.Fatal("expected an error when config[\"transport\"] is missing") + } + if sender != nil { + t.Fatalf("expected a nil Sender on error, got %#v", sender) + } +} + +func TestRegister_RegisteredUnderExpectedName(t *testing.T) { + found := false + for _, name := range notify.RegisteredTypes() { + if name == "webpush" { + found = true + break + } + } + if !found { + t.Errorf("expected %q registered in notify.RegisteredTypes(), got %v", "webpush", notify.RegisteredTypes()) + } +} From 8c349398e8da18bcadb37b0c7eab30c24e270876 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:32:34 +0000 Subject: [PATCH 09/12] docs: document providers/webpush MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a providers/webpush row to README's provider table, updates the "Project status" paragraph to explain why webpush is a deliberate, maintainer-approved exception to the "no new providers without an explicit ask" policy rather than a violation of it, adds a webpush.New construction example (with the one-Client-per-subscriber fan-out pattern) to docs/INTEGRATION.md, and flags webpush.Config's app-wide/per-recipient field split as a documented precedent in ARCHITECTURE.md §3.2. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- ARCHITECTURE.md | 5 +++++ README.md | 17 +++++++++++------ docs/INTEGRATION.md | 26 ++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1cb5ebb..b454f81 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -88,6 +88,11 @@ providers// all, because email doesn't dispatch a JSON payload — it composes an HTML body via a host-supplied `TemplateRenderer` instead. Don't force every provider into the HTTP-shaped `Config` convention; follow what the provider's transport actually needs. +- `webpush` is the first provider whose `Config` mixes two conceptually distinct field groups in one + flat struct: application-wide VAPID identity (shared across every subscription) and one + subscriber's `PushSubscription` destination (per-recipient). It's still one flat exported struct + fed through the same `New(cfg, w)` constructor shape as every other provider — flagged here only + so a future reader doesn't assume every `Config` field is per-recipient. ### 3.3 The `New` constructor convention diff --git a/README.md b/README.md index ad04e66..1ff1877 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,7 @@ module has no opinion on your event vocabulary. | `providers/telegram` | `BotToken`, `ChatID`, `BaseURL` (optional override) | Bot token is embedded in the dispatch URL path per Telegram's own API convention; injects `chat_id`. | | `providers/webhook` | `URL` | Generic/custom JSON dispatch — no destination allowlist, no payload field requirements. Also exposes `RenderPreview` for validating a custom template without dispatching. | | `providers/email` | see below | The one provider not built on `transport.Wrapper` — see [Email](#email). | +| `providers/webpush` | `VAPIDPublicKey`, `VAPIDPrivateKey`, `VAPIDSubject`, `Endpoint`, `P256dh`, `Auth`, `TTL` (optional), `Urgency`/`Topic` (optional) | Direct browser Web Push delivery (RFC 8030/8291/8292) — no third-party relay. Encrypts the payload per RFC 8291 (`aes128gcm`) and signs an RFC 8292 VAPID JWT per request; no `Config.Message` field requirement, since the payload shape is entirely up to the receiving service worker. | Every HTTP-based provider's `Config.Template` selects the JSON payload shape: `"minimal"` (default), `"detailed"`, or `"custom"` (uses `Config.CustomTemplate`, a Go `text/template` string with a @@ -113,7 +114,7 @@ or config file), not hardcoded at compile time. ```go import ( notify "github.com/Wikid82/go_notify_yourself" - _ "github.com/Wikid82/go_notify_yourself/providers/all" // registers all 8 built-in providers + _ "github.com/Wikid82/go_notify_yourself/providers/all" // registers all 9 built-in providers ) wrapper := transport.NewWrapper() @@ -219,11 +220,15 @@ server. See any `providers/*/*_test.go` file in this repo for the pattern. ## Project status Extracted from [Charon](https://github.com/Wikid82/charon)'s internal notification engine. The -provider list is intentionally exactly these seven HTTP providers plus email — see -`docs/plans/notifications_extraction_spec.md` in Charon's repo for the extraction design brief. -Long-term direction is an [Apprise](https://github.com/caronc/apprise)-style common interface over a -larger provider catalog; the `Sender` interface and per-package structure here are deliberately -shaped so that's additive later, not a breaking rework. +original provider list was intentionally exactly the seven HTTP providers plus email ported from +Charon — see `docs/plans/notifications_extraction_spec.md` in Charon's repo for the extraction +design brief. `providers/webpush` (issue #14) is the one deliberate, maintainer-approved exception +to "no new providers without an explicit ask": it's a genuinely different delivery mechanism (direct +browser push, not a relay) with no Apprise equivalent, not a straight port — see +`docs/plans/current_spec.md` for its own design brief. Long-term direction is an +[Apprise](https://github.com/caronc/apprise)-style common interface over a larger provider catalog; +the `Sender` interface and per-package structure here are deliberately shaped so that's additive +later, not a breaking rework. ## License diff --git a/docs/INTEGRATION.md b/docs/INTEGRATION.md index f216a62..66d7c83 100644 --- a/docs/INTEGRATION.md +++ b/docs/INTEGRATION.md @@ -162,6 +162,32 @@ you don't need runtime discovery: sender := discord.New(discord.Config{WebhookURL: "https://discord.com/api/webhooks/..."}, wrapper) ``` +`providers/webpush` (direct browser Web Push — no relay) follows the same typed-constructor shape, +but its `Config` mixes your application's VAPID identity (shared) with one subscriber's +`PushSubscription` destination (per-recipient) — see one `webpush.New` call per subscriber, reusing +the same `VAPIDPublicKey`/`VAPIDPrivateKey`/`VAPIDSubject` across all of them, to fan a single +`notify.Message` out to every subscriber your application has collected: + +```go +for _, sub := range subscriptions { // e.g. loaded from your own storage + sender := webpush.New(webpush.Config{ + VAPIDPublicKey: vapidPublicKey, // same for every subscriber + VAPIDPrivateKey: vapidPrivateKey, // same for every subscriber + VAPIDSubject: "mailto:ops@example.com", + Endpoint: sub.Endpoint, + P256dh: sub.P256dh, + Auth: sub.Auth, + }, wrapper) + if err := sender.Send(ctx, msg); err != nil { + log.Printf("webpush to %s failed: %v", sub.Endpoint, err) + } +} +``` + +`webpush.GenerateVAPIDKeyPair()` generates `VAPIDPublicKey`/`VAPIDPrivateKey` once at application +setup time; persist the result yourself (rotating it invalidates every subscription already +collected, since the browser binds each subscription to the exact public key it was created with). + **5. Dispatch a message:** ```go From 98a728e66c5465cb83719d4adb94dd4d0e5efdaf Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:41:39 +0000 Subject: [PATCH 10/12] test(webpush): add QA gate coverage for base64-decode-error branches; add QA report QA/Security final gate for issue #14 (Web Push). Closes real (non-fault-injection) coverage gaps: invalid-base64 (not merely wrong-length) p256dh/auth in encryptAES128GCMWithKeys, and malformed VAPID public/private keys reached through the real Client.Send path rather than only the lower-level buildVAPIDHeader seam. providers/webpush package coverage 87.7% -> 89.6%. Full gate results, SSRF/retry/crypto review, and two non-blocking findings (raw Endpoint echoed into a vapid.go error on malformed-URL input; the INTEGRATION.md example logs the raw Endpoint on send failure) are in docs/reports/qa_report.md, with concrete remediations for both. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- docs/reports/qa_report.md | 319 ++++++++++++++++++++++++++++++ providers/webpush/encrypt_test.go | 38 ++++ providers/webpush/webpush_test.go | 24 +++ 3 files changed, 381 insertions(+) create mode 100644 docs/reports/qa_report.md diff --git a/docs/reports/qa_report.md b/docs/reports/qa_report.md new file mode 100644 index 0000000..c3f5b55 --- /dev/null +++ b/docs/reports/qa_report.md @@ -0,0 +1,319 @@ +# QA / Security Report — `providers/webpush` (GitHub issue #14) + +- **Scope**: `providers/webpush`, `providers/internal/regconfig` (`IntField`), `transport` + (`sanitizeOutboundHeaders` allowlist), `providers/all` (registration wiring), and the + documentation updates (`README.md`, `docs/INTEGRATION.md`, `ARCHITECTURE.md`) shipped across + commits `1205a98`..`8c34939` on `development`. +- **Reviewer**: QA/Security gate (final stage of the plan → implement → review pipeline). +- **Reference**: `docs/plans/current_spec.md` (full spec read, including §3.8's error-handling + table and §7's acceptance criteria). +- **Verdict: PASS WITH TWO NON-BLOCKING FINDINGS.** Build/vet/staticcheck/test/coverage gates are + all green, the no-Charon-import gate is clean, and the RFC 8291 fixed-vector test is exact-byte + and passing. Two findings below (one MEDIUM, one LOW) are real, reproduced defects in + error-message/example-code hygiene around credential-equivalent values — neither blocks a merge + by itself (both require a genuinely malformed/misconfigured input to trigger, and neither is the + runtime's normal/happy path), but both should be fixed before this ships to reduce the chance of + a `PushSubscription.endpoint`'s bearer-token-equivalent path segment ending up in a log. + +--- + +## 1. Gate results + +| Gate | Result | +|---|---| +| `go build ./...` | PASS | +| `go vet ./...` | PASS (clean) | +| `staticcheck ./...` | PASS (clean) | +| `go test ./...` | PASS, all packages | +| `grep -r "Wikid82/charon" --include=*.go .` | **0 hits** — blocking gate clean | +| `scripts/test-coverage.sh` (repo-wide) | PASS — 94.6% (floor 85%) | +| `providers/webpush` package coverage | 89.6% (floor 85%, +4.6pt margin) | +| `transport` package coverage | 94.2% | +| `providers/internal/regconfig` package coverage | 100.0% | +| `providers/all` | No statements of its own (blank-import file); exercised via `providers/all/all_test.go`'s `TestAll_RegistersEveryBuiltInProvider`, which passes with `wantProviderCount = 9` | +| RFC 8291 Appendix A fixed-vector test | PASS, exact-byte (`TestEncryptAES128GCMWithKeys_RFC8291AppendixAVector` + a header-only cross-check) — not weakened to round-trip-only | +| Scope discipline (§1.3/§9 of spec) | No provider other than `webpush` added; diff touches exactly the files the spec's commit-slicing plan (§6) called for, nothing else | + +Diff confirmed via `git diff --stat 1205a98~1..8c34939`: 17 files, all within the spec's declared +scope (`ARCHITECTURE.md`, `README.md`, `docs/INTEGRATION.md`, `providers/all/{all,all_test}.go`, +`providers/internal/regconfig/*`, `providers/webpush/*`, `transport/{wrapper,wrapper_test}.go`). +No CI/CodeQL/release config touched. + +--- + +## 2. Security review + +### 2.1 SSRF surface — re-verified independently, not taken on faith + +Per this task's explicit instruction to re-verify Supervisor's "zero diff" claim rather than trust +it: confirmed by direct diff (`git diff 1205a98..99549d1 -- transport/`) that commit `99549d1` +("feat(transport): allow Content-Encoding/TTL/Urgency/Topic outbound headers") touches **only** +`sanitizeOutboundHeaders`'s `allowed` map in `transport/wrapper.go`, adding +`content-encoding`/`ttl`/`urgency`/`topic`. `transport/validate_default.go` +(`DefaultURLValidator`, `isPrivateIP`, `isAllowedIP`) and `hasDisallowedQueryAuthKey` in +`transport/wrapper.go` have **zero lines changed** anywhere in the webpush feature's commit range. +Confirmed clean, independently. + +The four newly-allowed headers are simple metadata (`Content-Encoding: aes128gcm`, a numeric +`TTL`, an `Urgency` enum, and a `Topic` string already regex-constrained by `webpush.go`'s +`topicPattern`) — none of them affect host resolution, redirect handling, or the request line, so +this is not a vector for request smuggling or host-header injection. `TestSanitizeOutboundHeadersAllowsWebPushHeaders` +proves the change is additive (an unlisted header, `X-Foo`, is still stripped) rather than a +wholesale relaxation. + +### 2.2 Retry/backoff — no new amplification vector + +`transport/retry.go` is untouched by this feature. `NewWrapper`'s default `RetryPolicy` (3 +attempts, 200ms/2s capped exponential backoff, jitter via `crypto/rand`) is unchanged and +applies to webpush requests identically to every other provider. `shouldRetry` does not retry on +4xx (a push service's 404/410 "subscription gone" is surfaced once, not retried — matches spec +§3.8's table). No unbounded loop, no provider-specific retry override introduced. + +### 2.3 Cryptography surface (new ground for this module) + +- **Randomness**: every place randomness is required uses `crypto/rand`, not `math/rand`: + `ecdh.P256().GenerateKey(rand.Reader)` (ephemeral ECDH keypair, `encrypt.go:53`), `rand.Read(salt)` + (16-byte salt, `encrypt.go:59`), `ecdsa.GenerateKey(elliptic.P256(), rand.Reader)` + (`GenerateVAPIDKeyPair`, `vapid.go:107`), and `ecdsa.Sign(rand.Reader, ...)` (VAPID JWT signing, + `vapid.go:82`). Confirmed via direct read — no `math/rand` import anywhere in + `providers/webpush`, no test-only randomness path reachable from a production call (the + deterministic `encryptAES128GCMWithKeys` seam is only ever called from the production + `encryptAES128GCM` with freshly-generated random inputs; tests call the seam directly with fixed + RFC vectors, which is the intended, documented test-only use). +- **VAPID JWT `exp` bound**: confirmed `vapidJWTLifetime = 12 * time.Hour` (`vapid.go:21`) and + `Exp: time.Now().Add(vapidJWTLifetime).Unix()` (`vapid.go:71`) — bounded, comfortably inside RFC + 8292's 24-hour recommended maximum, and re-signed fresh on every `Send` call (no caching/reuse + that could let a stale-but-still-valid token linger unnecessarily). `TestBuildVAPIDHeader_ProducesValidSignedJWT` + asserts the `exp` claim lands within 5 seconds of the expected value. +- **`ecdsa.Sign` not `ecdsa.SignASN1`**: confirmed (`vapid.go:82`), with manual raw `r||s` + left-zero-padded 32+32-byte encoding (`vapid.go:87-90`) — the correct JOSE/JWS ES256 shape, not + DER. `TestBuildVAPIDHeader_ProducesValidSignedJWT` round-trips this through `ecdsa.Verify` + against the reconstructed `big.Int`s, so this isn't just "compiles," it's verified-correct. +- **`MaxPlaintextSize = 3800` enforcement — fail-fast, confirmed by reading the control flow, not + assumed**: `encryptAES128GCMWithKeys` (`encrypt.go:75-78`) checks `len(plaintext) > + MaxPlaintextSize` as its **first** statement, before any base64 decoding, ECDH, HKDF, or AES + work. Both required boundary tests exist and pass: + `TestEncryptAES128GCMWithKeys_AcceptsPlaintextAtBoundary` (exactly `MaxPlaintextSize` succeeds) + and `TestEncryptAES128GCMWithKeys_RejectsOversizedPlaintext` (`MaxPlaintextSize+1` fails with the + named error). `TestClientSend_RejectsOversizedPlaintext` additionally exercises this through the + full `Send` path. Survived from plan to implementation intact. +- **RFC 8291 Appendix A fixed-vector test**: exact-byte assertion against the RFC's published + ciphertext (`TestEncryptAES128GCMWithKeys_RFC8291AppendixAVector`), plus an isolated + header-framing cross-check and a supplementary (explicitly-labeled-as-insufficient-alone) + self-round-trip test. This is the single highest-value correctness gate in the feature and it is + intact, not weakened. + +### 2.4 Credential-handling review — two findings (below) + +Applied the "no provider logs or echoes back a full credential in error messages, test fixtures, or +example code" check to all four flagged fields (`VAPIDPrivateKey`, `Endpoint`, `P256dh`, `Auth`) +plus `GenerateVAPIDKeyPair`'s returned private key. + +- `VAPIDPrivateKey`, `P256dh`, `Auth`: **clean.** Every error path that can fire on these fields + (`webpush.go`'s required-field checks, `encrypt.go`'s base64/length validation, + `verifyVAPIDKeyPairMatches`'s decode errors) names the *field*, not the *value* — confirmed by + reading every `fmt.Errorf` call site in `encrypt.go`, `vapid.go`, and `webpush.go`. No `t.Logf`/ + `fmt.Print*`/`log.*` call anywhere in the package touches a real key/secret value. Doc comments + (`GenerateVAPIDKeyPair`, `VAPIDPrivateKey`) both carry an explicit "don't log this" warning. +- `Endpoint`: **not clean** — see Finding 1 (MEDIUM) and Finding 2 (LOW) below. Both are real, + reproduced defects, not false positives. + +--- + +## 3. Findings + +### Finding 1 (MEDIUM): `buildVAPIDHeader` echoes the raw `Endpoint` — including any embedded +bearer-token-equivalent path segment — into the returned error when the endpoint fails URL parsing + +**Location**: `providers/webpush/vapid.go:59-62` + +```go +parsedEndpoint, err := neturl.Parse(endpoint) +if err != nil { + return "", fmt.Errorf("webpush: parse endpoint for VAPID audience: %w", err) +} +``` + +**Reproduced** (not a guess — ran directly against the package): + +```go +secretEndpoint := "https://fcm.googleapis.com/fcm/send/SECRET-BEARER-TOKEN-1234\x7f" +_, err := buildVAPIDHeader(pub, priv, "mailto:ops@example.com", secretEndpoint) +// err.Error() == `webpush: parse endpoint for VAPID audience: parse "https://fcm.googleapis.com/fcm/send/SECRET-BEARER-TOKEN-1234\x7f": net/url: invalid control character in URL` +``` + +`net/url.Parse`'s own error text embeds the full raw input string it failed to parse. Because +`vapid.go` wraps that error with `%w` instead of a generic message, a malformed `Endpoint` — +itself the "credential-equivalent" field this review was asked to scrutinize (per this task's own +framing: FCM/etc. endpoints commonly carry a bearer-token-equivalent path segment) — ends up +verbatim inside the error `Send` returns to the caller, which a host application is very likely to +log (see Finding 2, which shows the module's own recommended example code doing exactly that). + +**Why this matters despite the narrow trigger condition**: the trigger isn't attacker-controlled in +a typical deployment (a host's own stored `Endpoint` would have to become malformed — e.g. DB +corruption, a bad copy-paste, or a buggy upstream `PushSubscription` feed), so this isn't a remote +exploit. But it's a real gap relative to this module's own established convention: +`transport/wrapper.go`'s own destination-URL-parse-failure path (`buildSafeRequestURL`, around +line 267-270) deliberately does **not** wrap the underlying `net/url` error — it returns a generic +`"destination URL validation failed"` specifically to avoid this class of leak. `vapid.go`'s +endpoint-parse error is inconsistent with that precedent inside the same module. + +**Remediation** (concrete, minimal): + +```go +parsedEndpoint, err := neturl.Parse(endpoint) +if err != nil { + return "", fmt.Errorf("webpush: endpoint is not a valid URL") +} +``` + +(Matches `transport/wrapper.go`'s own generic-error convention for this exact class of failure.) + +**Severity**: MEDIUM. Not remotely exploitable by a third party in the normal flow, but a genuine, +reproduced instance of exactly the credential-echo pattern this review was asked to hunt for, with +a trivial fix and clear in-module precedent for the correct behavior. + +### Finding 2 (LOW): `docs/INTEGRATION.md`'s recommended webpush example logs the full subscriber +`Endpoint` on every send failure + +**Location**: `docs/INTEGRATION.md:181-183` + +```go +if err := sender.Send(ctx, msg); err != nil { + log.Printf("webpush to %s failed: %v", sub.Endpoint, err) +} +``` + +This is the module's own documented "here's how to fan a message out to every subscriber" example +— the pattern a host application is expected to copy. Per this task's framing, `Endpoint` "often +embeds a bearer-token-equivalent path segment for push services like FCM," so the sanctioned +example teaches host authors to put that value in their own logs on every failed delivery +(including the common case where `Send` fails downstream in `transport.Wrapper` for an entirely +unrelated reason, e.g. a transient 503). + +**Severity**: LOW. This is documentation/example code, not a runtime code path in the library +itself — it doesn't cause `go_notify_yourself` to leak anything on its own. But it's still exactly +the "example code" surface this review was asked to check, and it actively steers a host toward +logging a credential-equivalent value, which running `go vet`/`staticcheck`/tests cannot catch +since it's prose, not compiled code. + +**Remediation** (concrete): log a non-secret identifier instead of the raw endpoint — e.g. a +subscriber ID the host already tracks, or at most the endpoint's *host* (`neturl.Parse(sub.Endpoint).Host`, +which is stable across subscriptions to the same push service and carries no token): + +```go +if err := sender.Send(ctx, msg); err != nil { + log.Printf("webpush to subscriber %s failed: %v", sub.ID, err) // sub.ID, not sub.Endpoint +} +``` + +--- + +## 4. Coverage detail (packages touched by this feature) + +| Package | Coverage | Notes | +|---|---|---| +| `providers/webpush` | 89.6% | See below — remaining gap is unexported crypto-library error branches, not untested product behavior | +| `transport` | 94.2% | Unchanged by this feature except the additive header-allowlist entries, which are covered | +| `providers/internal/regconfig` | 100.0% | `IntField`'s 5 shape cases (missing key, wrong type, `int`, `int64`, `float64`) all present | +| `providers/all` | N/A — no statements of its own | The file is a pure blank-import list; its *effect* (webpush registered, count = 9) is exercised by `providers/all/all_test.go`, which passes | + +`providers/webpush`'s remaining uncovered branches, function by function (`go tool cover -func` +after this review's added tests): + +- `encryptAES128GCM` 71.4%, `GenerateVAPIDKeyPair` 70.0%, `mustMarshalJSON` 75.0%: the uncovered + lines are exclusively `if err != nil` branches immediately following + `ecdh.P256().GenerateKey(rand.Reader)`, `rand.Read(...)`, `ecdsa.GenerateKey(...)`, + `priv.Bytes()`/`priv.PublicKey.Bytes()`, and (for `mustMarshalJSON`) `json.Marshal` of a + fixed, hardcoded `map[string]string` literal. None of these are reachable without fault-injecting + `crypto/rand` itself or making `encoding/json` fail on a value that cannot fail to marshal — this + module has (correctly, per its dependency-free/no-extra-DI-seam design) no injectable RNG seam, + and adding one solely to hit these lines would be test-driven production-code complexity, not a + real coverage improvement. Consistent with CLAUDE.md's "coverage should not need artificial + padding tests" guidance from the spec's own Phase 3 notes. +- `verifyVAPIDKeyPairMatches` 86.7%, `encryptAES128GCMWithKeys` 87.8%: improved this session (see + §5) by adding tests for the base64-*decode-error* branches (invalid characters), which are + distinct from and previously not covered by the existing wrong-*length*-after-decode tests. The + remaining gap in these two functions is the same class of practically-unreachable + `priv.PublicKey.Bytes()`/HKDF/AES-construction error branches as above. +- `buildVAPIDHeader` 91.9%, `Send` 96.4%: high coverage; remaining gaps are the same class of + effectively-infallible stdlib error branches (`json.Marshal` of a small fixed struct, + `ecdsa.Sign` failure). + +**Assessment**: the aggregate 89.6% (package) / 94.6% (repo-wide) figures are not inflated by +avoiding hard cases — §3.8's full error-handling table (missing fields, malformed VAPID subject +prefix, mismatched keypair, invalid urgency/topic, oversized plaintext, non-JSON custom template, +wrapper/4xx/5xx propagation) is each backed by its own passing test in `webpush_test.go`. The +uncovered remainder is genuinely-infeasible-without-fault-injection stdlib error handling, which is +the correct and expected shape for well-tested Go crypto code — not a coverage gap that should +block this gate. + +--- + +## 5. Tests added by this QA pass + +Five new tests added to close real (non-fault-injection) coverage gaps identified during this +review — all pass, none touch production code: + +- `providers/webpush/encrypt_test.go`: + `TestEncryptAES128GCMWithKeys_RejectsInvalidBase64P256dh`, + `TestEncryptAES128GCMWithKeys_RejectsInvalidBase64Auth` — invalid-base64 (not merely + wrong-length-after-decode) `p256dh`/`auth` values, distinct branches from the existing malformed + tests. +- `providers/webpush/webpush_test.go`: + `TestClientSend_RejectsMalformedVAPIDPrivateKeyBase64`, + `TestClientSend_RejectsMalformedVAPIDPublicKeyBase64` — malformed VAPID keys reached through the + real `Client.Send` path (`verifyVAPIDKeyPairMatches`), not only through the lower-level + `buildVAPIDHeader` seam `vapid_test.go` already exercised. + +Net effect: `providers/webpush` package coverage 87.7% → 89.6%. Committed separately (see below); +`docs/reports/qa_report.md` is committed alongside it. + +**Note**: Findings 1 and 2 above are **not** fixed by this QA pass — per this agent's role +(testing + vulnerability assessment, reporting actionable findings), production-code and +documentation fixes are left for a follow-up commit rather than made unilaterally outside the +plan → implement → review pipeline this feature went through. Both have concrete, minimal +remediations included above and should be applied before this release ships. + +--- + +## 6. CodeQL caveat (per CLAUDE.md, not this feature's fault) + +Per CLAUDE.md's documented known gap: this repo's `go.mod` directive is `go 1.27.1`, and as of this +review CodeQL's bundled Go extractor still trails that version, so a green CodeQL run on this +feature's commits is **not** independently trustworthy evidence of a clean scan — it likely means +"0 findings because extraction failed," not "0 findings because the scan ran and found nothing." +This report's SSRF/crypto/credential-handling conclusions above come from direct code +review and reproduced tests in this session, not from CodeQL. Per CLAUDE.md, confirming real +CodeQL coverage requires checking the `autobuild` step's log for `requires newer Go version` on +this feature's CI run — not done as part of this local review (no CI run was triggered by this +session), flagged here so the maintainer checks it before treating CodeQL as having covered this +feature. + +--- + +## 7. Acceptance criteria cross-check (spec §7) + +| # | Criterion | Status | +|---|---|---| +| 1 | `go build ./...` succeeds at every commit | Verified at HEAD; per-commit bisectability not individually re-verified (would require checking out each of the 7 commits) | +| 2 | `go vet`/`staticcheck` clean at every commit | Verified at HEAD | +| 3 | `go test ./...` passes, coverage ≥85% for touched packages | PASS — see §4 | +| 4 | New/changed exported identifiers have doc comments | Verified by direct read of `webpush.go`, `vapid.go`, `encrypt.go` (only `MaxPlaintextSize`, `Config`+fields, `Client`, `New`, `Send`, `DefaultTTL`, `GenerateVAPIDKeyPair` are exported — all documented) | +| 5 | No `Wikid82/charon` import | PASS — 0 hits, confirmed | +| 6 | RFC 8291 fixed-vector test, exact-byte | PASS | +| 7 | `providers/all` `wantProviderCount` bumped, test passes | PASS (9) | +| 8 | README/INTEGRATION/ARCHITECTURE updated | PASS — present, though INTEGRATION.md's example has Finding 2 above | +| 9 | No provider other than `webpush` added | PASS — confirmed via diff scope | +| 10 | `notify.New("webpush", ...)` and `webpush.New(...)` both exercised, behaviorally equivalent | PASS — `register_test.go`'s `TestRegister_NewReturnsWorkingSender` round-trips config through both paths and asserts equality | + +--- + +## 8. Summary for the maintainer + +Ship-blocking gates (build/vet/staticcheck/test/coverage/no-Charon-import) are all green. The +feature's core correctness claim — the RFC 8291 exact-byte vector — holds. Two real but +narrow-trigger findings around `Endpoint` handling (MEDIUM: raw endpoint echoed into an error on +malformed-URL input; LOW: the docs example logs the raw endpoint on any send failure) should be +fixed before release; both have a one-line remediation included above. Neither affects the +happy-path or the common 4xx/5xx-from-push-service path already covered by tests. diff --git a/providers/webpush/encrypt_test.go b/providers/webpush/encrypt_test.go index 3a842e9..8864438 100644 --- a/providers/webpush/encrypt_test.go +++ b/providers/webpush/encrypt_test.go @@ -117,6 +117,23 @@ func TestEncryptAES128GCMWithKeys_RejectsMalformedP256dh(t *testing.T) { } } +func TestEncryptAES128GCMWithKeys_RejectsInvalidBase64P256dh(t *testing.T) { + ephemeral, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + salt := make([]byte, 16) + auth := base64.RawURLEncoding.EncodeToString(make([]byte, 16)) + + // "!!!" is not valid base64url, so this exercises the decode-error + // branch distinctly from the wrong-length-after-decode branch covered + // by TestEncryptAES128GCMWithKeys_RejectsMalformedP256dh. + _, err = encryptAES128GCMWithKeys(ephemeral, salt, "not valid base64!!!", auth, []byte("hello")) + if err == nil || !strings.Contains(err.Error(), "p256dh") || !strings.Contains(err.Error(), "base64") { + t.Fatalf("expected p256dh base64 decode error, got: %v", err) + } +} + func TestEncryptAES128GCMWithKeys_RejectsMalformedAuth(t *testing.T) { ephemeral, err := ecdh.P256().GenerateKey(rand.Reader) if err != nil { @@ -137,6 +154,27 @@ func TestEncryptAES128GCMWithKeys_RejectsMalformedAuth(t *testing.T) { } } +func TestEncryptAES128GCMWithKeys_RejectsInvalidBase64Auth(t *testing.T) { + ephemeral, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + receiver, err := ecdh.P256().GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("unexpected keygen error: %v", err) + } + salt := make([]byte, 16) + p256dh := base64.RawURLEncoding.EncodeToString(receiver.PublicKey().Bytes()) + + // "!!!" is not valid base64url, so this exercises the decode-error + // branch distinctly from the wrong-length-after-decode branch covered + // by TestEncryptAES128GCMWithKeys_RejectsMalformedAuth. + _, err = encryptAES128GCMWithKeys(ephemeral, salt, p256dh, "not valid base64!!!", []byte("hello")) + if err == nil || !strings.Contains(err.Error(), "auth") || !strings.Contains(err.Error(), "base64") { + t.Fatalf("expected auth base64 decode error, got: %v", err) + } +} + func TestEncryptAES128GCMWithKeys_RejectsOversizedPlaintext(t *testing.T) { ephemeral, err := ecdh.P256().GenerateKey(rand.Reader) if err != nil { diff --git a/providers/webpush/webpush_test.go b/providers/webpush/webpush_test.go index ecf7976..7a130a9 100644 --- a/providers/webpush/webpush_test.go +++ b/providers/webpush/webpush_test.go @@ -199,6 +199,30 @@ func TestClientSend_RejectsMismatchedVAPIDKeyPair(t *testing.T) { } } +func TestClientSend_RejectsMalformedVAPIDPrivateKeyBase64(t *testing.T) { + cfg := validConfig(t) + cfg.VAPIDPrivateKey = "not valid base64!!!" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "VAPID private key") { + t.Fatalf("expected VAPID private key decode error, got: %v", err) + } +} + +func TestClientSend_RejectsMalformedVAPIDPublicKeyBase64(t *testing.T) { + cfg := validConfig(t) + cfg.VAPIDPublicKey = "not valid base64!!!" + rt := &capturingRoundTripper{} + client := New(cfg, newTestWrapper(rt)) + + err := client.Send(context.Background(), notify.Message{Body: "x"}) + if err == nil || !strings.Contains(err.Error(), "VAPID public key") { + t.Fatalf("expected VAPID public key decode error, got: %v", err) + } +} + func TestClientSend_RejectsInvalidUrgency(t *testing.T) { cfg := validConfig(t) cfg.Urgency = "extremely-urgent" From 8f71f4b256ec803a4128a21e4b235dc69e214c33 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:43:28 +0000 Subject: [PATCH 11/12] fix(webpush): don't echo raw endpoint in VAPID audience-parse error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildVAPIDHeader wrapped net/url.Parse's error with %w, which embeds the full raw input — leaking a malformed Endpoint's bearer-token-equivalent path segment (e.g. FCM) into Send's returned error. Return a generic message instead, matching transport/wrapper.go's buildSafeRequestURL convention for the same class of failure. Fixes QA report Finding 1 (MEDIUM, docs/reports/qa_report.md). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- providers/webpush/vapid.go | 7 ++++++- providers/webpush/vapid_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/providers/webpush/vapid.go b/providers/webpush/vapid.go index d2ffd03..b37099c 100644 --- a/providers/webpush/vapid.go +++ b/providers/webpush/vapid.go @@ -58,7 +58,12 @@ func buildVAPIDHeader(vapidPublicKey, vapidPrivateKey, vapidSubject, endpoint st parsedEndpoint, err := neturl.Parse(endpoint) if err != nil { - return "", fmt.Errorf("webpush: parse endpoint for VAPID audience: %w", err) + // Deliberately not %w-wrapped: net/url's own parse error embeds the + // full raw input, and endpoint (a PushSubscription.Endpoint) often + // carries a bearer-token-equivalent path segment for push services + // like FCM. Matches transport/wrapper.go's buildSafeRequestURL + // convention for the same class of failure (QA report Finding 1). + return "", fmt.Errorf("webpush: endpoint is not a valid URL") } aud := parsedEndpoint.Scheme + "://" + parsedEndpoint.Host diff --git a/providers/webpush/vapid_test.go b/providers/webpush/vapid_test.go index 61cb998..38869de 100644 --- a/providers/webpush/vapid_test.go +++ b/providers/webpush/vapid_test.go @@ -176,3 +176,33 @@ func TestBuildVAPIDHeader_RejectsInvalidEndpoint(t *testing.T) { t.Fatal("expected an error for an unparsable endpoint") } } + +// TestBuildVAPIDHeader_MalformedEndpointErrorDoesNotLeakRawEndpoint guards +// against QA report Finding 1 (MEDIUM, docs/reports/qa_report.md): when +// endpoint fails net/url.Parse, the returned error must not echo the raw +// endpoint string back to the caller — net/url's own parse error embeds its +// full input, and a real PushSubscription.Endpoint commonly carries a +// bearer-token-equivalent path segment (e.g. FCM's send endpoint), so +// wrapping that error with %w leaks it into logs/error text. +func TestBuildVAPIDHeader_MalformedEndpointErrorDoesNotLeakRawEndpoint(t *testing.T) { + pubB64, privB64, err := GenerateVAPIDKeyPair() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + secretEndpoint := "https://fcm.googleapis.com/fcm/send/SECRET-BEARER-TOKEN-1234\x7f" + _, err = buildVAPIDHeader(pubB64, privB64, "mailto:ops@example.com", secretEndpoint) + if err == nil { + t.Fatal("expected an error for an unparsable endpoint") + } + + if strings.Contains(err.Error(), "SECRET-BEARER-TOKEN") { + t.Fatalf("error leaks the raw endpoint (credential-equivalent value): %v", err) + } + if strings.Contains(err.Error(), secretEndpoint) { + t.Fatalf("error leaks the raw endpoint string: %v", err) + } + if !strings.Contains(err.Error(), "not a valid URL") { + t.Fatalf("expected a generic 'not a valid URL' error, got: %v", err) + } +} From 95f67e6f1f159594f620c36cba2d2c87e02304d6 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 14 Sep 2026 17:44:52 +0000 Subject: [PATCH 12/12] docs(webpush): stop logging raw subscription endpoint in integration example MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes QA report Finding 2 (LOW): docs/INTEGRATION.md's webpush fan-out example logged sub.Endpoint verbatim on every Send failure, which for push services like FCM commonly embeds a bearer-token-equivalent path segment; it now logs only the endpoint's host via neturl.Parse. Also fixes stale provider-count/list omissions of webpush found during the final docs pass (README.md intro, ARCHITECTURE.md's providers/* list and §3.8, docs/INTEGRATION.md's "eight providers" bullets) so all three docs correctly reflect nine built-in providers including webpush. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012zpLzMxyw465hYA4WjoQVW --- ARCHITECTURE.md | 6 +++--- README.md | 2 +- docs/INTEGRATION.md | 15 +++++++++++---- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index b454f81..8bf6a0d 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -17,8 +17,8 @@ It has four layers: HTTP-based provider dispatches through: destination validation, retry/backoff, redirect re-validation, and request/response size caps. 3. **`providers/*`** — one package per notification service (`discord`, `slack`, `gotify`, - `pushover`, `ntfy`, `telegram`, `webhook`, `email`), each exposing a typed `Config` struct, a - `New(...)` constructor, and a `Client` implementing `notify.Sender`. + `pushover`, `ntfy`, `telegram`, `webhook`, `email`, `webpush`), each exposing a typed `Config` + struct, a `New(...)` constructor, and a `Client` implementing `notify.Sender`. 4. **`providers/all`** — a blank-import bundle that registers every built-in provider package with the root registry in one line, for consumers who want zero-touch discovery. @@ -189,7 +189,7 @@ preventing an easy, avoidable one. ### 3.8 Test expectations -Every provider package's tests should cover, mirroring the existing eight providers' patterns: +Every provider package's tests should cover, mirroring the existing nine providers' patterns: - **Table-driven `Send` tests** against a fake `transport.Wrapper`, built via an injected `ClientFactory` returning a `capturingRoundTripper` (see any `providers/*/*_test.go` for the diff --git a/README.md b/README.md index 1ff1877..7b5daf8 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ A standalone, dependency-free Go module for notification delivery: SSRF-safe outbound HTTP dispatch with retries, and a common `Sender` interface across Discord, Slack, Gotify, Pushover, -Ntfy, Telegram, generic webhooks, and email. +Ntfy, Telegram, generic webhooks, email, and direct browser Web Push. ``` go get github.com/Wikid82/go_notify_yourself diff --git a/docs/INTEGRATION.md b/docs/INTEGRATION.md index 66d7c83..211d86b 100644 --- a/docs/INTEGRATION.md +++ b/docs/INTEGRATION.md @@ -23,8 +23,8 @@ re-implementing the same things badly. - SSRF-safe outbound HTTP dispatch with retry/backoff (`transport.Wrapper`) — destination validation, redirect re-validation, request/response size caps. -- A uniform `Sender` interface across eight built-in provider types: Discord, Slack, Gotify, - Pushover, Ntfy, Telegram, generic webhook, and email. +- A uniform `Sender` interface across nine built-in provider types: Discord, Slack, Gotify, + Pushover, Ntfy, Telegram, generic webhook, email, and direct browser Web Push. - JSON payload templating with a shared `text/template` engine plus a `toJSON` helper. - A self-registering factory/discovery layer (`notify.Register`/`notify.New`/ `notify.RegisteredTypes`) for constructing a `Sender` by name at runtime. @@ -41,7 +41,7 @@ re-implementing the same things badly. **Reach for it if:** - You need two or more of {Discord, Slack, Gotify, Pushover, Ntfy, Telegram, generic webhook, - email} dispatch. + email, webpush} dispatch. - You want retry/backoff and SSRF hardening without writing it yourself. - You're fine supplying your own HTTP client factory / SSRF policy / SMTP mailer via the module's dependency-injection seams (see "Why it's built this way" below). @@ -179,7 +179,14 @@ for _, sub := range subscriptions { // e.g. loaded from your own storage Auth: sub.Auth, }, wrapper) if err := sender.Send(ctx, msg); err != nil { - log.Printf("webpush to %s failed: %v", sub.Endpoint, err) + // Log the endpoint's host, not the full endpoint: for push services like + // FCM, sub.Endpoint's path commonly embeds a bearer-token-equivalent + // segment that shouldn't end up in your logs. + host := "unknown" + if u, parseErr := neturl.Parse(sub.Endpoint); parseErr == nil { + host = u.Host + } + log.Printf("webpush to %s failed: %v", host, err) } } ```