Repository navigation
Record client IP and user agent in the audit log with trusted-proxy unwrap - #58
Merged
Merged
Conversation
…nwrap Request rows gain client_ip (real column, schema v3 additive migration, filterable in /admin/logs) and user_agent (payload). Headers are read only for client metadata; forwarded headers count solely when the direct peer is in CODEBUDDY2API_TRUSTED_PROXIES (hot setting, IP/CIDR list), otherwise the direct address wins so clients cannot spoof. The WebUI logs page shows the IP, an IP filter, Chinese labels for event kind/action/outcome and the new fields.
Reviewer's GuideThe PR captures sanitized request client metadata without reading bodies, resolves forwarded client IPs only through validated trusted proxies, stores and filters the new audit data via a v3 additive migration, and exposes it alongside localized event labels in the WebUI. Sequence diagram for trusted-proxy client metadata capturesequenceDiagram
participant Client
participant Proxy
participant API
participant AuditStore
Client->>Proxy: HTTP request
Proxy->>API: Request with client, X-Forwarded-For, X-Real-IP, User-Agent
API->>API: client_meta(scope, trusted_proxies)
alt direct peer matches trusted proxy
API->>API: Select rightmost-untrusted XFF hop
API->>API: Fallback to X-Real-IP
else direct peer is untrusted
API->>API: Use direct peer address
end
API->>API: safe_agent(User-Agent)
API->>AuditStore: Record client_ip and user_agent
AuditStore-->>API: Audit record committed
Entity relationship diagram for audit request client metadataerDiagram
REQUESTS {
text id PK
text client_ip
text payload
}
REQUESTS ||--|| AUDIT_PAYLOAD : contains
AUDIT_PAYLOAD {
text user_agent
}
Flow diagram for audit log client IP filteringflowchart LR
WebUI[Logs page] -->|client_ip filter| AdminAPI[logs_get]
AdminAPI -->|client_ip exact match| AuditStore[list_records]
AuditStore --> Requests[(requests.client_ip)]
Requests --> AdminAPI
AdminAPI --> WebUI
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="app/observability.py" line_range="53" />
<code_context>
+ headers = {}
+ for key, value in scope.get("headers") or []:
+ name = key.decode("latin-1").lower() if isinstance(key, (bytes, bytearray)) else str(key).lower()
+ if name in _META_HEADERS and name not in headers:
+ headers[name] = value.decode("latin-1", "replace").strip() if isinstance(value, (bytes, bytearray)) else str(value).strip()
+ client = scope.get("client")
</code_context>
<issue_to_address>
**Spoofed client addresses are recorded**
When a client supplies an X-Forwarded-For field and a trusted proxy appends its own value as a separate field, `client_meta` keeps only the first X-Forwarded-For field, so it ignores the proxy’s appended value and can record the client-supplied address as the requester.
Combine repeated X-Forwarded-For fields in order before parsing the chain.
Also at `app/observability.py:54`.
</issue_to_address>
### Comment 2
<location path="app/audit_store.py" line_range="168" />
<code_context>
+ # Additive v3 migration: record the audit-visible client address per request.
+ columns = [row[1] for row in self._db.execute("PRAGMA table_info(requests)").fetchall()]
+ if "client_ip" not in columns:
+ self._db.execute("ALTER TABLE requests ADD COLUMN client_ip TEXT")
+
def _init_accounting(self):
</code_context>
<issue_to_address>
**Old workers stop recording audit rows**
When a new version migrates a shared database while an old process is still writing to it, the v3 migration adds a tenth `requests` column, so old workers’ positional nine-value inserts fail with a column-count error and their audit rows are dropped; old workers that reopen the database also reject schema version 3.
Use a migration and rollout strategy that prevents old workers from writing to or reopening the database after the schema changes.
</issue_to_address>
### Comment 3
<location path="app/settings.py" line_range="125-127" />
<code_context>
"audit_max_bytes": _item(256 * 1024 * 1024, "integer", "审计明细预算", minimum=1024**2, maximum=1024**4),
"audit_retention_days": _item(30, "integer", "审计明细保留天数", minimum=1, maximum=36500),
"audit_diagnostic_bytes": _item(8192, "integer", "失败诊断最大字节", minimum=0, maximum=8192),
+ "trusted_proxies": _item("", "string", "可信代理地址（IP/CIDR，逗号分隔）",
+ env="CODEBUDDY2API_TRUSTED_PROXIES", allow_empty=True,
+ max_length=2000, validator=normalize_trusted_proxies),
}
</code_context>
<issue_to_address>
**Compose ignores trusted proxies**
When the service is deployed with Docker Compose and `CODEBUDDY2API_TRUSTED_PROXIES` is set in the host environment or `.env` file, compose does not pass the variable into the container, so `apply_persisted_settings` leaves `trusted_proxies` empty and `AuditMiddleware` records the proxy address instead of the client address.
Add `CODEBUDDY2API_TRUSTED_PROXIES` to the Compose environment mapping.
Also at `docs/deployment.md:16`.
</issue_to_address>
### Comment 4
<location path="app/observability.py" line_range="76" />
<code_context>
+ if not any(hop_ip in network for network in networks):
+ break
+ if chosen is None:
+ chosen = headers.get("x-real-ip") or (hops[0] if hops else None)
+ ip = chosen or remote
+ return ip, safe_agent(headers.get("user-agent"))
</code_context>
<issue_to_address>
**Malformed values are stored as client IPs**
When a trusted proxy supplies a malformed `X-Real-IP` or XFF fallback and no valid XFF hop is selected, `client_meta` copies the fallback without IP validation, and `safe_label` accepts ordinary identifier strings, so malformed values persist and appear as client addresses.
Parse fallback values with `ipaddress.ip_address` and ignore them when invalid.
Also at `app/audit_store.py:285`.
</issue_to_address>
### Comment 5
<location path="app/audit_store.py" line_range="459" />
<code_context>
params.extend((stamp, stamp, record_id))
if kind == "request":
- for key in ("model", "profile", "credential", "outcome", "status_code"):
+ for key in ("model", "profile", "credential", "outcome", "status_code", "client_ip"):
value = filters.get(key)
if value is not None and value != "":
</code_context>
<issue_to_address>
**Client IP filters time out**
When the audit database has many retained requests and an exact client-IP filter has few or no matches, `list_records` scans retained request rows because `client_ip` has no supporting index; `_run` interrupts the query after one second and the logs endpoint returns a degraded result instead of the matching records.
Add an index that supports filtering request rows by `client_ip`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and if the trusted-proxy configuration or forwarded-address parsing is wrong, audit records can persist incorrect client IPs or user agents, including spoofed proxy-chain identities. Reverting stops new records but does not remove the bounded set of historical metadata already written; the audit data can otherwise be cleared or regenerated through normal operational handling.
Blocking findings: app/observability.py:53, app/audit_store.py:168, app/settings.py:127, app/observability.py:76, app/audit_store.py:459
Variable-key i18n (zh/en dictionaries both loaded, zh default, persisted) with a top-left toggle in the sidebar and login pages. The audit logs page is fully translated (tabs, filters, columns, drawer, clear dialog), event kind/action and outcome render from label keys, and empty values show a neutral dash instead of asserting unknown. Dashboard, Trend and the shared chrome are translated; Models/Credentials/Settings keep zh strings until their keys land.
The provider no longer writes localStorage on mount, restoring the anonymous-visit zero-storage guarantee the e2e admin test asserts. The audit detail drawer renders known enum values (outcomes, error codes, stream modes, usage sources, stages, event codes) in the UI language with the raw value on hover.
Review fixes: combine repeated X-Forwarded-For fields in order before walking the chain (a proxy appending its hop as a separate field is no longer shadowed by a spoofed first field); validate every XFF hop and the X-Real-IP fallback with ipaddress and record the direct peer when a chain is broken or the fallback is not an address; create requests_client on migration so exact client-IP filters stay indexed; pass CODEBUDDY2API_TRUSTED_PROXIES through Compose; document the single-writer audit-schema rollout constraint.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
client_ip(real column, schema v3 additive migration, exact-match filter) anduser_agent(payload). Only client-metadata headers are read (user-agent, X-Forwarded-For, X-Real-IP); bodies and all other headers stay unread.CODEBUDDY2API_TRUSTED_PROXIES(hot setting, IP/CIDR list, validated at save); otherwise the direct address wins so clients cannot spoof. Rightmost-untrusted XFF hop wins; X-Real-IP is the fallback; UA is sanitized (single-line printable, ≤200 chars, secret-shaped values dropped).Verification
python -B tests/test_*.py: all 70 files pass, including new migration (v2→v3), sanitize/filter, and proxy unwrap/spoof-rejection tests.vp checkclean.Summary by Sourcery
Enhance audit logging with secure client metadata capture, trusted-proxy support, searchable WebUI presentation, and bilingual interface text.
New Features:
Bug Fixes:
Enhancements:
Deployment:
Documentation:
Tests: