fix(server): stop two endpoints echoing exception text back over HTTP - #55
Merged
Conversation
Two of the four Code of Conduct files pointed at GitHub features that are not real: - devrepro-doctor: report "via GitHub private message". GitHub has no private messaging. - local-ai-hardware-bench: report by "opening a private issue tagged `conduct`". GitHub has private *vulnerability reports*; it has no private issues. api-verity-lab gave a profile URL rather than a contact channel. Only tooltrace-bench named an address that works. Someone reporting harassment is the worst possible person to hand a dead end, so all four now carry the same wording: the maintainer's noreply address plus GitHub's real report-abuse form. Worse, and found while checking the above: all four SECURITY.md files direct reporters to GitHub's private vulnerability reporting, and it was **disabled on all four repositories**. Every documented security-disclosure path in this family of projects led to a page the reporter could not use. It is enabled now (a repository setting, so not visible in this diff). The disclosure SLAs also disagreed for the same solo maintainer -- 72h, 72h+7d, 7d, and 7d+30d. Standardized on 7 days to acknowledge and 30 to update, the most conservative of the four, and said plainly why: promising 72 hours when nobody is on call is a promise, not a policy. tests/test_contact_channels_exist.py pins this. It asserts the working address and the report-abuse form are present, that SECURITY.md still names private vulnerability reporting and still warns against public issues, and -- the point of the exercise -- that no document mentions "private message", "private issue" or "report-user functionality" again. Verified by reintroducing the devrepro-doctor wording and watching it fail.
CodeQL reported two `py/stack-trace-exposure` alerts (medium). Both are real, though for different reasons. `/readyz` is unauthenticated and returned `str(exc)` from whatever the sqlite3 probe raised. A sqlite3 message carries the database path and often schema detail, handed to anyone who can reach the port. A readiness probe needs one bit; the detail goes to the server's own log, where an operator can already see it. The job-enqueue handler returned `str(exc)` from `QueueFull`, whose message this codebase writes itself, so nothing untrusted was leaking today. The habit is still the thing worth removing: the next exception to reach that handler may not be one we wrote. It now returns a fixed message plus `max_active_jobs` as a field, which is also easier for a client to act on than parsing prose. tests/integration/test_selfhosted_server.py gains a test that makes `/readyz` fail with a message containing a database path and asserts the response body contains neither it nor the error text.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The Windows leg added in #53 turned up a second latent problem, alongside the retention bug: `scripts/e2e.py` failed the build on any non-zero exit from `apiverity regression`. EXIT_FINDINGS means "a regression was detected" -- the command working exactly as documented. And on a contended runner a mock server on localhost genuinely does go from 1.7ms to 16ms, past even the 400% tolerance that was chosen to be generous. The step's own comment already said its job was to "verify the command wiring and exit codes end-to-end"; it then gated on the one thing that depends on how loaded the machine is. That is the failure mode where a red build teaches people to ignore red builds. The step now accepts EXIT_OK or EXIT_FINDINGS and fails only on a usage, unreachable or internal code -- which is what "the wiring works" actually means. The comparison logic keeps its unit tests. Worth noting what the tool did right while failing this: it separated three genuine threshold breaches from four it reported as inconclusive, with the overlapping intervals printed and a suggestion to raise --iterations. The statistics were not the problem. Also formats the test added in the previous commit; CI checks `tests` too and I had only re-run ruff over `apiverity`.
CI's Format check covers tests as well as apiverity; the cherry-pick carried the code change but I re-ran the formatter before it, not after.
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
1 participant
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.
CodeQL reported two
py/stack-trace-exposurealerts (medium). Both are real, though for different reasons./readyz— the one that mattersUnauthenticated, and it returned
str(exc)from whatever the sqlite3 probe raised. A sqlite3 message carries the database path and often schema detail, handed to anyone who can reach the port. A readiness probe needs one bit; the detail now goes to the server's own log, where an operator can already see it./v1/jobs— the habit, not the leakThe enqueue handler returned
str(exc)fromQueueFull, whose message this codebase writes itself, so nothing untrusted was leaking today. The habit is still worth removing: the next exception to reach that handler may not be one we wrote. It now returns a fixed message plusmax_active_jobsas a field — also easier for a client to act on than parsing prose.What keeps it fixed
A new test makes
/readyzfail with an error message containing a database path, then asserts the response body contains neither the path nor the error text.Verification
ruff check/ruff format --checkmypypytest