🛡️ Sentinel: [HIGH] Fix unauthenticated Python webhooks and restore test execution - #53
Conversation
…ification Implemented secure, constant-time HMAC-SHA256 signature verification for inbound GitHub and Notion webhooks on the Python FastAPI server to match TypeScript SDK security parity. Resolved missing core HSG exports and path/circular issues in Python SDK to allow all tests to pass successfully. 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. |
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR adds fail-closed HMAC validation for GitHub and Notion webhooks, expands sector-aware memory processing and HSG storage, records maintenance events in SQLite, updates vector handling, and scopes JavaScript dashboard and cluster synchronisation routes by tenant. ChangesMemory processing and persistence
Webhook authentication
Tenant-isolated JavaScript routes
Vector runtime compatibility
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant WebhookRequest
participant github_webhook
participant notion_webhook
participant verify_github_signature
participant verify_notion_signature
participant PayloadProcessing
WebhookRequest->>github_webhook: raw body and x-hub-signature-256
github_webhook->>verify_github_signature: verify body, header, and secret
verify_github_signature-->>github_webhook: accept or HTTP 401/503
github_webhook->>PayloadProcessing: process verified payload
WebhookRequest->>notion_webhook: raw body and x-notion-signature
notion_webhook->>verify_notion_signature: verify body, header, and secret
verify_notion_signature-->>notion_webhook: accept or HTTP 401/503
notion_webhook->>PayloadProcessing: process verified payload
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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: 6
🧹 Nitpick comments (4)
packages/openmemory-py/tests/test_webhooks.py (2)
45-48: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise malformed hexadecimal signatures.
The GitHub malformed test stops at the missing-prefix branch, and Notion has no malformed-header test. Add
sha256=not-hexcases for both helpers to cover theirbytes.fromhex(...)rejection paths.Also applies to: 70-73
🤖 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-py/tests/test_webhooks.py` around lines 45 - 48, Extend test_github_webhook_rejects_malformed_header with a sha256=not-hex case, and add an equivalent malformed-header test for the Notion signature helper. Assert both helpers raise HTTPException with status code 401, covering their bytes.fromhex rejection paths.
10-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse an independent GitHub known-answer vector.
make_github_sigrepeats the production calculation, so an identical regression can still pass. Add GitHub’s published secret, payload, and expected signature as literals in a test. (docs.github.com)🤖 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-py/tests/test_webhooks.py` around lines 10 - 21, Replace the GitHub test’s reliance on make_github_sig with an independent known-answer vector using GitHub’s published secret, payload, and expected sha256 signature as literals. Update the relevant GitHub webhook test to validate against those constants while leaving make_notion_sig and the production verification behavior unchanged.packages/openmemory-py/tests/test_multilingual_dedup.py/test_multilingual_dedup.py (1)
67-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the HSG test doubles awaitable and signature-safe.
embedding_query_for_all_sectorsis awaited inhsg.py, but the lambda returns{}and causesTypeError; returnasync def ...stubs or compatible awaitable mocks. Also replace the zeroingcalc_decayfallback with a stub that accepts the full signature and propagatesinit_sal, so retrieval decay tests do not diverge from production.🤖 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-py/tests/test_multilingual_dedup.py/test_multilingual_dedup.py` around lines 67 - 72, The HSG test doubles in the multilingual dedup setup are incompatible with production calls: replace the non-awaitable embed-query lambda with an async stub that accepts arbitrary arguments and returns the empty mapping, and update the calc_decay fallback to accept the full production signature while returning the provided init_sal value. Use the existing mock/configuration symbols around embed_query_for_all_sectors and calc_decay, leaving unrelated doubles unchanged.packages/openmemory-py/src/openmemory/core/db.py (1)
141-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
logger.exception()to keep the traceback on maintenance-log failures.SonarCloud flags this as a quality-gate failure. Swap the format-string
logger.errorcall forlogger.exception()so the traceback isn't lost when the stats insert fails.🔧 Suggested fix
except Exception as e: - logger.error(f"[DB] Maintenance log error: {e}") + logger.exception("[DB] Maintenance log error")🤖 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-py/src/openmemory/core/db.py` around lines 141 - 148, Update the exception handler in log_maint_op to use logger.exception() instead of the format-string logger.error call, preserving the maintenance-log error context while retaining the traceback for stats insert or commit failures.Source: Linters/SAST tools
🤖 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-py/src/openmemory/memory/decay.py`:
- Around line 110-129: Update calc_decay so the computed seg_ratio is
constrained to the [0, 1] range before adjusting lambda_val, preserving
non-negative decay behavior when seg_idx exceeds max_seg.
In `@packages/openmemory-py/src/openmemory/memory/embed.py`:
- Around line 102-146: Reduce cognitive complexity in classify_content by
extracting the metadata sector lookup, pattern-scoring loop, and
additional-sector/confidence calculations into focused helper functions. Keep
classify_content’s existing classification results and fallback behavior
unchanged while delegating each branch to the new helpers and bringing its
complexity within the SonarCloud limit.
- Around line 170-227: Refactor extract_essence into focused helper functions
for sentence extraction, sentence scoring, and selection so its cognitive
complexity meets the allowed threshold while preserving current behavior. Use
the existing sec parameter in scoring to make extraction sector-aware, or remove
it consistently from extract_essence and update its hsg.py call site to pass
only content and the maximum length; do not leave the parameter unused.
In `@packages/openmemory-py/src/openmemory/memory/hsg.py`:
- Around line 248-258: Remove the unused project_id parameter from
add_hsg_memory, or wire it through the full HSG storage path so hsg_store and
q.ins_mem/vector-store writes apply the project scope. Ensure callers providing
project_id receive project-level isolation rather than silently ignoring it.
- Around line 55-91: Update compute_simhash to generate genuine 64-bit per-token
hash values instead of restricting h to 32 bits and reusing bit positions via i
% 32. Ensure the 64-bit vec accumulation checks distinct bit positions across
all 64 iterations, while preserving the existing empty-token fallback and
16-character hexadecimal output.
In
`@packages/openmemory-py/tests/test_multilingual_dedup.py/test_multilingual_dedup.py`:
- Around line 92-117: Make the cleanup around
_load_module("openmemory.memory.hsg", ...) exception-safe: move the stubs list
definition before the load, then perform the sys.modules removals inside a
finally block so cleanup runs whether loading succeeds or raises. Preserve the
existing stub names and conditional removal behavior.
---
Nitpick comments:
In `@packages/openmemory-py/src/openmemory/core/db.py`:
- Around line 141-148: Update the exception handler in log_maint_op to use
logger.exception() instead of the format-string logger.error call, preserving
the maintenance-log error context while retaining the traceback for stats insert
or commit failures.
In
`@packages/openmemory-py/tests/test_multilingual_dedup.py/test_multilingual_dedup.py`:
- Around line 67-72: The HSG test doubles in the multilingual dedup setup are
incompatible with production calls: replace the non-awaitable embed-query lambda
with an async stub that accepts arbitrary arguments and returns the empty
mapping, and update the calc_decay fallback to accept the full production
signature while returning the provided init_sal value. Use the existing
mock/configuration symbols around embed_query_for_all_sectors and calc_decay,
leaving unrelated doubles unchanged.
In `@packages/openmemory-py/tests/test_webhooks.py`:
- Around line 45-48: Extend test_github_webhook_rejects_malformed_header with a
sha256=not-hex case, and add an equivalent malformed-header test for the Notion
signature helper. Assert both helpers raise HTTPException with status code 401,
covering their bytes.fromhex rejection paths.
- Around line 10-21: Replace the GitHub test’s reliance on make_github_sig with
an independent known-answer vector using GitHub’s published secret, payload, and
expected sha256 signature as literals. Update the relevant GitHub webhook test
to validate against those constants while leaving make_notion_sig and the
production verification behavior unchanged.
🪄 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 Plus
Run ID: f5966585-3702-4a84-a7d4-67f4624dcc2a
📒 Files selected for processing (10)
.jules/sentinel.mdpackages/openmemory-py/src/openmemory/core/db.pypackages/openmemory-py/src/openmemory/core/vector_store.pypackages/openmemory-py/src/openmemory/memory/decay.pypackages/openmemory-py/src/openmemory/memory/embed.pypackages/openmemory-py/src/openmemory/memory/hsg.pypackages/openmemory-py/src/openmemory/memory/reflect.pypackages/openmemory-py/src/openmemory/server/routes/sources.pypackages/openmemory-py/tests/test_multilingual_dedup.py/test_multilingual_dedup.pypackages/openmemory-py/tests/test_webhooks.py
| def calc_decay( | ||
| sec: str, | ||
| init_sal: float, | ||
| days_since: float, | ||
| seg_idx: Optional[int] = None, | ||
| max_seg: Optional[int] = None, | ||
| ) -> float: | ||
| from ..core.constants import SECTOR_CONFIGS | ||
| cfg = SECTOR_CONFIGS.get(sec) | ||
| if not cfg: | ||
| return init_sal | ||
| lambda_val = cfg["decay_lambda"] | ||
| if seg_idx is not None and max_seg is not None and max_seg > 0: | ||
| seg_ratio = math.sqrt(seg_idx / max_seg) | ||
| lambda_val = lambda_val * (1 - seg_ratio) | ||
| decayed = init_sal * math.exp(-lambda_val * days_since) | ||
| alpha_reinforce = 0.08 | ||
| reinf = alpha_reinforce * (1 - math.exp(-lambda_val * days_since)) | ||
| return max(0.0, min(1.0, decayed + reinf)) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unclamped seg_ratio can flip decay into growth.
If seg_idx > max_seg is ever passed, seg_ratio = sqrt(seg_idx/max_seg) > 1, making lambda_val negative and turning the exponential-decay term into exponential growth (the final max(0.0, min(1.0, ...)) clamp prevents a crash, but the salience will saturate toward 1.0 instead of decaying). Clamping seg_ratio to [0, 1] would make the function robust to this input.
🛡️ Suggested fix
if seg_idx is not None and max_seg is not None and max_seg > 0:
- seg_ratio = math.sqrt(seg_idx / max_seg)
+ seg_ratio = min(1.0, math.sqrt(seg_idx / max_seg))
lambda_val = lambda_val * (1 - seg_ratio)📝 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.
| def calc_decay( | |
| sec: str, | |
| init_sal: float, | |
| days_since: float, | |
| seg_idx: Optional[int] = None, | |
| max_seg: Optional[int] = None, | |
| ) -> float: | |
| from ..core.constants import SECTOR_CONFIGS | |
| cfg = SECTOR_CONFIGS.get(sec) | |
| if not cfg: | |
| return init_sal | |
| lambda_val = cfg["decay_lambda"] | |
| if seg_idx is not None and max_seg is not None and max_seg > 0: | |
| seg_ratio = math.sqrt(seg_idx / max_seg) | |
| lambda_val = lambda_val * (1 - seg_ratio) | |
| decayed = init_sal * math.exp(-lambda_val * days_since) | |
| alpha_reinforce = 0.08 | |
| reinf = alpha_reinforce * (1 - math.exp(-lambda_val * days_since)) | |
| return max(0.0, min(1.0, decayed + reinf)) | |
| def calc_decay( | |
| sec: str, | |
| init_sal: float, | |
| days_since: float, | |
| seg_idx: Optional[int] = None, | |
| max_seg: Optional[int] = None, | |
| ) -> float: | |
| from ..core.constants import SECTOR_CONFIGS | |
| cfg = SECTOR_CONFIGS.get(sec) | |
| if not cfg: | |
| return init_sal | |
| lambda_val = cfg["decay_lambda"] | |
| if seg_idx is not None and max_seg is not None and max_seg > 0: | |
| seg_ratio = min(1.0, math.sqrt(seg_idx / max_seg)) | |
| lambda_val = lambda_val * (1 - seg_ratio) | |
| decayed = init_sal * math.exp(-lambda_val * days_since) | |
| alpha_reinforce = 0.08 | |
| reinf = alpha_reinforce * (1 - math.exp(-lambda_val * days_since)) | |
| return max(0.0, min(1.0, decayed + reinf)) |
🤖 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-py/src/openmemory/memory/decay.py` around lines 110 -
129, Update calc_decay so the computed seg_ratio is constrained to the [0, 1]
range before adjusting lambda_val, preserving non-negative decay behavior when
seg_idx exceeds max_seg.
| def compute_simhash(text: str) -> str: | ||
| from ..utils.text import canonical_token_set, stable_text_fallback_hash | ||
| tokens = canonical_token_set(text) | ||
| if not tokens: | ||
| return stable_text_fallback_hash(text) | ||
|
|
||
| hashes = [] | ||
| for t in tokens: | ||
| h = 0 | ||
| for c in t: | ||
| h = (h << 5) - h + ord(c) | ||
| h = h & 0xffffffff | ||
| if h >= 0x80000000: | ||
| h -= 0x100000000 | ||
| hashes.append(h) | ||
|
|
||
| vec = [0] * 64 | ||
| for h in hashes: | ||
| for i in range(64): | ||
| bit = 1 << (i % 32) | ||
| if h & bit: | ||
| vec[i] += 1 | ||
| else: | ||
| vec[i] -= 1 | ||
|
|
||
| hash_str = "" | ||
| for i in range(0, 64, 4): | ||
| nibble = ( | ||
| (8 if vec[i] > 0 else 0) + | ||
| (4 if vec[i + 1] > 0 else 0) + | ||
| (2 if vec[i + 2] > 0 else 0) + | ||
| (1 if vec[i + 3] > 0 else 0) | ||
| ) | ||
| hash_str += format(nibble, "x") | ||
|
|
||
| return hash_str | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
compute_simhash's 64-bit fingerprint is actually a 32-bit hash duplicated.
bit = 1 << (i % 32) repeats every 32 iterations of i (0..63), and each token's h is only ever masked to 32 bits. So for every token, h & bit is identical at position i and i+32, meaning vec[i] == vec[i+32] for all i in [0,32) after aggregation. The resulting 16-hex-character SimHash is just the first 8 hex characters repeated twice — halving the intended 64-bit discriminating power and doubling collision probability for the multilingual-dedup use case this hash feeds.
🐛 Suggested fix: use a genuine 64-bit hash per token
hashes = []
for t in tokens:
- h = 0
- for c in t:
- h = (h << 5) - h + ord(c)
- h = h & 0xffffffff
- if h >= 0x80000000:
- h -= 0x100000000
+ h = 0
+ for c in t:
+ h = (h * 0x100000001b3) ^ ord(c)
+ h &= 0xFFFFFFFFFFFFFFFF
hashes.append(h)
vec = [0] * 64
for h in hashes:
for i in range(64):
- bit = 1 << (i % 32)
+ bit = 1 << i
if h & bit:
vec[i] += 1
else:
vec[i] -= 1📝 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.
| def compute_simhash(text: str) -> str: | |
| from ..utils.text import canonical_token_set, stable_text_fallback_hash | |
| tokens = canonical_token_set(text) | |
| if not tokens: | |
| return stable_text_fallback_hash(text) | |
| hashes = [] | |
| for t in tokens: | |
| h = 0 | |
| for c in t: | |
| h = (h << 5) - h + ord(c) | |
| h = h & 0xffffffff | |
| if h >= 0x80000000: | |
| h -= 0x100000000 | |
| hashes.append(h) | |
| vec = [0] * 64 | |
| for h in hashes: | |
| for i in range(64): | |
| bit = 1 << (i % 32) | |
| if h & bit: | |
| vec[i] += 1 | |
| else: | |
| vec[i] -= 1 | |
| hash_str = "" | |
| for i in range(0, 64, 4): | |
| nibble = ( | |
| (8 if vec[i] > 0 else 0) + | |
| (4 if vec[i + 1] > 0 else 0) + | |
| (2 if vec[i + 2] > 0 else 0) + | |
| (1 if vec[i + 3] > 0 else 0) | |
| ) | |
| hash_str += format(nibble, "x") | |
| return hash_str | |
| def compute_simhash(text: str) -> str: | |
| from ..utils.text import canonical_token_set, stable_text_fallback_hash | |
| tokens = canonical_token_set(text) | |
| if not tokens: | |
| return stable_text_fallback_hash(text) | |
| hashes = [] | |
| for t in tokens: | |
| h = 0 | |
| for c in t: | |
| h = (h * 0x100000001b3) ^ ord(c) | |
| h &= 0xFFFFFFFFFFFFFFFF | |
| hashes.append(h) | |
| vec = [0] * 64 | |
| for h in hashes: | |
| for i in range(64): | |
| bit = 1 << i | |
| if h & bit: | |
| vec[i] += 1 | |
| else: | |
| vec[i] -= 1 | |
| hash_str = "" | |
| for i in range(0, 64, 4): | |
| nibble = ( | |
| (8 if vec[i] > 0 else 0) + | |
| (4 if vec[i + 1] > 0 else 0) + | |
| (2 if vec[i + 2] > 0 else 0) + | |
| (1 if vec[i + 3] > 0 else 0) | |
| ) | |
| hash_str += format(nibble, "x") | |
| return hash_str |
🤖 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-py/src/openmemory/memory/hsg.py` around lines 55 - 91,
Update compute_simhash to generate genuine 64-bit per-token hash values instead
of restricting h to 32 bits and reusing bit positions via i % 32. Ensure the
64-bit vec accumulation checks distinct bit positions across all 64 iterations,
while preserving the existing empty-token fallback and 16-character hexadecimal
output.
| hsg = _load_module("openmemory.memory.hsg", ROOT / "memory" / "hsg.py") | ||
|
|
||
| stubs = [ | ||
| "openmemory", | ||
| "openmemory.utils", | ||
| "openmemory.memory", | ||
| "openmemory.core", | ||
| "openmemory.ops", | ||
| "openmemory.core.db", | ||
| "openmemory.core.config", | ||
| "openmemory.core.constants", | ||
| "openmemory.core.vector_store", | ||
| "openmemory.utils.chunking", | ||
| "openmemory.utils.keyword", | ||
| "openmemory.utils.vectors", | ||
| "openmemory.memory.embed", | ||
| "openmemory.memory.decay", | ||
| "openmemory.ops.dynamics", | ||
| "openmemory.memory.user_summary", | ||
| "openmemory.memory.reflect", | ||
| "openmemory.memory.hsg", | ||
| "openmemory.utils.text", | ||
| ] | ||
| for s in stubs: | ||
| if s in sys.modules: | ||
| sys.modules.pop(s) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the module cleanup exception-safe.
The cleanup loop is skipped when _load_module(...) raises, leaving the stubbed openmemory.* modules in sys.modules and contaminating later tests. Define the stub list before loading HSG and remove the entries in a finally block.
Suggested structure
+ stubs = [...]
- hsg = _load_module("openmemory.memory.hsg", ROOT / "memory" / "hsg.py")
-
- stubs = [...]
- for s in stubs:
- if s in sys.modules:
- sys.modules.pop(s)
+ try:
+ hsg = _load_module("openmemory.memory.hsg", ROOT / "memory" / "hsg.py")
+ finally:
+ for s in stubs:
+ sys.modules.pop(s, None)📝 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.
| hsg = _load_module("openmemory.memory.hsg", ROOT / "memory" / "hsg.py") | |
| stubs = [ | |
| "openmemory", | |
| "openmemory.utils", | |
| "openmemory.memory", | |
| "openmemory.core", | |
| "openmemory.ops", | |
| "openmemory.core.db", | |
| "openmemory.core.config", | |
| "openmemory.core.constants", | |
| "openmemory.core.vector_store", | |
| "openmemory.utils.chunking", | |
| "openmemory.utils.keyword", | |
| "openmemory.utils.vectors", | |
| "openmemory.memory.embed", | |
| "openmemory.memory.decay", | |
| "openmemory.ops.dynamics", | |
| "openmemory.memory.user_summary", | |
| "openmemory.memory.reflect", | |
| "openmemory.memory.hsg", | |
| "openmemory.utils.text", | |
| ] | |
| for s in stubs: | |
| if s in sys.modules: | |
| sys.modules.pop(s) | |
| stubs = [ | |
| "openmemory", | |
| "openmemory.utils", | |
| "openmemory.memory", | |
| "openmemory.core", | |
| "openmemory.ops", | |
| "openmemory.core.db", | |
| "openmemory.core.config", | |
| "openmemory.core.constants", | |
| "openmemory.core.vector_store", | |
| "openmemory.utils.chunking", | |
| "openmemory.utils.keyword", | |
| "openmemory.utils.vectors", | |
| "openmemory.memory.embed", | |
| "openmemory.memory.decay", | |
| "openmemory.ops.dynamics", | |
| "openmemory.memory.user_summary", | |
| "openmemory.memory.reflect", | |
| "openmemory.memory.hsg", | |
| "openmemory.utils.text", | |
| ] | |
| try: | |
| hsg = _load_module("openmemory.memory.hsg", ROOT / "memory" / "hsg.py") | |
| finally: | |
| for s in stubs: | |
| sys.modules.pop(s, None) |
🤖 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-py/tests/test_multilingual_dedup.py/test_multilingual_dedup.py`
around lines 92 - 117, Make the cleanup around
_load_module("openmemory.memory.hsg", ...) exception-safe: move the stubs list
definition before the load, then perform the sys.modules removals inside a
finally block so cleanup runs whether loading succeeds or raises. Preserve the
existing stub names and conditional removal behavior.
|
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 6 conflict file(s). Commit:
8 file operation(s)
View agent analysis |
…ification Implemented secure, constant-time HMAC-SHA256 signature verification for inbound GitHub and Notion webhooks on the Python FastAPI server to match TypeScript SDK security parity. Resolved missing core HSG exports and path/circular issues in Python SDK to allow all tests to pass successfully. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
Resolved conflicts in: - .jules/sentinel.md (content) - packages/openmemory-py/src/openmemory/core/db.py (content) - packages/openmemory-py/src/openmemory/memory/decay.py (content) - packages/openmemory-py/src/openmemory/memory/embed.py (content) - packages/openmemory-py/src/openmemory/server/routes/sources.py (content) - packages/openmemory-py/tests/test_multilingual_dedup.py/test_multilingual_dedup.py (content) Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
- Implemented constant-time HMAC-SHA256 signature verification on Python FastAPI server for GitHub and Notion webhooks. - Resolved circular dependencies, missing SDK functions, and test isolation leaks in Python. - Addressed code smells by using logger.exception in db.py and removing unused parameter in hsg.py. 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-py/src/openmemory/core/db.py`:
- Around line 145-148: Update the maintenance logging flow around the stats
INSERT to match the schema defined by 001_initial.sql: insert ts and serialize
maint_type and count into the metrics field, preserving the existing commit and
exception handling behavior.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f25b645-3cb6-4e62-ae01-5cf4f99e433e
📒 Files selected for processing (3)
packages/openmemory-py/src/openmemory/core/db.pypackages/openmemory-py/src/openmemory/memory/embed.pypackages/openmemory-py/src/openmemory/memory/hsg.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/openmemory-py/src/openmemory/memory/embed.py
| db.execute("INSERT INTO stats(type, count, ts) VALUES (?,?,?)", (maint_type, count, ts)) | ||
| db.commit() | ||
| except Exception: | ||
| logger.exception("[DB] Maintenance log error") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the stats insert with the database schema.
001_initial.sql defines stats(id, ts, metrics), but this inserts into nonexistent type and count columns. Every maintenance log therefore fails, and the broad handler silently drops the telemetry. Serialise the maintenance type/count into metrics, or add and migrate the required columns consistently across supported databases.
🤖 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-py/src/openmemory/core/db.py` around lines 145 - 148,
Update the maintenance logging flow around the stats INSERT to match the schema
defined by 001_initial.sql: insert ts and serialize maint_type and count into
the metrics field, preserving the existing commit and exception handling
behavior.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
- Implemented constant-time HMAC-SHA256 signature verification on Python FastAPI server for GitHub and Notion webhooks. - Resolved circular dependencies, missing core SDK functions, and test isolation leaks in Python. - Removed nested template literals in TypeScript's `dashboard.ts`. - Simplified sentence scoring regular expressions in `embed.py` to prevent backtracking and lowered cognitive complexity to < 10. - Extracted and deduplicated duplicate string literals in `sources.py`. - Addressed code smells by using logger.exception in db.py and removing unused parameter in hsg.py. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/openmemory-js/src/server/routes/dashboard.ts (2)
113-126: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winValidate and cap
limbefore binding it asLIMIT.
activityandtop-memoriespassparseIntresults directly;NaN, negative, and arbitrarily large values are not rejected. The first two can produce database errors, while the last permits unnecessarily expensive queries. Reject invalid values with400and clamp to a sensible maximum.🤖 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/routes/dashboard.ts` around lines 113 - 126, Validate lim at the start of fetch_dashboard_memories before passing it to build_tenant_project_where or binding it as LIMIT: reject non-finite, non-integer, and non-positive values with a 400 response, and cap valid values at the route’s sensible maximum. Ensure both activity and top-memories use this validation while preserving the existing tenant/project filtering and ordering behavior.
155-168: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftTenant authentication does not make shared metrics tenant-scoped.
The statistics and maintenance routes require a tenant but query metrics stored without tenant identity, allowing cross-tenant operational data to be exposed.
packages/openmemory-js/src/server/routes/dashboard.ts#L155-L168: add tenant-aware metric storage/filtering for QPS and error data, or mark these fields global/admin-only.packages/openmemory-js/src/server/routes/dashboard.ts#L454-L456: apply the same restriction to maintenance metrics.🤖 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/routes/dashboard.ts` around lines 155 - 168, Make the statistics metrics tenant-scoped in the dashboard route around the query using tenant and project_id: store or filter QPS and error data by tenant, or explicitly restrict those fields to global/admin-only access. Apply the same tenant restriction to the maintenance metrics at packages/openmemory-js/src/server/routes/dashboard.ts:454-456; both sites require changes so authenticated tenants cannot receive other tenants’ operational data.packages/openmemory-py/src/openmemory/server/routes/sources.py (2)
33-33: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRestore the documented 400 response.
ingest_sourcestill raises HTTP 400 for unknown sources at Line 52, so removing the 400 entry makes the OpenAPI contract inaccurate. Restore that response mapping.🤖 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-py/src/openmemory/server/routes/sources.py` at line 33, Update the route decorator for ingest_source to restore the documented 400 response alongside the existing 500 response, matching the HTTPException raised for unknown sources while preserving the current endpoint behavior.
77-81: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReject whitespace-padded or non-canonical signatures before decoding.
bytes.fromhex()ignores whitespace and only raises when a non-hex character is present, so signatures such as"sha256=aa aa ..."or"sha256= ..."can be parsed without error before comparison. Add a strict 64-character hexadecimal check after prefix removal in bothverify_github_signature()andverify_notion_signature(), preserving the HTTP 401 response.🤖 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-py/src/openmemory/server/routes/sources.py` around lines 77 - 81, In both verify_github_signature() and verify_notion_signature(), validate the prefix-stripped signature before calling bytes.fromhex(): require exactly 64 hexadecimal characters with no whitespace or other separators, and raise the existing HTTP 401 invalid_signature response when validation fails. Preserve the subsequent decoding and comparison flow for canonical signatures.packages/openmemory-py/src/openmemory/memory/embed.py (1)
235-251: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSelection loop can return an empty string even when sentences exist.
If the first sentence is not shorter than
max_len(line 239's<check fails) and no other sentence fits within the remaining budget,selectedstays empty and" ".join([...])returns"". The earlierif not sents: return raw[:max_len]guard (line 229) only covers the "no sentences at all" case, not "no sentence fitsmax_len". This silently discards all content for the stored memory essence in edge cases (e.g. a long first sentence, or a very smallmax_len).🐛 Proposed fallback
selected.sort(key=lambda x: x["idx"]) - return " ".join([s["text"] for s in selected]) + if not selected: + return sents[0][:max_len] + return " ".join([s["text"] for s in selected])🤖 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-py/src/openmemory/memory/embed.py` around lines 235 - 251, Update the sentence-selection logic around first_sent and the scored iteration so it always returns content when scored sentences exist but none fit within max_len. Add a fallback that preserves a bounded sentence or raw text rather than allowing selected to remain empty, while keeping the existing behavior when one or more sentences fit.
🤖 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.
Outside diff comments:
In `@packages/openmemory-js/src/server/routes/dashboard.ts`:
- Around line 113-126: Validate lim at the start of fetch_dashboard_memories
before passing it to build_tenant_project_where or binding it as LIMIT: reject
non-finite, non-integer, and non-positive values with a 400 response, and cap
valid values at the route’s sensible maximum. Ensure both activity and
top-memories use this validation while preserving the existing tenant/project
filtering and ordering behavior.
- Around line 155-168: Make the statistics metrics tenant-scoped in the
dashboard route around the query using tenant and project_id: store or filter
QPS and error data by tenant, or explicitly restrict those fields to
global/admin-only access. Apply the same tenant restriction to the maintenance
metrics at packages/openmemory-js/src/server/routes/dashboard.ts:454-456; both
sites require changes so authenticated tenants cannot receive other tenants’
operational data.
In `@packages/openmemory-py/src/openmemory/memory/embed.py`:
- Around line 235-251: Update the sentence-selection logic around first_sent and
the scored iteration so it always returns content when scored sentences exist
but none fit within max_len. Add a fallback that preserves a bounded sentence or
raw text rather than allowing selected to remain empty, while keeping the
existing behavior when one or more sentences fit.
In `@packages/openmemory-py/src/openmemory/server/routes/sources.py`:
- Line 33: Update the route decorator for ingest_source to restore the
documented 400 response alongside the existing 500 response, matching the
HTTPException raised for unknown sources while preserving the current endpoint
behavior.
- Around line 77-81: In both verify_github_signature() and
verify_notion_signature(), validate the prefix-stripped signature before calling
bytes.fromhex(): require exactly 64 hexadecimal characters with no whitespace or
other separators, and raise the existing HTTP 401 invalid_signature response
when validation fails. Preserve the subsequent decoding and comparison flow for
canonical signatures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dd3c13f3-16e3-4352-b0b3-e23f1f7d21d0
📒 Files selected for processing (3)
packages/openmemory-js/src/server/routes/dashboard.tspackages/openmemory-py/src/openmemory/memory/embed.pypackages/openmemory-py/src/openmemory/server/routes/sources.py
…, and eliminate code smells - Implemented constant-time HMAC-SHA256 signature verification on Python FastAPI server for GitHub and Notion webhooks with strict hex validation. - Enforced limit query parameter input validation and admin-only restriction on global/operational stats and maintenance metrics inside TypeScript dashboard.ts. - Resolved circular dependencies, missing core SDK functions, and test isolation leaks in Python. - Simplified sentence scoring regular expressions in `embed.py` to prevent backtracking and lowered cognitive complexity to < 10. - Implemented robust sentence selection bounds fallback in `extract_essence`. - Addressed code smells by using logger.exception in db.py and removing unused parameter in hsg.py. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
…, and eliminate code smells - Implemented constant-time HMAC-SHA256 signature verification on Python FastAPI server for GitHub and Notion webhooks with strict hex validation. - Enforced limit query parameter input validation and admin-only restriction on global/operational stats and maintenance metrics inside TypeScript dashboard.ts. - Resolved circular dependencies, missing core SDK functions, and test isolation leaks in Python. - Simplified sentence scoring regular expressions in `embed.py` to prevent backtracking and lowered cognitive complexity to < 10. - Implemented robust sentence selection bounds fallback in `extract_essence`. - Addressed code smells by using logger.exception in db.py and removing unused parameter in hsg.py. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/openmemory-js/src/server/routes/dashboard.ts (2)
243-273: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
avg_qpsstill leaks global request-rate metrics to non-admin tenants.
qps_data/err_dataare now correctly restricted to admin-like tenants (Lines 245-260), butavg_qps(Lines 266-273) is computed unconditionally from the process-globalreqz.qps_hist, which aggregates traffic across all tenants. Any authenticated non-admin tenant calling/dashboard/statsstill receivesqps.average, an operational metric this PR is otherwise trying to lock behind admin access.🛡️ Proposed fix
const avg_qps = - reqz.qps_hist.length > 0 + is_admin && reqz.qps_hist.length > 0 ? Math.round( (reqz.qps_hist.reduce((a, b) => a + b, 0) / reqz.qps_hist.length) * 100, ) / 100 : 0;🤖 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/routes/dashboard.ts` around lines 243 - 273, Update the avg_qps calculation in the dashboard stats handler to return the metric only for is_admin tenants; non-admin tenants must receive the existing zero/default value instead of using the global reqz.qps_hist. Preserve the current averaging logic for admin tenants.
106-124: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReuse
build_tenant_project_whereinstead of re-implementing the tenant/project clause.
/dashboard/stats(Lines 187-196) and/dashboard/sectors/timeline(Lines 412-422) hand-roll the same tenant/project_idWHERE-clause logic thatbuild_tenant_project_where(Lines 106-124) already encapsulates. This tenant-scoping predicate is security-relevant; three independent copies increase the risk that a future edit updates one copy and misses the others, silently reopening cross-tenant leakage.♻️ Suggested consolidation
function build_tenant_project_where( tenant: string, project_id: string | undefined, is_pg: boolean, - limit: number, + limit?: number, ) { let where_clause = is_pg ? " WHERE user_id = $1" : " WHERE user_id = ?"; const params: any[] = [tenant]; if (project_id) { where_clause += is_pg ? " AND (project_id = $2 OR project_id = 'system_global' OR project_id IS NULL)" : " AND (project_id = ? OR project_id = 'system_global' OR project_id IS NULL)"; params.push(project_id); } - params.push(limit); + if (limit !== undefined) params.push(limit); return { where_clause, params }; }Then call
build_tenant_project_where(tenant, project_id, is_pg, undefined)from/dashboard/statsand/dashboard/sectors/timeline(appending the extracreated_atpredicate for the latter) instead of duplicating the clause.Also applies to: 181-197, 403-422
🤖 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/routes/dashboard.ts` around lines 106 - 124, Replace the duplicated tenant/project WHERE-clause construction in the `/dashboard/stats` and `/dashboard/sectors/timeline` handlers with `build_tenant_project_where(tenant, project_id, is_pg, undefined)`. Reuse its returned `where_clause` and `params`, preserving the stats query behavior and append the existing `created_at` predicate in the sectors timeline query without changing parameter ordering or tenant scoping.packages/openmemory-js/src/server/routes/system.ts (1)
159-192: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGate training writes with tenant ownership.
authenticate_api_requestruns globally, but/api/system/classifier/trainnever extracts or checksrequire_tenantbefore callingclassifier.train(...). This differs from other state-mutating endpoints and lets an authenticated caller retrain the shared sector classifier without proving ownership/scoping. Add an explicit tenant/ownership check before accepting untrusted training data.🤖 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/routes/system.ts` around lines 159 - 192, Update the /api/system/classifier/train handler to explicitly extract and validate the authenticated tenant using the existing require_tenant ownership mechanism before invoking classifier.train. Reject requests that lack valid tenant ownership, and only return the successful “Training started” response after this check passes.
🤖 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/routes/dashboard.ts`:
- Around line 100-104: Replace the tenant-name checks in is_admin_tenant with an
explicit admin role/flag supplied by the auth or configuration layer, and use
that value for dashboard admin endpoints and global metrics. Keep dev-no-auth
access limited to the intentionally local development path, without granting
admin access based on tenant identity names such as "admin" or "system".
---
Outside diff comments:
In `@packages/openmemory-js/src/server/routes/dashboard.ts`:
- Around line 243-273: Update the avg_qps calculation in the dashboard stats
handler to return the metric only for is_admin tenants; non-admin tenants must
receive the existing zero/default value instead of using the global
reqz.qps_hist. Preserve the current averaging logic for admin tenants.
- Around line 106-124: Replace the duplicated tenant/project WHERE-clause
construction in the `/dashboard/stats` and `/dashboard/sectors/timeline`
handlers with `build_tenant_project_where(tenant, project_id, is_pg,
undefined)`. Reuse its returned `where_clause` and `params`, preserving the
stats query behavior and append the existing `created_at` predicate in the
sectors timeline query without changing parameter ordering or tenant scoping.
In `@packages/openmemory-js/src/server/routes/system.ts`:
- Around line 159-192: Update the /api/system/classifier/train handler to
explicitly extract and validate the authenticated tenant using the existing
require_tenant ownership mechanism before invoking classifier.train. Reject
requests that lack valid tenant ownership, and only return the successful
“Training started” response after this check passes.
🪄 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 Plus
Run ID: b95b2a4a-20e6-4743-a5ee-c81d4c5187b7
📒 Files selected for processing (7)
packages/openmemory-js/src/core/vector/postgres.tspackages/openmemory-js/src/server/routes/dashboard.tspackages/openmemory-js/src/server/routes/system.tspackages/openmemory-js/src/utils/fetch.tspackages/openmemory-js/tests/sectors_tenant.test.tspackages/openmemory-py/src/openmemory/memory/embed.pypackages/openmemory-py/src/openmemory/server/routes/sources.py
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/openmemory-py/src/openmemory/server/routes/sources.py
- packages/openmemory-py/src/openmemory/memory/embed.py
| const is_admin_tenant = (tenant: string) => { | ||
| return ( | ||
| tenant === "admin" || tenant === "system" || tenant === "dev-no-auth" | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where req.tenant is assigned and whether "dev-no-auth" is a reachable/production value.
rg -n "dev-no-auth" --type=ts
rg -n "\.tenant\s*=" --type=ts packages/openmemory-js/src
rg -n "NODE_ENV" packages/openmemory-js/src/server/middlewareRepository: lucivskvn/OpenMemory-OSS
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -u
printf 'Files matching dashboard.ts:\n'
fd 'dashboard\.ts$' .
printf '\nRelevant occurrences:\n'
rg -n "is_admin_tenant|dev-no-auth|req\.tenant|\.tenant\s*=|tenant" packages/openmemory-js/src --glob '*.ts' || true
printf '\nMiddleware/auth-related files:\n'
fd -e ts 'middleware|auth|auth.*|router|express|fastify|server' packages/openmemory-js/src/server -t f || trueRepository: lucivskvn/OpenMemory-OSS
Length of output: 33632
🏁 Script executed:
#!/bin/bash
set -u
printf 'packages/openmemory-js/src/server/routes/dashboard.ts lines 1-30 and 93-515:\n'
sed -n '1,30p' packages/openmemory-js/src/server/routes/dashboard.ts
printf '\n--- 93-515 ---\n'
sed -n '93,515p' packages/openmemory-js/src/server/routes/dashboard.ts
printf '\npackages/openmemory-js/src/server/middleware/auth.ts lines 1-215:\n'
sed -n '1,215p' packages/openmemory-js/src/server/middleware/auth.ts
printf '\npackages/openmemory-js/src/server/server.ts lines 1-130:\n'
sed -n '1,130p' packages/openmemory-js/src/server/server.tsRepository: lucivskvn/OpenMemory-OSS
Length of output: 29119
🏁 Script executed:
#!/bin/bash
set -u
printf 'Dashboard and server registration context:\n'
rg -n "authenticate_api_request|authenticate|dash\\(|dashboard|require_auth|app\\.use|WARES" packages/openmemory-js/src/server packages/openmemory-js/src -g '*.ts'
printf '\nSearch for direct tenant/header assignment:\n'
rg -n "req\\[?['\"]?tenant|req\\.tenant|headers\\[.*tenant|user_id.*tenant|tenant_mismatch|admin|system|dev-no-auth" packages/openmemory-js/src --glob '*.ts'Repository: lucivskvn/OpenMemory-OSS
Length of output: 27339
Use an explicit admin permission for dashboard admin endpoints.
is_admin_tenant() currently grants global QPS/error metrics and /dashboard/maintenance based only on the tenanted identity. req.tenant is derived from the API key or the dev-only auth-bypass identity, so an authenticated caller whose tenant name can be mapped/stored as "admin" or "system" can access admin-only data by identity name rather than an explicit admin claim. Move admin checks onto a real role/flag from the auth/config layer and allow the dev/no-auth tenant only where intentionally local.
🤖 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/routes/dashboard.ts` around lines 100 -
104, Replace the tenant-name checks in is_admin_tenant with an explicit admin
role/flag supplied by the auth or configuration layer, and use that value for
dashboard admin endpoints and global metrics. Keep dev-no-auth access limited to
the intentionally local development path, without granting admin access based on
tenant identity names such as "admin" or "system".
…inate snyk ReDoS issues - Implemented constant-time HMAC-SHA256 signature verification on Python FastAPI server for GitHub and Notion webhooks with strict hex validation. - Enforced limit query parameter input validation and admin-only restriction on global/operational stats and maintenance metrics inside TypeScript dashboard.ts. - Resolved circular dependencies, missing core SDK functions, and test isolation leaks in Python. - Simplified sentence scoring regular expressions in `embed.py` to prevent backtracking and lowered cognitive complexity to < 10, protecting against ReDoS. - Implemented robust sentence selection bounds fallback in `extract_essence`. - Hoisted `import re` in `embed.py` to the top-level to satisfy snyk code analysis checks. - Addressed code smells by using logger.exception in db.py and removing unused parameter in hsg.py. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
|
1 participant
This pull request addresses a high-severity security vulnerability where the Python FastAPI server accepted and processed inbound GitHub and Notion webhook payloads without any cryptographic signature verification.
Changes include:
packages/openmemory-py/src/openmemory/server/routes/sources.py.classify_content,calc_decay,add_hsg_memory,embed_query_for_all_sectors,extract_essence,compress_vec_for_storage) inpackages/openmemory-pyto fix imports and circular dependencies.packages/openmemory-py/tests/test_webhooks.pyto assert the signature checks under all edge cases.PR created automatically by Jules for task 3694251325280325600 started by @lucivskvn
Summary by CodeRabbit