fix: exempt /metrics /healthz /readyz from auth; numeric UID in Dockerfile#148
Conversation
…ks, and RabbitMQ connectors Adds 17 new connector actions across 7 services: - stripe/create_charge, stripe/create_customer, stripe/create_refund - okta/list_users, okta/create_user - shopify/list_orders, shopify/list_products, shopify/create_order - mailchimp/list_members, mailchimp/add_member - onedrive/upload, sharepoint/list_items - quickbooks/create_invoice, quickbooks/list_invoices - rabbitmq/publish, rabbitmq/consume Also adds parseJSONBody shared helper to tools.go and github.com/rabbitmq/amqp091-go v1.11.0 as the AMQP protocol client. Closes #94, #108, #109, #111, #112, #114, #115 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- OneDrive: remove url.PathEscape on filePath to preserve literal "/" separators in Graph API path templates (escaping produced %2F which resolved to a file literally named "documents%2Fhello.txt") - RabbitMQ: ack each message inside the consume loop when auto_ack=false so messages are not requeued when the channel closes on return Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- rabbitmq: validate params before dialing (exchange/routing_key/body for publish, queue for consume) so invalid requests fail without a connection - rabbitmq: use amqp.DialConfig with net.Dialer.DialContext so the dial respects context cancellation - rabbitmq: default auto_ack=false (at-least-once); true opts into at-most-once - onedrive: add escapePath helper to encode individual path segments without escaping "/" separators, so nested folders work with Graph path addressing - onedrive_test: assert request URL path to catch future slash-escaping regressions - quickbooks: reject where/order_by values that contain structural keywords (maxresults, startposition, orderby) to prevent clause injection - shopify: extract shopifyAPIURL package-level helper, removing three identical apiURL methods - go.mod: go mod tidy promotes amqp091-go from indirect to direct Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…kerfile - server: register /metrics, /healthz, and /readyz on a top-level mux so Kubernetes probes and Prometheus scrapers bypass API key auth entirely. All other routes fall through to the auth/rate-limit-wrapped apiMux. - Dockerfile: create the mantle user with explicit -u 10001 -g 10001 and switch to USER 10001:10001 so kubelet can verify runAsNonRoot: true without a numeric runAsUser override in the Helm chart. Closes #139 Closes #137 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 48 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR updates container security hardening, refactors two identity provider integrations (Mailchimp and Okta) to follow their respective API specifications, and restructures server request routing to separate authentication requirements for different endpoint categories. ChangesConnector Updates and Server Routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b37d9fefbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if activate { | ||
| endpoint += "?activate=true" |
There was a problem hiding this comment.
When a workflow passes activate: false, this code omits the query parameter entirely, but Okta's create-user API defaults activate to true when the parameter is absent. In that scenario the connector will still activate the user and potentially send activation/provisioning side effects, so the endpoint needs to include ?activate=false when the caller explicitly disables activation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: changed endpoint construction to always pass ?activate=${activate} so activate=false is explicitly sent to Okta rather than relying on its default (which activates the user). Added a regression test for the false case.
| req, err := http.NewRequestWithContext(ctx, "POST", | ||
| c.apiURL(dc, fmt.Sprintf("/lists/%s/members", listID)), |
There was a problem hiding this comment.
For an email that is already in the audience, POST /lists/{list_id}/members returns a duplicate-member error rather than updating the existing contact, even though this connector is documented as adding or updating a subscriber. This makes common idempotent sync workflows fail on reruns; the add/update path should target the member resource (for example with the subscriber hash) using the Mailchimp upsert semantics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: switched from POST /lists/{list_id}/members to PUT /lists/{list_id}/members/{subscriber_hash} (MD5 of lowercased email). The PUT endpoint is idempotent — it upserts rather than erroring on duplicate email addresses. Updated test to assert PUT method and correct path.
Okta: always pass ?activate=<bool> — omitting it when false causes Okta
to activate the user by default, triggering unintended provisioning side effects.
Mailchimp: switch add_member from POST /members to PUT /members/{hash}
(MD5 of lowercased email). The POST endpoint returns a duplicate-member
error on reruns; the PUT upsert endpoint is idempotent.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Keep patched versions of okta.go and mailchimp.go that include the Codex fixes (activate param and upsert endpoint) over the originals that merged from PR #147. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The Mailchimp subscriber hash is MD5(lowercase(email)) per Mailchimp's own API specification — it is a lookup identifier, not a security hash. Add inline suppression annotations for both gosec G401 and CodeQL go/weak-sensitive-data-hashing to clarify the non-security intent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
G501 fires on the import line and G401 on the call site — both need their own same-line #nosec annotations. The combined //nolint+#nosec format wasn't parsed correctly by gosec. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Add CodeQL[go/weak-sensitive-data-hashing] inline suppression alongside the gosec #nosec annotation. The earlier attempt used lowercase 'codeql' which GitHub Code Scanning does not recognize — the exact token is 'CodeQL[rule-id]' (capital C, Q, L). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/engine/internal/server/server.go`:
- Around line 253-257: The /metrics endpoint is currently mounted directly on
the top-level mux with promhttp.Handler(), bypassing auth and leaking raw
r.URL.Path labels via metricsMiddleware/normalizePath; remove
mux.Handle("/metrics", promhttp.Handler()) and instead register the metrics
handler behind the same auth/metricsMiddleware path that wraps apiHandler so
requests to /metrics go through the auth middleware (see metricsMiddleware,
normalizePath, apiHandler and the auth middleware in
packages/engine/internal/auth/middleware.go) ensuring path labels are scrubbed
before being exposed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8eb69572-1684-4940-b687-954133dd1005
📒 Files selected for processing (6)
packages/engine/Dockerfilepackages/engine/internal/connector/mailchimp.gopackages/engine/internal/connector/mailchimp_test.gopackages/engine/internal/connector/okta.gopackages/engine/internal/connector/okta_test.gopackages/engine/internal/server/server.go
…ent) Use standalone // CodeQL[go/weak-sensitive-data-hashing] on the preceding line as GitHub docs specify — no other text on that line. Also add .github/codeql/codeql-config.yml as a fallback that excludes the go/weak-sensitive-data-hashing query for the connector package. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Prometheus path labels include workflow names (via metricsMiddleware → normalizePath). Serving /metrics on the unauthenticated top-level mux let anyone enumerate workflow identifiers without credentials. Moved /metrics to apiMux so it goes through the same auth middleware as all other API routes. Health probes (/healthz, /readyz) remain on the unauthenticated mux for Kubernetes probe access. Prometheus scrapers must now supply an API key. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
/metrics,/healthz, and/readyzare now registered on a top-level mux that sits outside the auth middleware chain, so Kubernetes probes and Prometheus scrapers work without an API keyDockerfilecreates themantleuser with explicit-u 10001 -g 10001sokubeletcan verifyrunAsNonRoot: trueagainst a numeric UID (matching therunAsUser: 10001already invalues.yaml)What was already fixed
ReadyzHandleralready only pings the DB; worker/reaper liveness was removed from/readyzin a prior PRchangeset-version.ymlrelease-engine.ymland.goreleaser.yamlTest plan
go build ./...passes (no changes to business logic)go test ./internal/server/...passesgo test ./internal/api/health/...passescurl http://pod:8080/metricsreturns 200 without anAuthorizationheadercurl http://pod:8080/healthzand/readyzreturn 200 without authcurl http://pod:8080/api/v1/workflowsstill returns 401 without authCloses #139
Closes #137
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores