Fix security, algorithm, and CI issues from repo audit - #20
Open
RyoK3N wants to merge 2 commits into
Open
Conversation
Addresses the findings in Issues.md: Security: - Stop writing request-supplied API keys into process-wide os.environ; thread them explicitly or scope the mutation to a single request via a new locked context manager (UI/backend/_env_scope.py) - GET /config no longer echoes stored API key values, only whether one is configured - Mask internal exception details in 500 responses (log full detail server-side instead) - Add optional bearer-token auth middleware (BUCK_API_AUTH_TOKEN), restrict CORS to the methods/headers actually used - Validate MCP server base URLs before using them for outbound requests / browser opens; constrain open_buck_ui's tab param to a known set - Fix a duplicate dead-code HTTPException in /rl/simulate Algorithms: - Require a minimum margin before reporting BUY/SELL over HOLD on a near-tie in TechnicalAnalyzer's signal aggregation - Cache trained LSTM checkpoints by a fingerprint of the input data + hyperparameters instead of retraining from scratch on every call, with atomic (temp-file + rename) writes so concurrent batch analysis can't read a torn checkpoint - Mark tools/dl/time_llm.py as an explicit non-functional placeholder - Attach a "not financial advice" disclaimer to every forecast response, the CLI output, the Claude system prompt, and the web UI footer Repo hygiene: - Restore .github/workflows/codeql.yml, which was 100% commented out and never actually ran despite looking active - Fix .gitignore's "input/" (singular) typo that let debug prompt/JSON dumps under the real "inputs/" directory get committed; untrack them - Add missing .gitkeep placeholders for output/, tools/rl/weights/, and the new tools/dl/lstm_weights/ cache dir; stop gitignoring docs/ so docs/CLAUDE_MCP.md (referenced by the README) actually exists - Pin requirements.txt / UI/backend/requirements.txt to the versions this is tested against, with upper bounds at the next major version - Add a frontend CI job (tsc, build) and pip-audit/npm audit steps - Add a regression test exercising Buck.batch_analyze() under real concurrent (thread-pool) execution across distinctly-trending symbols README rewritten to document the auth token, the key-fallback behavior, the CI changes, and a new Known Limitations section. All 198 tests pass; frontend type-checks and builds cleanly; verified end-to-end against a live backend + frontend (including the auth middleware and the /analyze fallback-to-server-key path).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+40
to
+60
| runs-on: ubuntu-latest | ||
| defaults: | ||
| run: | ||
| working-directory: UI/frontend | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Set up Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
| cache: 'npm' | ||
| cache-dependency-path: UI/frontend/package-lock.json | ||
| - name: Install dependencies | ||
| run: npm ci | ||
| - name: Type-check | ||
| run: npx tsc --noEmit | ||
| - name: Build | ||
| run: npm run build | ||
| - name: Audit npm dependencies for known CVEs | ||
| continue-on-error: true | ||
| run: npm audit --audit-level=high |
| INDIAN_API_KEY=req.indian_api_key, | ||
| ANTHROPIC_API_KEY=anthropic_key, | ||
| ): | ||
| return await _claude_predict_impl(req, openai_key, anthropic_key) |
| onServerKeyStatusRef.current?.({ | ||
| openai: server.openai_api_key_configured, | ||
| indian: server.indian_api_key_configured, | ||
| }) |
Concurrency: - Convert the shared stock-data context (agent_scripts/tools.py) from a module-level global to a ContextVar so concurrent analyses of different symbols are isolated per task/thread on the @tool read path - Lock Buck's analysis/forecast caches (batch_analyze spans threads via the run_in_executor path) - Give LiveSessionState a per-session RLock across record_step/ record_update and the reader paths; readers snapshot the steps deque instead of iterating it live Security: - Rate-limit the expensive endpoints (/analyze, /batch, /claude/*, /rl/*, /mcp/invoke) per client IP: BUCK_RATE_LIMIT_PER_MINUTE, default 30/min, 429 + Retry-After over the limit (UI/backend/ratelimit.py) - Enforce the bearer token on the /accuracy/ws handshake (header or ?token= query param) — BaseHTTPMiddleware never covered WS scopes - Mask the last raw-exception 500 detail (/mcp/invoke) Algorithms: - Add accuracy/backtest.py: walk-forward validation harness reporting per-signal hit-rates and confidence calibration (mean confidence on hits vs misses), with CLI entry point - Normalize the RL training reward to percent-of-capital per step instead of raw dollar P&L so learned policies are capital-scale invariant - Document that Buck's overall confidence is single-sourced from the analyzer-reported values (cannot diverge for the default pipeline) Packaging / CI / docs: - Add [build-system]/[project] to pyproject.toml; pip install -e . works - Broaden CI coverage to accuracy, realtime, mcp_server, UI/backend, tools - Add data-handling/retention policy to SECURITY.md - README: document the rate limiter, WS auth, and backtest harness - FIXES.md: full remediation record mapped to Issues.md findings Tests: - New backend-route suite: /config secret masking, 500-detail masking, bearer auth (HTTP + WS), rate limiter behavior - New backtest harness tests; ContextVar isolation tests (two-thread barrier test replaces the old module-global reset test) - Fix the batch-concurrency test to assert on the deterministic MA crossover per symbol instead of the aggregated overall_signal, which legitimately blends contrarian indicators (RSI reads a steady downtrend as oversold -> BUY) and failed spuriously 217 tests pass; frontend type-checks and builds; auth, rate limiting, and WS token enforcement verified against a live backend.
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
2 participants
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.
Addresses the findings in Issues.md:
Security:
Algorithms:
Repo hygiene:
README rewritten to document the auth token, the key-fallback behavior, the CI changes, and a new Known Limitations section.
All 198 tests pass; frontend type-checks and builds cleanly; verified end-to-end against a live backend + frontend (including the auth middleware and the /analyze fallback-to-server-key path).