Fix workspace passing contextId - clean up CFH for scoped keys (legacy) #424
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Added logic to reconstruct missing filenames from URLs in the CortexFileHandler, improving file upload responses. - Ensured that hash values are set correctly when missing, enhancing data integrity. - Refactored Redis key management to eliminate the use of scoped hash keys, improving security and clarity in key handling. - Updated tests to reflect changes in key management and ensure proper functionality across various scenarios.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors Redis key management in the Cortex File Handler to improve security isolation and simplify the scoping mechanism. The key change moves from hash-level scoping (e.g., hash:ctx:contextId) to Redis map-level scoping (e.g., storing hashes in FileStoreMap:ctx:contextId maps), ensuring strict context isolation and preventing cross-context data access.
- Removed the
getScopedHashKeyhelper function and all references to "scoped hash" keys - Enforced security isolation: context-scoped lookups never fall back to unscoped or legacy keys
- Simplified the
removeFromFileStoreMapfunction to work with hash and contextId parameters directly - Added filename reconstruction and hash enrichment logic in the upload handler to ensure complete metadata
- Updated
executeWorkspace.jsto properly spread all pathway arguments (including contextId) before overriding specific fields
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/executeWorkspace.js | Improved parameter passing by spreading all pathwayArgs first (including contextId), then overriding specific fields; added fallback for systemPrompt |
| helper-apps/cortex-file-handler/src/redis.js | Removed getScopedHashKey function; enforced strict context isolation in getFileStoreMap; simplified removeFromFileStoreMap to use hash + contextId parameters |
| helper-apps/cortex-file-handler/src/index.js | Added logic to reconstruct missing filenames from URLs and ensure hash field is set in upload responses |
| helper-apps/cortex-file-handler/tests/setRetention.test.js | Removed import of getScopedHashKey function |
| helper-apps/cortex-file-handler/tests/redisMigration.test.js | Updated tests to use hash + contextId parameters instead of getScopedHashKey; removed getScopedHashKey tests; updated documentation |
| helper-apps/cortex-file-handler/tests/deleteOperations.test.js | Simplified test to remove references to getScopedHashKey and scoped key concepts |
| helper-apps/cortex-file-handler/package.json | Bumped version from 2.8.0 to 2.8.1 |
| helper-apps/cortex-file-handler/package-lock.json | Updated version to match package.json |
Files not reviewed (1)
- helper-apps/cortex-file-handler/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Only allow fallback for unscoped keys (not context-scoped) | ||
| // Context-scoped keys are security-isolated and must match exactly | ||
| if (baseHash && !String(baseHash).includes(":")) { | ||
| if (hash && !String(hash).includes(":")) { |
There was a problem hiding this comment.
This use of variable 'hash' always evaluates to true.
Suggested change
| if (hash && !String(hash).includes(":")) { | |
| if (!String(hash).includes(":")) { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request refactors the Redis key management logic in the Cortex File Handler to simplify scoping, improve security isolation, and clarify migration from legacy key formats. The main changes remove the concept of "scoped hashes" in favor of storing the context scoping at the Redis map level, ensure context isolation for file metadata, and update related tests and documentation accordingly.
Redis Key Management Refactor and Security Improvements:
getScopedHashKeyfunction and the concept of "scoped hashes," so context scoping is now handled at the Redis map level (e.g.,FileStoreMap:ctx:<contextId>) rather than by modifying the hash key itself. [1] [2] [3]getFileStoreMapfunction to ensure that context-scoped lookups never fall back to unscoped or legacy keys, enforcing strict security isolation between contexts. [1] [2] [3] [4]removeFromFileStoreMapfunction to remove keys based on hash and contextId parameters, eliminating legacy logic for extracting base hashes from "scoped" keys. [1] [2]Test and Documentation Updates:
getScopedHashKey, and ensuring tests use the correct parameters for context-scoped operations. [1] [2] [3] [4]Minor Enhancements and Version Bump:
2.8.1to reflect these changes. [1] [2]