test(editor): close four more Phase 2 gaps; expose a real cross-org leak

P1 endpoint contract. catalog_functions can be perfect while no HTTP route
exposes it, and the struct can carry a doc that serialization drops. Added a
serialization test asserting name/signature/doc/kind/deprecated all survive
serde with doc non-empty, plus a router test asserting the query_functions
route is not 404, following the existing oneshot pattern in router/mod.rs.

P1 isolation fixture was vacuous and hid a REAL BUG. get_all_transform
(transform_udf.rs:52) matches with key().contains(org_id) -- a substring match,
not a prefix match on the "org/" boundary. org_alpha and org_beta do not
overlap, so the test sailed past it. The fixture is now acme / acme-prod, where
the key "acme-prod/prod_only_fn" DOES contain "acme".

P2 the signature assertion reduced to non-empty, so even "()" passed for a
two-argument transform. It now counts placeholders.

P1 the org-switch test proved a second request happened but not that the old
org's entries disappeared. It now mocks distinct per-org responses and asserts
the stale function is gone, the new one is present, and the org-independent
local catalog survives.

NOTE, beyond Phase 2 scope: the contains match in get_all_transform is a live
cross-tenant leak, not a test-only concern. get_all_transform feeds register_udf
(exec.rs:288), the production query path, so an org whose id is a prefix of
another org's currently gets that org's VRL functions registered into its
DataFusion context. Worth its own fix and regression test.

Web: 69 failing, 3 suites blocked on unwritten modules. cargo fmt clean.
This commit is contained in:
Prabhat Sharma 2026-08-02 12:21:36 -07:00
parent 27c703f828
commit 82b9fd58c2
3 changed files with 129 additions and 23 deletions

View File

@ -1514,6 +1514,30 @@ mod tests {
);
}
// ── tmp/code.md B4 — the query-function catalog route ─────────────────────
//
// catalog_functions() can be perfect while no HTTP route exposes it. This is
// the only assertion that fails if the endpoint is simply never registered.
#[tokio::test]
async fn query_functions_route_is_registered() {
let app = service_routes();
let req = Request::builder()
.uri("/myorg/query_functions")
.body(Body::empty())
.unwrap();
let response = app.oneshot(req).await.unwrap();
// Auth middleware may reject the request; what must NOT happen is 404,
// which would mean the route does not exist at all.
assert_ne!(
response.status(),
StatusCode::NOT_FOUND,
"GET /{{org_id}}/query_functions is not registered"
);
}
// ── is_origin_allowed unit tests ──────────────────────────────────────────
#[test]

View File

@ -844,7 +844,11 @@ mod tests {
let mut sorted = names.to_vec();
sorted.sort();
sorted.dedup();
assert_eq!(names, sorted.as_slice(), "names must be sorted and deduplicated");
assert_eq!(
names,
sorted.as_slice(),
"names must be sorted and deduplicated"
);
}
#[test]
@ -884,7 +888,10 @@ mod tests {
.iter()
.find(|f| f.name == "match_all")
.expect("match_all should be in the catalog");
assert!(!match_all.signature.is_empty(), "signature must be populated");
assert!(
!match_all.signature.is_empty(),
"signature must be populated"
);
assert!(!match_all.kind.is_empty(), "kind must be populated");
}
@ -895,11 +902,40 @@ mod tests {
.iter()
.find(|f| f.name == "match_all_raw")
.expect("match_all_raw should be in the catalog");
assert!(raw.deprecated, "rewriter aliases must be flagged deprecated");
assert!(
raw.deprecated,
"rewriter aliases must be flagged deprecated"
);
let canonical = catalog.iter().find(|f| f.name == "match_all").unwrap();
assert!(!canonical.deprecated, "match_all itself is not deprecated");
}
#[test]
fn catalog_functions_serialize_with_every_field_the_editor_needs() {
// Asserting on the Rust struct does not prove the HTTP body carries the
// fields: a rename or a skip_serializing_if would pass the struct tests
// and still ship an empty docs panel.
let catalog = catalog_functions("default");
let json = serde_json::to_value(&catalog).expect("catalog must serialize");
let arr = json.as_array().expect("catalog serializes to an array");
let match_all = arr
.iter()
.find(|f| f.get("name").and_then(|n| n.as_str()) == Some("match_all"))
.expect("match_all must be in the serialized catalog");
for field in ["name", "signature", "doc", "kind", "deprecated"] {
assert!(
match_all.get(field).is_some(),
"serialized entry is missing `{field}`"
);
}
assert!(
!match_all["doc"].as_str().unwrap_or("").is_empty(),
"documentation must be non-empty in the serialized payload"
);
assert!(!match_all["signature"].as_str().unwrap_or("").is_empty());
}
#[test]
fn catalog_functions_is_sorted_and_deduped() {
let catalog = catalog_functions("default");
@ -912,8 +948,10 @@ mod tests {
#[test]
fn catalog_functions_scopes_vrl_transforms_to_their_own_org() {
// Per-org VRL transforms are the third source in the union. An org must
// never be shown another org's functions.
// Org IDs deliberately OVERLAP as substrings. get_all_transform matches
// with `key().contains(org_id)`, so a fixture like org_alpha/org_beta
// passes even though "acme" matches the key "acme-prod/...". A prefix
// match on "{org}/" is what isolation actually requires.
use config::meta::function::Transform;
let mk = |name: &str| Transform {
function: ".".to_string(),
@ -923,35 +961,41 @@ mod tests {
trans_type: Some(0),
streams: None,
};
transform::QUERY_FUNCTIONS.insert("org_alpha/alpha_only_fn".to_string(), mk("alpha_only_fn"));
transform::QUERY_FUNCTIONS.insert("org_beta/beta_only_fn".to_string(), mk("beta_only_fn"));
transform::QUERY_FUNCTIONS.insert("acme/acme_only_fn".to_string(), mk("acme_only_fn"));
transform::QUERY_FUNCTIONS.insert("acme-prod/prod_only_fn".to_string(), mk("prod_only_fn"));
let alpha: Vec<String> = catalog_functions("org_alpha")
let acme: Vec<String> = catalog_functions("acme")
.iter()
.map(|f| f.name.clone())
.collect();
let beta: Vec<String> = catalog_functions("org_beta")
let prod: Vec<String> = catalog_functions("acme-prod")
.iter()
.map(|f| f.name.clone())
.collect();
assert!(alpha.contains(&"alpha_only_fn".to_string()), "own transform missing");
assert!(
!alpha.contains(&"beta_only_fn".to_string()),
"LEAKED another org's VRL transform into the catalog"
acme.contains(&"acme_only_fn".to_string()),
"own transform missing"
);
assert!(beta.contains(&"beta_only_fn".to_string()), "own transform missing");
assert!(
!beta.contains(&"alpha_only_fn".to_string()),
"LEAKED another org's VRL transform into the catalog"
!acme.contains(&"prod_only_fn".to_string()),
"LEAKED acme-prod's VRL transform into acme — substring org matching"
);
assert!(
prod.contains(&"prod_only_fn".to_string()),
"own transform missing"
);
assert!(
!prod.contains(&"acme_only_fn".to_string()),
"LEAKED acme's VRL transform into acme-prod"
);
// Built-ins are org-independent and must appear for both.
assert!(alpha.contains(&"match_all".to_string()));
assert!(beta.contains(&"match_all".to_string()));
assert!(acme.contains(&"match_all".to_string()));
assert!(prod.contains(&"match_all".to_string()));
transform::QUERY_FUNCTIONS.remove("org_alpha/alpha_only_fn");
transform::QUERY_FUNCTIONS.remove("org_beta/beta_only_fn");
transform::QUERY_FUNCTIONS.remove("acme/acme_only_fn");
transform::QUERY_FUNCTIONS.remove("acme-prod/prod_only_fn");
}
#[test]
@ -973,10 +1017,18 @@ mod tests {
.iter()
.find(|f| f.name == "gamma_fn")
.expect("org transform should be in the catalog");
assert_eq!(gamma.kind, "vrl", "org transforms must be distinguishable from builtins");
assert!(
gamma.signature.contains("2") || !gamma.signature.is_empty(),
"signature should reflect the declared argument count"
assert_eq!(
gamma.kind, "vrl",
"org transforms must be distinguishable from builtins"
);
// Count the placeholders rather than sniffing for a digit: the previous
// `contains("2") || !is_empty()` reduced to "non-empty", so even "()"
// passed for a two-argument transform.
assert_eq!(
gamma.signature.matches("arg").count(),
2,
"signature must expose one placeholder per declared argument, got {}",
gamma.signature
);
transform::QUERY_FUNCTIONS.remove("org_gamma/gamma_fn");
}

View File

@ -97,6 +97,36 @@ describe("Phase 2 — the server catalog is actually fetched (B4 wiring)", () =>
expect(queryFunctions.list).toHaveBeenCalledWith("otherorg");
});
it("DROPS the previous organisation's functions when the org changes", async () => {
// A second request is not enough: if the old org's entries survive the
// switch, one tenant sees another tenant's VRL function names.
vi.mocked(queryFunctions.list).mockImplementation((org: string) =>
Promise.resolve({
data: {
list:
org === "myorg"
? [{ name: "myorg_only_fn", signature: "(a)", doc: "A.", kind: "vrl" }]
: [{ name: "otherorg_only_fn", signature: "(a)", doc: "B.", kind: "vrl" }],
},
} as any),
);
const c = makeComposable({ storedValues: [] });
await run(c, "SELECT * FROM stream WHERE ");
await run(c, "SELECT * FROM stream WHERE ");
let names = (c.effectiveSuggestions.value as any[]).map((s) => s.name);
expect(names).toContain("myorg_only_fn");
c.autoCompleteData.value.org = "otherorg";
await run(c, "SELECT * FROM stream WHERE ");
await run(c, "SELECT * FROM stream WHERE ");
names = (c.effectiveSuggestions.value as any[]).map((s) => s.name);
expect(names).toContain("otherorg_only_fn");
expect(names, "stale org functions survived the switch").not.toContain("myorg_only_fn");
// Local catalog is org-independent and must survive.
expect(names).toContain("match_all");
});
it("does not call the service before an organisation is known", async () => {
const c = makeComposable({ storedValues: [] });
c.autoCompleteData.value.org = "";