Authorization analysis logic improvements (#61)

src/auth_analysis/extract/actix_web.rs

@@ -1,7 +1,7 @@
 use super::AuthExtractor;
 use super::axum::{
-    GuardFramework, apply_aliases, dedup_call_sites, expanded_guard_call_sites,
-    guard_calls_for_handler, inject_guard_checks, rust_param_aliases,
+    GuardFramework, apply_aliases, apply_typed_extractor_guards_to_units, dedup_call_sites,
+    expanded_guard_call_sites, guard_calls_for_handler, inject_guard_checks, rust_param_aliases,
 };
 use super::common::{
     attach_route_handler, call_name, collect_top_level_units, named_children, resolve_handler_node,
@@ -36,6 +36,13 @@ impl AuthExtractor for ActixWebExtractor {
 
         collect_top_level_units(root, bytes, rules, &mut model);
         collect_routes(root, root, bytes, path, rules, &mut model);
+        apply_typed_extractor_guards_to_units(
+            root,
+            bytes,
+            rules,
+            &mut model,
+            GuardFramework::ActixWeb,
+        );
 
         model
     }

src/auth_analysis/extract/axum.rs

@@ -35,6 +35,7 @@ impl AuthExtractor for AxumExtractor {
 
         collect_top_level_units(root, bytes, rules, &mut model);
         collect_routes(root, root, bytes, path, rules, &mut model);
+        apply_typed_extractor_guards_to_units(root, bytes, rules, &mut model, GuardFramework::Axum);
 
         model
     }
@@ -391,7 +392,61 @@ fn classify_rocket_param(
 /// non-route functions, and a false positive there suppresses
 /// downstream `V.id` flagging entirely; that path uses a structural
 /// recogniser keyed on the `<PREFIX>User<SUFFIX>?` shape.
+///
+/// Recognition is **outer-wrapper based**: classify by the outermost
+/// type name only, not by substring-anywhere on the whole text.  This
+/// avoids both directions of leakage:
+/// * A bare data-only extractor like `web::Path<u64>` early-returns
+///   `None` regardless of inner type tokens (preserves existing
+///   behaviour).
+/// * A policy-bearing wrapper like
+///   `GuardedData<ActionPolicy<X>, Data<AuthController>>` is
+///   classified by the outer `GuardedData`, not by whether the inner
+///   `Data<AuthController>` happens to lowercase-contain "auth".  The
+///   wrapper proves capability enforcement → `AuthCheckKind::Other`
+///   (the route-level short-circuit in `auth_check_covers_subject`
+///   suppresses missing-ownership-check for non-LoginGuard kinds).
 fn classify_guard_type(type_text: &str) -> Option<AuthCheckKind> {
+    let outer = outermost_type_name(type_text);
+    let outer_lower = outer.to_ascii_lowercase();
+
+    // Bare data-only extractors are *not* auth-bearing regardless of
+    // their inner generic args.  Outer-name match (case-insensitive
+    // exact) — `Path<u64>` / `web::Path<...>` / `Query<X>` /
+    // `Json<X>` / `Form<X>` / `State<X>` / `Extension<X>` /
+    // `Data<X>`.
+    if is_data_only_extractor_outer(&outer_lower) {
+        return None;
+    }
+
+    // Policy/guard-bearing outer wrapper.  Names containing
+    // `guarded` (e.g. `GuardedData`, `GuardedRoute`) signal the
+    // wrapper enforced a capability/permission check at request
+    // construction.  Distinct from `LoginGuard` because Policy
+    // enforcement is more than authentication, it's authorization.
+    if outer_lower.contains("guarded") || outer_lower.contains("guard") {
+        if outer_lower.contains("admin") {
+            return Some(AuthCheckKind::AdminGuard);
+        }
+        return Some(AuthCheckKind::Other);
+    }
+
+    if outer_lower.contains("admin") {
+        return Some(AuthCheckKind::AdminGuard);
+    }
+    if outer_lower.contains("user")
+        || outer_lower.contains("auth")
+        || outer_lower.contains("session")
+        || outer_lower.contains("identity")
+        || outer_lower.contains("principal")
+    {
+        return Some(AuthCheckKind::LoginGuard);
+    }
+
+    // Backwards-compat fallback: legacy whole-text substring check
+    // for unusual shapes whose outer wrapper is generic but whose
+    // qualified path still mentions an auth token.  Preserves
+    // pre-2026-05-02 behaviour for non-Guarded wrappers.
     let lower = type_text.to_ascii_lowercase();
     if is_extractor_wrapper(&lower) {
         return None;
@@ -409,6 +464,49 @@ fn classify_guard_type(type_text: &str) -> Option<AuthCheckKind> {
     }
 }
 
+/// Outermost type name: text before the first `<`, with reference
+/// markers (`&`, `&mut`, `&'a`, etc.) and module-path prefix
+/// (`std::collections::`) stripped.  Returns the empty string for
+/// inputs that don't parse as a type.
+fn outermost_type_name(type_text: &str) -> &str {
+    let trimmed = type_text.trim();
+    let mut after_refs = trimmed;
+    loop {
+        let next = after_refs
+            .trim_start_matches('&')
+            .trim_start_matches("mut ")
+            .trim_start();
+        // Strip any single lifetime token like `'a ` after the `&`.
+        let next = if let Some(rest) = next.strip_prefix('\'') {
+            rest.split_once(' ')
+                .map(|(_, after)| after.trim_start())
+                .unwrap_or(rest)
+        } else {
+            next
+        };
+        if next == after_refs {
+            break;
+        }
+        after_refs = next;
+    }
+    let prefix = after_refs.split('<').next().unwrap_or(after_refs).trim();
+    prefix.rsplit("::").next().unwrap_or(prefix).trim()
+}
+
+/// Outer wrapper name (lowercase, exact-match) that the engine treats
+/// as a bare data-only extractor: yielding the inner type to the
+/// handler without any auth side-effect.  Matched on the outer name
+/// only so policy-bearing wrappers carrying a data extractor as one
+/// of their generic args (e.g.
+/// `GuardedData<Policy, web::Path<u64>>`) are not mis-suppressed by
+/// the inner `Path<...>`.
+fn is_data_only_extractor_outer(outer_lower: &str) -> bool {
+    matches!(
+        outer_lower,
+        "path" | "query" | "json" | "form" | "extension" | "state" | "data" | "reqdata"
+    )
+}
+
 fn classify_rocket_guard_type(
     type_text: &str,
     binding: &str,
@@ -612,6 +710,14 @@ pub(crate) fn inject_guard_checks(
     for call in guard_calls {
         let kind = if rules.is_admin_guard(&call.name, &call.args) {
             AuthCheckKind::AdminGuard
+        } else if rules.is_policy_guard(&call.name) {
+            // Policy/capability-bearing typed extractor (e.g.
+            // meilisearch's `GuardedData<ActionPolicy<X>, _>`).
+            // Recorded as `Other` so the route-level short-circuit in
+            // `auth_check_covers_subject` covers any sink in the
+            // handler — the wrapper proves authorization, not just
+            // authentication.
+            AuthCheckKind::Other
         } else if rules.is_login_guard(&call.name) {
             AuthCheckKind::LoginGuard
         } else {
@@ -633,3 +739,153 @@ pub(crate) fn inject_guard_checks(
         });
     }
 }
+
+/// Walk every `Function`-kind unit in `model` and inject route-level
+/// guard checks for any parameter whose type is recognised as a
+/// typed auth/policy extractor (e.g. meilisearch's `GuardedData<P, D>`,
+/// `axum::extract::State<AuthCtx>`).  Complements the route-walk path
+/// in `collect_routes`: handlers registered by attribute macros
+/// (`#[routes::path(...)]`, `#[get("/path")]`) or by external
+/// service-config builders are never matched as route registrations
+/// here, so their typed-extractor guards would otherwise never be
+/// injected and `missing_ownership_check` would fire on every
+/// id-shaped sink they contain.
+///
+/// `RouteHandler`-kind units already had their guards injected during
+/// the route walk and are skipped to avoid duplicate `AuthCheck`
+/// entries.
+pub(crate) fn apply_typed_extractor_guards_to_units(
+    root: Node<'_>,
+    bytes: &[u8],
+    rules: &AuthAnalysisRules,
+    model: &mut crate::auth_analysis::model::AuthorizationModel,
+    framework: GuardFramework,
+) {
+    use crate::auth_analysis::model::AnalysisUnitKind;
+    let function_nodes = collect_function_definition_nodes(root);
+    for unit_idx in 0..model.units.len() {
+        let span = {
+            let unit = &model.units[unit_idx];
+            if unit.kind == AnalysisUnitKind::RouteHandler {
+                continue;
+            }
+            unit.span
+        };
+        let Some(handler_node) = function_nodes
+            .iter()
+            .find(|node| node.start_byte() == span.0 && node.end_byte() == span.1)
+            .copied()
+        else {
+            continue;
+        };
+        let guard_calls = guard_calls_for_handler(handler_node, "", bytes, framework);
+        if guard_calls.is_empty() {
+            continue;
+        }
+        let unit = &mut model.units[unit_idx];
+        inject_guard_checks(unit, &guard_calls, rules);
+    }
+}
+
+fn collect_function_definition_nodes<'tree>(root: Node<'tree>) -> Vec<Node<'tree>> {
+    let mut out = Vec::new();
+    walk_function_definitions(root, &mut out);
+    out
+}
+
+fn walk_function_definitions<'tree>(node: Node<'tree>, out: &mut Vec<Node<'tree>>) {
+    // Free / impl / trait fn definitions in tree-sitter-rust.
+    if node.kind() == "function_item" {
+        out.push(node);
+    }
+    for child in named_children(node) {
+        walk_function_definitions(child, out);
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn outermost_type_name_strips_refs_and_module_prefix() {
+        assert_eq!(outermost_type_name("GuardedData<P, D>"), "GuardedData");
+        assert_eq!(outermost_type_name("&GuardedData<P, D>"), "GuardedData");
+        assert_eq!(
+            outermost_type_name("&'a mut GuardedData<P, D>"),
+            "GuardedData"
+        );
+        assert_eq!(outermost_type_name("web::Path<u64>"), "Path");
+        assert_eq!(outermost_type_name("std::sync::Arc<Mutex<T>>"), "Arc");
+        assert_eq!(outermost_type_name(""), "");
+        assert_eq!(outermost_type_name("Bare"), "Bare");
+    }
+
+    #[test]
+    fn classify_guard_type_recognises_guarded_data_outer_wrapper() {
+        // Real meilisearch shape with both an admin-token-bearing inner
+        // type and a Data inner extractor — must classify as `Other`
+        // (route-level policy), not LoginGuard (filtered out by
+        // `has_prior_subject_auth`) and not None (over-suppression
+        // would happen if the inner `Data<>` early-return fired).
+        let kind = classify_guard_type(
+            "GuardedData<ActionPolicy<{ actions::KEYS_GET }>, Data<AuthController>>",
+        );
+        assert_eq!(kind, Some(AuthCheckKind::Other));
+    }
+
+    #[test]
+    fn classify_guard_type_data_only_extractor_outer_returns_none() {
+        // Outer `Data<>` is a bare actix data extractor — not auth.
+        // Even though the inner type lower-cases to contain "auth",
+        // the outer-wrapper recognition correctly returns None.
+        assert_eq!(
+            classify_guard_type("Data<AuthController>"),
+            None,
+            "outer Data<> is a bare data extractor, not auth-bearing"
+        );
+        assert_eq!(classify_guard_type("web::Path<UserId>"), None);
+        assert_eq!(classify_guard_type("Json<CreateUser>"), None);
+        assert_eq!(classify_guard_type("Form<LoginForm>"), None);
+    }
+
+    #[test]
+    fn classify_guard_type_preserves_existing_login_guard_recognition() {
+        assert_eq!(
+            classify_guard_type("LocalUserView"),
+            Some(AuthCheckKind::LoginGuard)
+        );
+        assert_eq!(
+            classify_guard_type("Authenticated"),
+            Some(AuthCheckKind::LoginGuard)
+        );
+        assert_eq!(
+            classify_guard_type("AdminUser"),
+            Some(AuthCheckKind::AdminGuard)
+        );
+        assert_eq!(
+            classify_guard_type("CurrentUser"),
+            Some(AuthCheckKind::LoginGuard)
+        );
+    }
+
+    #[test]
+    fn classify_guard_type_admin_guarded_takes_admin_priority() {
+        // `AdminGuard` outer wrapper has both "admin" and "guard" tokens
+        // — admin-priority rule wins inside the Guarded branch.
+        assert_eq!(
+            classify_guard_type("AdminGuard<P, D>"),
+            Some(AuthCheckKind::AdminGuard)
+        );
+        assert_eq!(
+            classify_guard_type("GuardedAdmin<X>"),
+            Some(AuthCheckKind::AdminGuard)
+        );
+    }
+
+    #[test]
+    fn classify_guard_type_unknown_outer_returns_none() {
+        assert_eq!(classify_guard_type("MyCustomWrapper<T>"), None);
+        assert_eq!(classify_guard_type(""), None);
+    }
+}

src/auth_analysis/extract/common.rs

@@ -3455,6 +3455,33 @@ pub fn extract_value_refs(node: Node<'_>, bytes: &[u8]) -> Vec<ValueRef> {
             index: None,
             span: span(node),
         }],
+        // Keyword / named arguments: `Model.objects.filter(organization_id=org.id)`.
+        // Tree-sitter exposes a `name` child (the schema column / parameter
+        // name) and a `value` child (the actual expression).  The default
+        // recurse-all-children arm would surface `organization_id` as a
+        // bare-identifier subject, which `is_id_like_name` then flags as
+        // a scoped-identifier user-input.  But the kwarg key is the
+        // ORM/RPC schema field name, fixed at call time, never
+        // attacker-controlled.  Only the value carries a subject.
+        //
+        // Covers Python `keyword_argument`, JavaScript / TypeScript
+        // `pair` (object property syntax used as kwargs in client libs
+        // like prisma's `where: { id: foo }` is handled separately),
+        // Ruby `pair` (hash kwargs in `Model.where(field: value)`), Go
+        // composite-literal element keys, PHP / C# named arguments.
+        "keyword_argument"
+        | "keyword_arg"
+        | "named_argument"
+        | "named_arg" => {
+            if let Some(value) = node
+                .child_by_field_name("value")
+                .or_else(|| node.child_by_field_name("argument"))
+            {
+                extract_value_refs(value, bytes)
+            } else {
+                Vec::new()
+            }
+        }
         _ => {
             let mut refs = Vec::new();
             for idx in 0..node.named_child_count() {

src/auth_analysis/extract/flask.rs

@@ -127,6 +127,9 @@ fn parse_flask_route_decorator(
     };
 
     let callee = text(function, bytes);
+    if callee_is_test_decorator(&callee) {
+        return None;
+    }
     let method_name = bare_method_name(&callee);
     let arguments = decorator_expr.child_by_field_name("arguments")?;
     let args = named_children(arguments);
@@ -173,6 +176,45 @@ fn parse_methods_keyword(arguments: Node<'_>, bytes: &[u8]) -> Option<Vec<HttpMe
     }
 }
 
+/// True iff the callee text matches a known Python test-framework
+/// decorator that incidentally collides with the Flask `<app>.<verb>`
+/// shape.  `unittest.mock.patch` is the dominant collision: it takes a
+/// string literal as its first positional arg (the import path of the
+/// thing being patched), and `bare_method_name("mock.patch")` is
+/// `patch`, which `parse_flask_route_decorator` previously matched as
+/// HTTP PATCH.  Every test method decorated with `@mock.patch("...")`
+/// was therefore being attached as a Flask route handler, which
+/// flipped its `unit.kind` to `RouteHandler` and made it pass
+/// `unit_has_user_input_evidence` unconditionally — flooding the
+/// pytest test suites with `missing_ownership_check` findings.
+///
+/// The denylist mirrors common mock / monkeypatch / parametrize forms.
+/// Conservative: matches only the canonical receiver chains; an
+/// imported alias `from unittest.mock import patch` then bare
+/// `@patch("x")` would still match `patch` as PATCH, but the
+/// decorator must also carry a string-literal first arg AND the
+/// route-attached unit must come back through the auth analysis to
+/// fire — handlers with a string-arg decorator are rare outside Flask
+/// itself, and the wider precondition path now covers most of those.
+fn callee_is_test_decorator(callee: &str) -> bool {
+    matches!(
+        callee,
+        "mock.patch"
+            | "mock.patch.object"
+            | "mock.patch.dict"
+            | "mock.patch.multiple"
+            | "unittest.mock.patch"
+            | "unittest.mock.patch.object"
+            | "unittest.mock.patch.dict"
+            | "unittest.mock.patch.multiple"
+            | "monkeypatch.setattr"
+            | "monkeypatch.setenv"
+            | "monkeypatch.delattr"
+            | "monkeypatch.delenv"
+            | "pytest.mark.parametrize"
+    )
+}
+
 fn keyword_argument_string(arguments: Node<'_>, bytes: &[u8], name: &str) -> Option<String> {
     let value = keyword_argument_value(arguments, bytes, name)?;
     string_literal_value(value, bytes)
@@ -331,6 +373,41 @@ fn inject_middleware_auth(
     }
 }
 
+#[cfg(test)]
+mod test_decorator_tests {
+    use super::callee_is_test_decorator;
+
+    /// Test-framework decorators that share their bare method name with
+    /// a Flask HTTP verb (`patch`, `delete`, ...) must be excluded
+    /// from `parse_flask_route_decorator`.  Without the denylist,
+    /// every `@mock.patch("module")` in pytest test files attaches
+    /// the test method as a Flask PATCH route handler — flooding the
+    /// auth-analysis with FPs.
+    #[test]
+    fn callee_is_test_decorator_recognises_canonical_forms() {
+        // unittest.mock variants.
+        assert!(callee_is_test_decorator("mock.patch"));
+        assert!(callee_is_test_decorator("mock.patch.object"));
+        assert!(callee_is_test_decorator("mock.patch.dict"));
+        assert!(callee_is_test_decorator("mock.patch.multiple"));
+        assert!(callee_is_test_decorator("unittest.mock.patch"));
+        assert!(callee_is_test_decorator("unittest.mock.patch.object"));
+        // pytest fixtures.
+        assert!(callee_is_test_decorator("monkeypatch.setattr"));
+        assert!(callee_is_test_decorator("monkeypatch.setenv"));
+        assert!(callee_is_test_decorator("pytest.mark.parametrize"));
+        // Negatives — real Flask decorators must still match.
+        assert!(!callee_is_test_decorator("app.route"));
+        assert!(!callee_is_test_decorator("app.get"));
+        assert!(!callee_is_test_decorator("app.post"));
+        assert!(!callee_is_test_decorator("app.patch"));
+        assert!(!callee_is_test_decorator("bp.delete"));
+        assert!(!callee_is_test_decorator("blueprint.put"));
+        assert!(!callee_is_test_decorator("router.get"));
+        assert!(!callee_is_test_decorator(""));
+    }
+}
+
 #[cfg(test)]
 mod fastapi_dependencies_tests {
     use super::is_depends_callee;

src/auth_analysis/extract/mod.rs

@@ -1,6 +1,6 @@
 use super::config::AuthAnalysisRules;
 use super::model::AuthorizationModel;
-use crate::utils::project::FrameworkContext;
+use crate::utils::project::{FrameworkContext, rust_file_imports_web_framework};
 use std::path::Path;
 use tree_sitter::Tree;
 
@@ -61,6 +61,18 @@ pub fn extract_authorization_model(
         }
     }
 
+    // Per-language web-framework signal used to gate the param-name arm
+    // of `unit_has_user_input_evidence`.  Combines the project-root
+    // manifest detection (`framework_ctx`) with a per-file `use`/`import`
+    // check, so a single file in a workspace whose root manifest does
+    // not name a web framework can still opt back in by directly
+    // importing one (e.g. `crates/collab/src/rpc.rs` in zed: workspace
+    // root has no axum, but the file uses `axum::Router`).
+    //
+    // Three-valued: `Some(true)` keeps step 3 firing, `Some(false)`
+    // suppresses it, `None` means no detection ran ─ behavior unchanged.
+    model.lang_web_framework_signal = compute_web_framework_signal(lang, framework_ctx, bytes);
+
     // **Dedup units by span across extractors.**  Multiple extractors
     // (e.g. Flask + Django on a Python file) each call
     // `collect_top_level_units`, producing one unit per top-level
@@ -80,6 +92,53 @@ pub fn extract_authorization_model(
     model
 }
 
+/// Compute the per-file web-framework signal used to gate the
+/// param-name arm of `unit_has_user_input_evidence`.
+///
+/// Currently emits a non-`None` value only for Rust files.  The Rust
+/// auth analysis is the single biggest source of internal-helper FPs
+/// in non-web crates (zed's GUI / editor crates); the other languages
+/// have their own handler-classification policies that already filter
+/// effectively, so they keep their existing behavior (None →
+/// fall-through to the param-name heuristic) until each is validated.
+///
+/// Three-valued semantics:
+/// * `Some(true)` ─ project root manifest names a Rust web framework
+///   (axum / actix_web / rocket), OR the file directly imports one.
+///   Param-name evidence stays on.
+/// * `Some(false)` ─ project root manifest was inspected (Cargo.toml
+///   exists) and named no Rust web framework, AND the file does not
+///   directly import one.  Param-name evidence is suppressed: the
+///   project has no HTTP boundary in Rust.
+/// * `None` ─ no detection ran (no `framework_ctx`, no Cargo.toml
+///   inspected).  Behavior unchanged.
+fn compute_web_framework_signal(
+    lang: &str,
+    framework_ctx: Option<&FrameworkContext>,
+    bytes: &[u8],
+) -> Option<bool> {
+    if !matches!(lang, "rust" | "rs") {
+        return None;
+    }
+    let project_signal = framework_ctx.and_then(|ctx| ctx.lang_has_web_framework("rust"));
+    if project_signal == Some(true) {
+        return Some(true);
+    }
+    // Project says "no Rust framework" or never inspected.  Consult the
+    // file's own imports as a per-file fallback; if the file uses an
+    // axum / actix_web / rocket symbol directly, treat it as a handler
+    // file even when the workspace-root Cargo.toml does not list the
+    // crate.  (Real example: zed's `crates/collab/src/rpc.rs` imports
+    // axum but the workspace root Cargo.toml does not.)
+    if rust_file_imports_web_framework(bytes) {
+        return Some(true);
+    }
+    // No file-level evidence either.  Only flip to `Some(false)` if a
+    // Cargo.toml manifest was actually inspected — single-file scans
+    // without project context get `None` and preserve prior behavior.
+    project_signal
+}
+
 fn deduplicate_units_by_span(model: &mut AuthorizationModel) {
     use crate::auth_analysis::model::{AnalysisUnit, AnalysisUnitKind};
     use std::collections::HashMap;