Add Go ConnectRPC API foundation#11
Conversation
|
Superseded Droid error —— View job Droid Code ReviewThe new Go ConnectRPC service is structurally sound and integrates cleanly with the web client, but 7 comments posted (1 × P1, 4 × P2, 2 × P3):
Superseded by a newer commit. |
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
| WHERE us.id = $1 | ||
| AND us.token_hash = $2 | ||
| AND us.revoked_at IS NULL | ||
| AND us.expires_at > NOW() |
There was a problem hiding this comment.
[P1] totalRiskScore diverges from the existing weighted/clamped REST metric
The REST endpoint (apps/api/src/routes/dashboard.ts) computes totalRiskScore via aggregateRiskScore(calculateFindingRiskScore(...)) from packages/shared/src/risk-scoring.ts: each open finding's base risk_score is raised to a severity floor, adjusted by evidence/age bonuses, clamped to [0,100], then aggregated as highest*0.72 + decayed residual + capped bonuses and clamped again to [0,100]. This Go query instead returns COALESCE(SUM(CASE WHEN sf.status='OPEN' THEN sf.risk_score ELSE 0 END),0)::int — an unbounded raw sum of the base column with no severity floor, bonuses, decay, or clamping. For any org with more than one open finding the two backends return materially different numbers (the Go value can far exceed 100), so the dashboard shows a different totalRiskScore depending on whether NEXT_PUBLIC_CONNECT_API_BASE_URL routes to the Go service. This breaks parity with the documented 0–100 posture metric.
| ConnectedApps: metrics.ConnectedApps, | ||
| EventIngestionRate: metrics.EventIngestionRate, | ||
| }, | ||
| }), nil |
There was a problem hiding this comment.
[P2] [security] Session validation omits the idle-timeout control enforced by the TS middleware
The TS verifySessionToken (apps/api/src/middleware/security.ts) rejects and revokes a session when lastSeenAt + APERIO_SESSION_IDLE_MINUTES (default 120m) <= now, in addition to checking revoked_at, expires_at, is_active, and MFA, and it refreshes last_seen_at on use. This Go query only filters on revoked_at IS NULL, expires_at > NOW(), u.is_active, and the MFA condition; it never consults last_seen_at. A stolen or abandoned session that the REST API would treat as idle-expired (inactive for >2h but still within the 12h absolute TTL) remains fully usable through the Go Connect endpoint, weakening an existing session-security control and producing inconsistent expiry behavior across backends (CWE-613). Add the idle-window predicate (and a matching idle-minutes config value) to restore parity.
| // ready for lower-level pgx usage as more endpoints move to Go. | ||
| db, err := sql.Open("pgx", cfg.DatabaseURL) | ||
| if err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
[P2] [security] http.Server has no timeouts, enabling Slowloris/resource-exhaustion DoS
The server is constructed with only Addr and Handler, leaving ReadTimeout, ReadHeaderTimeout, WriteTimeout, and IdleTimeout unset (no limit). An unauthenticated client can open many connections and trickle request headers/bodies one byte at a time (Slowloris), holding goroutines and connections open indefinitely and exhausting resources on this public-facing service (CWE-400). Set at least ReadHeaderTimeout/ReadTimeout/WriteTimeout/IdleTimeout on the server.
|
|
||
| - uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: "1.24" |
There was a problem hiding this comment.
[P2] Go toolchain version mismatch between go.mod and setup-go
go.mod declares go 1.25.0, but the go-connect job pins setup-go to 1.24. Every go invocation (go mod download, the go run buf, go test ./...) sees a required version higher than the installed 1.24 toolchain and only works because the default GOTOOLCHAIN=auto silently downloads and switches to go1.25.0 at runtime. This defeats the pin (the cached 1.24 install is unused, builds are slower) and would hard-fail under GOTOOLCHAIN=local or a restricted network. Bump the pin to match go.mod.
| go-version: "1.24" | |
| go-version: "1.25" |
|
|
||
| - run: go mod download | ||
|
|
||
| - run: go run github.com/bufbuild/buf/cmd/buf@v1.59.0 lint |
There was a problem hiding this comment.
[P2] CI lints protos but never verifies generated code is up to date
The only proto-related check is buf lint; CI never runs buf generate and diffs against the committed output, so drift between the .proto sources / buf.gen.yaml and the checked-in generated code goes undetected. This is already demonstrated concretely: buf.gen.yaml pins connectrpc/es v2.1.1 and bufbuild/es v2.10.0 (api_pb.ts is v2.10.0 output), yet the committed packages/connect/src/gen/aperio/v1/api_connect.ts is stale protoc-gen-connect-es v1.6.1 output. Add a buf generate + git diff --exit-code step so generated drift fails CI.
| COUNT(*) FILTER (WHERE sf.status = 'OPEN' AND sf.severity = 'CRITICAL')::int, | ||
| (SELECT COUNT(*)::int FROM integration_connections ic WHERE ic.organization_id = $1 AND ic.status = 'CONNECTED'), | ||
| (SELECT COUNT(*)::int FROM ingested_events ie WHERE ie.organization_id = $1 AND ie.created_at >= $2) | ||
| FROM security_findings sf |
There was a problem hiding this comment.
[P3] [security] No server-side Origin enforcement for the cookie-authenticated Connect endpoint
GetDashboardMetrics is authenticated via the aperio_session cookie (the web client sends credentials:"include"), but withCORS only sets CORS response headers and always calls next.ServeHTTP regardless of Origin. CORS response headers do not stop the server from executing the request, so unlike the REST middleware's hasAllowedCookieOrigin check (which 403s cookie-auth requests lacking an allowed Origin), there is no equivalent server-side origin gate here (CWE-352). Browser-based exploitation is largely mitigated because the Connect protocol's mandatory connect-protocol-version header forces a preflight that fails for disallowed origins and the endpoint is read-only, but the defense-in-depth origin check present in the REST path is missing. Enforce the Origin allow-list server-side before processing cookie-authenticated requests.
| /** | ||
| * @generated from service aperio.v1.AperioService | ||
| */ | ||
| export const AperioService = { |
There was a problem hiding this comment.
[P3] Stale connect-es v1 descriptor duplicates and conflicts with the v2 codegen
This file is generated by protoc-gen-connect-es v1.6.1 and exports a second AperioService that is incompatible with the installed v2 runtime (@connectrpc/connect ^2.1.1, @bufbuild/protobuf ^2.12.0). It imports the message symbols as runtime values from ./api_pb.js (CheckHealthRequest, etc.), but the v2 api_pb.ts exports those only as TS types plus *Schema descriptors, so this module would fail if imported and survives compilation only via @ts-nocheck. client.ts correctly imports the v2 AperioService from ./gen/aperio/v1/api_pb, so there is no current breakage, but shipping two same-named exports invites a future import from the wrong module that would break createClient(AperioService, transport). Delete this legacy generated file.
|
Droid finished @jonathanhaaswriter's task —— View job Droid Code ReviewThe Go ConnectRPC foundation is structurally sound and integrates cleanly with the web client. One additional validated issue was found: the Go CORS 1 comment posted (1 × P2):
One additional candidate (buf |
| return Config{ | ||
| Addr: env("APERIO_CONNECT_ADDR", ":4100"), | ||
| DatabaseURL: strings.TrimSpace(os.Getenv("DATABASE_URL")), | ||
| WebOrigin: strings.TrimRight(env("APERIO_WEB_ORIGIN", "http://localhost:3000"), "/"), |
There was a problem hiding this comment.
[P2] WebOrigin only supports a single origin, breaking credentialed RPCs in multi-origin deployments
FromEnv stores APERIO_WEB_ORIGIN verbatim into the single-valued Config.WebOrigin, and withCORS only emits Access-Control-Allow-Origin/Allow-Credentials when the request Origin string-equals that value (origin == a.cfg.WebOrigin). The TypeScript API reads the SAME env var via allowedWebOrigins() in apps/api/src/middleware/security.ts, which splits on ',' to support a comma-separated list of origins. In any deployment that sets APERIO_WEB_ORIGIN to a list (e.g. "https://app.aperio.com,https://staging.aperio.com")—the format the shared variable already supports on the Express side—Config.WebOrigin becomes the whole literal string, so no real browser Origin can ever match. The Go service then omits the CORS headers entirely and the browser blocks every credentialed Connect call, making the migrated GetDashboardMetrics endpoint unreachable from the web client. Parse WebOrigin into a trimmed allow-list and match membership, mirroring allowedWebOrigins().
|
Droid finished @jonathanhaaswriter's task —— View job Droid Code ReviewThis revision cleanly addresses the previously reported session idle-timeout, server-side Origin enforcement, HTTP server timeouts, and generated-code drift concerns. The one remaining correctness issue is that the ported Go per-finding risk scorer implements only the visibility and age bonuses, so 1 comment posted (1 × P1):
|
| )), 0, 100) | ||
| } | ||
|
|
||
| func calculateFindingRiskScore(finding riskFinding) int { |
There was a problem hiding this comment.
[P1] Ported calculateFindingRiskScore omits most evidence bonuses, so totalRiskScore still diverges from REST
This Go port of the per-finding scorer only applies the visibility bonus and the detected-at age adjustment, but the source of truth in packages/shared/src/risk-scoring.ts also adds bonuses for grantedRole/role (admin/super admin +6/+10), delegatedAdmin (+4), mfaEnrolled=false (+10), mfaEnforced=false (+8), riskReason mailbox (+4/+8), scopeCount, delegateCount/sendAsCount, comboKinds (+8), and externalEmailCount (up to +12). The REST endpoint (apps/api/src/routes/dashboard.ts) maps each open finding through calculateFindingRiskScore and feeds those scored values into aggregateRiskScore, and the Go aggregateRiskScore likewise builds highest*0.72 + weighted residual + capped bonuses from calculateFindingRiskScore(finding). Because the Go per-finding scorer omits those branches, any OPEN finding whose evidence carries those fields — the common case for the existing connectors (admin grants, MFA gaps, external sharing, OAuth scopes) — yields a different totalRiskScore from the Go endpoint than the REST endpoint, so the dashboard shows different numbers depending on whether NEXT_PUBLIC_CONNECT_API_BASE_URL routes to Go. The prior raw-SUM divergence was replaced, but parity is still not achieved. Port the remaining bonus branches (and the evidence parsing helpers they depend on) to match calculateFindingRiskScore.
|
Superseded Droid error —— View job Superseded Droid review in progress… Superseded by a newer commit. |
|
Droid finished @jonathanhaaswriter's task —— View job Droid Code ReviewThe Go ConnectRPC foundation is well-structured and the previously reported correctness, auth-parity, and DoS concerns are confirmed fixed at HEAD. The one remaining issue is that the new generated-code drift gate uses 1 comment posted (1 × P2):
|
| - name: Verify generated code is current | ||
| run: | | ||
| go run github.com/bufbuild/buf/cmd/buf@v1.59.0 generate | ||
| git diff --exit-code gen packages/connect/src/gen |
There was a problem hiding this comment.
[P2] Generated-code drift check misses newly created (untracked) files
The drift gate runs buf generate then git diff --exit-code gen packages/connect/src/gen. git diff only inspects tracked files, so modified or deleted committed outputs are caught, but a brand-new generated file is untracked and invisible to git diff, which then exits 0. Adding a new proto file/message/service (e.g. a new *.pb.go or *_pb.ts) and forgetting to commit the regenerated output therefore leaves CI green while the committed code is out of sync — exactly the drift this step was added to prevent. Stage first so untracked output is included in the comparison.
| git diff --exit-code gen packages/connect/src/gen | |
| git add -A gen packages/connect/src/gen && git diff --cached --exit-code gen packages/connect/src/gen |
|
Droid finished @jonathanhaaswriter's task —— View job Droid Code ReviewThe Go ConnectRPC foundation is well-structured and integrates cleanly with the web client. All previously reported issues are resolved at this HEAD: the ported risk scorer ( No new high-confidence, actionable issues were found in this round. 0 comments posted. |
1 participant
Summary
Validation