Size the provider auth headers dynamically and stop truncating parsed floats - #55
Merged
Conversation
Audit of every fixed-length buffer in the extension. Two of the ten had a path where an input of unbounded length could reach them, and both failed by silently truncating rather than by complaining. The auth headers in the OpenAI, Voyage and Gemini providers were built into a 512 byte stack buffer, which leaves about 489 for the key itself. Nothing kept the key under that: provider_load_api_key() accepts a file of up to MAX_API_KEY_FILE_SIZE, which is 4096, so the two bounds disagreed by a factor of eight. That gap is reachable rather than theoretical, because JWT-style bearer tokens for OpenAI-compatible gateways routinely run past a thousand characters. The result would have been a credential quietly cut in half and an authentication failure indistinguishable from a wrong key. These now use psprintf(), matching how the url in the same functions is already built and freed, which removes the second bound altogether. provider_parse_float_array() read numeric literals into a 32 byte buffer and kept only the leading characters of anything longer, then handed that to atof(). A run that long is not a number any provider could legitimately send, since %.17g never exceeds 24 characters, but the failure mode was to score on a value of an entirely different magnitude without a word. It now stops instead, and because all three callers treat a short count as a dimension mismatch, the malformed response is reported as one. The remaining seven buffers are provably bounded and are left alone. The BM25 term keys cannot be reached by anything too long since #50 added the guard that drops such terms at tokenization; the three dbname buffers are NAMEDATALEN, which is the bound PostgreSQL itself puts on a database name; and the worker's captured error message is diagnostic text whose full version has already reached the server log, which is now said explicitly in the comment so the next audit need not rederive it. Closes #31
📝 WalkthroughWalkthroughThe float-array parser now rejects oversized numeric literals with 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | -2 |
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.
A rejected literal returned a short count, which every caller reports as "Dimension mismatch" -- pointing the operator at their model or dimension configuration rather than at a malformed response. Return a distinguished value instead and give it its own message. A sentinel rather than an error_msg out-param, since error_msg is only initialised at the top-level entry points. It also fails safe: -1 can never equal a dimension, so a missed call site still rejects.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/provider_common.c (1)
255-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd boundary regression coverage for
PROVIDER_PARSE_MALFORMED.Test the maximum accepted literal length and the first rejected length. Verify that the rejected case returns
PROVIDER_PARSE_MALFORMEDwithout advancing*pospast the offending run. Add one provider-level assertion that this result produces the malformed-numeric error instead of a dimension mismatch. The PR objective makes this boundary part of the parser contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provider_common.c` around lines 255 - 268, Add regression tests around the numeric parsing boundary exercised by the value-buffer check: verify the maximum accepted literal length succeeds and the first longer length returns PROVIDER_PARSE_MALFORMED without advancing *pos beyond the offending run. Add a provider-level assertion confirming this result reports the malformed-numeric error rather than a dimension mismatch, using the existing parser and provider test helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/provider_common.c`:
- Around line 255-268: Add regression tests around the numeric parsing boundary
exercised by the value-buffer check: verify the maximum accepted literal length
succeeds and the first longer length returns PROVIDER_PARSE_MALFORMED without
advancing *pos beyond the offending run. Add a provider-level assertion
confirming this result reports the malformed-numeric error rather than a
dimension mismatch, using the existing parser and provider test helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e8a558e-30f8-43da-8a5b-ab4ad92f0653
📒 Files selected for processing (7)
src/provider_common.csrc/provider_common.hsrc/provider_gemini.csrc/provider_ollama.csrc/provider_openai.csrc/provider_voyage.csrc/worker.c
mason-sharp
approved these changes
Aug 11, 2026
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.
Closes #31.
What the audit found
Ten fixed-length buffers exist in the extension. Eight are provably bounded; two had a path where unbounded input could reach them, and both failed by silently truncating.
The auth headers, and this one is reachable.
provider_openai.c,provider_voyage.candprovider_gemini.ceach built their header intochar auth_header[512], which leaves roughly 489 bytes for the key after the prefix. Nothing held the key under that:provider_load_api_key()accepts a file up toMAX_API_KEY_FILE_SIZE, which is 4096, so the two bounds disagreed by a factor of eight. That is not a theoretical gap, because JWT-style bearer tokens for OpenAI-compatible gateways routinely exceed a thousand characters, and the failure mode would be a credential quietly cut in half producing an authentication error indistinguishable from a wrong key. They now usepsprintf(), which matches howurlis already built and freed a few lines above in each of the same three functions, and removes the second bound rather than merely raising it.The float parser.
provider_parse_float_array()read numeric literals intochar value_buf[32], kept only the leading characters of anything longer, and passed that toatof(). A literal that long is not something a provider could legitimately send, since%.17gnever exceeds 24 characters, but the failure mode was to score on a number of an entirely different magnitude in silence. It now stops instead, and since all three callers already treat a short count as a dimension mismatch, that turns a silent wrong answer into a reported bad response with no signature change.What was deliberately left alone
The other eight are bounded by construction, and the reasoning is recorded here so the next audit does not have to rederive it:
bm25.candbm25.hcannot be reached by anything overlong since Drop terms too long for the BM25 hash key #50 added the guard dropping such terms at tokenization, which is what made those keys safe;dbname[NAMEDATALEN]buffers inworker.care sized to the bound PostgreSQL itself places on a database name, so truncation cannot occur;EmitErrorReport(). The comment now says so explicitly.Test plan
No new tests, said plainly rather than dressed up. Neither change has a test seam worth building. Exercising the auth header needs a live endpoint that will echo the header back, and the float parser is not reachable from SQL, so covering either would mean standing up a stub HTTP server purely to assert a string length. The auth header change is also a strict improvement regardless of input:
psprintf()cannot truncate at any length, so there is no boundary left to test.