fix: support recursive attributes in lookup-entity

[erberkson]

…ributes

fix: support recursive attributes in lookup-entity

[matrixreview]

🔴 MatrixReview — RED

⚙️ = code-backed  ·  🔎 = doc-backed  ·  💭 = AI suggestion  ·  📖 = doc citation  ·  📝 = PR location

Risk: 5 files directly affected
Findings: 12 (1 code-backed, 13 doc-backed, 3 AI suggestions)

🔴 SECURITY — 5 findings (4 doc-backed, 1 AI) · expand 🔽

• 🔎 [SECURITY] The PR introduces a new function decodeCursorValue that decodes a cursor token from a string. This function is used in the new expandRecursiveRelation method for pagination. If the cursor is us...

  Read more · expand 🔽

  ...er-controlled (e.g., from an API request), an attacker could potentially inject a maliciously crafted token that, when decoded, could cause unexpected behavior, such as panics or information disclosure, if the decoding fails or returns an unexpected value. The function does not validate the structure of the decoded token beyond type assertion, which could lead to security issues if the token is tampered with.

  - *Also flagged by: ARCHITECTURE, STYLE* 📖 *authorization-service_security_section.md lines 39-41* 📝 `internal/engines/entity_filter.go line 257`

• 🔎 [SECURITY] The PR adds a new constant _maxBFSDepth = 100 to limit recursive relation expansion depth, which is a good practice to prevent infinite recursion or excessive resource consumption. However, the d...

  Read more · expand 🔽

  ...epth limit is hard-coded and may not be sufficient for all schemas or could be too restrictive. An attacker could craft a deeply nested recursive relation chain to trigger the depth limit error, potentially causing a denial-of-service by forcing the system to traverse many levels before hitting the limit. The error message returned includes the depth limit value, which could be considered information disclosure.

  - *Also flagged by: ARCHITECTURE, STYLE, ONBOARDING* 📖 *authorization-service_security_section.md lines 10-15* 📝 `internal/engines/entity_filter.go line 18`

• 🔎 [SECURITY] The expandRecursiveRelation function performs a BFS traversal over recursive relations without any rate limiting or cost limiting beyond the depth limit. An attacker could create a large number o...

  Read more · expand 🔽

  ...f recursive relations (e.g., a densely connected graph) and trigger a lookup that would cause the engine to traverse many entities, consuming significant CPU and memory resources, leading to a potential denial-of-service attack.

  📖 *authorization-service_security_section.md lines 10-15* 📝 `internal/engines/entity_filter.go line 274`

• 🔎 [BUG] In the attributeEntrance function, when expandRecursive is true and a cursor is present, the code fetches all seed entities without cursor filtering (`seedPagination := database.NewCursorPagina...

  Read more · expand 🔽

  ...tion(database.Sort("entity_id"))`). This could lead to performance issues if there are a large number of entities, as it loads all entities of that type into memory, defeating the purpose of pagination.

  📖 *authorization-service_security_section.md lines 10-15* 📝 `internal/engines/entity_filter.go line 211`

• 💭 [BUG] In the expandRecursiveRelation function, the cursor value is decoded and used to filter entities (if cursorValue == "" || entity.GetId() >= cursorValue). This comparison assumes that entity IDs...

  Read more · expand 🔽

  ... are strings and that a lexicographic comparison is appropriate for pagination. However, if entity IDs are not lexicographically ordered (e.g., numeric IDs like "2" vs "10"), this could lead to incorrect pagination results, potentially skipping or duplicating entities. This could break the pagination logic for recursive attribute lookups.

  📝 `internal/engines/entity_filter.go line 356`

🟡 ARCHITECTURE — 4 findings (1 code-backed, 3 doc-backed) · expand 🔽

• ⚙️ HIGH: 1 files depend on modified it.

  Show affected files

  • asset.js (fan-in: ?, tags: none)

• 🔎 [ARCHITECTURE] The PR adds a new method SelfCycleRelationsForPermission to the LinkedSchemaGraph to detect recursive relations. This introduces a new schema analysis capability that is not documented in the a...

  Read more · expand 🔽

  ...rchitecture. The architecture documentation describes the schema server's role as validating requests and passing them to the database access layer, but does not mention runtime schema analysis for recursion detection. This could affect the performance and consistency of the schema server.

  📖 *infrastructure.md lines 40-45* 📝 `internal/schema/linked_schema.go line 514`

• 🔎 [ARCHITECTURE] The PR modifies the findEntranceLeaf function in linked_schema.go to handle nested path chains and preserve tuple-set relations. This change affects the schema graph traversal logic, which is a...

  Read more · expand 🔽

  ... core part of the permission evaluation engine. The architecture documentation describes the permission server's invoker breaking down queries into sub-queries, but does not detail the schema graph traversal algorithms. Changes to this core logic without architectural documentation could impact the consistency and correctness of permission evaluations.

  📖 *infrastructure.md lines 33-38* 📝 `internal/schema/linked_schema.go line 272`

• 🔎 [PERFORMANCE] The PR introduces a BFS expansion algorithm for recursive relations in entity_filter.go with a depth limit. This algorithm could lead to increased database queries and memory usage for deep recur...

  Read more · expand 🔽

  ...sion graphs, impacting the performance of the lookup engine. The architecture documentation states that loading authorization data presents a 'significant downside in terms of performance and scalability' and that the authorization service must be 'fast'. The new recursive expansion could exacerbate performance issues if not carefully optimized.

  📖 *authorization-service_architecture_section.md lines 31-38* 📝 `internal/engines/entity_filter.go line 270`

🟢 LEGAL — No issues found

🟡 STYLE — 1 findings (1 AI) · expand 🔽

• 💭 [STYLE] The test 'should stop same-type recursive attribute expansion at the BFS depth limit' uses a hardcoded loop to create tuples up to '_maxBFSDepth'. The test may be inefficient and could be simplifie...

  Read more · expand 🔽

  ...d. However, no explicit style rule is violated.

  📝 `internal/engines/lookup_test.go line 6390`

🔴 ONBOARDING — 2 findings (1 doc-backed, 1 AI) · expand 🔽

• 🔎 [CHORE] The PR description is minimal and does not follow the recommended issue/PR template structure. The CONTRIBUTING_onboarding_section.md suggests discussing new features with community members/maintai...

  Read more · expand 🔽

  ...ners and ensuring to follow the steps before contributing. The PR description lacks context about the problem being solved, the design decisions, or any prior discussion.

  📖 *CONTRIBUTING_onboarding_section.md lines 3-8*

• 💭 [CHORE] The PR includes extensive test additions (check_test.go, lookup_test.go, linked_schema_test.go) but does not appear to have corresponding updates to the test documentation or contribution guideline...

  Read more · expand 🔽

  ...s about testing standards. The CONTRIBUTING_onboarding_section.md advises to ensure no duplicate issues and to discuss features, but does not explicitly require tests. However, the intro_onboarding_section.md states 'We're collaboratively working with our community to make Permify the best it can be! You can develop new features, fix existing issues...' which implies maintaining quality, including testing. The lack of any mention of test updates in the PR description or linked documentation suggests the contributor may not have considered updating test guidelines.

  📖 *intro_onboarding_section.md lines 10-12* 📝 `internal/engines/check_test.go line 2375`

👆 Click expand on any gate above to see full findings with evidence and citations.

---

Powered by MatrixReview · Report incorrect finding

⚙️ Generate Fix