fix(calendar): bind omitted purpose to calendar_read - #541
Conversation
8d56da4 to
036ff73
Compare
The #524 purpose column rejected parent-domain create/use/rotate because is absent, keep explicit non-calendar values rejectable, and restore the foreign-key inserts so they fail for a missing project rather than NOT NULL. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
54af635 to
e1390a3
Compare
opencode-agent
Bot
added
priority: medium
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| export function installCalendarSubscriptionSchema(database) { | ||
| const db = requireDatabase(database); |
There was a problem hiding this comment.
📝 Info: Schema installer intentionally unwired
installCalendarSubscriptionSchema is exported but never called from server/db.mjs or server/app.mjs. The doctoring record defers route/db integration to a later #413 slice, so this matches the stated scope rather than being a missed wiring step.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const recordUsage = db.prepare(` | ||
| UPDATE calendar_subscriptions | ||
| SET last_used_at_ms = CASE | ||
| WHEN last_used_at_ms IS NULL OR last_used_at_ms < ? THEN ? | ||
| ELSE last_used_at_ms | ||
| END | ||
| WHERE secret_hash = ? | ||
| AND project_id = ? | ||
| AND purpose = ? | ||
| AND audience = ? | ||
| AND revoked_at_ms IS NULL | ||
| AND ? >= created_at_ms | ||
| AND ? < expires_at_ms | ||
| AND membership_version = ? | ||
| AND EXISTS ( | ||
| SELECT 1 | ||
| FROM projects p | ||
| JOIN memberships m ON m.org_id = p.org_id | ||
| JOIN users u ON u.id = m.user_id | ||
| WHERE p.id = calendar_subscriptions.project_id | ||
| AND m.user_id = calendar_subscriptions.subject_id | ||
| AND (CAST(m.id AS TEXT) || ':' || CAST(u.token_version AS TEXT)) = ? | ||
| ) | ||
| `); |
There was a problem hiding this comment.
📝 Info: Usage transition rejects rejoin and session revocation
recordUsage requires both the stored membership_version and a live membership_id:token_version join to equal the supplied issuance epoch. Remove-then-rejoin changes the membership id and session invalidation changes the token version, so either makes the transition match no row and return null before any usage or audit write.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const replaceSecret = db.prepare(` | ||
| UPDATE calendar_subscriptions | ||
| SET secret_hash = ?, | ||
| membership_version = ?, | ||
| expires_at_ms = ?, | ||
| rotated_at_ms = ? | ||
| WHERE subscription_id = ? | ||
| AND subject_id = ? | ||
| AND project_id = ? | ||
| AND purpose = ? | ||
| AND revoked_at_ms IS NULL | ||
| AND ? >= created_at_ms | ||
| AND ? > ? | ||
| AND EXISTS ( | ||
| SELECT 1 | ||
| FROM projects p | ||
| JOIN memberships m ON m.org_id = p.org_id | ||
| JOIN users u ON u.id = m.user_id | ||
| WHERE p.id = calendar_subscriptions.project_id | ||
| AND m.user_id = calendar_subscriptions.subject_id | ||
| AND (CAST(m.id AS TEXT) || ':' || CAST(u.token_version AS TEXT)) = ? | ||
| ) | ||
| `); |
There was a problem hiding this comment.
📝 Info: rotate UPDATE omits audience filter, still safe
The rotate statement replaceSecret filters on purpose but not audience, unlike the use path. Safe: the audience column is CHECK-pinned to a single value and the domain revalidates it after rotation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export function installCalendarSubscriptionSchema(database) { | ||
| const db = requireDatabase(database); | ||
| db.exec(` | ||
| CREATE TABLE IF NOT EXISTS calendar_subscriptions ( | ||
| subscription_id TEXT PRIMARY KEY, | ||
| secret_hash TEXT NOT NULL CHECK(length(secret_hash) = 64), | ||
| subject_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| project_id INTEGER NOT NULL REFERENCES projects(id) ON DELETE CASCADE, | ||
| name TEXT NOT NULL CHECK(length(name) BETWEEN 1 AND 120), | ||
| purpose TEXT NOT NULL CHECK(purpose = '${CALENDAR_PURPOSE}'), | ||
| audience TEXT NOT NULL CHECK(audience = '${CALENDAR_AUDIENCE}'), | ||
| membership_version TEXT NOT NULL CHECK(length(membership_version) BETWEEN 1 AND 128), | ||
| created_at_ms INTEGER NOT NULL CHECK(created_at_ms >= 0), | ||
| expires_at_ms INTEGER NOT NULL CHECK(expires_at_ms > created_at_ms), | ||
| last_used_at_ms INTEGER, | ||
| rotated_at_ms INTEGER, | ||
| revoked_at_ms INTEGER, | ||
| CHECK(last_used_at_ms IS NULL OR last_used_at_ms >= created_at_ms), | ||
| CHECK(rotated_at_ms IS NULL OR rotated_at_ms >= created_at_ms), | ||
| CHECK(revoked_at_ms IS NULL OR revoked_at_ms >= created_at_ms) | ||
| ); | ||
| CREATE UNIQUE INDEX IF NOT EXISTS calendar_subscription_secret_hash_index | ||
| ON calendar_subscriptions(secret_hash); | ||
| CREATE INDEX IF NOT EXISTS calendar_subscription_subject_project_index | ||
| ON calendar_subscriptions(subject_id, project_id, revoked_at_ms, expires_at_ms); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS subscription_rotations ( | ||
| rotation_event_id INTEGER PRIMARY KEY, | ||
| subscription_id TEXT NOT NULL REFERENCES calendar_subscriptions(subscription_id) ON DELETE CASCADE, | ||
| rotated_at_ms INTEGER NOT NULL CHECK(rotated_at_ms >= 0), | ||
| expires_at_ms INTEGER NOT NULL CHECK(expires_at_ms > rotated_at_ms) | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS subscription_rotation_history_index | ||
| ON subscription_rotations(subscription_id, rotated_at_ms); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS subscription_usage_events ( | ||
| usage_event_id INTEGER PRIMARY KEY, | ||
| subscription_id TEXT NOT NULL REFERENCES calendar_subscriptions(subscription_id) ON DELETE CASCADE, | ||
| used_at_ms INTEGER NOT NULL CHECK(used_at_ms >= 0) | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS subscription_usage_history_index | ||
| ON subscription_usage_events(subscription_id, used_at_ms); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS calendar_subscription_audit_outbox ( | ||
| audit_event_id INTEGER PRIMARY KEY, | ||
| subscription_id TEXT NOT NULL, | ||
| event_type TEXT NOT NULL CHECK(event_type IN ('created', 'used', 'rotated', 'revoked')), | ||
| subject_id INTEGER NOT NULL, | ||
| project_id INTEGER NOT NULL, | ||
| occurred_at_ms INTEGER NOT NULL CHECK(occurred_at_ms >= 0), | ||
| delivered_at_ms INTEGER | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS calendar_subscription_audit_delivery_index | ||
| ON calendar_subscription_audit_outbox(delivered_at_ms, audit_event_id); | ||
| `); | ||
| } |
There was a problem hiding this comment.
📝 Info: Foreign-key enforcement left to caller
The schema declares cascading foreign keys, but SQLite disables enforcement by default. installCalendarSubscriptionSchema does not enable it and documents this as a bootstrap responsibility. A production bootstrap that omits PRAGMA foreign_keys = ON silently loses cascade/integrity guarantees.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const recordUsage = db.prepare(` | ||
| UPDATE calendar_subscriptions | ||
| SET last_used_at_ms = CASE | ||
| WHEN last_used_at_ms IS NULL OR last_used_at_ms < ? THEN ? | ||
| ELSE last_used_at_ms | ||
| END | ||
| WHERE secret_hash = ? | ||
| AND project_id = ? | ||
| AND purpose = ? | ||
| AND audience = ? | ||
| AND revoked_at_ms IS NULL | ||
| AND ? >= created_at_ms | ||
| AND ? < expires_at_ms | ||
| AND membership_version = ? | ||
| AND EXISTS ( | ||
| SELECT 1 | ||
| FROM projects p | ||
| JOIN memberships m ON m.org_id = p.org_id | ||
| JOIN users u ON u.id = m.user_id | ||
| WHERE p.id = calendar_subscriptions.project_id | ||
| AND m.user_id = calendar_subscriptions.subject_id | ||
| AND (CAST(m.id AS TEXT) || ':' || CAST(u.token_version AS TEXT)) = ? | ||
| ) | ||
| `); |
There was a problem hiding this comment.
📝 Info: Usage event still recorded when timestamp unchanged
When the clock has not advanced, the last_used_at_ms CASE keeps the old value, yet the update still reports one changed row because the row matched. A usage event and audit row are still written, which is the intended per-authorization semantics.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async rotateSubscriptionAtomically(subscriptionId, binding) { | ||
| return withSavepoint(db, ROTATE_SAVEPOINT, () => { | ||
| const membershipVersion = matchLiveMembershipVersion( | ||
| liveMembershipVersion, | ||
| binding.project_id, | ||
| binding.subject_id, | ||
| binding.membership_version, | ||
| ); | ||
| if (!membershipVersion) return null; | ||
| const result = replaceSecret.run( | ||
| binding.new_secret_hash, | ||
| membershipVersion, | ||
| binding.expires_at_ms, | ||
| binding.now_ms, | ||
| subscriptionId, | ||
| binding.subject_id, | ||
| binding.project_id, | ||
| resolveCalendarPurpose(binding.purpose), | ||
| binding.now_ms, | ||
| binding.now_ms, | ||
| binding.expires_at_ms, | ||
| binding.now_ms, | ||
| membershipVersion, | ||
| ); | ||
| if (Number(result.changes) !== 1) return null; | ||
| const current = findScopedById.get(subscriptionId, binding.subject_id, binding.project_id); | ||
| insertRotation.run(subscriptionId, binding.now_ms, binding.expires_at_ms); | ||
| insertAudit.run( | ||
| subscriptionId, | ||
| 'rotated', | ||
| current.subject_id, | ||
| current.project_id, | ||
| binding.now_ms, | ||
| ); | ||
| return normalizeSubscriptionRow(current); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
📝 Info: Rotate rebinds epoch without matching stored value
rotateSubscriptionAtomically checks only that the supplied membership version matches live membership, not the stored membership_version. This is the sanctioned rebind-after-rejoin path, unlike recordUsage which requires stored, supplied, and live to all agree.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } catch (error) { | ||
| let rollbackSucceeded = false; | ||
| try { | ||
| database.exec(`ROLLBACK TO ${savepointName}`); | ||
| rollbackSucceeded = true; | ||
| } catch { | ||
| // An unconfirmed rollback must leave the savepoint open rather than risk committing failed state. | ||
| } | ||
| if (rollbackSucceeded) { | ||
| try { | ||
| database.exec(`RELEASE ${savepointName}`); | ||
| } catch { | ||
| // Cleanup failure must never replace the causal operation error after state is rolled back. | ||
| } | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
📝 Info: Rollback-failure path intentionally leaves savepoint open
withSavepoint skips RELEASE when ROLLBACK TO fails, to avoid committing failed state, and rethrows the causal error. This matches the two race tests covering release/rollback cleanup failure. Intentional, not a defect.
Was this helpful? React with 👍 or 👎 to provide feedback.
| insertAudit.run( | ||
| current.subscription_id, | ||
| 'used', | ||
| current.subject_id, | ||
| current.project_id, | ||
| binding.now_ms, | ||
| ); | ||
| pruneUsageAudit.run(current.subscription_id); |
There was a problem hiding this comment.
📝 Info: 'used' audit rows inserted then pruned before commit
recordUsageAtomically inserts a used outbox row then deletes all used rows for the subscription before release, so no read evidence survives commit. The insert exists only so a forced-outbox-failure rolls back authorization state; the domain emits the durable used audit through its own sink. Intentional per the doctoring record.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Buyer/security outcome
Refs #413. This stacked persistence slice makes reusable calendar subscriptions durable without storing plaintext credentials. It preserves project/purpose/audience binding, issuance membership/session epoch revocation, first-transition-only revoke evidence, and now bounds high-frequency usage evidence so calendar polling cannot grow durable history/outbox state without limit.
This PR does not change the protected calendar HTTP route or customer UI. Runtime composition remains in child #549.
Exact current stack
Fresh state after the latest repair:
develop:2c328875e00e86537df3e965170be80532571cad;cursor/bc-4522f1d4-ae7e-434e-b612-d250afa4d097-5645@e2f560078d3e2862c5dc6c395c159d12dfe1e6fa;cursor/bc-f7050a51-e6e0-448d-bbfa-39c45a93d707-0fd7@1e491bac2fc42b1f17c93666223ce1da78c6ba61;ahead, zero behind, with exact current parent as merge base;ARCHITECTURE.md,CHANGELOG.md,docs/doctoring/calendar-subscription-sqlite.md,package.json,server/calendar_subscription_sqlite.mjs,tests/unit/calendar-subscription-sqlite-expiry.test.mjs,tests/unit/calendar-subscription-sqlite-indexes.test.mjs,tests/unit/calendar-subscription-sqlite-issuance-epoch.test.mjs,tests/unit/calendar-subscription-sqlite-race.test.mjs,tests/unit/calendar-subscription-sqlite-retention.test.mjs,tests/unit/calendar-subscription-sqlite.test.mjs, andtests/unit/coverage-script-contract.test.mjs;The PR is currently Ready rather than Draft, but it is not merge-authorized while prerequisites and exact-head governance evidence remain incomplete.
Persistence/security contract
calendar_readand audience toscopeweave:calendar; explicit broader purpose is rejected;membership_id:token_versionepoch to equal the supplied and independently resolved live epoch;last_used_at_msplus only the configured recent usage-event window (256by default, bounded to1..10000);usedoutbox rows before commit, preventing high-frequency feed polling from producing an unbounded durable delivery backlog;Test-first repair: bounded usage evidence
Current-head review identified that every successful authorization appended both a usage row and an audit-outbox row indefinitely. The registered regression
tests/unit/calendar-subscription-sqlite-retention.test.mjsfirst failed on the predecessor implementation: three authorizations withusageEventLimit: 2retained all three usage rows instead of only the two newest rows.Production repair
36d854d32ff1918c3fc524528ccbf87677d6ee14added bounded per-subscription usage retention and lifecycle-only durable outbox behavior while preserving the existing forced-outbox-failure rollback contract.1e491bac2fc42b1f17c93666223ce1da78c6ba61then aligned the doctoring/traceability record with that executable behavior. The validated unbounded-growth thread is resolved; currently open Devin threads are informational analyses rather than identified defects.Current behavioral evidence
Current-head-associated runs on
1e491bac2fc42b1f17c93666223ce1da78c6ba61are terminal success:33124578344— success;33124578345— success;33124578769— success.The registered unit suite includes the expiry, bounded-retention, index, race, issuance-epoch, restart/rollback, tenant-nondisclosure and coverage-contract scenarios. Exact 100% owned production statement/branch/function/line evidence remains a separate mandatory merge gate; ordinary unit-test success is not substituted for that measurement.
These GitHub Actions successes are behavioral evidence only under the repository's current evidence contract: pull-request workflows still check out GitHub's synthetic merge revision rather than the contributor head. ScopeWeave #523 owns the repository-native exact-head checkout repair, while organization-reusable SAST/Security exact-head integrity remains in the existing
.githubowner path. No predecessor, synthetic-only, status-only, model-only, skipped, pending or stale evidence is promoted to merge authority.There is no qualifying independent current-head/last-push approval. The current requested reviewer is the repository owner and must not be treated as independent approval.
Merge gate
Do not integrate independently of #539/#506. Before merge, refetch the unchanged exact head and live base, preserve the current security semantics, require all then-applicable exact-contributor-head CI/browser E2E/owned coverage/docstrings/SAST/security/dependency/supply-chain/package/provenance/migration/recovery gates, zero valid unresolved defects, and qualifying independent current-head approval under live branch protection/rulesets.
Pending, queued, skipped-required, cancelled, absent, neutral-required, failed, stale, predecessor, synthetic-only, status-only, author-only, or model-only evidence is non-passing.
Documentation / rollback
docs/doctoring/calendar-subscription-sqlite.mdrecords the normalized data model, credential and tenant invariants, bounded usage-evidence contract, transactional failure behavior, rollback/recovery and APA 7 references. Rollback before route integration removes this adapter/tests/docs/registrations together; after route integration, rollback must preserve lifecycle evidence and must not restore a broad session JWT in calendar URLs as a steady state.