feat(attestation): end-to-end ingest pipeline (Plan 4) - #6
Conversation
Replace the placeholder attestation flow with the full ingest pipeline
per spec §4.5. The control plane now receives a raw JWS token and
performs all steps in-process:
1. Pre-parse unverified JWS for iss/kid/iat (jws.ts)
2. Resolve candidate keys from principal_keys (valid at iat, not revoked)
- kid present: select that key directly
- kid absent: try all active candidate keys (legacy fallback)
3. Synchronous Ed25519 signature verification (trustEngineClient.ts)
- In-process via Node crypto.verify, mirrors gRPC VerifyAttestation
4. Schema validation (schemaValidator.ts)
- behavioral@0 (legacy allowlisted) + behavioral@1 (native)
5. trust_delta range + sign validation (negative for negative_incident)
6. Dedup on token_digest (sha256 of JWS)
7. Lazy subject principal creation
8. Transactional storage with composite FK to principal_keys
New files:
- jws.ts / jws.test.ts — JWS pre-parser (6 tests)
- schemaValidator.ts / schemaValidator.test.ts — attestation schema validation
- trustEngineClient.ts / trustEngineClient.test.ts — in-process JWS verification (8 tests)
- principalRepository.ts — added getIssuer, getKeyByKid, getActiveKeysAt
Updated:
- attestationService.ts — full ingest flow replacing pre-verified payload
- routes/attestations.ts — accepts { token } only, no pre-verified payload
No attestation sync event written (score recomputation emits edge-visible events).
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
WalkthroughChangesThe attestation flow now accepts raw JWS tokens, resolves issuer keys, verifies signatures, validates schemas and trust constraints, maps subjects, enforces tenant visibility, deduplicates tokens, and persists records transactionally. Supporting parsing, authorization, repository, migration, documentation, and test coverage were added. ChangesAttestation ingest
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AttestationsRoute
participant AttestationService
participant PrincipalRepository
participant TrustEngineClient
participant AttestationRepository
Client->>AttestationsRoute: POST /submit with token
AttestationsRoute->>AttestationService: submitAttestation(jwsToken)
AttestationService->>PrincipalRepository: load issuer and candidate keys
AttestationService->>TrustEngineClient: verify JWS signature
TrustEngineClient-->>AttestationService: verified claims
AttestationService->>AttestationRepository: deduplicate and persist attestation
AttestationRepository-->>Client: attestation response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
PR Summary by QodoAttestation: end-to-end JWS ingest with in-process verification
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
4 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // control-plane/src/domains/attestation/jws.ts | ||
| export interface JWSHeader { | ||
| alg: string; | ||
| kid?: string; | ||
| typ?: string; | ||
| } |
There was a problem hiding this comment.
1. New typescript app logic 📘 Rule violation § Compliance
This PR adds new application/source logic in TypeScript (.ts) under control-plane/src/, violating the requirement that VeriLink repository source code be written in Go. This increases compliance/audit risk by introducing a non-approved implementation language for core functionality.
Agent Prompt
## Issue description
The PR introduces new `.ts` implementation files containing application logic, but the compliance checklist requires all repository source code to be written in Go.
## Issue Context
New TypeScript modules were added for attestation ingest (JWS parsing, schema validation, signature verification), which are not permitted under PR Compliance ID 856631.
## Fix Focus Areas
- control-plane/src/domains/attestation/jws.ts[1-65]
- control-plane/src/domains/attestation/schemaValidator.ts[1-44]
- control-plane/src/grpc/trustEngineClient.ts[1-148]
- control-plane/src/domains/attestation/attestationService.ts[1-163]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| function parseJws(token: string): DecodedJws { | ||
| const parts = token.split('.'); | ||
| if (parts.length !== 3) { | ||
| throw new Error('invalid JWS: expected 3 compact parts'); | ||
| } | ||
| const [headerB64, payloadB64, signatureB64] = parts; | ||
| const header = JSON.parse(base64urlDecode(headerB64).toString('utf8')); | ||
| const payload = JSON.parse(base64urlDecode(payloadB64).toString('utf8')); | ||
| const signature = base64urlDecode(signatureB64); | ||
| const signedInput = Buffer.from(headerB64 + '.' + payloadB64, 'utf8'); | ||
| return { headerB64, payloadB64, signature, header, payload, signedInput }; | ||
| } |
There was a problem hiding this comment.
2. verifyattestation() custom jws verify 📘 Rule violation ⛨ Security
The new verification code manually parses the compact JWS and uses low-level crypto.verify rather than a JWS/JOSE-compliant verification library. This can lead to non-compliant or subtly incorrect JWS handling compared to JOSE/JWS standards and violates the checklist requirement to use JWS-compliant libraries for signatures.
Agent Prompt
## Issue description
`trustEngineClient.verifyAttestation()` implements JWS parsing and signature verification manually (custom base64url decode + JSON parse + `crypto.verify`). The compliance requirement expects use of a JWS/JOSE-compliant library for producing/consuming signatures.
## Issue Context
The current implementation constructs `signedInput` from `headerB64.payloadB64` and verifies with `crypto.verify`, bypassing standard JOSE/JWS validation behavior (header validation rules, strict base64url parsing, etc.).
## Fix Focus Areas
- control-plane/src/grpc/trustEngineClient.ts[43-59]
- control-plane/src/grpc/trustEngineClient.ts[105-108]
- control-plane/src/grpc/trustEngineClient.ts[125-145]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const { header, payload } = preParsed; | ||
| const issuerId = payload.iss; | ||
| const kid = header.kid; | ||
| const iat = payload.iat ? new Date(payload.iat * 1000) : undefined; | ||
|
|
||
| if (!issuerId) { | ||
| throw new AppError(CODES.BAD_REQUEST, 'JWS missing iss claim'); | ||
| } | ||
| if (!iat) { | ||
| throw new AppError(CODES.BAD_REQUEST, 'JWS missing iat claim'); | ||
| } |
There was a problem hiding this comment.
4. Invalid iat not rejected 🐞 Bug ≡ Correctness
submitAttestation() treats any truthy payload.iat as valid and constructs a Date without checking finiteness/validity, so non-numeric iat values can propagate into key lookup SQL and cause request/DB errors (and iat=0 is incorrectly treated as missing).
Agent Prompt
### Issue description
`iat` is derived via a truthiness check and multiplication, which allows invalid dates (e.g., string iat -> `NaN`) to pass the `if (!iat)` guard and be used in DB queries. Also, `iat=0` (Unix epoch) is rejected because it is falsy.
### Issue Context
`getActiveKeysAt()` uses the Date directly as a query parameter in timestamp comparisons.
### Fix Focus Areas
- control-plane/src/domains/attestation/attestationService.ts[41-52]
- control-plane/src/domains/principal/principalRepository.ts[172-182]
### Suggested fix
- Extract `iatRaw = payload.iat` and require `typeof iatRaw === 'number' && Number.isFinite(iatRaw)`.
- Build `iat = new Date(iatRaw * 1000)` and require `!Number.isNaN(iat.getTime())`.
- If invalid, throw `new AppError(CODES.BAD_REQUEST, 'JWS iat must be a unix timestamp (seconds)')`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
I found four additional blocking issues not covered by the existing review:
-
Expired attestations are accepted (
control-plane/src/grpc/trustEngineClient.ts,control-plane/src/domains/attestation/attestationService.ts:85-153). The verifiedexpis copied intoexpires_at, but neither verification nor ingest checks it against the current time. A correctly signed token whoseexpis in the past therefore persists successfully. The attestation verification contract requires expiration validation; please reject expired tokens and add a boundary test. -
observation_idpairing invariants are not enforced (attestationService.ts:118-155). Ingest only deduplicates the exact token digest, then inserts the row. It never loads an existing(issuer_id, subject_id, observation_id)peer to ensure thatattestation_typeandtrust_deltamatch. The productization spec requires mismatched public/private pairs to return 4xx; otherwise later grouping can collapse contradictory edges. This check and insert need to be concurrency-safe. -
The legacy
behavioral@0escape hatch is unrestricted (schemaValidator.ts:21-23). Any issuer can select schema version 0 and bypass facts validation forever. The spec limits v0 to explicitly allowlisted legacy issuers and requiresBEHAVIORAL_V0_CUTOFF, after which it must be rejected. Issuer identity and cutoff policy need to participate in validation. -
Supported native attestation types cannot be ingested (
schemaValidator.ts:13-15). Every type other thanbehavioralis rejected, includingnegative_incident, even though the service explicitly validates its negative delta and the v1 contract definestransaction_summary,kyb,security_audit, andnegative_incident. Please add the versioned schemas and tests for each supported type rather than treating them as unknown.
1. Expired attestations are now rejected
- trustEngineClient.ts: check exp against current time after
successful signature verification; return invalid if expired
- Test: expired token is rejected
2. observation_id pairing invariants enforced
- attestationRepository.ts: added findObservationPeer() to load
existing attestation with same (issuer_id, subject_id, observation_id)
- attestationService.ts: after dedup check, if observation_id present,
load peer and validate attestation_type and trust_delta match;
reject with 400 on mismatch
3. behavioral@0 allowlist + cutoff
- schemaValidator.ts: behavioral@0 only accepted from issuers in
BEHAVIORAL_V0_ALLOWLIST env var; rejected after BEHAVIORAL_V0_CUTOFF
- Env vars read dynamically (not cached at module load)
- Tests: allowlist enforcement, cutoff enforcement, non-behavioral @0
4. All native attestation types supported
- schemaValidator.ts: added transaction_summary@1, kyb@1,
security_audit@1, negative_incident@1 with per-type required fields
- Tests: valid + missing-field for each type
Total: 36 TS tests pass, all Go tests pass, typecheck clean.
|
All four blocking findings fixed in
36 TS tests pass, all Go tests pass, typecheck clean. |
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Re-checking e615dd5, the prior findings are not fully resolved and I found one end-to-end blocker:
-
Go-signed VeriLink attestations cannot be ingested (
control-plane/src/grpc/trustEngineClient.ts:71-84).toPayload()readsattestation_type,facts,trust_level_delta, etc. from the JWT top level, butpkg/attestation.AttestationClaimsanddocs/specs/attestation_schema.mdput these signed claims underpayload.vli(vli.type,vli.facts, ...). The TS tests use a non-conformant flattened fixture, masking the incompatibility. A token produced by the repository Go client therefore yields an empty type/facts and is rejected. Please parsevliand add the Plan 4 Go-client-to-control-plane integration test. -
The observation pairing check remains raceable (
attestationService.ts:125-174). Two conflicting first submissions can both observe no peer and then both insert. Additionally,withTransaction()supplies a client that is ignored while repository methods query the global pool, so even the insert is not executed on that transaction. The peer check and insert need one concurrency-safe database operation/transaction, backed by locking or a constraint that makes the invariant enforceable. -
An unset allowlist still permits every issuer to use v0 (
schemaValidator.ts:8-12).allowlist.length === 0returnstrue, contradicting the explicit-allowlist requirement and allowing any new issuer to bypass native schema validation. Missing/empty configuration should fail closed (and GA startup must fail without a valid cutoff); an invalid cutoff currently also silently disables the cutoff. The post-cutoff response also needs the specified 422 rather than the generic 400 path. -
The v1 schemas are still not the specified schemas (
schemaValidator.ts:34-137). Most validators only check key presence, accept wrong field types and arbitrary extra properties, and enforce neither the 8 KB limit nor depth 4. More directly,behavioral@1rejects the required nested issuer-defined data object and does not requireobservation_tsor the schema URI. Please validate against the versioned JSON Schema artifacts required by section 6.4 and test conforming/rejected payloads. -
The expiration boundary is off by one second (
trustEngineClient.ts:146-151). JWTexprequires current time to be strictly before the expiration time, butnow > expaccepts a token during the exactexpsecond. Usenow >= expand cover the boundary test requested in the original review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@control-plane/src/domains/attestation/attestationService.ts`:
- Around line 143-151: Add observation pairing validation before constructing
the attestation payload: when vp.observationId is present, load the referenced
observation and verify its type and trust delta match vp.attestationType and
vp.trustLevelDelta, rejecting mismatches or missing observations. Keep
observationId unset for attestations without a reference and only persist
validated identifiers in the payload and returned object.
In `@control-plane/src/domains/attestation/jws.ts`:
- Around line 32-65: Extract the base64url decoding and JSON parsing from
preParseJWS in control-plane/src/domains/attestation/jws.ts:32-65 into a shared
segment-decoding helper, preserving the existing validation behavior. Update
parseJws in control-plane/src/grpc/trustEngineClient.ts:43-59 to use that
helper, remove its manual base64urlDecode and '=' padding logic, and use native
Buffer.from(..., 'base64url') through the shared implementation.
In `@control-plane/src/domains/principal/principalRepository.ts`:
- Around line 163-170: Update getKeyByKid to accept the attestation iat and
enforce valid_from <= iat, valid_until being null or >= iat, and revoked_at IS
NULL in its query. Update attestationService.submitAttestation to pass iat when
calling principalRepo.getKeyByKid, preserving direct kid-based key selection
while applying the same validity-window rules as getActiveKeysAt.
In `@control-plane/src/grpc/trustEngineClient.test.ts`:
- Around line 50-159: Extend the verifyAttestation test suite with regression
cases for a non-numeric trust_level_delta claim, a payload missing sub, and an
already-expired exp claim. Use the existing signJws, makeKey, and KeyCandidate
helpers, and assert each result is invalid with an appropriate error matching
the corrected toPayload/verifyAttestation behavior.
In `@control-plane/src/grpc/trustEngineClient.ts`:
- Around line 74-76: Validate string-valued jwsPayload.facts before assigning
factsJson, parsing it as JSON and rejecting or normalizing invalid input so
malformed values produce the established clean 400 response rather than an
uncaught SyntaxError. Preserve JSON.stringify handling for non-string facts and
update the surrounding trust-engine request flow in the relevant client method.
- Around line 110-113: Validate that the JWS payload contains a non-empty sub
claim before deriving subjectId in the trust-engine client flow, matching the
existing issuer validation behavior. Reject the attestation when sub is missing
or empty so createPrincipal is never called with an empty identifier.
- Around line 87-148: Update verifyAttestation to enforce the payload’s exp
claim before accepting a verified attestation: when exp is present, compare it
with the current Unix time and reject tokens that are already expired. Ensure
the rejection returns valid: false with a clear expiration error, while
preserving acceptance of tokens without an exp claim if that is the existing
contract.
- Around line 71-85: Validate numeric claims produced by toPayload before they
reach downstream validation: in verifyAttestation, reject the token when
trustLevelDelta, issuedAtUnix, or expiresAtUnix is not finite, returning an
invalid result with an appropriate claim-specific error. Do not pass NaN or
infinite values to the caller or persistence layer.
In `@control-plane/src/routes/attestations.ts`:
- Around line 12-19: Replace the two inline dynamic imports in the token
validation block with a single top-level static import of AppError and CODES,
then use those imported symbols when throwing the missing-token error. Keep the
existing validation condition and error message unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4193d90c-326e-4a60-ae31-2b2185bb8cb9
📒 Files selected for processing (10)
control-plane/src/domains/attestation/attestationService.tscontrol-plane/src/domains/attestation/jws.test.tscontrol-plane/src/domains/attestation/jws.tscontrol-plane/src/domains/attestation/schemaValidator.test.tscontrol-plane/src/domains/attestation/schemaValidator.tscontrol-plane/src/domains/principal/principalRepository.tscontrol-plane/src/grpc/trustEngineClient.test.tscontrol-plane/src/grpc/trustEngineClient.tscontrol-plane/src/routes/attestations.tsdocs/superpowers/plans/2026-07-27-verilink-4-attestation-ingest.md
1. Go-signed attestations now ingestible
- toPayload() reads vli nested claims (per pkg/attestation.AttestationClaims)
with fallback to top-level for legacy tokens
- Test: parses vli nested claims (Go-compatible format)
2. observation_id pairing is race-safe
- Repository methods accept PoolClient for transaction-scoped queries
- findObservationPeer uses SELECT ... FOR UPDATE
- dedup + peer check + insert all in one withTransaction(client)
3. behavioral@0 allowlist fails closed
- Empty/unset allowlist rejects all v0 (was: allowed all)
- Missing/invalid cutoff rejects all v0 (was: disabled cutoff)
- Post-cutoff returns 422 (CODES.UNPROCESSABLE) not 400
4. v1 schemas match spec §6.4
- behavioral@1: requires observation_ts, primitive facts
- transaction_summary@1: required fields + type checking
- kyb@1, security_audit@1, negative_incident@1: required fields + types
- 8KB size limit + depth-4 check on all schemas
5. Expiration boundary fixed
- now >= exp (was: now > exp) — rejects at exact exp second
- Test: exp boundary at exact second
40 TS tests pass, all Go tests pass, typecheck clean.
|
All five findings fixed in
40 TS tests pass, all Go tests pass, typecheck clean. |
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Reviewing the latest head 1a6799d, these issues remain beyond the existing CodeRabbit comments:
-
The observation invariant is still not concurrency-safe (
attestationRepository.ts:75-87,attestationService.ts:139-166).SELECT ... FOR UPDATEonly locks rows returned by the query. When the first two attestations for an observation arrive concurrently, both transactions can see no peer, acquire no lock, and insert conflicting type/delta values. Use a transaction-scoped advisory lock keyed by(issuer, subject, observation_id), a lockable parent row, or a database constraint/model that enforces the invariant even when no peer exists; add a concurrent integration test. -
Legacy Whimsy tokens still select the wrong schema version (
trustEngineClient.ts:88). The compatibility contract says an omittedvli.schema_versiondefaults to"0"for the allowlisted legacy path, buttoPayload()still defaults it to"1". A current Whimsy token with no version will therefore be validated as behavioral@1 and rejected. Native v1 must require an explicit version while only the identified legacy format receives the v0 default. -
The claimed schema enforcement remains bypassable/incompatible (
schemaValidator.ts:43-126). Required fields whose value isnullpass bothrequireString()andrequireNumber(). Depth traversal stops at arrays, so nested objects under arrays bypass the depth-four check. The 8 KB limit uses JavaScript string length rather thanBuffer.byteLength(json, "utf8"), allowing multibyte payloads larger than 8 KB. Typed schemas also accept arbitrary properties despiteadditionalProperties: false, while behavioral@1 still rejects the bounded nested issuer-defined data object required by section 6.4. These should be real versioned JSON Schema validations with negative tests for nulls, extras, UTF-8 size, and array nesting. -
Lazy subject creation still races (
attestationService.ts:130-134). Concurrent distinct attestations for the same previously unseen subject can both observe it missing; onecreatePrincipal()then fails its primary-key insert and surfaces a database 500 before the transaction/catch block. Make creation idempotent (INSERT ... ON CONFLICT DO NOTHINGfollowed by fetch) and cover concurrent first-seen submissions. -
facts_hashis not RFC 8785 JCS (attestationService.ts:11-21). Recursively sorting with locale-dependentlocaleCompare()and then callingJSON.stringify()is not the required RFC 8785 canonicalization, particularly for property ordering across Unicode keys. This can produce a different hash across runtimes/languages and defeats the cross-language exact-content identity contract. Use a tested JCS implementation and shared Go/TS fixtures.
1. observation_id pairing concurrency-safe via advisory lock - pg_advisory_xact_lock keyed by hash(issuer:subject:observation_id) - Serializes concurrent first-submissions even when no peer row exists 2. Legacy schema_version defaults correctly - vli present without schema_version → defaults to "1" (native) - No vli (flattened legacy) → defaults to "0" (allowlisted) - Tests: both paths covered 3. Schema enforcement hardened - null required fields now rejected (was: passed) - Extra properties rejected on typed schemas (additionalProperties: false) - Size check uses Buffer.byteLength(json, 'utf8') not string length - Depth check traverses into arrays (was: stopped at arrays) - behavioral@1 allows bounded nested "data" object per spec §6.4 - Tests: null fields, extra props, wrong types, behavioral data object 4. Lazy subject creation is idempotent - INSERT ... ON CONFLICT (id) DO NOTHING + fetch if existed - Concurrent first-seen submissions no longer race on PK violation 5. facts_hash uses RFC 8785 JCS (not localeCompare) - Sort by UTF-16 code unit order (charCodeAt), not locale - Drop undefined values - Deterministic across runtimes/languages - Tests: UTF-16 ordering, deterministic hash, nested objects, arrays 52 TS tests pass, all Go tests pass, typecheck clean.
|
All five findings fixed in
52 TS tests pass, all Go tests pass, typecheck clean. |
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Reviewing round-three head b05968f, I found four remaining issues not covered by the old CodeRabbit review:
-
Every attestation with
observation_idcrashes in the ESM runtime (attestationRepository.ts:96-99). The package has"type": "module"and compiles withNodeNext, buthashToLockKey()callsrequire("node:crypto");requireis undefined in an ES module. The advisory-lock path therefore throws before querying PostgreSQL. Use a top-level ESM import (import { createHash } from "node:crypto") and exercise this through an actual repository/service integration test. -
Schema-version inference still contradicts both compatibility paths (
trustEngineClient.ts:79-84). VeriLink/Go and the documented Whimsy format both nest claims undervli; Whimsy is distinguished by its allowlisted issuer and omitted version, not by flattening the payload. The currenthasVli ? "1" : "0"therefore treats a real legacy Whimsy token as v1, while also silently upgrading a native token that violates the requirement that v1 specifyschema_version. Preserve “version absent” through verification, then let ingest select v0 only for an allowlisted legacy issuer; reject it for native issuers. -
The behavioral nested-data cap is not enforced (
schemaValidator.ts:137-156). Section 6.4 allows a nested issuer-defineddataobject up to 4 KB, but validation only applies the 8 KB whole-facts limit. A ~7 KBdataobject is accepted. Measurefacts.dataseparately at 4096 UTF-8 bytes and add boundary tests. More broadly, the required versioned JSON Schema artifacts are still absent fromdocs/specs, so this remains a hand-written approximation rather than validation against the schemas required before ingest. -
The new JCS tests do not test production code (
canonicalize.test.ts:5-44). They copy and test a second implementation ofcanonicalize()instead of importing or reaching the function used bysubmitAttestation; a regression in the service implementation leaves every test green. Move canonicalization to an exported module, test that exact function with official RFC 8785 vectors (including numeric/string edge cases), and add the cross-language Go/TS hash fixture required by the spec.
1. ESM runtime crash fixed
- hashToLockKey() used require('node:crypto') which is undefined in ESM
- Replaced with top-level import { createHash } from 'node:crypto'
2. Schema version inference corrected
- toPayload() now preserves empty string when schema_version absent
- attestationService resolves: allowlisted issuer → "0", native → reject
- No more guessing v1 or v0 in the verifier; ingest owns the decision
3. behavioral@1 nested data cap enforced
- facts.data measured separately with Buffer.byteLength at 4KB limit
- Tests: data under 4KB accepted, over 4KB rejected
4. JCS tests now test production code
- canonicalize() and computeFactsHash() exported from canonicalize.ts
- attestationService imports from canonicalize.ts (not inline copy)
- Tests import and exercise the exact production function
- Tests: UTF-16 ordering, deterministic hash, nested/arrays, numbers, booleans
58 TS tests pass, all Go tests pass, typecheck clean.
|
All four findings fixed in
58 TS tests pass, all Go tests pass, typecheck clean. |
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Reviewing round-four head 3e208e7, I found five remaining issues:
-
The production canonicalizer still produces non-JCS ordering for integer-like keys (
canonicalize.ts:14-31). It sorts entries lexically and then rebuilds a plain object, butJSON.stringify()enumerates integer-index property names numerically regardless of insertion order. For example{"10":"a","2":"b"}is emitted as{"2":"b","10":"a"}, while RFC 8785 UTF-16 ordering requires"10"before"2". Behavioraldatapermits these keys, so cross-languagefacts_hashcan still differ. Serialize sorted entries directly or use a vetted JCS library, and add this regression plus official RFC vectors. -
Native v1 still accepts a missing
kid(attestationService.ts:62-91). The no-kidbranch tries all active issuer keys before schema resolution and there is no later check restricting that fallback to legacy v0. A native v1 token with nokidtherefore verifies and persists, contrary to the requirement thatkidis mandatory for native submissions. After resolving the effective schema version, reject missingkidunless the submission is the allowlisted legacy path. -
Legacy subjects are not converted to VeriLink principal IDs (
attestationService.ts:126-130). Whimsy subjects are legacy fingerprints/strings; the compatibility contract requires lazy creation of a newvrl:p:<uuid>principal and preservation of the source value inmetadata.legacy_did. The service still inserts the raw signedsubasprincipals.id, so the first seeded issuer creates identifiers outside the native namespace and loses the legacy mapping metadata. -
The normalized stored payload records the unresolved schema version (
attestationService.ts:162-174). For an allowlisted legacy token,schemaVersionis resolved to"0", butpayload.schema_versionis built fromvp.schemaVersion, which remains empty. The JSONB payload andschema_versioncolumn therefore disagree for every compatibility submission. Use the resolvedschemaVersionconsistently and add a legacy persistence test. -
Invalid visibility becomes a database 500 instead of a validation 4xx (
trustEngineClient.ts:90,attestationService.ts:162-179). Any supplied value is string-coerced and persisted without checkingparticipants | public; PostgreSQL then raises the visibility CHECK violation (23514), which is not mapped and escapes as an internal error. Validate the signed claim before persistence and test invalid values. The same validation layer should reject malformed optional identifiers rather than relying on database failures.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane/src/domains/attestation/attestationService.ts (1)
167-173: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStored
payload.schema_versionand theschema_versioncolumn disagree for legacy tokens.Line 173 persists the resolved
schemaVersion('0'for allowlisted legacy issuers), but Line 167 embeds the rawvp.schemaVersionin the JSON payload, which is''in exactly that case —toPayloadpreserves absent versions as an empty string by design. Two records of the same fact, disagreeing. Anything auditing or replaying frompayloadwill read a blank version.🐛 Proposed fix
- payload: { type: vp.attestationType, facts, trust_level_delta: vp.trustLevelDelta, schema_version: vp.schemaVersion, visibility: vp.visibility, observation_id: vp.observationId }, + payload: { type: vp.attestationType, facts, trust_level_delta: vp.trustLevelDelta, schema_version: schemaVersion, visibility: vp.visibility, observation_id: vp.observationId },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane/src/domains/attestation/attestationService.ts` around lines 167 - 173, Update the payload construction in the attestation persistence flow to use the resolved schemaVersion for payload.schema_version instead of the raw vp.schemaVersion. Keep the schema_version column and JSON payload aligned, including the legacy allowlisted issuer path that resolves an absent version to '0'.
♻️ Duplicate comments (4)
control-plane/src/domains/principal/principalRepository.ts (1)
151-188: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
getKeyByKidstill ignores the key validity window thatgetActiveKeysAtenforces.Previously flagged and unchanged: this query filters only on
principal_id,key_id, andrevoked_at IS NULL, whilegetActiveKeysAtadditionally requiresvalid_from <= iat AND (valid_until IS NULL OR valid_until >= iat). SinceattestationService.submitAttestation(Line 53-58) takes thegetKeyByKidpath whenever the JWS header carries akid, an issuer can sign with a not-yet-valid or already-rolled-over key and still pass verification simply by naming thatkid. Key rotation is unenforceable on the fast path.Thread
iatthrough and apply the same predicate; the call site atattestationService.tsLine 54 needs to pass it.🔒 Proposed fix
-export async function getKeyByKid(principalId: string, keyId: string): Promise<PrincipalKey | null> { +export async function getKeyByKid(principalId: string, keyId: string, iat: Date): Promise<PrincipalKey | null> { const { rows } = await pool.query( `SELECT * FROM principal_keys - WHERE principal_id = $1 AND key_id = $2 AND revoked_at IS NULL`, - [principalId, keyId] + WHERE principal_id = $1 AND key_id = $2 AND revoked_at IS NULL + AND valid_from <= $3 + AND (valid_until IS NULL OR valid_until >= $3)`, + [principalId, keyId, iat] ); return rows[0] || null; }#!/bin/bash # Confirm getKeyByKid's current predicate and all call sites. fd -t f 'principalRepository.ts' --exec ast-grep run --pattern 'export async function getKeyByKid($$$) { $$$ }' --lang typescript {} rg -nP --type=ts -C3 '\bgetKeyByKid\s*\('🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane/src/domains/principal/principalRepository.ts` around lines 151 - 188, Update getKeyByKid to accept an iat argument and apply the same valid_from/valid_until validity-window predicates used by getActiveKeysAt, while retaining the principal_id, key_id, and non-revoked filters. Update attestationService.submitAttestation’s getKeyByKid call to pass the attestation iat value.control-plane/src/grpc/trustEngineClient.ts (2)
88-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPre-serialized
factsstring is still passed through unvalidated.Previously flagged and unchanged apart from the
vliindirection.attestationService.tsLine 96 doesJSON.parse(vp.factsJson)inside atrythat only special-casesSchemaValidationError, so a token withvli.facts: "hello"throws an uncaughtSyntaxError(500) instead of a 400. Line 123 parses it a second time outside anytry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane/src/grpc/trustEngineClient.ts` around lines 88 - 90, The factsJson construction in the trust-engine client must validate pre-serialized vli.facts strings before forwarding them. Update the flow around vli.facts and the corresponding attestationService parsing to reject invalid or non-object JSON with the existing 400/schema-validation error path, avoiding uncaught SyntaxError failures and unsafe repeated parsing.
91-93: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
toPayloadmaps untrusted JWS claims into typed fields with no validation. Both findings below share one root cause: decoded claim values are coerced (Number(...), pass-through string) and handed to callers as if they were validated, so malformed claims defeat downstream checks instead of being rejected. Both were raised on earlier commits and survived thevlirefactor.
control-plane/src/grpc/trustEngineClient.ts#L91-L93: reject non-finite results fromNumber(...)fortrust_level_delta,iat, andexp.NaNcurrently bypasses thetrust_deltarange/sign checks inattestationService.ts, fails open on the new expiration guard at Line 161, and yields an Invalid Date forissued_at.control-plane/src/grpc/trustEngineClient.ts#L88-L90: whenvli.factsis already a string, parse it before accepting it asfactsJson, so theJSON.parsecalls atattestationService.tsLines 96 and 123 cannot throw an uncaughtSyntaxErrorand turn a bad token into a 500.The cleanest shape is to have
verifyAttestationreturn{ valid: false, error }for any claim that fails these checks, rather than forwarding corrupt values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane/src/grpc/trustEngineClient.ts` around lines 91 - 93, Update verifyAttestation and its toPayload claim mapping in control-plane/src/grpc/trustEngineClient.ts at lines 91-93 and 88-90: reject non-finite Number results for trust_level_delta, iat, and exp by returning { valid: false, error }, and parse string-valued vli.facts before assigning factsJson, likewise returning an invalid result when parsing fails. Ensure malformed claims are never forwarded to attestationService.ts.control-plane/src/grpc/trustEngineClient.test.ts (1)
192-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression cases for the still-open
toPayloadgaps.The
vlimapping coverage is good, but there is still no test for a non-numerictrust_level_delta, a non-JSON stringfacts, or a payload missingsub— all three remain unhandled intrustEngineClient.ts. Adding them here would lock in the fixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane/src/grpc/trustEngineClient.test.ts` around lines 192 - 264, Add regression tests alongside the existing attestation parsing tests for non-numeric vli trust_level_delta, non-JSON string facts, and a signed payload missing sub. Use verifyAttestation and assert each case’s expected invalid or fallback behavior, covering the corresponding toPayload handling gaps in trustEngineClient.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@control-plane/src/domains/attestation/attestationRepository.ts`:
- Around line 76-95: Make the client parameter required in findObservationPeer
and remove the pool fallback, ensuring every caller uses a transactional
PoolClient before acquiring the advisory lock and querying attestations. Update
callers such as attestationService to satisfy the required client contract.
- Around line 97-100: Update hashToLockKey’s caller when passing lockKey to
pg_advisory_xact_lock so the bigint is converted to a string before becoming a
query parameter. Preserve the existing hash calculation and advisory-lock
behavior, changing only the parameter serialization.
In `@control-plane/src/domains/attestation/attestationService.ts`:
- Around line 133-139: Update the comment above
attestationRepo.findByTokenDigest in the withTransaction callback to remove the
claim that the dedup check uses a lock. Describe it as an in-transaction
duplicate check, and preserve the existing 23505 unique-constraint handling that
provides concurrency protection.
- Around line 126-130: Validate verifiedSubjectId before principal lookup and
reject missing or empty subjects before any database work. Move the lazy
principal creation into the attestation withTransaction callback so it commits
or rolls back with the attestation, and update the principal repository methods
used by this flow (getPrincipal and createPrincipal) to accept and use the
transaction PoolClient rather than the shared pool.
- Around line 82-93: Remove the local BEHAVIORAL_V0_ALLOWLIST parsing from the
schema-version resolution block and import/reuse the exported
isV0AllowedForIssuer predicate from schemaValidator.ts with verifiedIssuerId.
Ensure the predicate remains the single source of truth for legacy v0
eligibility before assigning the default version or throwing the existing
AppError.
In `@control-plane/src/domains/attestation/canonicalize.test.ts`:
- Around line 5-35: Add a test alongside the existing canonicalize ordering
cases that uses at least one non-BMP Unicode object key and asserts the exact
RFC 8785 UTF-16 code-unit ordering in the canonical output. Keep the assertion
focused on canonicalize’s serialized key order so any comparator change is
detected.
---
Outside diff comments:
In `@control-plane/src/domains/attestation/attestationService.ts`:
- Around line 167-173: Update the payload construction in the attestation
persistence flow to use the resolved schemaVersion for payload.schema_version
instead of the raw vp.schemaVersion. Keep the schema_version column and JSON
payload aligned, including the legacy allowlisted issuer path that resolves an
absent version to '0'.
---
Duplicate comments:
In `@control-plane/src/domains/principal/principalRepository.ts`:
- Around line 151-188: Update getKeyByKid to accept an iat argument and apply
the same valid_from/valid_until validity-window predicates used by
getActiveKeysAt, while retaining the principal_id, key_id, and non-revoked
filters. Update attestationService.submitAttestation’s getKeyByKid call to pass
the attestation iat value.
In `@control-plane/src/grpc/trustEngineClient.test.ts`:
- Around line 192-264: Add regression tests alongside the existing attestation
parsing tests for non-numeric vli trust_level_delta, non-JSON string facts, and
a signed payload missing sub. Use verifyAttestation and assert each case’s
expected invalid or fallback behavior, covering the corresponding toPayload
handling gaps in trustEngineClient.ts.
In `@control-plane/src/grpc/trustEngineClient.ts`:
- Around line 88-90: The factsJson construction in the trust-engine client must
validate pre-serialized vli.facts strings before forwarding them. Update the
flow around vli.facts and the corresponding attestationService parsing to reject
invalid or non-object JSON with the existing 400/schema-validation error path,
avoiding uncaught SyntaxError failures and unsafe repeated parsing.
- Around line 91-93: Update verifyAttestation and its toPayload claim mapping in
control-plane/src/grpc/trustEngineClient.ts at lines 91-93 and 88-90: reject
non-finite Number results for trust_level_delta, iat, and exp by returning {
valid: false, error }, and parse string-valued vli.facts before assigning
factsJson, likewise returning an invalid result when parsing fails. Ensure
malformed claims are never forwarded to attestationService.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58d82b62-1b22-40ec-bcff-1f73720f7326
📒 Files selected for processing (9)
control-plane/src/domains/attestation/attestationRepository.tscontrol-plane/src/domains/attestation/attestationService.tscontrol-plane/src/domains/attestation/canonicalize.test.tscontrol-plane/src/domains/attestation/canonicalize.tscontrol-plane/src/domains/attestation/schemaValidator.test.tscontrol-plane/src/domains/attestation/schemaValidator.tscontrol-plane/src/domains/principal/principalRepository.tscontrol-plane/src/grpc/trustEngineClient.test.tscontrol-plane/src/grpc/trustEngineClient.ts
1. JCS integer-key ordering fixed
- Canonicalizer now serializes sorted entries directly to string
instead of rebuilding a plain object (JS engine reorders integer keys)
- '10' before '2' per RFC 8785 UTF-16 order, not numeric order
- Tests: integer-like keys, mixed integer/string keys
2. Native v1 kid enforcement
- After schema version resolution, reject missing kid unless legacy v0
- isLegacy flag tracks allowlisted v0 path; native v1 requires kid
3. Legacy subject → vrl:p:<uuid> mapping
- Non-vrl:p: subjects (did:key, fingerprints) get a new vrl:p:<uuid> principal
- Original subject preserved in metadata.legacy_did
- createPrincipal() now accepts metadata parameter
4. Persisted payload uses resolved schema_version
- payload.schema_version set to resolved value (was: raw vp.schemaVersion)
5. Visibility validated before persistence
- Reject values other than 'participants' or 'public' with 400
- Prevents database CHECK violation (23514) from surfacing as 500
60 TS tests pass, all Go tests pass, typecheck clean.
|
All five findings fixed in
60 TS tests pass, all Go tests pass, typecheck clean. |
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Reviewing round-five head d01a53e, I found four new issues beyond CodeRabbit’s existing findings:
-
Legacy subject mapping creates a new principal for every attestation (
attestationService.ts:143-158). The lookup usesgetPrincipal(verifiedSubjectId), but the created row is stored under a randomvrl:p:<uuid>with the legacy value only in metadata. The next token for the same legacysubcannot find that row and creates another UUID, fragmenting one subject across multiple graph nodes; concurrent first submissions can do the same. Add an atomic repository lookup/upsert by the original legacy identifier, backed by a unique indexed mapping (not an unindexed best-effort JSON scan), and use the returned native ID consistently. -
Explicit legacy v0 tokens are incorrectly required to carry
kid(attestationService.ts:85-100).isLegacyis set only when an absent version is defaulted to"0". If an allowlisted legacy issuer explicitly signsschema_version: "0"and omitskid,isLegacyremains false and the native-kid check rejects it. Derive legacy status from the resolved version plus the single allowlist predicate, not from whether defaulting happened, and test both omitted and explicit v0. -
An explicitly empty visibility is silently accepted as participants (
attestationService.ts:127-132).vp.visibility || "participants"treats signedvisibility: ""as absent, bypassing the new enum validation; persistence also records an empty value inpayload.visibilitywhile the column containsparticipants. SincetoPayload()already supplies the compatibility default only when the claim is nullish, validatevp.visibilitydirectly and use the same normalized value in both representations. -
The JCS implementation accepts invalid Unicode that RFC 8785 requires rejecting (
canonicalize.ts:36-38).JSON.stringify()escapes lone UTF-16 surrogates instead of failing, so facts containing a JSON string such as"\\ud800"are hashed even though RFC 8785/I-JSON requires lone surrogates to terminate canonicalization. Cross-language decoders may replace or reject that value, recreating the hash mismatch this code is meant to prevent. Use a compliant JCS implementation or explicitly validate Unicode, and add official RFC vectors rather than only locally constructed ordering cases.
1. Legacy subject mapping no longer fragments
- findOrCreateByLegacyDID() in principalRepository: looks up by
metadata->>'legacy_did', creates vrl:p:<uuid> atomically
- ON CONFLICT + re-query handles concurrent first submissions
- Same legacy sub always resolves to the same native principal
2. Legacy v0 with explicit schema_version: '0' now accepted without kid
- isLegacy derived from (allowlisted issuer + version '0'), not
from whether defaulting happened
- Explicit v0 from allowlisted issuer correctly skips kid requirement
3. Empty visibility rejected, not silently defaulted
- toPayload() defaults nullish to 'participants' (already correct)
- Service validates vp.visibility directly; explicit '' is rejected
- Payload uses the same validated visibility value
4. JCS rejects lone surrogates per RFC 8785/I-JSON
- canonicalizeString() checks for unpaired high/low surrogates
- Throws on violation instead of passing to JSON.stringify
- Tests: lone high, lone low, valid surrogate pair (emoji)
63 TS tests pass, all Go tests pass, typecheck clean.
|
All four findings fixed in
63 TS tests pass, all Go tests pass, typecheck clean. |
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Reviewing round-six head e64dbb3, I found three remaining issues introduced or left unresolved by this revision:
-
The legacy mapping is still not atomic under concurrency (
control-plane/src/domains/principal/principalRepository.ts:53-83). Two requests for the same unseen legacy DID can both miss the initialSELECT, generate different random UUIDs, and both successfully insert becauseON CONFLICT (id)only protects those different UUIDs. The retry path is never reached, so the same legacy subject is again split across graph nodes. Back the mapping with a unique indexed column/expression (or mapping table) and perform an atomic upsert returning the one canonical principal; add a concurrent first-ingest test. -
Malformed native subjects are silently converted into legacy principals (
control-plane/src/domains/attestation/attestationService.ts:142-153). The conversion is selected by!subjectId.startsWith('vrl:p:'), not byisLegacy. Thus a native v1 token signed withsub: "arbitrary"is accepted and mapped instead of rejected, while strings such asvrl:p:not-a-uuidbypass conversion and are stored as native IDs. Only the allowlisted legacy-v0 path should map a legacy subject; native submissions should require and validate the completevrl:p:<uuid>form. -
The new lone-surrogate rejection is exposed as a 500 (
control-plane/src/domains/attestation/attestationService.ts:137-140,canonicalize.ts:42-54). Schema validation accepts string values containing a lone surrogate, thencomputeFactsHash()throws a plainErroroutside the validation catch. The global mapper turns that intoINTERNAL, so attacker-controlled malformed facts now produce a server error rather than a deterministic validation response. Validate JCS compatibility within the schema/4xx boundary or translate canonicalization failures toBAD_REQUEST, with a submit-level regression test rather than only a unit test ofcanonicalize().
1. Atomic legacy mapping backed by unique index
- Migration 007: unique index on (metadata->>'legacy_did')
- findOrCreateByLegacyDID: INSERT without ON CONFLICT so concurrent
inserts on the same legacy DID hit 23505 (unique violation) and
re-query. No more fragmentation across graph nodes.
2. Native v1 subjects validated, only legacy v0 maps
- Legacy subject mapping gated on isLegacy (allowlisted v0 path only)
- Native v1 requires vrl:p:<uuid> format with UUID regex validation
- Arbitrary strings and malformed vrl:p: IDs rejected with 400
3. JCS surrogate error mapped to 400
- computeFactsHash() wrapped in try/catch
- Lone surrogate Error translated to AppError(BAD_REQUEST)
- No more 500 on attacker-controlled malformed facts
63 TS tests pass, all Go tests pass, typecheck clean.
|
All three findings fixed in
63 TS tests pass, all Go tests pass, typecheck clean. |
|
All three blockers fixed in
64 TS tests pass, all Go tests pass, typecheck clean. |
| subjectId: req.query.subject_id as string, | ||
| limit: req.query.limit ? parseInt(req.query.limit as string, 10) : undefined, | ||
| offset: req.query.offset ? parseInt(req.query.offset as string, 10) : undefined, | ||
| callerTenantId: req.user?.tenantId || undefined, |
There was a problem hiding this comment.
The visibility fix still uses an arbitrary single tenant and does not implement staff access. authMiddleware obtains OIDC context with SELECT ... FROM tenant_memberships WHERE user_id = $1 LIMIT 1 and no ordering or active-tenant selection, then this route forwards only that one tenant ID. A user who owns/participates in multiple tenants therefore sees participant attestations for whichever membership PostgreSQL happens to return, while participant rows belonging to their other tenant are hidden. Also, although the stated rule and repository comment allow staff, staff/admin are passed through this same tenant predicate and never receive a privileged path. Resolve an explicit active tenant (or all authorized tenant IDs) and pass a separate, auditable staff bypass; add route/repository tests for multi-tenant and staff callers.
There was a problem hiding this comment.
Fixed in d81410c.
-
OIDC auth now loads ALL tenant memberships (was: LIMIT 1). req.user.tenantIds contains every authorized tenant ID.
-
listAttestations uses ANY(uuid[]) — participant rows visible if caller owns the issuer OR subject principal via ANY of their tenant IDs. Multi-tenant users see all their participant rows.
-
Staff/admin bypass: req.user.isStaff = true if any role is staff/admin. Staff bypass all visibility filters entirely.
1. OIDC auth loads all tenant memberships (was: LIMIT 1)
- req.user.tenantIds: all authorized tenant IDs
- req.user.isStaff: true if any role is staff/admin
2. listAttestations uses ANY(uuid[]) for multi-tenant
- Participant rows visible if caller owns issuer OR subject
via any of their tenant IDs
- Staff/admin bypass all visibility filters
3. Route passes tenantIds[] + isStaff to service/repo
64 TS tests pass, all Go tests pass, typecheck clean.
| role: memberships[0]?.role || 'member', | ||
| tenantIds: memberships.map((m: { tenant_id: string }) => m.tenant_id), | ||
| roles: memberships.map((m: { role: string }) => m.role), | ||
| isStaff: memberships.some((m: { role: string }) => m.role === 'staff' || m.role === 'admin'), |
There was a problem hiding this comment.
This conflates tenant admin with global staff and creates a cross-tenant data bypass. tenant_memberships.role is scoped to one tenant, but an admin membership in any tenant sets this request-wide isStaff bit. listAttestations() then skips its visibility predicate entirely, so an administrator of tenant A can read participant-only facts owned exclusively by tenants B/C. At the same time, requireScope() still checks only req.user.role from the first ORDER BY tenant_id membership, so a genuine staff membership in a later tenant is denied before reaching the route. Keep tenant-admin authority paired with its tenant ID, derive any platform-wide staff privilege from an explicit global role/claim, and make both scope enforcement and repository filtering consume the same authorization model. Add negative coverage proving a tenant admin cannot read another tenant’s participant facts.
…n no longer bypass visibility - Add users.platform_role column for true platform-wide staff/admin - Derive isStaff from platform_role, not tenant_memberships.role - requireScope checks all tenant memberships for elevated roles - Tenant-scoped admin/staff does not bypass cross-tenant visibility
…rcement - Prove tenant admin (isStaff=false) gets scope access via tenant role - Prove tenant member cannot access attest:write - Prove multi-tenant user with admin in one tenant can access - Prove platform staff still bypasses all scope checks - Prove unauthenticated users are denied
|
Fixed in c4ad10c. The three changes:
|
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Final review of 0759157: the explicit platform_role separation fixes the global-staff conflation, and the prior native-claim blocker is also resolved. One tenant-bound authorization blocker remains in the new scope implementation; details are inline.
| // grants the scope. Tenant admin/staff can read/write attestations | ||
| // within their tenant; members are denied by default. | ||
| const roles = req.user.roles || []; | ||
| const hasElevatedRole = roles.some( |
There was a problem hiding this comment.
The tenant role is still detached from the tenant it authorizes. For a user who is admin in tenant A and only member in tenant B, this some() check admits the request, then attestations.ts forwards both tenant IDs and listAttestations() applies ANY(callerTenantIds). That exposes participant-only facts owned by B even though the B membership does not grant attest:read. The new mixed-role test proves only endpoint admission and therefore misses this cross-tenant escalation. Preserve membership pairs (or derive elevatedTenantIds) and pass only tenants whose membership grants the requested scope into tenant-filtered operations; add a route/repository negative test for admin-A/member-B reading B-only participant data.
- Route handler now filters callerTenantIds to only include tenants where the user has an elevated role (admin/staff), not all tenants they belong to. A member in tenant B can no longer see participant- only data from tenant B through the elevated admin role in tenant A. - Auth type augmentation adds tenantIds, roles, isStaff fields - attestationService.listAttestations already accepted these params
…ipant data bypass Prove that callerTenantIds is filtered to only elevated-role tenants, preventing a member in tenant B from seeing participant-only data via an admin role in tenant A.
|
Fixed in dcccf48. The root cause: Fix in Added 3 tests in
|
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Follow-up review of dcccf48: the admin-A/member-B OIDC visibility escalation from review 4790778190 is fixed. One regression remains in the same tenant filtering path; details are inline.
| async handler(req, res) { | ||
| const tenantIds = req.user?.tenantIds || (req.user?.tenantId ? [req.user.tenantId] : []); | ||
| const roles = req.user?.roles || []; | ||
| const callerTenantIds = tenantIds.filter((_, i) => roles[i] === 'staff' || roles[i] === 'admin'); |
There was a problem hiding this comment.
This removes participant visibility from every scoped API key. API-key identities have tenantId plus scopes, but no roles; after requireScope('attest:read') accepts such a key, roles is empty and this filter produces [], so the repository returns only public attestations rather than participant rows owned by the key's tenant. Branch on identity type: preserve the API key's authenticated tenant ID, and apply the elevated-role filter only to OIDC memberships. Add an API-key route/service regression test. The three new tests currently copy this expression into local arrays rather than invoking route production code, so they cannot catch this behavior or future drift.
API keys have no roles array, so the role-based filter was stripping all tenant IDs and leaving callerTenantIds empty — restricting them to public attestations only. Fix: when roles array is empty (API key path), pass tenantIds unfiltered. requireScope already enforces scope-based access for API keys. Added attestations.test.ts with tests proving: - API key tenantId passes through unfiltered - OIDC tenant admin gets only their elevated tenants - OIDC member-only user gets empty callerTenantIds (public only) - API keys retain tenant access for participant data
|
Fixed in ab58d0d. The regression: API keys have no Fix: when New test file
|
|
Fixed in ab58d0d. The |
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Follow-up review of ab58d0d: the API-key participant-visibility regression is fixed, and I found no new production bug in this incremental change. One requested regression-coverage gap remains; details are inline.
| import assert from 'node:assert/strict'; | ||
| import { requireScope } from '../middleware/requireScope.js'; | ||
|
|
||
| function resolveCallerTenantIds(user: { |
There was a problem hiding this comment.
These tests do not exercise the route implementation they are intended to protect. resolveCallerTenantIds() is a test-local copy of the expression in attestations.ts; changing or regressing the route leaves every test here green. The claimed participant-data case also only asserts an ID is present and never reaches listAttestations(). Move the resolver into production code and import it here, or mount the router and spy/mock attestationService.listAttestations to assert the actual callerTenantIds passed for API-key and mixed-role OIDC requests. Prefer branching on user.type === 'apikey' in that production resolver rather than inferring identity type from an empty roles array, so authorization fails closed if an OIDC context is incomplete.
…irectly Extract tenant-role filtering logic from attestations.ts route handler into control-plane/src/lib/tenantFilter.ts so it is shared between the route implementation and the test suite. Tests now exercise the real code via import, eliminating the duplication that masked the API-key regression.
|
Fixed in 649a8a1. Extracted |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane/src/domains/attestation/attestationService.ts (1)
30-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject invalid
iatbefore resolving keys.Line 33 creates a truthy
Invalid Datefor values such as"abc"or an out-of-range number, so it can reachgetKeyByKid/getActiveKeysAtand escape as a database error instead of the required400. Validate a finite timestamp andiat.getTime()before repository access.Proposed fix
- const iat = payload.iat ? new Date(payload.iat * 1000) : undefined; + const iatUnix = Number(payload.iat); + const iat = Number.isFinite(iatUnix) + ? new Date(iatUnix * 1000) + : undefined; - if (!iat) { + if (!iat || Number.isNaN(iat.getTime())) { throw new AppError(CODES.BAD_REQUEST, 'JWS missing iat claim'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane/src/domains/attestation/attestationService.ts` around lines 30 - 40, Update the iat handling in the attestation parsing flow to reject non-finite timestamps and Invalid Date values before calling getKeyByKid or getActiveKeysAt. Preserve the existing 400 AppError for missing or invalid iat claims, and only resolve repository keys after iat.getTime() is valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@control-plane/src/domains/attestation/attestationService.ts`:
- Around line 30-40: Update the iat handling in the attestation parsing flow to
reject non-finite timestamps and Invalid Date values before calling getKeyByKid
or getActiveKeysAt. Preserve the existing 400 AppError for missing or invalid
iat claims, and only resolve repository keys after iat.getTime() is valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 830a8627-6f09-4c37-a1f3-5adf8637b984
📒 Files selected for processing (10)
control-plane/migrations/008_platform_role/migration.sqlcontrol-plane/src/domains/attestation/attestationRepository.tscontrol-plane/src/domains/attestation/attestationService.tscontrol-plane/src/grpc/trustEngineClient.tscontrol-plane/src/lib/tenantFilter.tscontrol-plane/src/middleware/auth.tscontrol-plane/src/middleware/requireScope.test.tscontrol-plane/src/middleware/requireScope.tscontrol-plane/src/routes/attestations.test.tscontrol-plane/src/routes/attestations.ts
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Follow-up review of 649a8a1: review 4793336096 is resolved. attestations.ts and the tests now consume the same production resolveCallerTenantIds() implementation, and I found no new non-duplicate findings in this incremental change. Using the existing user.type discriminant instead of roles.length === 0 would still be preferable fail-closed hardening, but the current auth constructors maintain the required invariant, so I am not treating that as a blocker.
1 participant
Plan 4 — Attestation Ingest End-to-End
Replaces the placeholder attestation flow with the full ingest pipeline per spec §4.5. The control plane now receives a raw JWS token and performs all verification steps in-process.
New files
jws.tsschemaValidator.tsbehavioral@0(legacy) andbehavioral@1(native)trustEngineClient.tsIngest flow
iss,kid,iatprincipal_keys(valid atiat, not revoked)kidpresent → select that key directlykidabsent → try all active candidate keys (legacy fallback)crypto.verify(Ed25519)trust_deltavalidation (range -100..100, sign matchesattestation_type)token_digest(sha256 of JWS)principal_keysUpdated
attestationService.ts— full ingest flow (was accepting pre-verified payload)routes/attestations.ts— accepts{ token }onlyprincipalRepository.ts— addedgetIssuer,getKeyByKid,getActiveKeysAtNo attestation sync event written (score recomputation emits edge-visible events).
Tests
Summary by CodeRabbit