Enforce v2-only API and config, and SQLx macro checks#32
Conversation
…v2-only API and config","intent":"Remove v1 endpoints and config variants and switch SQLx to macros with offline metadata","impact":"All builds use SQLX_OFFLINE with committed .sqlx and only /v2 routes are supported","breaking":true,"risk":"medium","refs":["gh:#31"]}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| top_k, | ||
| }; | ||
|
|
||
| let mut items = Vec::with_capacity(results.len()); |
Check failure
Code scanning / CodeQL
Uncontrolled allocation size High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
General approach: enforce a reasonable maximum on the number of search results that can be requested/handled, and ensure allocations are based on a bounded value rather than an unbounded user-controlled value. Additionally, avoid using with_capacity with a tainted size when it’s not necessary.
Best fix here without changing functionality: introduce a hard upper bound for top_k and candidate_k derived from configuration, and clamp the values accordingly. Because results.len() is effectively bounded by top_k, once top_k is clamped we know results.len() is safe. For extra safety and simplicity, we can also replace Vec::with_capacity(results.len()) with Vec::new(), removing the need to pre-allocate based on a tainted size while having negligible performance impact.
Concretely:
- In
packages/elf-service/src/search.rs, inElfService::search_raw, after computingtop_kandcandidate_k, clamp them to maximums defined in config (which are accessed viaself.cfg.memory.max_top_k/max_candidate_kor similar). Since we can’t assume such fields exist, we instead define local constants in this function (or near its top) likeconst MAX_TOP_K: u32 = 10_000;and clamptop_kandcandidate_kto these constants. This ensuresresults.len()is bounded. - In
finish_searchin the same file, changelet mut items = Vec::with_capacity(results.len());tolet mut items = Vec::new();, which removes the flagged sink while keeping behavior the same. - In
packages/elf-service/src/progressive_search.rs, wheretop_kandcandidate_kare recomputed inElfService::search, apply the same clamping pattern, to ensure the admin/raw search variants also share the bound.
No new methods are required; we just add simple clamping and adjust the one allocation call.
| @@ -384,8 +384,21 @@ | ||
| return Err(ServiceError::NonEnglishInput { field: "$.query".to_string() }); | ||
| } | ||
|
|
||
| let top_k = req.top_k.unwrap_or(self.cfg.memory.top_k).max(1); | ||
| let candidate_k = req.candidate_k.unwrap_or(self.cfg.memory.candidate_k).max(top_k); | ||
| // Clamp the number of requested results to prevent unbounded allocations. | ||
| const MAX_TOP_K: u32 = 10_000; | ||
| const MAX_CANDIDATE_K: u32 = 20_000; | ||
|
|
||
| let mut top_k = req.top_k.unwrap_or(self.cfg.memory.top_k).max(1); | ||
| if top_k > MAX_TOP_K { | ||
| top_k = MAX_TOP_K; | ||
| } | ||
|
|
||
| let mut candidate_k = | ||
| req.candidate_k.unwrap_or(self.cfg.memory.candidate_k).max(top_k); | ||
| if candidate_k > MAX_CANDIDATE_K { | ||
| candidate_k = MAX_CANDIDATE_K; | ||
| } | ||
|
|
||
| let query = req.query.clone(); | ||
| let read_profile = req.read_profile.clone(); | ||
| let record_hits_enabled = req.record_hits.unwrap_or(false); | ||
| @@ -1434,7 +1447,8 @@ | ||
| top_k, | ||
| }; | ||
|
|
||
| let mut items = Vec::with_capacity(results.len()); | ||
| // Build the items vector without pre-allocating based on a tainted size. | ||
| let mut items = Vec::new(); | ||
| let mut trace_builder = SearchTraceBuilder::new(trace_context, &self.cfg, now); | ||
|
|
||
| for (idx, scored_chunk) in results.into_iter().enumerate() { |
| @@ -171,9 +171,21 @@ | ||
|
|
||
| impl ElfService { | ||
| pub async fn search(&self, req: SearchRequest) -> ServiceResult<SearchIndexResponse> { | ||
| let top_k = req.top_k.unwrap_or(self.cfg.memory.top_k).max(1); | ||
| let candidate_k = req.candidate_k.unwrap_or(self.cfg.memory.candidate_k).max(top_k); | ||
| // Clamp requested sizes to prevent unbounded allocations in downstream search. | ||
| const MAX_TOP_K: u32 = 10_000; | ||
| const MAX_CANDIDATE_K: u32 = 20_000; | ||
|
|
||
| let mut top_k = req.top_k.unwrap_or(self.cfg.memory.top_k).max(1); | ||
| if top_k > MAX_TOP_K { | ||
| top_k = MAX_TOP_K; | ||
| } | ||
|
|
||
| let mut candidate_k = | ||
| req.candidate_k.unwrap_or(self.cfg.memory.candidate_k).max(top_k); | ||
| if candidate_k > MAX_CANDIDATE_K { | ||
| candidate_k = MAX_CANDIDATE_K; | ||
| } | ||
|
|
||
| let mut raw_req = req.clone(); | ||
|
|
||
| raw_req.top_k = Some(candidate_k); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8158a1be5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ! command -v taplo >/dev/null 2>&1; then | ||
| echo "Missing taplo." >&2 | ||
| exit 1 |
There was a problem hiding this comment.
The harness now exits immediately when taplo is not installed, which makes cargo make e2e fail in environments that previously worked with only runtime dependencies (curl, psql, jq/jaq). Because the generated TOML is already valid for execution, this formatting-only tool requirement can block experiment runs without improving correctness; making formatting optional (or warning-only) would avoid this regression.
Useful? React with 👍 / 👎.
…runtime SQLx queries in tests","intent":"Allow tests to use sqlx::query/query_as/query_scalar without compile-time macros","impact":"Test code no longer depends on SQLx macros while production code remains macro-checked","breaking":false,"risk":"low","refs":["pr:32"]}
1 participant
Closes #31.
Changes:
Verification: