diff --git a/.github/workflows/protect-changelog.yml b/.github/workflows/protect-changelog.yml index 01bd74bd..99d91d13 100644 --- a/.github/workflows/protect-changelog.yml +++ b/.github/workflows/protect-changelog.yml @@ -25,3 +25,21 @@ jobs: echo "It is auto-generated by git-cliff on release." echo "Remove the CHANGELOG.md change from this PR." exit 1 + + # Guard against changelog links drifting to a defunct GitHub org after a + # repo transfer. The canonical remote is solutions-plug/predictIQ; any + # other org (e.g. the legacy popsman01) must never appear in generated + # compare/commit/issue links. See cliff.toml for the git-cliff remote + # configuration that produces these links. + - name: Fail if CHANGELOG.md links point to a non-canonical org + if: | + !(github.actor == 'github-actions[bot]' && + startsWith(github.head_ref, 'changelog-update/')) + run: | + if grep -nE 'github\.com/(popsman01)/predictIQ' CHANGELOG.md; then + echo "❌ CHANGELOG.md contains links to a defunct GitHub org." + echo "Expected org: solutions-plug (see cliff.toml remote config)." + echo "Regenerate the changelog with git-cliff so links use the canonical remote." + exit 1 + fi + echo "✅ CHANGELOG.md links use the canonical org." diff --git a/API_SPEC.md b/API_SPEC.md index be8f7923..27fb5f72 100644 --- a/API_SPEC.md +++ b/API_SPEC.md @@ -23,6 +23,9 @@ Clients should monitor these headers and migrate before the sunset date. Deprecated versions are supported for a minimum of **12 months** after the deprecation announcement before being removed. +For the authoritative versioning and deprecation policy — including current sunset +dates and the version roadmap — see [`docs/api-versioning.md`](docs/api-versioning.md). + ## Table of Contents - [Overview](#overview) @@ -40,25 +43,6 @@ announcement before being removed. http://0.0.0.0:8080 ``` -### API Versioning - -The API uses URL path versioning (`/api/v1/`). The current stable version is **v1**. - -Clients may also send an `API-Version` header (e.g. `API-Version: v1`) to explicitly -declare the version they target. If omitted, the server defaults to the current version. - -### Deprecation Policy - -When a version is deprecated: -- Responses will include a `Deprecation` header set to `true`. -- A `Sunset` header will indicate the date after which the version will be removed. -- A `Link` header will point to migration documentation. - -Clients should monitor these headers and migrate before the sunset date. - -Deprecated versions are supported for a minimum of **12 months** after the deprecation -announcement before being removed. - ## Authentication The API uses Bearer token authentication. Include your API key in the `Authorization` header: diff --git a/IMPLEMENTATION_SUMMARY.md b/IMPLEMENTATION_SUMMARY.md deleted file mode 100644 index e61c2cc6..00000000 --- a/IMPLEMENTATION_SUMMARY.md +++ /dev/null @@ -1,212 +0,0 @@ -# Implementation Summary: Backend Issues #470-#473 - -This document summarizes the implementation of four backend issues to improve security, reliability, and data quality. - -## Issue #470: Separate webhook routing security model from admin routes - -**Status**: ✅ COMPLETED - -### Changes Made - -1. **Updated [services/api/src/main.rs](services/api/src/main.rs#L230-L248)** - - Added comprehensive documentation comment explaining webhook security model - - Separated webhook routes from admin routes with distinct middleware stacks - - Webhook middleware order: validation → security headers → provider signature → tracing - - Admin middleware includes API key auth and IP whitelist (webhooks don't) - -2. **Enhanced [services/api/openapi.yaml](services/api/openapi.yaml)** - - Expanded webhook endpoint documentation - - Clearly documented provider-signed security model vs API key admin model - - Added explanation of why webhooks use different security approach - - Documented header requirements and response format - -### Acceptance Criteria Met -- ✅ Webhook route has dedicated middleware stack -- ✅ Admin auth is not required for valid provider events -- ✅ Route policy is documented in OpenAPI -- ✅ Security models are clearly separated in code and comments - ---- - -## Issue #473: Remove memory leak pattern in analytics rate limiter keying - -**Status**: ✅ COMPLETED - -### Changes Made - -1. **Fixed [services/api/src/security.rs](services/api/src/security.rs)** - - Added missing `pub struct RateLimiter` definition (was referencing a field without defining the struct) - - Added documentation explaining the RateLimiter structure and no-leak guarantee - - All dynamic strings use proper `String` ownership, no `Box::leak()` magic - -2. **Enhanced [services/api/src/rate_limit.rs](services/api/src/rate_limit.rs)** - - Added comprehensive documentation for `analytics_rate_limit_middleware` - - Documented key generation strategy with explicit "no Box::leak" guarantee - - All paths use owned Strings: IP fallback is properly owned, not static - -3. **Added Tests in [services/api/src/security.rs](services/api/src/security.rs)** - - `rate_limiter_allows_under_limit()` - verifies limits work - - `rate_limiter_blocks_over_limit()` - confirms blocking - - `rate_limiter_separate_buckets_per_key()` - ensures isolation - - `rate_limiter_resets_window_after_expiry()` - validates window reset - - `rate_limiter_cleanup_removes_expired_entries()` - tests cleanup - - `analytics_rate_limiter_no_allocations_leaked()` - documents no-leak design - -### Acceptance Criteria Met -- ✅ No leaked allocations for per-request key generation (String ownership) -- ✅ Behavior remains equivalent -- ✅ Tests demonstrate correct behavior without leaks -- ✅ Documentation proves no Box::leak usage - ---- - -## Issue #471: Store recipient when logging sent email events - -**Status**: ✅ COMPLETED - -### Changes Made - -1. **Enhanced [services/api/src/email/queue.rs](services/api/src/email/queue.rs)** - - Updated `mark_completed()` with comprehensive documentation for PII handling - - Improved error handling: explicit warning logs when recipient lookup fails - - Better fallback behavior with proper logging instead of silent .unwrap_or_default() - -2. **Enhanced [services/api/src/db.rs](services/api/src/db.rs)** - - Added documentation to `email_create_event()` explaining PII considerations - - Documented storage of recipient email in email_events table - - Provided guidance on analytics queries needing PII protection - -3. **Added Tests in [services/api/src/email/queue.rs](services/api/src/email/queue.rs)** - - `test_mark_completed_stores_recipient()` - documents expected behavior - - Test verifies recipient email is stored, not empty - - Describes analytics query expectations - -### Acceptance Criteria Met -- ✅ Event records include real recipient address (not empty) -- ✅ Existing analytics queries still work (recipient field populated) -- ✅ Regression test is added (test_mark_completed_stores_recipient) -- ✅ PII handling is documented in both queue.rs and db.rs - ---- - -## Issue #472: Recover orphaned processing jobs on startup - -**Status**: ✅ COMPLETED - -### Changes Made - -1. **Added Configuration in [services/api/src/config.rs](services/api/src/config.rs)** - - New field: `email_stale_job_threshold_secs` (default: 3600 seconds = 1 hour) - - Configurable via `EMAIL_STALE_JOB_THRESHOLD_SECS` environment variable - - Initialization in `Config::from_env()` with proper default - -2. **Enhanced [services/api/src/email/queue.rs](services/api/src/email/queue.rs)** - - Changed `EMAIL_PROCESSING_KEY` from set to **sorted set** with timestamps - - Added `DEFAULT_STALE_JOB_THRESHOLD_SECS` constant - - Updated `dequeue()` to store timestamp when jobs enter processing - - Enhanced `recover_orphaned_jobs()` to: - - Accept configurable stale threshold parameter - - Use `zrangebyscore` to find only truly stale jobs - - Cross-check age before recovery - - Provide clear logging with count and threshold - - Updated `mark_completed()`, `mark_failed()`, `get_stats()`, `get_processing_count()` to use sorted set operations (zrem, zcard instead of srem, scard) - - Updated `start_worker()` to accept and pass `stale_job_threshold_secs` - -3. **Updated [services/api/src/main.rs](services/api/src/main.rs)** - - Passes `state.config.email_stale_job_threshold_secs` to email queue worker - - Worker now calls `recover_orphaned_jobs(stale_threshold)` on startup - -4. **Added Tests in [services/api/src/email/queue.rs](services/api/src/email/queue.rs)** - - `test_recover_orphaned_jobs_stale_detection()` - documents stale detection logic - - Explains test scenarios for different thresholds - - Demonstrates idempotent behavior - -### Acceptance Criteria Met -- ✅ Worker startup scans and re-queues stale processing jobs -- ✅ Idempotent behavior guaranteed (sorted set + timestamp-based detection) -- ✅ Recovery scenario test is added -- ✅ Stale job threshold is configurable via environment variable - ---- - -## Key Design Decisions - -### 1. Webhook Security Model Separation -- **Why**: Webhooks are provider-signed (external service), admin routes are user-authenticated -- **How**: Separate middleware stacks, no API key needed for webhooks -- **Benefit**: Clear security boundaries, easier to audit - -### 2. Processing Jobs as Sorted Set with Timestamps -- **Why**: Enables time-based staleness detection without external storage -- **How**: Store processing start timestamp as Redis sorted set score -- **Benefit**: True idempotent recovery, no false positives on slow jobs - -### 3. Recipient Storage with Error Handling -- **Why**: Necessary for analytics but must handle failures gracefully -- **How**: Explicit error logging if lookup fails, don't silently ignore -- **Benefit**: Observable failures, easier debugging - -### 4. Configurable Stale Threshold -- **Why**: Different deployments may have different job processing times -- **How**: Environment variable with sensible default (1 hour) -- **Benefit**: Operational flexibility without code changes - ---- - -## Files Modified - -1. ✅ [services/api/src/main.rs](services/api/src/main.rs) - Router configuration, stale threshold -2. ✅ [services/api/src/config.rs](services/api/src/config.rs) - Email stale threshold config -3. ✅ [services/api/src/rate_limit.rs](services/api/src/rate_limit.rs) - Rate limiter docs -4. ✅ [services/api/src/security.rs](services/api/src/security.rs) - RateLimiter struct definition, tests -5. ✅ [services/api/src/email/queue.rs](services/api/src/email/queue.rs) - Orphan recovery, recipient storage, tests -6. ✅ [services/api/src/db.rs](services/api/src/db.rs) - PII handling documentation -7. ✅ [services/api/openapi.yaml](services/api/openapi.yaml) - Webhook security documentation - ---- - -## Testing - -All changes include: -- Unit tests for rate limiter behavior -- Documentation of integration test scenarios -- Test coverage for idempotent recovery behavior -- Documentation of PII handling expectations - -Run tests with: -```bash -cargo test -p predictiq-api -``` - ---- - -## Deployment Notes - -### New Environment Variables -- `EMAIL_STALE_JOB_THRESHOLD_SECS` - Configures orphan recovery sensitivity (default: 3600) - -### Breaking Changes -- **None** - all changes are backward compatible - -### Performance Impact -- Minimal - recovery runs once on startup -- Cleanup task still runs every 300s (unchanged) -- Sorted set operations (zrem, zadd) are O(log N), same as set operations - -### Migration Notes -- No database migrations needed -- Redis sorted set automatically created on first use -- Old jobs in set operations will be re-added to sorted set on next startup - ---- - -## Summary - -All four issues have been successfully implemented with: -- ✅ Clear separation of concerns (webhook vs admin security) -- ✅ No memory leaks (proper String ownership) -- ✅ Complete recipient tracking (PII protected) -- ✅ Robust orphan recovery (idempotent, configurable) -- ✅ Comprehensive tests and documentation - -Ready for pull request and deployment. diff --git a/cliff.toml b/cliff.toml index 7f5e37e1..1c4b85d9 100644 --- a/cliff.toml +++ b/cliff.toml @@ -1,3 +1,14 @@ +# Changelog configuration for git-cliff. +# +# IMPORTANT: The canonical repository is https://github.com/solutions-plug/predictIQ +# The `remote` below MUST point at the `solutions-plug` org. If the repo is ever +# transferred again, update this value (and regenerate CHANGELOG.md) so that +# compare/commit/issue links do not silently drift to a defunct org. + +[remote.github] +owner = "solutions-plug" +repo = "predictIQ" + [changelog] header = """ # Changelog diff --git a/docs/api-versioning.md b/docs/api-versioning.md index 8eac625a..43a36efc 100644 --- a/docs/api-versioning.md +++ b/docs/api-versioning.md @@ -94,3 +94,7 @@ Unsupported versions are ignored and the server falls back to the default versio - `services/api/src/versioning.rs` — sunset header injection middleware - `API_SPEC.md` — full endpoint reference + +--- + +> **Single source of truth:** This document is the canonical reference for API versioning, sunset dates, and the v2 roadmap. `API_SPEC.md` links here rather than restating version status or sunset dates, so update versioning details only in this file. diff --git a/scripts/generate-api-spec.js b/scripts/generate-api-spec.js index 9c3bd51c..1e34654c 100755 --- a/scripts/generate-api-spec.js +++ b/scripts/generate-api-spec.js @@ -78,11 +78,98 @@ function extractEndpoints(doc) { return endpoints; } +/** + * Resolve a `$ref` pointer (e.g. `#/components/parameters/Limit`) against the + * parsed OpenAPI document. Returns the referenced object, or null when the + * pointer cannot be resolved. + */ +function resolveRef(doc, ref) { + if (typeof ref !== 'string' || !ref.startsWith('#/')) return null; + const segments = ref.slice(2).split('/').map(s => s.replace(/~1/g, '/').replace(/~0/g, '~')); + let node = doc; + for (const segment of segments) { + if (!node || typeof node !== 'object') return null; + node = node[segment]; + } + return node && typeof node === 'object' ? node : null; +} + +/** + * Collect pagination parameters declared in the OpenAPI spec. + * + * Parameters are gathered from `components.parameters` (the canonical place + * for reusable pagination params) and from any operation-level `parameters` + * entries whose name is one of the pagination fields. `$ref` entries are + * resolved so the documented defaults/maximums stay in sync with the spec. + */ +function extractPaginationParams(doc) { + const PAGINATION_NAMES = ['limit', 'offset', 'cursor']; + const seen = new Map(); + + const addParam = (param) => { + if (!param || typeof param !== 'object') return; + const name = param.name; + if (!PAGINATION_NAMES.includes(name)) return; + if (!seen.has(name)) seen.set(name, param); + }; + + const components = (doc && doc.components) || {}; + const componentParams = components.parameters || {}; + for (const param of Object.values(componentParams)) { + addParam(param); + } + + const paths = (doc && doc.paths) || {}; + for (const pathItem of Object.values(paths)) { + if (!pathItem || typeof pathItem !== 'object') continue; + for (const method of HTTP_METHODS) { + const op = pathItem[method]; + if (!op || !Array.isArray(op.parameters)) continue; + for (const raw of op.parameters) { + const param = raw && raw.$ref ? resolveRef(doc, raw.$ref) : raw; + addParam(param); + } + } + } + + // Preserve a stable, documented ordering. + return PAGINATION_NAMES.map(name => seen.get(name)).filter(Boolean); +} + +/** + * Render the Pagination section from the parameters declared in the spec. + * Returns an empty string when the spec declares no pagination parameters so + * the generated docs never claim pagination support that does not exist. + */ +function renderPaginationSection(doc) { + const params = extractPaginationParams(doc); + if (params.length === 0) return ''; + + let md = `## Pagination\n\nList endpoints support pagination via the following query parameters:\n\n`; + md += `| Parameter | Type | Default | Maximum | Description |\n`; + md += `|-----------|------|---------|---------|-------------|\n`; + + params.forEach(param => { + const schema = param.schema || {}; + const type = schema.type || 'string'; + const def = schema.default !== undefined ? String(schema.default) : '—'; + const max = schema.maximum !== undefined ? String(schema.maximum) : '—'; + const description = (param.description || '').replace(/\s+/g, ' ').trim() || '—'; + md += `| \`${param.name}\` | ${type} | ${def} | ${max} | ${description} |\n`; + }); + + md += `\nResponses that return collections include pagination metadata so clients can\n`; + md += `page through results without guessing at the total size.\n\n`; + + return md; +} + /** * Generate markdown from OpenAPI spec */ function generateMarkdown(spec) { const endpoints = extractEndpoints(spec.doc); + const paginationSection = renderPaginationSection(spec.doc); let md = `# ${spec.title} - API Specification @@ -97,7 +184,7 @@ ${spec.description} - [Endpoints](#endpoints) - [Error Handling](#error-handling) - [Rate Limiting](#rate-limiting) - +${paginationSection ? '- [Pagination](#pagination)\n' : ''} ## Overview ### Base URL @@ -193,7 +280,13 @@ The API implements rate limiting to ensure fair usage: When rate limited (HTTP 429), the response includes a \`Retry-After\` header indicating how many seconds to wait before retrying. ---- +`; + + // Pagination is rendered from the OpenAPI spec so it survives regeneration + // instead of living as a hand-edited section after the generation marker. + md += paginationSection; + + md += `--- **Generated from:** \`services/api/openapi.yaml\` **Last Updated:** ${new Date().toISOString()}