Yaak Logo
Yaak
Feedback/Three unguarded recursion/cache paths in main (1b28dfd)

Three unguarded recursion/cache paths in main (1b28dfd)

Orionix ·2 months ago
Needs TriageImprovementQuestion

Hi Greg, Thanks for Yaak — the Tauri + crates workspace architecture is a pleasure to read. While going through the rendering and resolution code, three points caught my attention; I’m posing them as questions, in case there’s a safeguard I missed.

  1. Folder cycle → stack overflow. resolve_environments (crates/yaak-models/src/queries/environments.rs:168-172) recurses on folder.folder_id, base case only at None (line 174). For a cyclic parent (or self-parent), get_folder always succeeds → unbounded recursion. upsert_folder (folders.rs:58-60) and batch_upsert don’t enforce acyclicity, and the three resolve_*_for_folder (folders.rs:118,130,150) share the pattern. A Rust overflow not being catchable via catch_unwind, sending/rendering a request under that folder aborts the process — and persists after restart. Question: is there acyclicity validation upstream of upsert_folder (plugin / sync)?
  2. Nested function args → MAX_DEPTH bypass. The depth > MAX_DEPTH guard lives in render() (renderer.rs:110-113), but the Val::Fn branch calls render_value(a.value, …, depth) (renderer.rs:157) without incrementing or checking depth. A deep ${[ f(a=f(a=…)) ]} recurses render_value without going through the guard again. Question: is the guard supposed to cover the function argument recursion?
  3. HTTP client cache keyed on id only. get_client (crates/yaak-http/src/manager.rs:31-41) looks up by opt.id without comparing validate_certificates/proxy/client_cert. If the same plugin_context.id serves multiple sends with different TLS postures, a danger_accept_invalid_certs client (client.rs:134-138) might be reused for a request meant to validate. Question: should the key include the TLS posture, or is a client guaranteed mono-posture per id? Nothing urgent in tone — mainly wanted to verify my reading of the flow. Happy to provide full traces or a minimal repro if useful. And by the way: are these recursive resolvers considered a trust boundary against imported/synced data, or assumed always well-formed? — Mathieu Bellot

Comments (2)

Sign in to leave a comment.

Su

Hi Greg, I’d like to take the “Nested function args → MAX_DEPTH bypass” item from this report (only that item for now). I reproduced it on current main with a focused yaak-templates test: 51 nested identity(value=…) calls render successfully as Ok(“ok”) instead of returning RenderStackExceededError. I plan to make nested function-argument recursion count toward MAX_DEPTH and add boundary regression tests, while preserving valid nesting up to the limit. I found no existing PR for this. Please let me know if someone is already working on it.

Su

PR is up: https://github.com/mountain-loop/yaak/pull/523

It keeps the existing shared render-depth budget, adds the missing guard and increment for nested function arguments, and covers both Throw and ReturnEmpty behavior. I kept the other two items out of scope.

Type to search feedback...