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/ARCHITECTURE.md b/ARCHITECTURE.md index 1cb5ebb..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. @@ -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 @@ -184,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/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/README.md b/README.md index ad04e66..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 @@ -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..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). @@ -162,6 +162,39 @@ 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 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) + } +} +``` + +`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 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). 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/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/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/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 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..8864438 --- /dev/null +++ b/providers/webpush/encrypt_test.go @@ -0,0 +1,311 @@ +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_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 { + 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_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 { + 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) + } +} 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()) + } +} diff --git a/providers/webpush/vapid.go b/providers/webpush/vapid.go new file mode 100644 index 0000000..b37099c --- /dev/null +++ b/providers/webpush/vapid.go @@ -0,0 +1,127 @@ +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 { + // 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 + + 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..38869de --- /dev/null +++ b/providers/webpush/vapid_test.go @@ -0,0 +1,208 @@ +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") + } +} + +// 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) + } +} 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..7a130a9 --- /dev/null +++ b/providers/webpush/webpush_test.go @@ -0,0 +1,387 @@ +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_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" + 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) + } +} 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" ./... 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)