Conversation
Hash both provided and expected API keys using SHA-256 before comparing them in constant-time with crypto.timingSafeEqual. This ensures that the compared buffers are always of equal length, avoiding early returns on key length mismatch and completely eliminating timing attacks aiming to leak key length and characters. Added exhaustive unit tests under tests/auth.test.ts to verify the middleware behavior under multiple scenarios. 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. |
|
WalkthroughThe PR hardens API-key validation with fixed-length SHA-256 comparisons and adds authentication tests. It also reformats database, memory, vector, route, export, source, and utility code, with one clarified waypoint migration failure path. ChangesOpenMemory updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant authenticate_api_request
participant validate_api_key
participant crypto
Request->>authenticate_api_request: Submit protected request
authenticate_api_request->>validate_api_key: Validate API key
validate_api_key->>crypto: Hash values and compare digests
crypto-->>validate_api_key: Return comparison result
validate_api_key-->>authenticate_api_request: Accept or reject request
authenticate_api_request-->>Request: Continue or return 401/403
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (4)
.jules/sentinel.md (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: "completely eliminating" is redundant.
Consider writing just "eliminating" for conciseness.
🤖 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 @.jules/sentinel.md at line 4, Update the Prevention text in sentinel.md to replace “completely eliminating any timing leaks” with “eliminating any timing leaks,” leaving the hashing and timingSafeEqual guidance unchanged.Source: Linters/SAST tools
packages/openmemory-js/tests/auth.test.ts (2)
81-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
ApiKeyauthorization scheme.
extract_api_keyinauth.ts(line 89) supportsAuthorization: ApiKey <key>, but no test exercises this path. Consider adding a case to ensure this scheme isn't accidentally broken.♻️ Proposed test case
it("should accept requests with valid API key via Authorization ApiKey header", () => { const req = { url: "/api/memory/all", headers: { authorization: "ApiKey super-secret-test-api-key-123456789", }, }; const res = mockResponse(); let nextCalled = false; const next = () => { nextCalled = true; }; authenticate_api_request(req, res, next); expect(nextCalled).toBe(true); expect((req as any).tenant).toBeDefined(); });🤖 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/tests/auth.test.ts` around lines 81 - 97, Add a test alongside the existing Bearer authorization case in auth.test.ts that calls authenticate_api_request with the same valid key using the “ApiKey” authorization scheme, and assert next is called and req.tenant is defined.
4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
afterAllto resetenv.api_key.
env.api_keyis set inbeforeAllbut never restored. This could leak state into other test files that depend onenv.api_keybeing unset or holding a different value.♻️ Proposed fix
+import { describe, it, expect, beforeAll, afterAll } from "bun:test"; import { env } from "../src/core/config"; beforeAll(() => { env.api_key = "super-secret-test-api-key-123456789"; }); + +afterAll(() => { + env.api_key = undefined; +});🤖 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/tests/auth.test.ts` around lines 4 - 6, Reset env.api_key after the auth test suite completes by adding an afterAll hook alongside beforeAll. Restore it to its prior or expected unset value, while preserving the existing "super-secret-test-api-key-123456789" setup in beforeAll.packages/openmemory-js/src/core/db.ts (1)
260-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the cleanup error instead of silently swallowing it
The cleanup
catchblock has an empty body with only a comment. If thedrop tablecleanup fails, there's no diagnostic trail. Aconsole.warnwould help operators correlate migration failures with cleanup issues.♻️ Proposed fix
} catch (cleanupError) { - // Cleanup failed, but log original error + console.warn( + "[DB] Waypoints migration cleanup failed:", + cleanupError, + ); }🤖 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/core/db.ts` around lines 260 - 262, Update the cleanup catch block in the migration flow to log cleanupError with console.warn, including context that dropping the table failed while preserving the original error handling.
🤖 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/core/db.ts`:
- Around line 243-265: Update the DbInitError construction in the waypoints
migration catch block to pass migrationError through its optional cause field,
while preserving the existing failure message and rethrow behavior.
- Around line 429-441: Update the get_segment_count type definition and its
implementation to accept an is_system flag, matching get_max_segment,
get_segments, and get_mem_by_segment. When is_system is true, omit the user_id
and project_id SQL filters; otherwise preserve the existing conditional
filtering behavior.
---
Nitpick comments:
In @.jules/sentinel.md:
- Line 4: Update the Prevention text in sentinel.md to replace “completely
eliminating any timing leaks” with “eliminating any timing leaks,” leaving the
hashing and timingSafeEqual guidance unchanged.
In `@packages/openmemory-js/src/core/db.ts`:
- Around line 260-262: Update the cleanup catch block in the migration flow to
log cleanupError with console.warn, including context that dropping the table
failed while preserving the original error handling.
In `@packages/openmemory-js/tests/auth.test.ts`:
- Around line 81-97: Add a test alongside the existing Bearer authorization case
in auth.test.ts that calls authenticate_api_request with the same valid key
using the “ApiKey” authorization scheme, and assert next is called and
req.tenant is defined.
- Around line 4-6: Reset env.api_key after the auth test suite completes by
adding an afterAll hook alongside beforeAll. Restore it to its prior or expected
unset value, while preserving the existing "super-secret-test-api-key-123456789"
setup in beforeAll.
🪄 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: 485842c9-f315-49f0-a86f-085a39f97bc3
📒 Files selected for processing (24)
.jules/sentinel.mdpackages/openmemory-js/src/ai/graph.tspackages/openmemory-js/src/ai/mcp.tspackages/openmemory-js/src/core/db.tspackages/openmemory-js/src/core/migrate.tspackages/openmemory-js/src/core/vector/postgres.tspackages/openmemory-js/src/core/vector/valkey.tspackages/openmemory-js/src/index.tspackages/openmemory-js/src/memory/decay.tspackages/openmemory-js/src/memory/decay_utils.tspackages/openmemory-js/src/memory/embed.tspackages/openmemory-js/src/memory/hsg.tspackages/openmemory-js/src/ops/dynamics.tspackages/openmemory-js/src/ops/extract.tspackages/openmemory-js/src/server/middleware/auth.tspackages/openmemory-js/src/server/middleware/validate.tspackages/openmemory-js/src/server/routes/langgraph.tspackages/openmemory-js/src/sources/base.tspackages/openmemory-js/src/sources/index.tspackages/openmemory-js/src/temporal_graph/index.tspackages/openmemory-js/src/temporal_graph/store.tspackages/openmemory-js/src/utils/chunking.tspackages/openmemory-js/src/utils/fetch.tspackages/openmemory-js/tests/auth.test.ts
| await _exec_direct( | ||
| "alter table waypoints_new rename to waypoints", | ||
| ); | ||
|
|
||
| console.log("[DB] Waypoints migration completed successfully"); | ||
| console.log( | ||
| "[DB] Waypoints migration completed successfully", | ||
| ); | ||
| } catch (migrationError: any) { | ||
| console.error("[DB] Waypoints migration failed:", migrationError.message); | ||
| console.error( | ||
| "[DB] Waypoints migration failed:", | ||
| migrationError.message, | ||
| ); | ||
| // Attempt cleanup if migration partially completed | ||
| try { | ||
| await _exec_direct("drop table if exists waypoints_new"); | ||
| await _exec_direct( | ||
| "drop table if exists waypoints_new", | ||
| ); | ||
| } catch (cleanupError) { | ||
| // Cleanup failed, but log original error | ||
| } | ||
| throw new DbInitError(`Waypoints migration failed: ${migrationError.message}`); | ||
| throw new DbInitError( | ||
| `Waypoints migration failed: ${migrationError.message}`, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Pass migrationError as cause to DbInitError
The DbInitError class accepts an optional cause field (preserving the original error's stack trace), but the throw at line 263 omits it. This loses the original stack trace, making debugging harder.
🛡️ Proposed fix
throw new DbInitError(
`Waypoints migration failed: ${migrationError.message}`,
+ migrationError,
);📝 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.
| await _exec_direct( | |
| "alter table waypoints_new rename to waypoints", | |
| ); | |
| console.log("[DB] Waypoints migration completed successfully"); | |
| console.log( | |
| "[DB] Waypoints migration completed successfully", | |
| ); | |
| } catch (migrationError: any) { | |
| console.error("[DB] Waypoints migration failed:", migrationError.message); | |
| console.error( | |
| "[DB] Waypoints migration failed:", | |
| migrationError.message, | |
| ); | |
| // Attempt cleanup if migration partially completed | |
| try { | |
| await _exec_direct("drop table if exists waypoints_new"); | |
| await _exec_direct( | |
| "drop table if exists waypoints_new", | |
| ); | |
| } catch (cleanupError) { | |
| // Cleanup failed, but log original error | |
| } | |
| throw new DbInitError(`Waypoints migration failed: ${migrationError.message}`); | |
| throw new DbInitError( | |
| `Waypoints migration failed: ${migrationError.message}`, | |
| ); | |
| throw new DbInitError( | |
| `Waypoints migration failed: ${migrationError.message}`, | |
| migrationError, | |
| ); |
🤖 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/core/db.ts` around lines 243 - 265, Update the
DbInitError construction in the waypoints migration catch block to pass
migrationError through its optional cause field, while preserving the existing
failure message and rethrow behavior.
| get: (segment, user_id, project_id) => { | ||
| let sql = "select count(*) as c from memories where segment=?"; | ||
| const params: any[] = [segment]; | ||
| if (user_id) { sql += " and user_id=?"; params.push(user_id); } | ||
| if (project_id) { sql += " and project_id=?"; params.push(project_id); } | ||
| if (user_id) { | ||
| sql += " and user_id=?"; | ||
| params.push(user_id); | ||
| } | ||
| if (project_id) { | ||
| sql += " and project_id=?"; | ||
| params.push(project_id); | ||
| } | ||
| return get_async(sql, params); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
get_segment_count lacks is_system parameter — inconsistent with sibling methods
get_max_segment, get_segments, and get_mem_by_segment all accept an is_system flag that skips user_id/project_id filtering when true. get_segment_count does not, meaning it always applies filters when user_id/project_id are truthy. No current caller is affected (the sole caller passes only segment), but this inconsistency could cause incorrect scoping if a system-level count is ever needed.
🛡️ Proposed fix — add is_system to type and implementation
Type definition (line 36-42):
get_segment_count: {
get: (
segment: number,
user_id?: string,
project_id?: string,
+ is_system?: boolean,
) => Promise<any>;
};Implementation (line 429-441):
- get: (segment, user_id, project_id) => {
+ get: (segment, user_id, project_id, is_system) => {
let sql = "select count(*) as c from memories where segment=?";
const params: any[] = [segment];
- if (user_id) {
+ if (!is_system && user_id) {
sql += " and user_id=?";
params.push(user_id);
}
- if (project_id) {
+ if (!is_system && project_id) {
sql += " and project_id=?";
params.push(project_id);
}
return get_async(sql, params);
},📝 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.
| get: (segment, user_id, project_id) => { | |
| let sql = "select count(*) as c from memories where segment=?"; | |
| const params: any[] = [segment]; | |
| if (user_id) { sql += " and user_id=?"; params.push(user_id); } | |
| if (project_id) { sql += " and project_id=?"; params.push(project_id); } | |
| if (user_id) { | |
| sql += " and user_id=?"; | |
| params.push(user_id); | |
| } | |
| if (project_id) { | |
| sql += " and project_id=?"; | |
| params.push(project_id); | |
| } | |
| return get_async(sql, params); | |
| } | |
| }, | |
| get: (segment, user_id, project_id, is_system) => { | |
| let sql = "select count(*) as c from memories where segment=?"; | |
| const params: any[] = [segment]; | |
| if (!is_system && user_id) { | |
| sql += " and user_id=?"; | |
| params.push(user_id); | |
| } | |
| if (!is_system && project_id) { | |
| sql += " and project_id=?"; | |
| params.push(project_id); | |
| } | |
| return get_async(sql, params); | |
| }, |
🤖 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/core/db.ts` around lines 429 - 441, Update the
get_segment_count type definition and its implementation to accept an is_system
flag, matching get_max_segment, get_segments, and get_mem_by_segment. When
is_system is true, omit the user_id and project_id SQL filters; otherwise
preserve the existing conditional filtering behavior.
1 participant
Mitigated a timing-attack key-length leak vulnerability in the request authentication middleware by hashing both the provided and expected API keys with SHA-256 before comparing them with timingSafeEqual. Also implemented dynamic getters in the auth config to facilitate proper runtime configuration mapping, and added comprehensive unit tests to prevent regressions.
PR created automatically by Jules for task 13865929738334658773 started by @lucivskvn
Summary by CodeRabbit
Bug Fixes
Tests