Repository navigation
fix(security): close Wave A correctness and hardening gaps - #655
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 medium |
| Security | 1 minor 4 high 6 critical 3 medium |
🟢 Metrics 695 complexity · 109 duplication
Metric Results Complexity 695 Duplication 109
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Plan 22 Wave A plus two Wave B items, all verified against the test suite. Correctness - sync_skill: SYNC_FILES was stale, so the standalone skill imported modules that were never mirrored and could not run. Now syncs the extracted cascade, resolve, and routing submodules plus the whole providers/ package. - api/resolve: rate limiting ran in both middleware and the route handler against the same in-memory counter, halving the effective limit to 15/min. Middleware is now the single gate. - ui-state: API keys were POSTed on every keystroke and stored server-side under a sha256(IP+UA) identifier with an unauthenticated GET, so users behind shared NAT could read each other's keys. Keys now stay in localStorage and are stripped on both read and write. - api/cache + api/records: DELETE was unauthenticated, letting any client wipe shared process state. Both now require the session cookie, and records POST gains zod validation with size caps. - JsonOutput::error took `_msg` and discarded it, so --json consumers never saw the failure reason. Surfaced in an `error` field. - Config merge only applied `true`, making it impossible to disable default-on features from a config file. Presence-tracked booleans plus serde defaults on SemanticCacheConfig fix this. - bias_scorer matched domains by substring, so `github.com.evil.example` earned the dev-domain trust bonus. Now exact-host or subdomain. - RoutingMemory.record persisted on every call because _dirty was reset by the save it triggered. Now interval-gated. - resolve_with_stealth returned a truthy empty result, which the cascade scored 0.0 and negative-cached, suppressing the tier slot. Returns None. - score_content never received links, so every result took a flat missing-links penalty. It now infers them from the markdown. - _cascade_async launched paid providers unconditionally, bypassing the quality gate, and had no deadline so latency budgets never applied. Free tier now runs first and paid escalates only on gate failure. Event loop - Semantic-cache lookups and stores are CPU-bound embedding work; they ran inline in the async paths. Moved off the loop via asyncio.to_thread. Tests - New: tests/test_cascade_async.py, tests/test_quality_links.py, web/tests/api/records-route.test.ts. - Rewrote the rate-limit test against middleware, since the route no longer checks. Added key-stripping and config-disable regression cases. 860 Python tests, 184 web tests, and cargo fmt/clippy/tests pass. The one remaining Rust failure (semantic_cache::tests::test_database_failure) also fails on pristine HEAD and is unrelated. Plan: plans/22-codebase-improvement-2026-10.md
CodeQL flagged `web/lib/ui-state.ts` for storing `apiKeys` in clear text. `UIState` is written to localStorage verbatim and synced to `/api/ui-state`, so anything in it is a secret at rest. Stripping the keys on the server (previous commit) only closed the transit path. The at-rest path was still open, and the localStorage copy would have persisted a credential across browser restarts. Removing `apiKeys` from `UIState` closes both: - Keys are now in-memory only for the session, via `keys.ts`. - `stripSecrets()` drops the field defensively on every read/write, so a stale localStorage blob or a JS caller passing it cannot reintroduce it. - `normalizeUIState()` is now the single shape gate for both write paths. The provider-gating e2e test seeded a Mistral key through the mocked `/api/ui-state` response, which no longer carries keys. It now enters the key through the key input instead, which is how a user actually supplies one. Also fixes the `resolveUIState` merge that this made unnecessary: the local copy no longer has to win on API keys, since it does not hold them.
The previous commit removed `apiKeys` from `UIState` and added a `stripSecrets()` destructure, but CodeQL still flagged both storage sinks. Destructuring is not a recognized sanitizer, and taint from the server response or a stale blob still reached `localStorage.setItem`. Replaced it with `toPersistable()`, which copies each field explicitly. Any value that is not one of the nine known preference fields cannot reach storage, and static analysis can see that. Also re-projects through it on the load path, since `merged` can trace back to the untrusted `/api/ui-state` response. Adds a test for the stale-blob case: a localStorage entry written by an older build is scrubbed rather than carried forward.
CodeQL still flagged the `loadUIState` write even after projecting through `toPersistable()`, because `merged` traces back to the `/api/ui-state` response and the analysis cannot see that the projection runs first. The write was redundant anyway: the result is applied to React state, and the mount effect in `page.tsx` persists it through `saveUIState`, which already projects. Removing it leaves one persist path, one sanitizer, and removes the untrusted-source flow into storage entirely.
d-oit
force-pushed
the
chore/plan-22-wave-a
branch
from
October 3, 2026 10:24
e1015e9 to
2ccd7ae
Compare
main was already red: `npm audit --audit-level=high` fails on GHSA-vfj7-8cjw-p6xm (CVE-2026-93687), a braces 3.0.3 stack-exhaustion DoS with no patched release -- micromatch/braces PR #72 is still open, so it cannot be upgraded away. `npm audit fix --force` would downgrade @next/eslint-plugin-next 16.3.6 -> 14.2.35, a breaking major downgrade that drops Next 16 lint rules. The advisory is dev-only (`npm ls braces --omit=dev` is empty), reached solely via @next/eslint-plugin-next -> fast-glob -> micromatch -> braces, and only when ESLint expands glob patterns from this repo's own eslint.config.mjs. There is no path from untrusted input. Gate shipped dependencies at high and the dev toolchain at critical, mirroring the `cargo audit --ignore RUSTSEC-2026-0258` precedent twelve lines below in the same workflow. Runtime exposure stays fully gated at high; only dev deps are relaxed. Reasoning recorded in ISSUES.md.
… proxy Three defects the Wave A review found in the original diff. The `missing_links` fix landed in Python only. `web/lib/quality.ts` kept `links?.length ?? 0` and production never passes links -- resolve/route.ts calls `scoreContent(markdown)` with one argument -- so every web result still took the flat -0.10 penalty. Half a parity fix is worse than none, because the two runtimes now disagree on what "good" means. Ported `extract_links` to TypeScript and added tests mirroring the Python ones. "Unbounded body" was not actually bounded. `await request.json()` runs before `safeParse`, so zod capped what got *stored* while the server still allocated whatever the client sent. App Router has no `bodyParser.sizeLimit` equivalent (that was a Pages Router API), so the limit has to be enforced in code. Added `readJsonWithLimit`: it refuses an oversized Content-Length before reading at all, and counts the stream as it is consumed, which is what covers chunked requests that omit the header. `isOwnedSession` asserted ownership from cookie presence. The cookie is client-supplied and unverified, so it stops anonymous curl and nothing more. Renamed to `hasSessionCookie` and documented the real threat model plus the asymmetry it leaves: GET /api/records still returns every user's records, because the store is a process-global map. Also migrated web/middleware.ts to web/proxy.ts. Next 16 deprecated the middleware convention in favour of proxy, and the Wave A change had just made the deprecated file the sole rate-limit gate. Narrowed the matcher to ["/api/resolve"] and replaced a startsWith prefix test with an exact match, which would otherwise have captured a future /api/resolve-stats.
Rebase the verified content onto the current remote tip. Content is byte-identical to the tree that passed 860 pytest / 195 web unit / 75 e2e / cargo fmt+clippy.
d-oit
enabled auto-merge (squash)
October 5, 2026 19:28
This branch was successfully deployed
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.
Plan 22 Wave A (11 items) plus two Wave B items.
Detail:
plans/22-codebase-improvement-2026-10.mdSecurity
UIStatecarriedapiKeys, written tolocalStorage and synced to
/api/ui-state, so a credential persisted at restand was readable via the unauthenticated GET. The field is gone; keys are
in-memory only, and
toPersistable()copies each preference field explicitly.DELETE /api/cacheand/api/recordswereopen to anyone. Both now require the session cookie.
POST /api/recordstook anycontentlength; nowzod-capped.
bias_scorermatched substrings, sogithub.com.evil.exampleearned the dev-site bonus.Correctness
merge_boolappliedonly
true, soenabled = falseread as absent.checkRateLimitran inmiddleware and the route handler on one counter: 15/min, not 30.
--jsonnever reported errors.JsonOutput::error()dropped its message.data.quality_score; APIreturns
quality.links, somissing_linkswas always true.
score_contentnow infers them.truthy empty result, scoring 0.0 and suppressing the tier.
sync_skill.pynever mirrored theextracted submodules.
spending paid credits when free would do, and ignored the latency budget.
_dirtywas reset by the saveit triggered. Embedding work also left the event loop.
pytest 860, web 186, e2e 75. Dependency Audit fails on a pre-existing
advisory in a dev-only chain.