docs(phase-32.1): add security threat verification
This commit is contained in:
Jakub Zych
@@ -0,0 +1,99 @@
|
||||
---
|
||||
phase: "32.1"
|
||||
slug: "mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con"
|
||||
status: verified
|
||||
# threats_open = count of OPEN threats at or above workflow.security_block_on severity (the blocking gate)
|
||||
threats_open: 0
|
||||
asvs_level: 1
|
||||
created: "2026-11-05"
|
||||
---
|
||||
|
||||
# Phase 32.1 — Security
|
||||
|
||||
> Per-phase security contract: threat register, accepted risks, and audit trail. Evidence paths: F = `summercms.go`, U = `sm-user-plugin`, O = `sm-oauthserver-plugin`, V = `vue-oauth-layer`, S = `sm-summercmsio-app` (all under `../`). Baseline for "unchanged" claims: `2cea312`, the commit before the phase.
|
||||
|
||||
---
|
||||
|
||||
## Trust Boundaries
|
||||
|
||||
| Boundary | Description | Data Crossing |
|
||||
|----------|-------------|---------------|
|
||||
| MCP client to wristband authorize | Untrusted scope string and redirect cross here; the phase only changes which names are legal | scope string, redirect URI, PKCE challenge (high) |
|
||||
| pier dispatch to route handler | the handler gets an MCPCall built from the resolved subject, not from tool arguments | subject, client id, token id (high) |
|
||||
| OAuth grant scopes to MCPAllows / user.scope | the grant is the consented set; Implies may add only names the plugin marked | granted scopes (high) |
|
||||
| DELETE /oauth/authorizations/{client_id} | a signed-in user names a client; another user's rows must stay | client id, user session (high) |
|
||||
| Operator config `mcp_scopes` | names enter metadata and the allow-list at boot | scope names (medium) |
|
||||
| MCP consent JSON to the browser | client name, labels and admin name are attacker-influenced (DCR name, phrase text) | display strings (medium) |
|
||||
| Page POST back to the consent route | the browser can omit fields; the server intersection is the control | decision, scopes, backend_admin (high) |
|
||||
| Test output and changelog notes | tokens and authorization codes must not reach logs or the public post | test output, release notes (medium) |
|
||||
| git push of a release tag | the tag is what other repositories require | tag (low) |
|
||||
|
||||
---
|
||||
|
||||
## Threat Register
|
||||
|
||||
| Threat ID | Category | Component | Severity | Disposition | Mitigation | Status |
|
||||
|-----------|----------|-----------|----------|-------------|------------|--------|
|
||||
| T-32.1-01 | Elevation of privilege | wristband resolvedScopes | high | mitigate | F/modules/wristband/server.go:203 `mcpEmptyScopes` (five names incl. backend); :210-218 never widens a non-empty list; used by metadata :194 and authorize.go:29. `DefaultOptions` and OIDC discovery unchanged | closed |
|
||||
| T-32.1-02 | Elevation of privilege | pier filter.go read/write ceiling | high | mitigate | filter.go and identity.go unchanged since baseline; ceiling identity.go:80-82 `writes()` used at filter.go:66,:75; no `Implies` reference in pier, bouncer or wristband | closed |
|
||||
| T-32.1-03 | Spoofing | wristband Authorize redirect | high | mitigate | 63d05e8 touched only `allowedAuthorizeScopes`; authorize.go order: client lookup :62-82, exact redirect :84-88, PKCE :95-108, scope parse :110 | closed |
|
||||
| T-32.1-04 | Information disclosure | pact.MCPCall.Subject.TokenID | medium | mitigate | TokenID rule in pact/README.md:170, pier/README.md:13 and docs; dispatch.go:211-215 adds no log line; subject from `id.subjectFor` :172; audit.go unchanged | closed |
|
||||
| T-32.1-05 | Elevation of privilege | MCPUserGuard Implies helper | high | mitigate | U/classes/mcp_guard.go:160-175 `mcpEffectiveScopes`: personal tokens unchanged, clone only, read/write Implies only, exact stock scope required, no write-back; both call sites (:94, :145); `CompileAPITokenScopes` rejects backend/offline_access and bad Implies (api_token_scopes.go:139,:148). Residual: WR-01, WR-02 | closed |
|
||||
| T-32.1-06 | Elevation of privilege | MCP consent upsert | high | mitigate | O/controllers/mcp_consent_api.go: deny → `DenyPending` (:180-183); silent re-grant → `TouchConsent` (:355-358); decision → `replaceMCPConsent` ON CONFLICT sets scopes (:382-389), replace not union. Test `TestMCPConsentReplace`. Residual: CR-01, WR-03, WR-04 | closed |
|
||||
| T-32.1-07 | Elevation of privilege | authorizations.revoke | high | mitigate | O/controllers/authorizations.go:137-163: `LockUserGrants` advisory lock in-tx before deletes of consent, codes and tokens; `MCPSubjectRevoked` after commit. Test `TestMCPRevokeDeletesCode`. Closed at L1 only — see CR-01 | closed |
|
||||
| T-32.1-08 | Information disclosure | PendingRequest / IssueCode | high | mitigate | F/modules/wristband/consent.go:139-174 `lookupOwnedPending` unchanged, all misses → `ErrPendingNotFound` → 404. Plugin's `mcpPendingClientID` (mcp_consent_api.go:322-332) looks up by request id alone but both callers run it after the owner-checked `PendingRequest` | closed |
|
||||
| T-32.1-09 | Tampering | grantedScopes when scopes is absent | medium | mitigate | mcp_consent_api.go:247-250 absent scopes = requested set; offline override only when present (:224-226); request struct takes decision, scopes, backend_admin only | closed |
|
||||
| T-32.1-10 | Information disclosure | consent and revoke logs | medium | mitigate | phase diff `df2eddd..a7743e7` adds no log line in plugin non-test code; error-code sets unchanged | closed |
|
||||
| T-32.1-11 | Tampering | app/pages/connect.vue | high | mitigate | no `v-html`/innerHTML in V/app/; connect.vue uses text interpolation and attribute bindings; V/tests/connect.test.ts:107-109 asserts no `v-html`. Residual: WR-06 | closed |
|
||||
| T-32.1-12 | Elevation of privilege | read checkbox | low | accept | AR-32.1-01 | closed |
|
||||
| T-32.1-13 | Information disclosure | tests and changelog-post.py | medium | mitigate | phase test messages print only codes, error keys, scope lists and ids; notes contain no token prefixes; S/scripts/changelog-post.py:407-411 exits without token, :318-326 refuses a world-readable token file | closed |
|
||||
| T-32.1-14 | Spoofing | release tags | low | accept | AR-32.1-02 | closed |
|
||||
|
||||
*Status: open · closed · open — below high threshold (non-blocking)*
|
||||
*Severity: critical > high > medium > low — only open threats at or above workflow.security_block_on count toward threats_open*
|
||||
*Disposition: mitigate (implementation required) · accept (documented risk) · transfer (third-party)*
|
||||
|
||||
---
|
||||
|
||||
## Accepted Risks Log
|
||||
|
||||
| Risk ID | Threat Ref | Rationale | Accepted By | Date |
|
||||
|---------|------------|-----------|-------------|------|
|
||||
| AR-32.1-01 | T-32.1-12 | The read checkbox is disabled and the page's own POST includes read. A client that omits read only narrows the grant; the server must not reinsert read because the 422 smoke posts backend alone. Narrowing is not a privilege gain. `grantedScopes` does not special-case read. | 32.1-04-PLAN | 2026-11-05 |
|
||||
| AR-32.1-02 | T-32.1-14 | Tagging does not change PKCE, redirect matching or the read/write ceiling; the checkpoint is the control on publishing, not a new auth path. | 32.1-05-PLAN | 2026-11-05 |
|
||||
|
||||
*Accepted risks do not resurface in future audit runs.*
|
||||
|
||||
---
|
||||
|
||||
## Residual Risk (open review findings, outside the L1 register)
|
||||
|
||||
`32.1-REVIEW-DISPOSITION.md` lists these as `open`. The auditor confirmed each is still present in the code on 2026-11-05. They do not count toward `threats_open` at ASVS L1, but CR-01 would make T-32.1-07 a blocking open threat at L2.
|
||||
|
||||
| Finding | Severity | Touches | Gap |
|
||||
|---------|----------|---------|-----|
|
||||
| CR-01 | critical | T-32.1-07, T-32.1-06 | `srv.IssueCode` commits `MarkIssued` in its own transaction; `recordMCPConsent`/`replaceMCPConsent` then write the consent without `LockUserGrants` (O/controllers/mcp_consent_api.go:228-240). A revoke landing between the two is undone: the consent is re-inserted and the next authorize auto-allows. |
|
||||
| WR-01, WR-02 | warning | T-32.1-05 | `mcpEffectiveScopes` re-implies a declared scope the user cleared on /connect while read/write remains, and does not intersect the catalog with `golem15.oauthserver.mcp_scopes`. |
|
||||
| WR-04 | warning | T-32.1-06 | narrowing a consent does not revoke refresh tokens carrying the removed scopes or offline_access. |
|
||||
| WR-03 | warning | T-32.1-06 | the consent backfill seeds from tokens the verifier would refuse (nil-expiry, inactive/revoked clients, rotated refresh tokens). |
|
||||
| WR-06 | warning | T-32.1-11 | /connect `load()` has no generation guard; a stale `consent_required:false` can auto-allow a request the user no longer views. |
|
||||
|
||||
---
|
||||
|
||||
## Security Audit Trail
|
||||
|
||||
| Audit Date | Threats Total | Closed | Open | Run By |
|
||||
|------------|---------------|--------|------|--------|
|
||||
| 2026-11-05 | 14 | 14 | 0 | msd-security-auditor (ASVS L1, block_on high) |
|
||||
|
||||
---
|
||||
|
||||
## Sign-Off
|
||||
|
||||
- [x] All threats have a disposition (mitigate / accept / transfer)
|
||||
- [x] Accepted risks documented in Accepted Risks Log
|
||||
- [x] `threats_open: 0` confirmed
|
||||
- [x] `status: verified` set in frontmatter
|
||||
- [ ] CR-01 given a disposition in 32.1-REVIEW-DISPOSITION.md (fix or accept)
|
||||
|
||||
**Approval:** verified 2026-11-05 (ASVS L1; CR-01 outstanding)
|
||||
Reference in New Issue
Block a user