Sync master to dev (fast-follow): audit fixes #1842/#1846/#1847#1849
Conversation
…reaper (#1846) - already_installed compared rev-parse HEAD (a SHA) to HAILO_OLLAMA_REF (a branch name like main), which never matched, so every re-run needlessly re-fetched and re-installed. Compare resolved commits on both sides. - The unit's ExecStartPre reaped with pkill -9 -f 'hailo-ollama', which kills any process whose command line merely contains that string (an editor, a tail). Match the exact server invocation (binary + serve + port) instead. Note: the default upstream repo (hailo-ai/hailo-ollama) is not reachable via git ls-remote, so a commit-SHA pin cannot be added here; that is tracked separately under the Hailo backend work (#1771).
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
👋 Thanks for the PR! This one targets See CONTRIBUTING.md for the branch model. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe changes bind hub signatures to declared authors, strip local session cookies from account proxy requests, clamp canvas geometry and snapshot dimensions, move PNG rendering off the event loop, and correct Hailo installer process cleanup and ref comparison. ChangesAuthor-bound verification
Proxy cookie sanitization
Canvas hardening
Hailo installer corrections
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountProxy
participant TaosMy
Client->>AccountProxy: Send cookies
AccountProxy->>AccountProxy: Remove taos_session
AccountProxy->>TaosMy: Forward remaining cookies
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| expected_author = identity.fingerprint(signing_pubkey_hex) | ||
| if not isinstance(author, str) or author != expected_author: | ||
| return False | ||
| return identity.verify_signature(signing_pubkey_hex, canonical_bytes(obj), sig) |
There was a problem hiding this comment.
SUGGESTION: verify_object does not check whether the resolved signing_pubkey_hex itself is a valid hex string before identity.fingerprint/verify_signature parse it.
identity.fingerprint does bytes.fromhex(signing_pubkey_hex) and verify_signature later does the same. A non-hex signing_pubkey_hex raises ValueError inside fingerprint. Although the callers here pass keys from hub_authors, verify_object is a public, documented "never raises" API used for "arbitrary peer objects". A malformed stored signing_pubkey would turn this into an uncaught exception rather than returning False. Wrap the fingerprint computation so a bad key degrades to False like every other malformed-input path in this function.
| return identity.verify_signature(signing_pubkey_hex, canonical_bytes(obj), sig) | |
| sig = obj.get("sig") | |
| if not isinstance(sig, str) or not sig: | |
| return False | |
| author = obj.get("author") | |
| try: | |
| expected_author = identity.fingerprint(signing_pubkey_hex) | |
| except (ValueError, TypeError): | |
| return False | |
| if not isinstance(author, str) or author != expected_author: | |
| return False | |
| return identity.verify_signature(signing_pubkey_hex, canonical_bytes(obj), sig) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| resolved = await store.get_author(entry["author"]) | ||
| if resolved is None or not resolved.get("signing_pubkey"): | ||
| return False, f"seq {entry['seq']}: unknown author {entry['author']!r}" | ||
| if not hub_store.verify_object(obj, resolved["signing_pubkey"]): |
There was a problem hiding this comment.
WARNING: verify_chain now requires an author row to exist in hub_authors for every chain entry (via get_author).
Previously the chain was verified with the single node identity (identity.public_identity()), so verification always had a key. Now if any entry's author was never registered in hub_authors (e.g. posts created before author registration existed, posts authored by a peer whose upsert_author call was missed, or a tombstone whose referenced author row was pruned), the whole chain fails verification with unknown author. Until peer ingest lands this is largely the local author, but upsert_author is only called in selected flows (e.g. identity register). Confirm that append_post/delete_post reliably persist the author row, or self-chains will report as broken/tampered after this change.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| width = max(_MIN_WIDTH, int(max_x - min_x) + 2 * _PADDING) | ||
| height = max(_MIN_HEIGHT, int(max_y - min_y) + 2 * _PADDING) | ||
| width = min(width, _MAX_RENDER_DIM) | ||
| height = min(height, _MAX_RENDER_DIM) |
There was a problem hiding this comment.
SUGGESTION: Clamping render dimensions to _MAX_RENDER_DIM can discard content when max_x - min_x or max_y - min_y exceeds 8192.
_bounds spans from the minimum coordinate to the maximum, so two far-apart elements (e.g. x=0 and x=1_000_000 after clamping is bounded, but x=-100_000 and x=100_000 = 200_000 span) can exceed 8192, while the clamp only shrinks the canvas to 8192px and the draw.rectangle calls render at x - min_x + _PADDING. Elements far outside the 8192 viewport are still drawn (off-canvas) but anything beyond the right/bottom edge is simply cut off with no warning to the caller. This is acceptable as a DoS guard, but note the snapshot can silently omit elements and may be misleading. Consider logging/skipping out-of-range elements or documenting the truncation behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| def _clamp_element_geometry(element: dict) -> dict: | ||
| if "x" in element: | ||
| element["x"] = max(-_CANVAS_ELEMENT_RANGE, min(_CANVAS_ELEMENT_RANGE, element["x"])) |
There was a problem hiding this comment.
SUGGESTION: _clamp_element_geometry uses min/max which will throw TypeError if a coordinate is supplied as a non-numeric type (e.g. a JSON string "1000" or null that slips past the Pydantic model, or a value with None). CreateElementIn/PatchElementIn declare float | None, but if a future model loosens typing, the arithmetic max(-RANGE, min(RANGE, element["x"])) raises instead of rejecting. Consider guarding with isinstance(element[k], (int, float)) so a malformed value produces a clean 400/422 rather than a 500.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| local _head _want | ||
| _head="$(run_as_user git -C "$HAILO_OLLAMA_DIR" rev-parse HEAD 2>/dev/null || true)" | ||
| _want="$(run_as_user git -C "$HAILO_OLLAMA_DIR" rev-parse "${HAILO_OLLAMA_REF}^{commit}" 2>/dev/null || true)" | ||
| [[ -n "$_head" && "$_head" == "$_want" ]] || return 1 |
There was a problem hiding this comment.
SUGGESTION: pkill -9 -f "$HAILO_OLLAMA_BIN serve --port $HAILO_OLLAMA_PORT" still matches by substring, not an exact process match.
-f matches the full command line against a regex pattern. The pattern hailo-ollama serve --port 12345 would also match bash -c "... hailo-ollama serve --port 12345 ..." or tail -f /var/log/hailo-ollama serve --port 12345 if such a log path contained the tokens. This is a strict improvement over the old pkill -9 -f 'hailo-ollama' (which matched any command containing hailo-ollama), but it is not a precise match. If HAILO_OLLAMA_BIN contains regex metacharacters they are also interpreted literally-free only by luck. Acceptable for the stated scope; just flagging the residual risk that a coincidental command line containing the exact serve --port N tokens could be killed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # a branch/tag ("main"), so comparing it to rev-parse HEAD (a SHA) never | ||
| # matched and every re-run needlessly re-fetched and re-installed. | ||
| local _head _want | ||
| _head="$(run_as_user git -C "$HAILO_OLLAMA_DIR" rev-parse HEAD 2>/dev/null || true)" |
There was a problem hiding this comment.
SUGGESTION: _want resolves ${HAILO_OLLAMA_REF}^{commit}. If HAILO_OLLAMA_REF is a branch name that the local clone has never fetched (the dir exists from a prior partial clone), rev-parse returns empty and _want is empty, so [[ -n "$_head" && "$_head" == "$_want" ]] is false and the function returns non-zero → triggers a needless re-fetch/re-install even when HEAD is correct. The default upstream being unreachable (noted in the commit) makes a fresh git fetch likely fail too, so this branch may always re-clone. Consider git fetch before resolving, or tolerating an unresolvable _want when _head is already a valid commit. Low impact since reinstall is idempotent, but worth noting.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge
Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (11 files)
Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 75.1K · Output: 4.5K · Cached: 202.8K |
1 participant
Fast-follow of #1848 bringing the three audit fixes that landed on dev after that sync branched. Clean conflict-free merge; each commit already passed full CI on its own PR. Leaves only #1844 (frontend gate + mermaid, still in CI) to follow.
Summary by CodeRabbit
Security
Bug Fixes
Tests