Skip to content

Expand Go ConnectRPC read paths#12

Merged
jonathanhaaswriter merged 3 commits into
mainfrom
expand-go-rpc-wide-events
Jun 5, 2026
Merged

Expand Go ConnectRPC read paths#12
jonathanhaaswriter merged 3 commits into
mainfrom
expand-go-rpc-wide-events

Conversation

@jonathanhaaswriter
Copy link
Copy Markdown
Contributor

Summary

  • migrate findings list/detail reads behind Go ConnectRPC with generated TypeScript client support
  • add wide-event telemetry contracts and Go RPC emission for bounded request metrics
  • add contract and Go security workflows inspired by Cerebro validation patterns

Validation

  • go test ./...
  • npm run typecheck
  • npm run verify
  • npm run build:web
  • buf lint
  • buf breaking against origin/main
  • govulncheck with patched Go toolchain

@factory-droid
Copy link
Copy Markdown
Contributor

factory-droid Bot commented Jun 5, 2026

Droid finished @jonathanhaaswriter's task —— View job


Security Review

Code Review

Reviewed the Go ConnectRPC ListFindings/GetFinding handlers, the generated proto/TypeScript client code, and the new telemetry helper. Queries are correctly organization-scoped with bound parameters, and the TS client return shape mirrors the existing REST contract.

No blocking issues found. Both candidate findings were low-severity and did not meet the reporting bar:

  • Unused telemetry API — code-hygiene/maintainability only; unused exported Go symbols don't break the build and cause no runtime/security/correctness issue.
  • Filter values emitted to telemetry — speculative, already-mitigated concern; telemetry.write only marshals JSON to stderr (control chars escaped), so the "high-cardinality/DoS" impact is hypothetical with no realistic exploit path.

No inline comments posted.

@factory-droid
Copy link
Copy Markdown
Contributor

factory-droid Bot commented Jun 5, 2026

Superseded Droid error —— View job


Code Review

Reviewed the new Go ConnectRPC ListFindings/GetFinding handlers, the wide-event telemetry helper, the generated proto/TS client, the Connect web client, and the two new CI workflows. Tenant scoping, parameterized SQL, keyset cursor pagination, risk-score recompute, and NULL handling all correctly mirror the existing REST contract. Findings (2):

  • [P2] [security] go-security.yml runs govulncheck@latest, which is fetched/compiled/executed in CI under the contents:write GITHUB_TOKEN on the push: main trigger — a mutable supply-chain dependency. Sibling workflows pin buf@v1.59.0; pin govulncheck to an exact version.
  • [P3] normalizedLimit clamps limit > 100 to 100 instead of rejecting, diverging from the REST endpoint's zod .max(100) validation that the PR intends to mirror.

Dismissed (already covered or intentional): the "unused telemetry API" item was already considered in a prior review comment; the go 1.26.4 pin in go-security.yml is the intended "patched Go toolchain" called out in the PR description (reverting to 1.25.x would reintroduce stdlib vulns).

Superseded by a newer commit.

@factory-droid
Copy link
Copy Markdown
Contributor

factory-droid Bot commented Jun 5, 2026

Droid finished @jonathanhaaswriter's task —— View job


Security Review

Code Review

Reviewed the new Go ConnectRPC ListFindings/GetFinding handlers, the wide-event telemetry helper, the generated proto/TypeScript client, the Connect web client, and the two new CI workflows. Tenant scoping, parameterized SQL, keyset cursor pagination, risk-score recompute, and NULL handling all correctly mirror the existing REST contract.

No blocking issues found. The two candidate findings were low-severity CI-hardening suggestions (missing least-privilege permissions: blocks in the new go-security.yml and contracts.yml workflows) and did not meet the reporting bar — they are defensive "be safer" items without a concrete, realistic trigger path (the token scope is unverified and the exploit relies on a hypothetical supply-chain compromise of pinned tooling).

No inline comments posted.

@jonathanhaaswriter jonathanhaaswriter merged commit e985c99 into main Jun 5, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant