Conversation
Enforce tenant validation and scoping in advanced memory dynamics endpoints and propagate the tenant identifier down to the corresponding helper functions. Also resolve a parameter order mismatch in q.upd_feedback. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
WalkthroughDynamics endpoints now require tenant context and enforce tenant-scoped memory and waypoint access. Retrieval operations propagate tenant filters, API key comparison uses length checks and raw-byte timing-safe comparison, and feedback persistence includes a timestamp with reordered arguments. A security notice documents the tenant-isolation issue. ChangesTenant isolation and security hardening
Feedback persistence update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant dynroutes
participant require_tenant
participant dynamics
participant Database
Client->>dynroutes: dynamics request
dynroutes->>require_tenant: resolve tenant
require_tenant-->>dynroutes: tenant context
dynroutes->>dynamics: invoke tenant-scoped operation
dynamics->>Database: query tenant-scoped memories and waypoints
Database-->>dynamics: filtered records
dynamics-->>dynroutes: operation result
dynroutes-->>Client: response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/openmemory-js/src/ops/dynamics.ts`:
- Around line 171-179: Make tenant context mandatory across the dynamics
retrieval APIs: update buildAssociativeWaypointGraphFromMemories to require
tenant and always use the tenant-scoped waypoint query, update the
spreading-activation function at lines 209-211 to require tenant, and update the
memories retrieval function at lines 241-247 to require tenant and always use
its scoped query. Apply these changes at
packages/openmemory-js/src/ops/dynamics.ts lines 171-179, 209-211, and 241-247,
removing optional and unscoped fallback paths.
- Around line 175-179: Validate both waypoint endpoints belong to the active
tenant before they are used: update the waypoint query in
packages/openmemory-js/src/ops/dynamics.ts lines 175-179 to enforce ownership
for src_id and dst_id, and add an ownership check before upd_seen in
packages/openmemory-js/src/server/routes/dynamics.ts lines 271-276 so
tenant-scoped waypoints cannot reinforce another tenant’s memory.
In `@packages/openmemory-js/src/server/routes/dynamics.ts`:
- Around line 245-249: Replace truthiness-based ownership checks in the
reinforced memory authorization block (lines 245-249), activation seed
validation block (lines 340-352), and source/target memory authorization block
(lines 498-505) with strict equality checks requiring each relevant user_id to
equal tenant; reject missing or mismatched owners in all three sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17a16819-b05e-4e53-9afe-2cb845f89ce7
📒 Files selected for processing (4)
.jules/sentinel.mdpackages/openmemory-js/src/memory/hsg.tspackages/openmemory-js/src/ops/dynamics.tspackages/openmemory-js/src/server/routes/dynamics.ts
Enforce tenant validation and scoping in advanced memory dynamics endpoints and propagate the tenant identifier down to the corresponding helper functions. Also resolve a parameter order mismatch in q.upd_feedback. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
…666522845447929763
Enforce tenant validation and scoping in advanced memory dynamics endpoints and propagate the tenant identifier down to the corresponding helper functions. Also resolve a parameter order mismatch in q.upd_feedback. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
Enforce tenant validation and scoping in advanced memory dynamics endpoints and propagate the tenant identifier down to the corresponding helper functions. Also resolve a parameter order mismatch in q.upd_feedback. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/openmemory-js/src/server/middleware/auth.ts`:
- Around line 92-96: Update validate_api_key to convert provided and expected to
buffers before validation, compare their byteLength values, and return false
when they differ before calling crypto.timingSafeEqual. Preserve the timing-safe
comparison for equal-length buffers.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43b5f9bf-4217-4296-877f-23cfea2c11d5
📒 Files selected for processing (3)
packages/openmemory-js/src/ops/dynamics.tspackages/openmemory-js/src/server/middleware/auth.tspackages/openmemory-js/src/server/routes/dynamics.ts
| function validate_api_key(provided: string, expected: string): boolean { | ||
| if (!provided || !expected) return false; | ||
|
|
||
| // Hash both values to prevent key length leakage via timing or error channels | ||
| const provided_hash = crypto.createHash("sha256").update(provided).digest(); | ||
| const expected_hash = crypto.createHash("sha256").update(expected).digest(); | ||
|
|
||
| return crypto.timingSafeEqual(provided_hash, expected_hash); | ||
| if (provided?.length !== expected?.length) | ||
| return false; | ||
| return crypto.timingSafeEqual(Buffer.from(provided), Buffer.from(expected)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Compare buffer byte lengths to prevent RangeError crashes.
crypto.timingSafeEqual strictly requires the two buffers to have identical byte lengths; otherwise, it throws a RangeError. Since a string's length counts characters rather than bytes, comparing string lengths does not guarantee equal buffer byte lengths (for instance, if an API key contains multi-byte characters). This can lead to uncaught exceptions and 500 errors.
Convert the strings to buffers first, then compare their byteLength.
🔧 Proposed fix
function validate_api_key(provided: string, expected: string): boolean {
- if (provided?.length !== expected?.length)
- return false;
- return crypto.timingSafeEqual(Buffer.from(provided), Buffer.from(expected));
+ const providedBuf = Buffer.from(provided || "");
+ const expectedBuf = Buffer.from(expected || "");
+ if (providedBuf.byteLength !== expectedBuf.byteLength) {
+ return false;
+ }
+ return crypto.timingSafeEqual(providedBuf, expectedBuf);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function validate_api_key(provided: string, expected: string): boolean { | |
| if (!provided || !expected) return false; | |
| // Hash both values to prevent key length leakage via timing or error channels | |
| const provided_hash = crypto.createHash("sha256").update(provided).digest(); | |
| const expected_hash = crypto.createHash("sha256").update(expected).digest(); | |
| return crypto.timingSafeEqual(provided_hash, expected_hash); | |
| if (provided?.length !== expected?.length) | |
| return false; | |
| return crypto.timingSafeEqual(Buffer.from(provided), Buffer.from(expected)); | |
| } | |
| function validate_api_key(provided: string, expected: string): boolean { | |
| const providedBuf = Buffer.from(provided || ""); | |
| const expectedBuf = Buffer.from(expected || ""); | |
| if (providedBuf.byteLength !== expectedBuf.byteLength) { | |
| return false; | |
| } | |
| return crypto.timingSafeEqual(providedBuf, expectedBuf); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/openmemory-js/src/server/middleware/auth.ts` around lines 92 - 96,
Update validate_api_key to convert provided and expected to buffers before
validation, compare their byteLength values, and return false when they differ
before calling crypto.timingSafeEqual. Preserve the timing-safe comparison for
equal-length buffers.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
|
@coderabbitai resolve merge conflict |
|
✅ Merge conflicts resolved successfully! Resolved 4 conflict file(s). Commit: 3 file operation(s)
View agent analysis |
Resolved conflicts in: - .jules/sentinel.md (content) - packages/openmemory-js/src/ops/dynamics.ts (content) - packages/openmemory-js/src/server/middleware/auth.ts (content) - packages/openmemory-js/src/server/routes/dynamics.ts (content) Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
1 participant
🚨 Severity: HIGH
💡 Vulnerability: Cross-Tenant Data Leakage in Advanced Memory Dynamics Routes
🎯 Impact: Authenticated users could retrieve, reinforce, or query memories and waypoint graphs belonging to other tenants by invoking
/dynamicsendpoints without proper tenant scoping or ownership checks.🔧 Fix:
/dynamics/retrieval/energy-based,/dynamics/reinforcement/trace,/dynamics/activation/spreading,/dynamics/waypoints/graph, and/dynamics/waypoints/calculate-weight.✅ Verification: Ran the test suite successfully and verified that the omnibus test Phase 2 passes regression-free.
PR created automatically by Jules for task 1666522845447929763 started by @lucivskvn
Summary by CodeRabbit