Conversation
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
WalkthroughThis pull request upgrades the Go toolchain from 1.25.7 to 1.26.1 across all build configurations and dependencies. It introduces webhook URL blacklist configuration for SSRF protection and adds database connection pool parameters. The API key authentication now uses constant-time comparison. New configuration fields support webhook HTTP timeouts and enqueue retry behavior. Dependencies are updated including OpenAI client, River job queue, OpenTelemetry stack, and security libraries. Pagination validation is enhanced with an invariant check. Test code is modernized to use direct pointer creation instead of helper functions. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/pagination.go (1)
5-24:⚠️ Potential issue | 🟡 MinorMake the sentinel error describe the actual failure.
The new guard checks
hasMore && encodeLast == nil, butErrPaginationInvariantViolatedsayshasMore with empty list. Those are not the same condition, and the doc comment still talks about list emptiness instead of the missing encoder precondition. Rename the error/message, or move the empty-list invariant check to the caller.Suggested cleanup
-// ErrPaginationInvariantViolated indicates hasMore was true with an empty list (repository invariant violation). -var ErrPaginationInvariantViolated = errors.New("pagination invariant violated: hasMore with empty list") +// ErrPaginationEncoderRequired indicates BuildListPaginationMeta was called with hasMore=true and no encoder. +var ErrPaginationEncoderRequired = errors.New("pagination invariant violated: hasMore requires encodeLast") @@ -// encodeLast is called only when hasMore is true to produce next_cursor. Callers must ensure -// that when hasMore is true, the underlying list is non-empty so encodeLast can safely access the last item. +// encodeLast is called only when hasMore is true to produce next_cursor. +// When hasMore is true, callers must pass encodeLast and ensure it can safely encode the last item.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/pagination.go` around lines 5 - 24, The sentinel error ErrPaginationInvariantViolated is misleading for the guard in BuildListPaginationMeta that checks hasMore && encodeLast == nil; rename or replace it with a clearer error (e.g., ErrMissingEncodeLast or errors.New("pagination invariant violated: hasMore is true but encodeLast is nil")) and update the BuildListPaginationMeta doc comment to reflect that callers must supply a non-nil encodeLast when hasMore is true; keep the existing empty-list invariant language only if you also add a separate check/caller responsibility for non-empty lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 46-49: The gosec suppression currently lacks a path and therefore
disables G704/G706/G101 globally; restrict the suppression to test-only files by
adding a path filter limiting it to *_test.go and tests/** (or create two
entries) instead of the global rule — update the linters block that lists
"gosec" and the "text: 'G70[46]|G101'" to include a path: pattern matching test
files (e.g., path: "_test.go$" or path: "tests/") so only test code is excluded,
or alternatively remove the global suppression and add targeted `#nosec` comments
in the specific test files that triggered the false positives.
In `@internal/config/config.go`:
- Around line 243-283: The config parsing added new Database* fields but the
Postgres bootstrap (in pkg/database/postgres.go — look for functions like
NewPostgresDB, OpenPostgres, or NewPGPool that build the pool from the
connection string) never applies them; update that bootstrap to read the loaded
Config (e.g., fields DatabaseMaxConns, DatabaseMinConns,
DatabaseMaxConnLifetime, DatabaseMaxConnIdleTime, DatabaseHealthCheckPeriod,
DatabaseConnectTimeout) and set the corresponding pool options on the
pgxpool.Config (or whatever pool type is used): set MaxConns/MinConns, set
MaxConnLifetime and MaxConnIdleTime using time.Duration, set HealthCheckPeriod,
and apply ConnectTimeout on the connection config before creating the pool so
the environment variables actually change runtime behavior.
In `@internal/service/webhooks_service_test.go`:
- Around line 67-68: Tests currently construct NewWebhooksService with a nil
blacklist so the new URL-host validation paths in CreateWebhook and
UpdateWebhook (loopback/private/blacklisted checks) are never exercised; update
webhooks_service_test.go to instantiate NewWebhooksService with a non-nil
blacklist and add test cases using mockWebhooksRepo/noopPublisher that call
CreateWebhook and UpdateWebhook with URLs that are loopback (e.g., 127.0.0.1),
private (e.g., 10.x.x.x), and entries present in the blacklist to assert the
methods return the expected validation errors/rejections, ensuring the SSRF
guard logic is covered.
In `@internal/service/webhooks_service.go`:
- Around line 121-149: The validateWebhookURLHost function currently only
rejects literal IPs or exact blacklist hits and returns early when blacklist is
empty; change it to always perform address checks: after parsing and
canonicalizing the host (canonicalizeHost/u.Hostname), if host is a literal IP
keep the existing netip.ParseAddr checks, otherwise resolve the hostname (e.g.,
via net.LookupIP or net.DefaultResolver.LookupIPAddr) and iterate all returned
IPs, Unmap each and reject any that IsLoopback, IsPrivate, IsLinkLocalUnicast,
IsLinkLocalMulticast, or IsUnspecified; keep the blacklist check on the
canonical host but also consider blocking any resolved IP that matches a
blacklist entry (if blacklist stores IPs) and remove the early return when
blacklist is empty so literal IP checks still run; finally, duplicate the same
IP resolution+validation in the HTTP client's dial path (the DialContext
implementation used for webhook delivery) to prevent DNS rebinding during
connection establishment.
In `@Makefile`:
- Around line 203-206: Make the goose and river versions consistent: update any
references to GOOSE_VERSION and RIVER_VERSION outside the Makefile (specifically
in the Dockerfile and CI workflow files tests.yml, migrations-validate.yml, and
api-contract-tests.yml) so they use the exact values from the Makefile
(GOOSE_VERSION := v3.27.0 and RIVER_VERSION := v0.31.0), and replace any
occurrences of older tags (v3.26.0, v0.30.2) or `@latest` with these pinned
versions to avoid environment drift.
---
Outside diff comments:
In `@internal/service/pagination.go`:
- Around line 5-24: The sentinel error ErrPaginationInvariantViolated is
misleading for the guard in BuildListPaginationMeta that checks hasMore &&
encodeLast == nil; rename or replace it with a clearer error (e.g.,
ErrMissingEncodeLast or errors.New("pagination invariant violated: hasMore is
true but encodeLast is nil")) and update the BuildListPaginationMeta doc comment
to reflect that callers must supply a non-nil encodeLast when hasMore is true;
keep the existing empty-list invariant language only if you also add a separate
check/caller responsibility for non-empty lists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34fba094-5275-4d4b-bef9-2e5b2b381c9c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.env.example.github/workflows/api-contract-tests.yml.github/workflows/code-quality.yml.github/workflows/migrations-validate.yml.github/workflows/tests.yml.golangci.ymlDockerfileMakefilecmd/api/app.gogo.modinternal/api/middleware/auth.gointernal/config/config.gointernal/service/embedding_provider_test.gointernal/service/pagination.gointernal/service/pagination_test.gointernal/service/webhooks_service.gointernal/service/webhooks_service_test.gotests/integration_test.go
✱ Stainless preview buildsThis PR will update the ✅ hub-typescript studio · code
This comment is auto-generated by GitHub Actions and is automatically kept up to date as you push. |
xernobyl
changed the title
mattinannt
left a comment
mattinannt
left a comment
There was a problem hiding this comment.
AI-assisted review, human-reviewed and approved before posting. Requesting changes for three issues that introduce dead configuration or make webhook delivery less reliable in production:
WEBHOOK_ENQUEUE_*is parsed and documented but not wired into any retry path.WEBHOOK_HTTP_TIMEOUT_SECONDSis parsed and documented but the sender still uses a hard-coded timeout.- The new SSRF-safe dialer now pins delivery to the first resolved IP, which can fail healthy multi-address webhook endpoints.
Recommended direction: either fully wire the new config through the runtime behavior in this PR, or remove the new knobs until the behavior exists. For the dialer, keep the SSRF validation but preserve multi-address connection attempts instead of hard-pinning allowed[0].
2 participants
What does this PR do?
Production hardening: CI upgrades, security hardening, config validation, and hiding webhook signing keys.
1. CI and tooling
make tests2. Config and security
3. Pagination
ErrPaginationInvariantViolatedwhenhasMoreis true butencodeLastis nil4. Hide webhook signing key (security)
signing_keyfrom GET, List, and Update responsesWebhookPublic(nosigning_key),ToWebhookPublic(), andListWebhooksPublicResponseWebhookPublicDataand point GET/PATCH webhook and List webhooks responses to itsigning_key) so clients can store it once5. Other
ptrFloat64with Go 1.22+new()for pointer fieldsnew()usage in testsReview Provenance
Base:
main(orchore/ci_config_security_new_patternif that is the PR base)How should this be tested?
make buildmake tests— all integration tests passmake fmtandmake lint— no new warningsGET /v1/webhooks/{id}— response omitssigning_keyGET /v1/webhooks— list items omitsigning_keyPATCH /v1/webhooks/{id}— response omitssigning_keyPOST /v1/webhooks— response includessigning_keyChecklist
Required
make buildmake testsmake fmtandmake lint; no new warningsAppreciated
docs/if applicablemake tests-coveragefor new logic