Fix bugs found in a full-project audit - #36
Merged
Merged
Conversation
A review pass over the backend, frontend, iOS app and deployment config turned up a number of real defects. Highlights: Data loss and silent failure - openai_provider: `_call()` returned the completion coroutine without awaiting it, so the network request happened outside `retry_async`. Backoff, FallbackTriggered degradation and the context-overflow max_tokens reduction were dead code for every OpenAI-family provider. Same class of bug fixed in anthropic_provider. - iOS: background uploads treated `error == nil` as success without checking the HTTP status, popping the pending queue on 401/403/400. A foreground WS flush could also double-pop against an in-flight HTTP task, deleting records that were never sent. - Sub-analysis loops appended assistant messages with tool_calls they then skipped, leaving orphan tool_use ids that 400 on the next turn. Auth and isolation - Feishu webhook accepted tokenless event POSTs, allowing forged user messages; added the missing encrypted-event (AES-256-CBC) support and signature freshness checks, plus event_id dedup and a real reconnect loop with link-loss detection. - docker-compose gave the watch service no env_file, so API_AUTH_TOKEN never reached it and port 8765 was unauthenticated. - .dockerignore did not match frontend/.env.local, baking the bearer token into the public JS bundle. The frontend now resolves the token at runtime instead of at build time, which also unbreaks the dashboard for Docker installs with auth enabled. - create_page's patch branch ran before page_id validation, allowing a path-traversal write. - Personalised pages now render in a genuine opaque-origin sandbox via a validated postMessage bridge and srcdoc, so agent-generated HTML can no longer reach the app origin, and no token appears in any URL. Correctness - watch_db_reader used pd.Timestamp.now().timestamp(), reading a naive local clock as UTC; east of UTC the query window landed in the future and returned nothing. - Decibels, METs and the 6-minute walk test were aggregated with SUM. - Timezone handling in page_helpers, data_store_reader and the frontend date parsing/formatting. - hime.sh stop used bare `pkill -f uvicorn|vite|multiprocessing`, which killed unrelated processes on the same machine. - Telegram allowed_updates encoding, HTML truncation on tag boundaries, and offset/cursor persistence so restarts stop replaying messages. Also: blocking I/O moved off the event loop, rate-limit buckets split per endpoint, WS disconnect detection in the streaming service, several resource leaks and retain cycles, iOS Keychain token storage, widget unit conversions, and nginx/entrypoint/CI fixes. Backend tests 348 -> 441, frontend 61 -> 75, ESLint 25 errors -> 0. iOS sources compile-checked with swiftc (xcodebuild is unusable in this environment); Docker build, shellcheck and nginx -t could not be run locally, and the Feishu decryption path was verified against a reimplementation of the SDK cipher rather than a live payload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAKcG78BBmgDCuZf174MQL
There was a problem hiding this comment.
Pull request overview
This PR addresses a wide set of defects uncovered in a full-project audit, spanning backend reliability/security, frontend auth/runtime behavior, iOS/watchOS correctness, and deployment/script hardening. It focuses on preventing silent failures and data loss, tightening authentication/isolation boundaries, and fixing timestamp/aggregation/streaming correctness issues across the stack.
Changes:
- Fixes multiple backend/iOS data-loss and silent-failure paths (auth handling, queue/persistence correctness, streaming disconnect handling, async task lifetimes).
- Hardens auth and sandboxing (token handling, personalised page isolation, webhook validation, Docker/nginx proxy correctness).
- Improves correctness/perf in time handling, aggregation, and frontend buffering/rendering; expands tests and CI checks.
Reviewed changes
Copilot reviewed 96 out of 96 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_personalised_page_auth.py | CSP/srcdoc regression tests |
| setup.sh | Safer token + health summary |
| pyproject.toml | Align Python deps versions |
| prompts/create_page_guide.md | Guidance for sandboxed pages |
| ios/Server/server.py | Auth + WS + retention fixes |
| ios/Server/data_api.py | Close SQLite connections |
| ios/hime/himewatch/WatchHealthManager.swift | Prevent duplicate HK observers |
| ios/hime/himewatch/WatchConnectivityManager.swift | Widget/log throttling + composite ctx |
| ios/hime/hime/TasksView.swift | Surface write failures to UI |
| ios/hime/hime/SettingsView.swift | Debounced token save + status label |
| ios/hime/hime/ServerConfig.swift | Keychain token + IPv6 parsing |
| ios/hime/hime/ReportsView.swift | Shared VM + locale-safe parsing |
| ios/hime/hime/PhoneConnectivityManager.swift | Composite WC context + bg task safety |
| ios/hime/hime/LogManager.swift | Async log file writer |
| ios/hime/hime/himeApp.swift | Bg task expiration handling |
| ios/hime/hime/DashboardViewModel.swift | Shared singleton + unit fixes |
| ios/hime/hime/DashboardView.swift | Locale/calendar-safe date formats |
| ios/hime/hime/ContentView.swift | Personalised page token passthrough + tab clamp |
| ios/hime/hime/ChatView.swift | Off-main image downscale + better image load UX |
| ios/hime/hime/ChatStreamClient.swift | WS auth via Authorization header |
| ios/hime/hime/CatViewModel.swift | Leak fixes + timer invalidation safety |
| hime.sh | Safer env parsing + targeted pkill |
| frontend/src/test/setup.js | Avoid unused param warnings |
| frontend/src/test/mocks/api.js | Mock auth helpers + page HTML fetch |
| frontend/src/pages/Skills.jsx | Serialize enable/disable PUTs + cmd/ctrl-s |
| frontend/src/pages/ReportsView.jsx | Stable badge + UTC parsing + loading UX |
| frontend/src/pages/PromptEditor.jsx | Unsaved-edit protection + cmd/ctrl-s |
| frontend/src/pages/KnowledgeBase.jsx | Discard stale inspect responses |
| frontend/src/lib/utils.js | Parse UTC timestamps consistently |
| frontend/src/i18n/locales/zh.json | Auth + UI strings updates |
| frontend/src/i18n/locales/en.json | Auth + UI strings updates |
| frontend/src/context/AppContext.jsx | History trimming + WS lifecycle robustness |
| frontend/src/components/StatisticsPanel.jsx | Auth-aware count call + perf fixes |
| frontend/src/components/AuthTokenPrompt.jsx | Runtime token prompt UI |
| frontend/src/App.jsx | Mount auth prompt globally |
| frontend/.env.example | Warn about build-time token |
| docs/INSTALL.md | Document dashboard token flow |
| docs/DEPLOYMENT.md | Token/runtime auth + page sandbox docs |
| Dockerfile.frontend | Prevent token baking into bundle |
| docker/nginx.conf | Correct upgrade header + upload size |
| docker/entrypoint-backend.sh | Avoid expensive recursive chown |
| docker-compose.yml | Ensure watch exporter gets env_file |
| data/personalised_pages/_shared/hime-ui.js | postMessage bridge for /data |
| backend/weixin/transport.py | Persist cursor/dedup to disk |
| backend/weixin/sender.py | UTF-8 byte-safe truncation |
| backend/weixin/qr_login.py | 0600 token file creation |
| backend/weixin/cdn.py | Typed errors + backoff/jitter |
| backend/telegram/sender.py | Tag-safe truncation + plain retry |
| backend/telegram/poller.py | Persist offset/dedup + allowed_updates fix |
| backend/services/streaming_service.py | Detect WS disconnect while idle |
| backend/requirements.txt | Add cryptography + keep versions |
| backend/messaging/inbox.py | Clarify non-thread-safe queue contract |
| backend/main.py | Scheduler claim-release + auth hardening |
| backend/ios_gateway/gateway.py | Return real delivery outcome |
| backend/ios_gateway/device_store.py | Filter tokens by APNs environment |
| backend/ios_gateway/apns.py | Revoke BadDeviceToken + env filter |
| backend/data_readers/watch_db_reader.py | UTC/time.time cutoff correctness |
| backend/data_readers/data_store_reader.py | Avoid phantom DB file creation |
| backend/data_readers/apple_health_features.py | Fix aggregation semantics |
| backend/config.py | TRUST_PROXY_HEADERS setting |
| backend/api/stream_routes.py | Constant-time WS token compare + state save lock |
| backend/api/skill_routes.py | Write/delete across actual roots + frontmatter escaping |
| backend/api/data_routes.py | Async init off event loop |
| backend/api/config_routes.py | Locked app_state writes |
| backend/api/agent_tasks.py | Thread off SQLite + 404 on missing |
| backend/api/agent_state.py | Per-endpoint rate limits + bounded buckets |
| backend/api/agent_lifecycle.py | Safer restart + task retention + rate limit split |
| backend/api/agent_diagnostics.py | Thread off DB reads + honest failures |
| backend/agent/trigger_evaluator.py | Drop-on-full trigger queue |
| backend/agent/tools/update_md_tool.py | Repo-anchored prompts path |
| backend/agent/tools/push_report_tool.py | Background task strong references |
| backend/agent/tools/page_helpers.py | UTC window computation |
| backend/agent/tools/create_page_tool.py | Path traversal + sandbox tightening |
| backend/agent/tools/code_tool.py | Correct df refresh watermark |
| backend/agent/prompt_loader.py | Repo-anchored PROMPTS_DIR |
| backend/agent/memory_manager.py | Cache schema + per-DB chat lock |
| backend/agent/llm/openai_provider.py | Await inside retry + tool-call finish reasons |
| backend/agent/llm/anthropic_provider.py | Retry-safe streaming + thinking replay |
| backend/agent/llm/init.py | Redact/cap usage logs + rotate |
| backend/agent/fact_verifier.py | Dedup before unique index |
| backend/agent/errors.py | Robust status/token overflow detection |
| backend/agent/autonomous_agent.py | Strong-ref background warmup |
| backend/agent/agent_prompts.py | Use PROMPTS_DIR everywhere |
| .github/workflows/ci.yml | Add shellcheck + docker build |
| .dockerignore | Ignore nested frontend .env.local |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The sqlite3.connect() allowlist used a substring test, but the regex
captures everything up to the first ',' or ')', so an expression that
merely mentions an allowed name passed while pointing elsewhere:
sqlite3.connect(HEALTH_DB_PATH + "_evil.db") # accepted
Require the argument to be exactly one of the injected names. This also
rejects str(HEALTH_DB_PATH) and f"file:{HEALTH_DB_PATH}?mode=ro";
that is intentional — the guide directs pages at the query_health /
query_memory helpers rather than opening connections themselves, so a
fail-closed check costs nothing. The error message now says so.
Exact matching alone is not enough, because reflection never reaches the
regex at all:
getattr(sqlite3, "conn" + "ect")("/tmp/evil.db")
So getattr/setattr/delattr join the blocked-token list; without them the
DB allowlist is decorative.
Regression tests cover both, and were validated by reverting each fix in
turn: restoring the substring test fails exactly the three cases that
smuggle an allowed name into a larger expression (the bare "/etc/passwd"
and unknown-identifier cases still fail closed either way), and removing
the reflection tokens fails all three getattr/setattr/delattr cases.
Page ids are unique per parametrised case because _recent_creations is
module-level state and its 60s dedup window would otherwise let one
case's success mask the next.
Reported by GitHub Copilot on the audit PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAKcG78BBmgDCuZf174MQL
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
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.
A review pass over the backend, frontend, iOS app and deployment config turned up a number of real defects. Highlights:
Data loss and silent failure
_call()returned the completion coroutine without awaiting it, so the network request happened outsideretry_async. Backoff, FallbackTriggered degradation and the context-overflow max_tokens reduction were dead code for every OpenAI-family provider. Same class of bug fixed in anthropic_provider.error == nilas success without checking the HTTP status, popping the pending queue on 401/403/400. A foreground WS flush could also double-pop against an in-flight HTTP task, deleting records that were never sent.Auth and isolation
Correctness
pkill -f uvicorn|vite|multiprocessing, which killed unrelated processes on the same machine.Also: blocking I/O moved off the event loop, rate-limit buckets split per endpoint, WS disconnect detection in the streaming service, several resource leaks and retain cycles, iOS Keychain token storage, widget unit conversions, and nginx/entrypoint/CI fixes.
Backend tests 348 -> 441, frontend 61 -> 75, ESLint 25 errors -> 0. iOS sources compile-checked with swiftc (xcodebuild is unusable in this environment); Docker build, shellcheck and nginx -t could not be run locally, and the Feishu decryption path was verified against a reimplementation of the SDK cipher rather than a live payload.
Claude-Session: https://claude.ai/code/session_01YAKcG78BBmgDCuZf174MQL
Summary
What does this PR do and why?
Scope of changes
Testing
How did you verify the change works? Include commands, steps, or screenshots.
python -m pytest tests/ -x -qpassesnpm run buildpasses (if frontend touched)Checklist
en.json/zh.json, backendbackend/i18n/locales/, iOSLocalizable.xcstrings).env.exampleAGENTS.md