perf: use predefined function for context check policies #959
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant changes to the handling of predefined functions and context checks across multiple files. The modifications focus on enhancing the flexibility and type safety of predefined functions by introducing new data structures like Changes
Suggested reviewers
Possibly related PRs
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/typegraph/core/src/lib.rs (1)
234-235: Avoid including potentially sensitive debug information in error messagesUsing
format!("Error while serializing context check: {e:?}")may expose internal details. Consider usinge'sDisplayimplementation or chaining the error for a cleaner message without revealing sensitive information.Apply this diff to adjust the error message:
- .map_err(|e| format!("Error while serializing context check: {e:?}"))?, + .map_err(|e| format!("Error while serializing context check: {}", e))?,src/common/src/typegraph/runtimes/deno.rs (1)
52-52: Avoid including potentially sensitive debug information in error messagesUsing
anyhow!("invalid predefined function materializer: {e:?}")may expose internal details. Consider usinge'sDisplayimplementation or chaining the error with.context()to avoid revealing sensitive information.Apply this diff to adjust the error message:
- .map_err(|e| anyhow!("invalid predefined function materializer: {e:?}")) + .map_err(|e| anyhow!("invalid predefined function materializer: {}", e))src/typegraph/core/src/validation/materializers.rs (1)
31-72: Consider extracting error message mapping to a separate functionThe match expression for error messages is repeated across multiple variants. Consider extracting this to a helper function to improve maintainability.
+fn get_predefined_function_name(predef: &PredefinedFunctionMatData) -> &'static str { + match predef { + P::True => "true", + P::False => "false", + P::Allow => "allow", + P::Deny => "deny", + P::Pass => "pass", + P::ContextCheck { .. } => "context_check", + P::InternalPolicy { .. } => "internal_policy", + _ => unreachable!(), + } +}src/typegate/src/runtimes/deno/deno.ts (1)
37-42: Consider using constants for policy outputsThe string literals "ALLOW", "DENY", "PASS" are used multiple times. Consider defining them as constants to prevent typos and improve maintainability.
+const POLICY_RESULT = { + ALLOW: "ALLOW", + DENY: "DENY", + PASS: "PASS", +} as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/common/src/typegraph/runtimes/deno.rs(2 hunks)src/typegate/src/runtimes/deno/deno.ts(2 hunks)src/typegate/src/typegraphs/typegate.py(2 hunks)src/typegraph/core/src/conversion/runtimes.rs(1 hunks)src/typegraph/core/src/errors.rs(0 hunks)src/typegraph/core/src/global_store.rs(4 hunks)src/typegraph/core/src/lib.rs(5 hunks)src/typegraph/core/src/runtimes/deno.rs(2 hunks)src/typegraph/core/src/runtimes/mod.rs(1 hunks)src/typegraph/core/src/validation/errors.rs(0 hunks)src/typegraph/core/src/validation/materializers.rs(2 hunks)src/typegraph/core/wit/typegraph.wit(1 hunks)src/typegraph/deno/src/runtimes/deno.ts(1 hunks)src/typegraph/python/typegraph/runtimes/deno.py(1 hunks)
💤 Files with no reviewable changes (2)
- src/typegraph/core/src/errors.rs
- src/typegraph/core/src/validation/errors.rs
✅ Files skipped from review due to trivial changes (1)
- src/typegraph/deno/src/runtimes/deno.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (18)
src/typegraph/core/src/lib.rs (3)
55-65: Conversion betweenContextCheckandContextCheckXimplemented effectivelyThe
Fromtrait implementation allows seamless conversion betweenContextCheckandContextCheckX, enhancing code clarity and type safety.
199-199: Update to usePredefinedFunctionMatData::InternalPolicyis appropriateThe change to
DenoMaterializer::Predefined(PredefinedFunctionMatData::InternalPolicy)correctly reflects the updated structure and ensures consistency in handling internal policies.
225-229: Proper serialization of context check parametersThe context check is correctly converted and serialized into JSON, ensuring accurate data is passed to the materializer.
src/typegraph/core/src/runtimes/deno.rs (1)
30-30: UpdateDenoMaterializer::Predefinedvariant to usePredefinedFunctionMatDataThe
DenoMaterializerenum now usesPredefinedFunctionMatData, which is consistent with the new data structures and improves the representation of predefined functions.src/common/src/typegraph/runtimes/deno.rs (2)
15-22: Definition ofContextCheckXenum is appropriateThe
ContextCheckXenum is correctly defined with necessary variants and serialization attributes, facilitating proper handling of context checks.
24-36: Definition ofPredefinedFunctionMatDataenum is well-structuredThe
PredefinedFunctionMatDataenum effectively encapsulates different types of predefined function materializer data, and the use of serde attributes ensures correct serialization and deserialization.src/typegraph/core/src/validation/materializers.rs (1)
4-4: LGTM! Good use of type-safe enumsThe introduction of
PredefinedFunctionMatDataenhances type safety by replacing string-based matching with enum variants.src/typegraph/python/typegraph/runtimes/deno.py (1)
121-121: LGTM! Proper alignment with new parameter structureThe addition of
param=Nonecorrectly aligns with the new predefined function parameter structure while maintaining backward compatibility.src/typegate/src/runtimes/deno/deno.ts (2)
33-36: LGTM! Improved type definitionThe type definition for predefinedFuncs correctly specifies that each function takes a parameter and returns a resolver.
45-68: Verify context access securityThe context_check implementation accesses nested context properties using a dot-notation path. Ensure that:
- The path traversal is properly bounded
- Access to sensitive context properties is restricted
✅ Verification successful
Context access implementation is secure
The context_check implementation uses proper path traversal with optional chaining and follows secure patterns consistent with the rest of the codebase. The path is properly bounded and access to context properties is safely implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of context property access to verify consistent security patterns rg -A 3 'context\??\.\[.*\]|context\??\.[\w.]+'Length of output: 54504
src/typegate/src/typegraphs/typegate.py (1)
97-97: LGTM! Good simplification of policy definitionThe change to use
Policy.contextsimplifies the admin-only check implementation while potentially improving performance by avoiding runtime overhead.src/typegraph/core/src/global_store.rs (4)
17-17: LGTM! Import of PredefinedFunctionMatData.The import aligns with the PR's objective to use predefined functions for context checks.
59-59: LGTM! Updated HashMap type for predefined functions.The change from
HashMap<String, MaterializerId>toHashMap<PredefinedFunctionMatData, MaterializerId>improves type safety and allows for more structured data.
92-92: LGTM! Updated default materializer.The change to use
PredefinedFunctionMatData::Passas the default materializer aligns with the new type system.
350-367: LGTM! Enhanced predefined function handling.The updated
get_predefined_deno_functionmethod now:
- Accepts an optional parameter for flexible context checks
- Uses structured
PredefinedFunctionMatDatafor better type safety- Maintains the same caching behavior for performance
src/typegraph/core/src/conversion/runtimes.rs (1)
131-131: LGTM! Simplified predefined function data conversion.The change improves the conversion logic by directly serializing the entire
predefobject instead of manually constructing a JSON object. This approach is more maintainable and type-safe.src/typegraph/core/wit/typegraph.wit (1)
271-271: LGTM! Added optional parameter to predefined function interface.The addition of
param: option<string>to thematerializer-deno-predefinedrecord:
- Enhances flexibility in context check policies
- Maintains backward compatibility through optional parameter
- Aligns with the PR's objective
src/typegraph/core/src/runtimes/mod.rs (1)
298-298: LGTM! Updated predefined function handling.The change correctly propagates the new
paramfield toStore::get_predefined_deno_function, maintaining consistency with the interface changes.
Natoandro
requested review from
zifeo,
michael-0acf4,
Yohe-Am and
luckasRanarison
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/typegate/src/typegraphs/typegate.json (1)
810-824: Improved type safety with structured context check definition.The change from a script-based approach to a structured definition enhances type safety and maintainability. The predefined function pattern with explicit key-value pairs makes the context check more predictable and easier to validate.
This structured approach:
- Reduces runtime errors by moving validation to compile time
- Makes context checks more maintainable and discoverable
- Aligns with the PR's goal of secure main thread execution
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/typegate/src/typegraphs/typegate.json(2 hunks)
🔇 Additional comments (1)
src/typegate/src/typegraphs/typegate.json (1)
Line range hint
810-951: Verify authentication configuration alignment.The context check policy and authentication configuration appear to be correctly aligned (both use "admin" as the username), but let's verify there are no discrepancies in the authentication setup.
Run this script to check the authentication configuration across the codebase:
✅ Verification successful
Authentication configuration is properly aligned
The context check policy using "admin" username is consistent with the authentication configuration across all typegraph definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for authentication configuration patterns rg -A 5 '"protocol":\s*"basic"' --type json # Search for other potential username definitions rg -A 3 '"users":\s*\[' --type jsonLength of output: 1365
Natoandro
dismissed stale reviews from Yohe-Am and luckasRanarison
via
2856393
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/typegate/src/runtimes/deno/deno.ts (2)
58-58: Improve error message in default case.The error message "unreachable" could be more descriptive to help with debugging.
- throw new Error("unreachable"); + throw new Error(`Unsupported context check type: ${value.type}`);
45-68: Enhance type safety and validation in context_check.Consider adding the following improvements:
- Type validation for the
valueparameter- Validation for the
keyformat before splitting- context_check: ({ key, value }) => { + context_check: ({ key, value }: { key: string, value: { type: 'not_null' } | { type: 'value', value: unknown } | { type: 'pattern', value: string } }) => { + if (!key || typeof key !== 'string') { + throw new Error('Context check key must be a non-empty string'); + } let check: (value: any) => boolean; switch (value.type) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/typegate/src/runtimes/deno/deno.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (3)
src/typegate/src/runtimes/deno/deno.ts (3)
259-259: LGTM! Clean and consistent with the new predefined functions structure.The update properly handles the new function signature by passing the parameters correctly.
33-68: Well-structured implementation that achieves the PR objectives.The changes successfully implement:
- Parameterized predefined functions for improved flexibility
- A robust context check policy with proper type handling
- Consistent error handling and type safety
The implementation aligns well with the goal of improving function execution while maintaining security.
54-56: Verify regex pattern safety.The RegExp creation with user-provided patterns could potentially lead to ReDoS (Regular Expression Denial of Service) attacks if not properly validated.
Consider adding pattern validation or timeout mechanisms. Let's check for existing pattern validations in the codebase:
✅ Verification successful
RegExp usage is properly safeguarded.
The pattern implementation is secure as it's part of a type-safe policy system with multiple validation layers. The patterns are validated during policy registration and type checking, with strict rules enforced at the type system level.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for pattern validation or regex safety measures rg -A 3 'new RegExp|pattern.*validation'Length of output: 1902
Script:
#!/bin/bash # Search for pattern-related type definitions and validations rg -A 5 'type.*Pattern|interface.*Pattern|pattern.*validate|validatePattern' # Search for pattern-related sanitization or security measures ast-grep --pattern 'function $FUNC($_) { $$$ pattern $$$ }'Length of output: 146
Script:
#!/bin/bash # Search for pattern-related type definitions and schema validation rg -A 5 'pattern.*type|pattern.*schema|pattern.*check' # Look for type definitions around the pattern case ast-grep --pattern 'case "pattern": $$$'Length of output: 4699
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #959 +/- ##
==========================================
+ Coverage 77.72% 77.78% +0.06%
==========================================
Files 154 154
Lines 19154 19044 -110
Branches 1930 1925 -5
==========================================
- Hits 14888 14814 -74
+ Misses 4243 4207 -36
Partials 23 23 ☔ View full report in Codecov by Sentry. |
<!-- Pull requests are squashed and merged using: - their title as the commit message - their description as the commit body Having a good title and description is important for the users to get readable changelog. --> <!-- 1. Explain WHAT the change is about --> Improve performance by running the functions in the main thread when it can be done securely: - Use predefined function for context check policies - Use context check policy for `admin_only` <!-- 2. Explain WHY the change cannot be made simpler --> <!-- 3. Explain HOW users should update their code --> #### Migration notes --- - [ ] The change comes with new or modified tests - [ ] Hard-to-understand functions have explanatory comments - [ ] End-user documentation is updated to reflect the change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced predefined function handling with optional parameters. - Introduced a more flexible context checking mechanism. - Simplified policy definition for admin access. - **Improvements** - Updated runtime function registration process. - Improved type safety for predefined function validation. - Streamlined error handling for function materialization. - **Changes** - Removed deprecated error handling functions. - Modified internal representations of predefined functions. - Updated function signatures for predefined Deno functions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Improve performance by running the functions in the main thread when it can be done securely:
admin_onlyMigration notes
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes