feat(api): implement API mode endpoints for issues 76-79#82
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new HTTP API server supporting payments, webhooks, and forwarding targets, along with CLI and configuration updates to run the daemon in webhook, API, or combined modes. The review feedback highlights critical stability and security concerns in the API server, including potential memory leaks from unbounded maps (rateBuckets and idempotencyMap), a vulnerability to Denial of Service (DoS) due to unlimited request body sizes in JSON decoding, and a high risk of request ID collisions from timestamp-based generation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| s.bucketsMu.Lock() | ||
| defer s.bucketsMu.Unlock() | ||
|
|
||
| bucket, ok := s.rateBuckets[key] | ||
| if !ok { | ||
| bucket = &rateBucket{start: window} | ||
| s.rateBuckets[key] = bucket | ||
| } | ||
|
|
||
| if bucket.start != window { | ||
| bucket.start = window | ||
| bucket.count = 0 | ||
| } | ||
| if bucket.count >= s.rateLimit { | ||
| return false | ||
| } | ||
| bucket.count++ | ||
| return true |
There was a problem hiding this comment.
The rateBuckets map grows indefinitely as new unique IP addresses or API keys make requests. Since there is no cleanup mechanism or eviction policy, this can lead to a memory leak and eventual Out-Of-Memory (OOM) crash under high load or over time.
To prevent this, we should periodically clean up old inactive buckets from the map when its size exceeds a certain threshold.
s.bucketsMu.Lock()
defer s.bucketsMu.Unlock()
// Clean up old buckets to prevent memory leak
if len(s.rateBuckets) > 2000 {
for k, b := range s.rateBuckets {
if b.start != window {
delete(s.rateBuckets, k)
}
}
}
bucket, ok := s.rateBuckets[key]
if !ok {
bucket = &rateBucket{start: window}
s.rateBuckets[key] = bucket
}
if bucket.start != window {
bucket.start = window
bucket.count = 0
}
if bucket.count >= s.rateLimit {
return false
}
bucket.count++
return true| func decodeJSONBody(r *http.Request, out any) error { | ||
| decoder := json.NewDecoder(r.Body) | ||
| if err := decoder.Decode(out); err != nil { | ||
| return err | ||
| } | ||
| if decoder.More() { | ||
| return NewError(http.StatusBadRequest, errBadRequest, "invalid JSON body") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The decodeJSONBody function decodes the request body directly without limiting its size. An attacker could send an extremely large JSON payload to exhaust server memory and cause a Denial of Service (DoS) via Out-Of-Memory (OOM).
It is a security best practice to wrap the request body with http.MaxBytesReader to enforce a reasonable limit (e.g., 1MB) before decoding.
func decodeJSONBody(r *http.Request, out any) error {
// Limit request body to 1MB to prevent OOM/DoS attacks
r.Body = http.MaxBytesReader(nil, r.Body, 1048576)
decoder := json.NewDecoder(r.Body)
if err := decoder.Decode(out); err != nil {
return err
}
if decoder.More() {
return NewError(http.StatusBadRequest, errBadRequest, "invalid JSON body")
}
return nil
}| func (s *Server) readIdempotent(cacheKey string) (any, bool) { | ||
| if strings.TrimSpace(cacheKey) == "" { | ||
| return nil, false | ||
| } | ||
| s.idempotencyMu.Lock() | ||
| defer s.idempotencyMu.Unlock() | ||
| cached, ok := s.idempotencyMap[cacheKey] | ||
| return cached, ok | ||
| } | ||
|
|
||
| func (s *Server) writeIdempotent(cacheKey string, payload any) { | ||
| if strings.TrimSpace(cacheKey) == "" { | ||
| return | ||
| } | ||
| s.idempotencyMu.Lock() | ||
| defer s.idempotencyMu.Unlock() | ||
| s.idempotencyMap[cacheKey] = payload | ||
| } |
There was a problem hiding this comment.
The idempotencyMap stores cached responses indefinitely without any eviction policy or TTL. Over time, this will cause a memory leak and potential Out-Of-Memory (OOM) crashes as more unique idempotency keys are processed.
We should introduce a structured entry that tracks the creation time of each cached response and evict entries older than 24 hours when the map size exceeds a safe threshold.
type idempotentEntry struct {
payload any
createdAt time.Time
}
func (s *Server) readIdempotent(cacheKey string) (any, bool) {
if strings.TrimSpace(cacheKey) == "" {
return nil, false
}
s.idempotencyMu.Lock()
defer s.idempotencyMu.Unlock()
cached, ok := s.idempotencyMap[cacheKey]
if !ok {
return nil, false
}
if entry, ok := cached.(idempotentEntry); ok {
return entry.payload, true
}
return cached, true
}
func (s *Server) writeIdempotent(cacheKey string, payload any) {
if strings.TrimSpace(cacheKey) == "" {
return
}
s.idempotencyMu.Lock()
defer s.idempotencyMu.Unlock()
// Evict old entries if map grows too large to prevent OOM
if len(s.idempotencyMap) > 5000 {
now := time.Now()
for k, v := range s.idempotencyMap {
if entry, ok := v.(idempotentEntry); ok {
if now.Sub(entry.createdAt) > 24*time.Hour {
delete(s.idempotencyMap, k)
}
} else {
delete(s.idempotencyMap, k)
}
}
}
s.idempotencyMap[cacheKey] = idempotentEntry{
payload: payload,
createdAt: time.Now(),
}
}| import ( | ||
| "context" | ||
| "database/sql" | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "log" | ||
| "net/http" | ||
| "strconv" | ||
| "strings" | ||
| "sync" | ||
| "time" |
There was a problem hiding this comment.
Add "crypto/rand" to the imports block to support secure random request ID generation.
| import ( | |
| "context" | |
| "database/sql" | |
| "encoding/json" | |
| "errors" | |
| "fmt" | |
| "log" | |
| "net/http" | |
| "strconv" | |
| "strings" | |
| "sync" | |
| "time" | |
| import ( | |
| "context" | |
| "crypto/rand" | |
| "database/sql" | |
| "encoding/json" | |
| "errors" | |
| "fmt" | |
| "log" | |
| "net/http" | |
| "strconv" | |
| "strings" | |
| "sync" | |
| "time" |
| func newRequestID() string { | ||
| return time.Now().UTC().Format(time.RFC3339Nano) | ||
| } |
There was a problem hiding this comment.
Using time.Now().UTC().Format(time.RFC3339Nano) to generate request IDs is highly prone to collisions under high concurrency (multiple requests arriving at the exact same nanosecond). It also exposes precise server timing information.
We should use a cryptographically secure pseudo-random generator (crypto/rand) to generate unique, collision-free request IDs.
func newRequestID() string {
b := make([]byte, 16)
if _, err := rand.Read(b); err != nil {
return fmt.Sprintf("req_%d", time.Now().UnixNano())
}
return fmt.Sprintf("%x-%x-%x-%x-%x", b[0:4], b[4:6], b[6:8], b[8:10], b[10:])
}![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: 50599f0e95
ℹ️ 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".
|
|
||
| result, err := service.Status(r.Context(), paymentsvc.StatusInput{ | ||
| Provider: intent.ProviderCode, | ||
| Environment: s.environment, |
There was a problem hiding this comment.
When POST /api/v1/payments creates a payment with environment=production while the daemon default remains sandbox, this follow-up status call passes s.environment instead of the environment used for that payment. paymentsvc.Status loads provider credentials for the supplied environment, so production payments are queried with sandbox credentials (or reported as not configured); the same pattern is present for refund and reconcile. Store/recover the payment environment, or require it on these follow-up endpoints, before calling the payment service.
Useful? React with 👍 / 👎.
| func (s *Store) ListPaymentIntents(ctx context.Context, provider domain.ProviderCode, environment domain.Environment, status domain.PaymentStatus, limit, offset int) ([]domain.PaymentIntent, error) { | ||
| _ = environment |
There was a problem hiding this comment.
GET /api/v1/payments?environment=production validates and passes the environment filter, but the store discards it here, so callers receive the same mixed payment list and total regardless of whether they ask for sandbox or production. Since the create API accepts an environment, this makes environment-scoped diagnostics unreliable; the query needs an environment persisted on payments or another reliable predicate instead of ignoring the argument.
Useful? React with 👍 / 👎.
| if status != "" { | ||
| query.WriteString(" AND we.event_type = ?") | ||
| args = append(args, status) |
There was a problem hiding this comment.
For GET /api/v1/webhook-events?status=..., the API exposes processing_status on each rendered event, but this predicate compares the requested status to event_type. In contexts like status=processed or status=failed, the list and matching count omit the intended events unless their event type happens to equal that status, so webhook diagnostics return misleading results; filter we.processing_status instead.
Useful? React with 👍 / 👎.
wauputr4
force-pushed
the
codex/api-core-74
branch
from
cc89849 to
e3dd7a2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54a68b4182
ℹ️ 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".
| key := strings.TrimSpace(r.Header.Get("X-API-Key")) | ||
| if key == "" { | ||
| key = remoteAddr |
There was a problem hiding this comment.
When a request supplies an invalid X-API-Key, this uses that unverified header value as the rate-limit bucket before authentication runs, so an unauthenticated client can avoid the per-minute limit by changing the bogus key on each request. For protected API endpoints exposed to the network, this defeats the intended throttling of failed auth attempts and also lets an attacker grow rateBuckets with arbitrary keys; use the remote address until the key has been validated, or only bucket by the configured API key after successful auth.
Useful? React with 👍 / 👎.
| '404': | ||
| $ref: '#/components/schemas/ErrorResponse' |
There was a problem hiding this comment.
In an OpenAPI responses map, a $ref must point to a Response Object, not directly to a schema. This and the other error entries that reference #/components/schemas/ErrorResponse make the newly added spec invalid for validators/code generators, so clients generated from internal/api/openapi.yaml can fail before reaching the API; wrap the error schema under a response component with description and content and reference that instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc62e20e47
ℹ️ 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 err != nil { | ||
| return nil, err | ||
| } | ||
| environment, err := parseEnvironment(payload.Environment) |
There was a problem hiding this comment.
When the API daemon is started with --environment production (or RUTE_BAYAR_ENV=production) and the client omits the optional environment field, this call falls through parseEnvironment(""), which hard-codes sandbox instead of using s.environment. The OpenAPI schema does not require environment, so production API clients can create sandbox payments with the wrong provider credentials unless they remember to send the field on every request; default the empty payload value from the server environment before building CreateInput.
Useful? React with 👍 / 👎.
| event := auditlog.Event{ | ||
| RequestID: requestID, | ||
| ActorType: "api-client", | ||
| ActorID: strings.TrimSpace(r.Header.Get("X-API-Key")), |
There was a problem hiding this comment.
For every authenticated API request, this copies the full X-API-Key secret into auditlog.Event.ActorID, and sqlite.RecordAuditEvent persists that value in audit_logs.actor_id. In deployments with API-key auth enabled, a routine audit-log export or database read now exposes the credential needed to call the API; store a non-secret identifier or a hash/fingerprint instead of the raw header value.
Useful? React with 👍 / 👎.
| required: | ||
| - reference | ||
| - provider |
There was a problem hiding this comment.
POST /api/v1/payments rejects omitted or zero amounts with amount must be greater than zero, but the PaymentIntent request schema only requires reference and provider. Clients generated from this contract can therefore send schema-valid payment-create requests that always fail at runtime; add amount to the required list so the spec matches the handler.
Useful? React with 👍 / 👎.
1 participant
Summary
webhook,api, andallexecution via CLIwebhook serve --mode.Closes #74 #75 #76 #77 #78 #79