Skip to content

fix: sanitize JSON body before storing to Postgres to prevent SQLSTATE 22P05#1601

Open
hdot123 wants to merge 41 commits into
looplj:unstablefrom
hdot123:fix/sqlstate-22p05-unicode-escape
Open

fix: sanitize JSON body before storing to Postgres to prevent SQLSTATE 22P05#1601
hdot123 wants to merge 41 commits into
looplj:unstablefrom
hdot123:fix/sqlstate-22p05-unicode-escape

Conversation

@hdot123

@hdot123 hdot123 commented May 5, 2026

Copy link
Copy Markdown

Summary

When storing request bodies in Postgres JSONB columns, invalid Unicode escape sequences (e.g., malformed \u sequences) cause SQLSTATE 22P05 errors: unsupported Unicode escape sequence.

This manifests as warnings in logs:

Failed to save request body due to error, retrying with placeholder
ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05)

Fix

Use xjson.SafeJSONRawMessage() which:

  1. Validates JSON with Go's json.Valid()
  2. If invalid, repairs using the jsonrepair library
  3. Falls back to empty JSON object {} if repair fails

Changes

  • CreateRequest(): sanitize httpRequest.JSONBody with SafeJSONRawMessage() before storing
  • CreateRequestExecution(): sanitize channelRequest.JSONBody with SafeJSONRawMessage() before storing

This prevents the SQLSTATE 22P05 error and eliminates the warning logs when clients like Codex send JSON with invalid Unicode escapes.

Testing

Existing tests should pass. The fix is backward compatible - valid JSON is stored unchanged, only invalid JSON is repaired.

tomoon jc added 30 commits April 27, 2026 19:42
…-D100 Redis连接池 + F-D101 SQLite WAL

Phase 1 第一批修复 (8/16 findings):

F-D4  [P0] http.Client.Timeout: Do() 方法增加 context deadline 守卫,无 deadline 时默认 300s
F-D5  [P0] ResponseHeaderTimeout: Transport 增加 60s 响应头超时
F-D13 [P1] NewHttpClient 零配置: 非 InsecureSkipVerify 模式使用完整 Transport 配置
F-D6  [P0] SaveDataFromReader: defer f.Close() 改为显式 Close + 错误检查
F-D18 [P1] Channels/Models 分页: 补齐 validatePaginationArgs 调用
F-D100[P0] Redis 连接池: Config 增加 7 个池化/超时字段,newRedisOptions 应用默认值
F-D101[P0] SQLite WAL: 追加 PRAGMA journal_mode=WAL + PRAGMA busy_timeout=5000

新增测试:
- TestHttpClient_Timeout_NonStreaming (短超时注入)
- TestHttpClient_Timeout_ContextRespected
- TestNewHttpClient_DefaultTransport
- TestNewHttpClientWithProxy_TimeoutSettings
…露 + F-D15+F-D16+F-D17 Transformer

Phase 1 第二批修复 (8/16 findings):

F-D2  [P0] NoAuth 回退: err!=nil 改为 errors.Is(err, ErrAPIKeyRequired)
F-D3  [P0] NoAuth Key: 硬编码常量改为启动时随机生成
F-D1  [P0] Redis FlushAll: 替换为 SCAN+DEL (Clear) 和 tag-based 删除 (Invalidate)
F-D28 [P1] 日志脱敏: AuthConfig/Request 实现 slog.LogValuer,API Key 显示为 sk-xxxx****xxxx
F-D29 [P1] 错误截断: wrapHttpError 响应体限制 200 字符
F-D30 [P1] 错误白名单: PublicError 接口 + 5xx 返回通用消息
F-D15 [P1] OpenAI outbound: 补上 ReasoningBudget/ReasoningSummary 字段映射
F-D16 [P1] Anthropic stream: content_block_start 非 tool_use 返回空 response
F-D17 [P1] AISDK reasoning: 映射到 ReasoningContent 而非普通 text

测试回归修复:
- copilot_test.go: 502 响应体检查改为 'internal server error'
- F-D21: Merge requestCount + usageAgg into single DB query to eliminate quota check race
- F-D51: Add ±20% random jitter to negative cache TTL (5s base) to prevent thundering herd
- F-D53: Replace unsafe key.(string) type assertions with comma-ok checks in Redis store
- F-D54: Add IsExhausted() + block routing when top candidate is rate-limit exhausted
- F-D55: Reduce tag TTL from 720h (30d) to 168h (7d)
F-D74 [P2] WithBody panic — already fixed, verified.
F-D75 [P2] PrependStream early termination — add runtime finalizer
  and idempotent Close() to ensure underlying stream is closed.
F-D76 [P2] SSE decoder contained context — replace stored ctx field
  with done channel + cancel func; context flows through constructor.
F-D77 [P2] Non-streaming retry body consumption — save original body
  before retry loop and restore before each attempt.
F-D82: Add FixedComplexityLimit(10_000) to GraphQL server to prevent
  DoS via deeply nested or expensive queries.

F-D83: Combine RequestStats 4 separate COUNT queries into a single
  SQL query with conditional aggregation (SUM CASE WHEN).

F-D84: Replace fmt.Printf with slog.Warn in TriggerGcCleanup panic
  recovery handler.

F-D87: Add select default case to pass-through channel send in
  captureRawProviderStream to prevent deadlock when consumer
  hasn't started reading.
…3 fixes

F-D71: Add Rollback() method to migrator with completed-migration tracking.
  Adds a completed[] list and Rollback(ctx) method that processes migrations
  in reverse order on failure, enabling rollback diagnostics.

F-D72: Remove SkipSoftDelete context from GC cleanup to respect soft-delete
  status. Hard-deletion now only targets records not protected by soft-delete.

F-D73: Change S3.SecretKey to *string to distinguish 'no change' (nil) from
  'clear the key' (""). Update mergeSettings and isS3Provided accordingly.

F-D94: Add maxSize parameter to ParseDataURL (default 10MB). Reject data URLs
  whose raw or decoded base64 content exceeds the limit.

F-D95: Add sanitizeOAuthError() helper that masks token-like values in error
  messages before logging. Applied to auto-refresh and persistence error logs.

F-D96: Fix MergeHTTPHeaders to create and return a new map instead of
  modifying dest in-place, preventing data races when dest is shared.

F-D97: Add byte-based capacity limit (100MB) to chunkbuffer alongside the
  chunk count limit. When exceeded, oldest chunks are dropped automatically.
…ixes

F-D49: Merge TokenStats 3 separate aggregation queries into single query
  using conditional SUM(CASE WHEN ...) for today/week/month periods.

F-D59: Fix v0.3.0.go and v0.4.0.go data migrations to use passed-in ctx
  instead of context.Background() for trace/log correlation.

F-D68: Fix WithSystemBypass to propagate errors from WithBypassPrivacy
  instead of silently discarding them. Updated all ~35 callers with
  proper error handling matching each function's return signature.

F-D70: Add project ownership verification to WithProjectID middleware —
  verify authenticated user has UserProject membership before allowing
  access to the requested project ID.
- F-D58: GC cleanup notification (already present on branch-2)
- F-D61: Read backup cron from config, fallback to default
- F-D62: Close overlapping time windows before restoring price with overwrite
- F-D91: Use UTC with Z suffix in backup filenames
- F-D92: Encrypt proxy preset passwords with AES-GCM before storage
- F-D93: RunCleanupNow returns CleanupStats (already present on branch-2)
- NewFromConfig already returns (Cache[T], error) instead of panicking (F-D85)
- lo.Assign syntax already cleaned up (F-D86)
- Regenerate ent code for missing KeyHash predicates
- Fix NewProjectService 1-value return in system.go and v0.3.0.go
- Fix SetKeyHash backfill on immutable field via raw SQL Modify
- Add missing llm import in outbound.go
- Fix broken QueryContext().Scan() chain in dashboard.resolvers.go
F-D7: External storage deletion now retries 3x with exponential backoff
(1s, 2s, 4s). Failures after all retries are logged at Error level and
tracked in Worker.cleanupFailures instead of being silently dropped.

F-D34: Stop() now waits for in-flight cleanup tasks via sync.WaitGroup.
runCleanupWithSystemContext wraps with wg.Add/Done so Stop() can
guarantee graceful shutdown: cancel -> wg.Wait() -> Executor.Shutdown().

F-D72: Removed schematype.SkipSoftDelete(ctx) from runCleanup. GC now
respects soft-delete semantics instead of bypassing them.
… match

F-D25: Split triggerAutoBackup into success/failure paths.
  - Success → UpdateAutoBackupLastRun(ctx, '') updates LastBackupAt + clears error
  - Failure → new UpdateAutoBackupError(ctx, errMsg) only sets LastBackupError

F-D26: Backup now includes Users, Roles, UserProjects, UserRoles, SystemConfig.
  - Added BackupUser/BackupRole/BackupUserProject/BackupUserRole/BackupSystemConfig types
  - Added collectUsers/collectRoles/collectUserProjects/collectUserRoles/collectSystemConfig

F-D27: restoreAPIKeys uses exact match first, then prefix match fallback.
  - Exact key match is tried first for full-length keys
  - Prefix match (8 chars) + name/type disambiguation for masked/truncated keys
- F-D74: WithBody default case now returns error instead of panic(err)
  (signature changed to (*RequestBuilder, error), caller updated)
- F-D50: Nodes resolver implemented using Noder per-ID with IsNotFound handling
- F-D45/46: Added eager loading to Users/Roles/Projects list queries
  (.WithProjectUsers/.WithUserRoles/.WithUsers/.WithRoles)
- F-D47: Added scope guards to CreateChannel, DeleteChannel, CreateUser, DeleteRole
Adapt all NewSystemService and NewDataStorageService calls in test files
to handle the 2-value (*Service, error) return signature.

Files modified:
- internal/server/middleware/thread_test.go
- internal/server/middleware/trace_test.go
- internal/ent/migrate/datamigrate/migrator_test.go
- internal/ent/migrate/datamigrate/v0.4.0_test.go
- internal/server/api/openai_retrieve_test.go
- internal/server/api/request_content_test.go
- internal/server/api/request_live_test.go
tomoon jc and others added 11 commits April 28, 2026 00:45
Remove initialized: true references (replaced by once sync.Once in F2).
Adjust streaming test assertion: systemService is nil so once.Do sets
enabled = false.
When AXONHUB_SANITIZE_SPAWN_AGENT_ARGS=1 is set, strips empty 'items' array
from spawn_agent tool call arguments where message is present and non-empty.
Disabled by default, zero overhead when off.

- New: spawn_agent_sanitizer.go — maybeSanitizeSpawnAgentArgs()
- New: spawn_agent_sanitizer_test.go — 17 test cases
- Modified: outbound_convert.go — non-streaming response exit point
- Modified: aggregator.go — streaming aggregation exit point
…E 22P05

When storing request bodies in Postgres JSONB columns, invalid Unicode
escape sequences (e.g., malformed \u sequences) cause SQLSTATE 22P05
'unsupported Unicode escape sequence' errors.

This fix uses xjson.SafeJSONRawMessage() which:
1. Validates JSON with json.Valid()
2. If invalid, repairs with jsonrepair library
3. Falls back to empty JSON object if repair fails

Changes:
- CreateRequest(): sanitize httpRequest.JSONBody before storing
- CreateRequestExecution(): sanitize channelRequest.JSONBody before storing

Fixes the warning 'Failed to save request body due to error, retrying with
placeholder' that appears when Codex or other clients send JSON with
invalid Unicode escapes.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces comprehensive enhancements including secure API key hashing, JWT revocation, encrypted proxy passwords, and improved backup/restore functionality. It also hardens the system with atomic data storage writes, better error handling to prevent internal leakage, and optimized dashboard queries. Feedback focuses on correcting error suppression in migrations, fixing garbage collection logic for soft-deleted records, and addressing performance concerns such as synchronous cleanup in the dumper and per-request database queries in middleware. Additionally, potential data loss in the pass-through stream and inefficient goroutine usage in the SSE decoder were identified as areas for improvement.

Comment on lines +34 to +37
ctx, err = authz.WithSystemBypass(ctx, "database-migrate")
if err != nil {
return nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The migration returns nil when authz.WithSystemBypass fails. This silently suppresses the error and marks the migration as completed in the Migrator tracking, even though the migration logic was never executed. It should return the actual error to allow the migrator to handle the failure correctly.

Suggested change
ctx, err = authz.WithSystemBypass(ctx, "database-migrate")
if err != nil {
return nil
}
ctx, err = authz.WithSystemBypass(ctx, "database-migrate")
if err != nil {
return err
}

Comment on lines +31 to +34
ctx, err = authz.WithSystemBypass(ctx, "database-migrate")
if err != nil {
return nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the v0.3.0 migration, returning nil on a bypass error silently skips the migration while marking it as successful. The error should be propagated.

Suggested change
ctx, err = authz.WithSystemBypass(ctx, "database-migrate")
if err != nil {
return nil
}
ctx, err = authz.WithSystemBypass(ctx, "database-migrate")
if err != nil {
return err
}

Comment thread internal/server/gc/gc.go
Comment on lines 153 to +155
ctx = ent.NewContext(ctx, w.Ent)
ctx = schematype.SkipSoftDelete(ctx)
// F-D72: Do NOT use SkipSoftDelete. GC should respect soft-delete semantics:
// only hard-delete records that have been soft-deleted past their retention period.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current implementation of the GC worker will not achieve its goal of hard-deleting records to reclaim space. By removing SkipSoftDelete from the context, the ent framework's soft-delete filter will remain active, meaning the query will only see records that have not been deleted yet. Furthermore, the Delete() operation will likely trigger another soft-delete (an UPDATE) rather than a hard DELETE. To hard-delete records that have already been soft-deleted, you must: 1) Use SkipSoftDelete(ctx) to see all records, and 2) Filter by DeletedAt (e.g., DeletedAtLT(cutoff)) instead of CreatedAt to target records that were soft-deleted past the retention period.

Suggested change
ctx = ent.NewContext(ctx, w.Ent)
ctx = schematype.SkipSoftDelete(ctx)
// F-D72: Do NOT use SkipSoftDelete. GC should respect soft-delete semantics:
// only hard-delete records that have been soft-deleted past their retention period.
ctx = ent.NewContext(ctx, w.Ent)
ctx = schematype.SkipSoftDelete(ctx)

Comment thread internal/dumper/dumper.go
}

fmt.Printf("Successfully dumped struct to file: %s\n", fullPath)
d.cleanup()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Calling d.cleanup() synchronously within the dump methods can significantly increase latency for the caller, especially if the dump directory contains many files or if disk I/O is slow (due to os.ReadDir and sort.Slice). Consider running the cleanup process in a background goroutine to avoid blocking the main execution flow.

Suggested change
d.cleanup()
go d.cleanup()

Comment on lines +623 to +624
tmpKey := fmt.Sprintf(".tmp/%s-%s", uuid.New().String(), filepath.Base(key))
if err := afero.WriteFile(fs, tmpKey, data, 0o777); err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The atomicWriteFile function uses a .tmp/ prefix for temporary files. If the underlying filesystem is local and the .tmp directory does not exist, afero.WriteFile will fail. You should ensure the parent directory of the temporary file exists before attempting to write.

Suggested change
tmpKey := fmt.Sprintf(".tmp/%s-%s", uuid.New().String(), filepath.Base(key))
if err := afero.WriteFile(fs, tmpKey, data, 0o777); err != nil {
tmpKey := fmt.Sprintf(".tmp/%s-%s", uuid.New().String(), filepath.Base(key))
_ = fs.MkdirAll(".tmp", 0o777)
if err := afero.WriteFile(fs, tmpKey, data, 0o777); err != nil {

Comment on lines +35 to +37
exists, err := client.UserProject.Query().
Where(userproject.UserID(user.ID), userproject.ProjectID(projectID.ID)).
Exist(c.Request.Context())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Performing a database query to verify project access on every request in the middleware can become a performance bottleneck. Consider caching the user's project access list or using a more efficient way to verify permissions (e.g., including project IDs in the JWT claims if applicable).

Comment on lines +258 to 259
default:
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The default case in the select block will cause events to be silently dropped if the rawStreamCh is full or if the consumer has not started reading yet. This leads to data loss in the pass-through stream. If the intention is to ensure all events are captured, the default case should be removed, or the channel buffer should be appropriately sized.

Comment thread llm/httpclient/decoder.go
Comment on lines +98 to +102
ch := make(chan recvResult, 1)
go func() {
event, err := s.sseStream.Recv()
ch <- recvResult{event: event, err: err}
}()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Creating a new goroutine for every single SSE event in Next() is highly inefficient and can lead to excessive resource consumption and goroutine leaks. If Next() returns false due to context cancellation, the goroutine will remain blocked on Recv() until the server sends data or closes the connection. Consider using a single background goroutine per stream to read from the SSE source and push to a channel.

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