|
| 1 | +# Security Best Practices Review - General Hardening |
| 2 | + |
| 3 | +Date: 2026-05-02 |
| 4 | +Repository: `/Users/toddhebebrand/breeze` |
| 5 | +Reviewer mode: `security-best-practices` |
| 6 | + |
| 7 | +## Executive Summary |
| 8 | + |
| 9 | +I reviewed the existing security documentation, prior remediation reports, CI security workflows, and selected high-risk API/web/agent surfaces. The previous canonical report shows the original tracked findings as remediated; this pass found no new critical or high-confidence tenant-isolation bypass. |
| 10 | + |
| 11 | +I did find four actionable hardening items: |
| 12 | + |
| 13 | +- One medium-risk open-redirect/phishing weakness in billing portal return URL handling. |
| 14 | +- One medium-risk push notification token logging issue. |
| 15 | +- One low-risk CI scanner gap where npm audit is currently advisory-only because the pinned pnpm version is known to use a retired advisory endpoint. |
| 16 | +- One low-risk OAuth DCR cleanup gap if dynamic client registration is enabled. |
| 17 | + |
| 18 | +Best next work: fix the two medium findings first because they are small, low-regression changes with clear security value. Then make npm audit blocking again by bumping pnpm and wiring DCR cleanup if MCP OAuth DCR is expected in any production-like environment. |
| 19 | + |
| 20 | +## Method |
| 21 | + |
| 22 | +1. Loaded the project instructions and security-best-practices guidance for TypeScript/React/Node-style web services and Go components. |
| 23 | +2. Reviewed previous security reports and the security policy to avoid re-reporting remediated findings. |
| 24 | +3. Reviewed app bootstrap, CORS/CSP/body-limit/rate-limit posture, CI scanning workflows, OAuth DCR notes, billing portal forwarding, browser auth helpers, and notification senders. |
| 25 | +4. Ran static searches for high-risk sinks: token storage, bearer injection, raw HTML, redirects, outbound fetch, command execution, path joins, secrets, unsafe TODOs, and unauthenticated public surfaces. |
| 26 | + |
| 27 | +Local scanner note: the local shell in this Codex environment does not have `pnpm`, `npm`, `corepack`, or `govulncheck` on `PATH`, so I could not execute a fresh local `pnpm audit` or `govulncheck` run. CI does contain security scanning workflows, reviewed below. |
| 28 | + |
| 29 | +## Medium Findings |
| 30 | + |
| 31 | +### G-001: Billing portal accepts arbitrary return URLs |
| 32 | + |
| 33 | +Rule ID: EXPRESS-REDIRECT-001 |
| 34 | +Severity: Medium |
| 35 | + |
| 36 | +Location: |
| 37 | +- [/Users/toddhebebrand/breeze/apps/api/src/routes/externalServices.ts:82](/Users/toddhebebrand/breeze/apps/api/src/routes/externalServices.ts:82) |
| 38 | +- [/Users/toddhebebrand/breeze/apps/api/src/routes/externalServices.ts:110](/Users/toddhebebrand/breeze/apps/api/src/routes/externalServices.ts:110) |
| 39 | +- [/Users/toddhebebrand/breeze/apps/web/src/components/layout/Header.tsx:68](/Users/toddhebebrand/breeze/apps/web/src/components/layout/Header.tsx:68) |
| 40 | + |
| 41 | +Evidence: |
| 42 | +- The API schema accepts any syntactically valid URL: `returnUrl: z.string().url()`. |
| 43 | +- The validated value is forwarded as `return_url` to the billing service without same-origin or allowlist enforcement. |
| 44 | +- The normal web caller sends `window.location.href`, which is same-origin in the current UI flow, but the server-side endpoint does not enforce that contract. |
| 45 | + |
| 46 | +Impact: |
| 47 | +- Any authenticated partner user can ask the API to create a billing portal session with an attacker-controlled return URL. If the upstream billing/Stripe flow honors that value, the returned billing link can be used as a trusted Breeze-to-billing-to-attacker redirect chain for phishing or session-confusion attacks. |
| 48 | + |
| 49 | +Fix: |
| 50 | +- Validate `returnUrl` server-side against `PUBLIC_APP_URL`, `DASHBOARD_URL`, or `CORS_ALLOWED_ORIGINS`. |
| 51 | +- Prefer accepting only same-origin relative paths from the client, then build the absolute return URL server-side. |
| 52 | +- Add tests that reject `https://example.com/back` and accept the configured dashboard origin. |
| 53 | + |
| 54 | +Mitigation: |
| 55 | +- If the billing service already performs stricter allowlisting, document it and still add API-side validation so the security boundary is visible in this repo. |
| 56 | + |
| 57 | +False positive notes: |
| 58 | +- Exploitability depends on the upstream billing service honoring the forwarded `return_url`. The Breeze API currently does not show that protection. |
| 59 | + |
| 60 | +### G-002: APNS stub logs push notification tokens |
| 61 | + |
| 62 | +Rule ID: EXPRESS-CONFIG-001 / GO-CONFIG-001 equivalent secret logging control |
| 63 | +Severity: Medium |
| 64 | + |
| 65 | +Location: |
| 66 | +- [/Users/toddhebebrand/breeze/apps/api/src/services/notifications.ts:176](/Users/toddhebebrand/breeze/apps/api/src/services/notifications.ts:176) |
| 67 | + |
| 68 | +Evidence: |
| 69 | +- `sendAPNS()` logs `{ token, title: payload.title }` while returning a stubbed response. |
| 70 | +- Push tokens are bearer-like device identifiers. They should be treated as sensitive operational data because logs are commonly shipped to third-party aggregation and retained longer than application data. |
| 71 | + |
| 72 | +Impact: |
| 73 | +- APNS device tokens can leak into API logs whenever mobile notification dispatch hits the APNS path. A log-reader or compromised log sink could harvest device identifiers and correlate users/devices, and in some push systems a token leak can enable unauthorized notification attempts if paired with provider credentials. |
| 74 | + |
| 75 | +Fix: |
| 76 | +- Remove the token from the warning or log only a short hash/fingerprint, for example `sha256(token).slice(0, 12)`. |
| 77 | +- Add a regression test around `sendAPNS()` or notification dispatch logging if the project has logger test utilities. |
| 78 | + |
| 79 | +Mitigation: |
| 80 | +- Review current logs for accidental APNS token exposure if this code has run in a connected environment. |
| 81 | + |
| 82 | +False positive notes: |
| 83 | +- This path is currently a stub, but it is production code in the API service and receives real `device.apnsToken` values when called. |
| 84 | + |
| 85 | +## Low Findings |
| 86 | + |
| 87 | +### G-003: npm audit is advisory-only because CI pins pnpm 9.15.0 |
| 88 | + |
| 89 | +Rule ID: EXPRESS-DEPENDENCY-001 |
| 90 | +Severity: Low |
| 91 | + |
| 92 | +Location: |
| 93 | +- [/Users/toddhebebrand/breeze/package.json:24](/Users/toddhebebrand/breeze/package.json:24) |
| 94 | +- [/Users/toddhebebrand/breeze/.github/workflows/security.yml:15](/Users/toddhebebrand/breeze/.github/workflows/security.yml:15) |
| 95 | +- [/Users/toddhebebrand/breeze/.github/workflows/security.yml:47](/Users/toddhebebrand/breeze/.github/workflows/security.yml:47) |
| 96 | + |
| 97 | +Evidence: |
| 98 | +- The repo pins `packageManager` to `pnpm@9.15.0`. |
| 99 | +- The security workflow also sets `PNPM_VERSION: '9.15.0'`. |
| 100 | +- The workflow documents that this pnpm version calls a retired advisory endpoint and leaves `pnpm audit --audit-level=critical` as `continue-on-error: true`. |
| 101 | + |
| 102 | +Impact: |
| 103 | +- Critical npm dependency advisories may be visible in logs but will not fail pull requests or scheduled scans. In an RMM product with remote execution and agent update paths, dependency advisory response should be blocking for critical findings. |
| 104 | + |
| 105 | +Fix: |
| 106 | +- Bump pnpm to at least the workflow-commented fixed version, update `packageManager`, and remove `continue-on-error`. |
| 107 | +- Keep the current Trivy, CodeQL, Dependabot, and gitleaks workflows; they are useful complementary controls. |
| 108 | + |
| 109 | +Mitigation: |
| 110 | +- Until the pnpm bump lands, review the weekly security workflow output manually. |
| 111 | + |
| 112 | +False positive notes: |
| 113 | +- This is a process/control gap, not an exploitable code path. It is still worth fixing because the workflow explicitly says it is currently advisory-only. |
| 114 | + |
| 115 | +### G-004: OAuth DCR cleanup helper is not scheduled |
| 116 | + |
| 117 | +Rule ID: EXPRESS-DOS-001 |
| 118 | +Severity: Low |
| 119 | + |
| 120 | +Location: |
| 121 | +- [/Users/toddhebebrand/breeze/apps/api/src/config/env.ts:23](/Users/toddhebebrand/breeze/apps/api/src/config/env.ts:23) |
| 122 | +- [/Users/toddhebebrand/breeze/apps/api/src/routes/oauth.ts:90](/Users/toddhebebrand/breeze/apps/api/src/routes/oauth.ts:90) |
| 123 | +- [/Users/toddhebebrand/breeze/apps/api/src/oauth/provider.ts:36](/Users/toddhebebrand/breeze/apps/api/src/oauth/provider.ts:36) |
| 124 | +- [/Users/toddhebebrand/breeze/apps/api/src/oauth/provider.ts:49](/Users/toddhebebrand/breeze/apps/api/src/oauth/provider.ts:49) |
| 125 | +- [/Users/toddhebebrand/breeze/apps/api/src/oauth/provider.ts:274](/Users/toddhebebrand/breeze/apps/api/src/oauth/provider.ts:274) |
| 126 | + |
| 127 | +Evidence: |
| 128 | +- `OAUTH_DCR_ENABLED` defaults to disabled in production, but can be enabled. |
| 129 | +- The route blocks registration only when DCR is disabled. |
| 130 | +- The provider enables dynamic registration with `initialAccessToken: false` when DCR is enabled. |
| 131 | +- The cleanup helper itself documents that anyone can create a client when DCR is enabled and that the cleanup is not wired into the worker registry. |
| 132 | + |
| 133 | +Impact: |
| 134 | +- If production or hosted environments enable DCR, unauthenticated clients can accumulate abandoned OAuth client rows until manual cleanup runs. Rate limiting reduces abuse speed, but the table still grows without a scheduled lifecycle control. |
| 135 | + |
| 136 | +Fix: |
| 137 | +- Wire `cleanupStaleOauthClients()` into a daily BullMQ/worker job when MCP OAuth is enabled. |
| 138 | +- Consider making production DCR require an initial access token once dashboard issuance exists. |
| 139 | + |
| 140 | +Mitigation: |
| 141 | +- Keep `OAUTH_DCR_ENABLED=false` in production until the cleanup is scheduled or externally cron-driven. |
| 142 | + |
| 143 | +False positive notes: |
| 144 | +- This is conditional on DCR being enabled. The default production value is safer, but the feature flag exists and the code comments already identify the missing scheduler. |
| 145 | + |
| 146 | +## Positive Controls Observed |
| 147 | + |
| 148 | +- API bootstrap applies secure headers, CSP, request body limits, CORS allowlisting, and global rate limiting. |
| 149 | +- Access tokens are no longer persisted in browser local storage; the persisted auth store excludes `tokens`. |
| 150 | +- Metrics scraping has a dedicated bearer token path and production redacts org IDs from Prometheus labels by default. |
| 151 | +- Security workflows exist for CodeQL, gitleaks, Trivy, npm audit, and Go `govulncheck`. |
| 152 | +- Prior reports show substantial remediation coverage across agent result binding, helper output redaction, route authorization, RLS, and queue dedupe issues. |
| 153 | + |
| 154 | +## Suggested Next Work Order |
| 155 | + |
| 156 | +1. Fix G-002 by removing APNS token logging. This is the smallest and lowest-risk code change. |
| 157 | +2. Fix G-001 by enforcing same-origin billing portal return URLs in the API and updating tests. |
| 158 | +3. Bump pnpm and make `pnpm audit --audit-level=critical` blocking again. |
| 159 | +4. Wire OAuth DCR cleanup if DCR is expected outside local development. |
| 160 | +5. Continue deeper manual review in these areas: public installer/download endpoints, remote desktop/tunnel ticket lifecycles, AI tool execution approvals, and file browser upload/download path handling. |
0 commit comments