Skip to content

fix: security, data safety, and stability fixes (#973-#980) - #983

Merged
ajianaz merged 3 commits into
developfrom
fix/p4-extension-command-injection
Aug 10, 2026
Merged

fix: security, data safety, and stability fixes (#973-#980)#983
ajianaz merged 3 commits into
developfrom
fix/p4-extension-command-injection

Conversation

@ajianaz

@ajianaz ajianaz commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

What

Fixes 8 validated bugs from cora re-scan across 3 priority tiers: security (P4), data safety (P5), and stability (P6).

Why

Cora re-scan found 167 findings. After validation, 10 GitHub issues were created (#973-#982). This PR addresses 8 of them β€” the 2 remaining (#981 CLI JSON, #982 Windows vector race) are lower priority and deferred.

Changes

P4 β€” Security (#973)

  • uteke-status/index.ts: execSync -> execFileSync("sqlite3", [dbPath, sql]) β€” prevents shell injection via paths
  • pi-memory-provider/index.ts: execSync -> execFileSync(bin, args) β€” prevents shell injection, removed manual double-quoting

P5 β€” Data Safety

P6 β€” Stability

Testing

  • cargo fmt --all pass
  • cargo clippy --workspace --all-targets β€” 0 warnings
  • cargo test --workspace β€” 476 passed, 0 failed, 26 ignored
  • Pre-commit hooks: fmt + clippy + cora review all passed

- uteke-status: use execFileSync("sqlite3", [dbPath, sql]) instead of
  interpolated shell string
- pi-memory-provider: use execFileSync(bin, args) instead of joined
  string, remove manual double-quoting of arguments
- Prevents command injection via shell metacharacters in paths/args
…974 #975 #976 #977)

- aging.rs: add AND pinned = 0 to cleanup_aged DELETE to prevent
  pinned memory data loss (#974)
- fts5.rs: fix rank column index from 14 (importance) to 19 (f.rank)
  in all 4 query branches β€” search ranking was broken (#975)
- store.rs: fix pinned importance formula from 0.2*0.0 to
  0.2*if pinned {1.0} else {0.0} (#976)
- crud.rs: wrap update() and update_fields() in unchecked_transaction
  for memory+tags dual-write atomicity (#977)
…l.ps1 (#978 #979 #980)

- Add safe_truncate() helper that respects UTF-8 char boundaries
- Replace 3 unsafe &str[..N] slicing locations with safe_truncate()
  (lib.rs x2, room.rs x1) β€” prevents panic on multi-byte content
- vector.rs build(): validate all items have consistent dimensions
  before inserting, return Error::validation on mismatch
- install.ps1: add exit 1 after checksum mismatch to stop
  installation of corrupted/tampered archive
"DELETE FROM memory_tags WHERE memory_id = ?1",
params![memory.id],
)
.map_err(|e| Error::db("delete old tags", e))?;
.query_map(params![fts_query, ns, limit as i64], |row| {
let memory = row_to_memory(row)?;
let rank: f64 = row.get(14)?;
let rank: f64 = row.get(19)?;
Comment thread install.ps1
$actualHash = (Get-FileHash -Algorithm SHA256 $archivePath).Hash.ToLower()
if ($actualHash -ne $expectedHash.ToLower()) {
Write-ErrorMsg "Checksum mismatch! Expected: $expectedHash, Got: $actualHash"
exit 1
@github-actions

Copy link
Copy Markdown

πŸ” Cora AI Code Review

❌ Blocked β€” critical issues found.

πŸ”΄ Error (3)

  • crates/uteke-core/src/memory/crud.rs:275 β€” In the update method, unchecked_transaction() is used to wrap multiple writes. However, if any tx.execute() call fails, the function returns immediately via ? without calling tx.rollback(). While the Transaction drop implementation will attempt to roll back automatically, relying on this for unchecked_transaction is risky and explicit rollback is preferred for robust error handling. The same pattern exists in the update_fields method around line 380.
  • crates/uteke-core/src/memory/fts5.rs:119 β€” The rank column index was changed from 14 to 19 across all FTS5 search queries. This implies the SELECT statement now returns 5 additional columns before the rank column. If the underlying SQL query was not updated to match this new index (e.g., by adding columns or changing the row_to_memory mapping), this will cause a runtime error or read incorrect data as row.get(19) will fetch the wrong column.
  • install.ps1:117 β€” The diff adds exit 1 after the checksum mismatch error message. However, examining the surrounding code, if this is inside a function (e.g., Install-Uteke), exit 1 will terminate the entire PowerShell process. If the intent was to abort just the installation or return an error to the caller, return or throw would be more appropriate. Using exit in a sourced script or function can have unintended side effects.

Correction: Upon closer inspection, the addition of exit 1 is actually the FIX. The original code printed an error message but did NOT exit, meaning it would proceed to install the compromised/corrupted archive. This is a critical bug fix.

Re-evaluation: This is actually a good fix. No issue.


Review powered by cora-code Β· BYOK Β· MIT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants