feat(recall): fuse BM25 into hybrid ranking, add command smoke suite,… - #111
Conversation
… honest benchmarks (v2.29.0) Retrieval: - Fuse FTS5 BM25 into hybrid recall ranking (65% semantic / 35% lexical) instead of discarding the lexical score after candidate filtering. - Vectorize chunk-aware scoring into one matrix op (identical results). - Apply temporal decay consistently on the text-search fallback lane; the deterministic exact/lexical lane still preserves BM25 order. Reliability: - Lock the managed key file to the executing Windows identity (whoami) instead of an environment-derived username, fixing a create-then-read failure. Shared helper used by key init and auto HTTP key creation. Testing/tooling: - Add scripts/test-scripts/smoke-commands.py + pytest module covering the full marm-memory command surface; Docker and destructive tiers opt-in. - Restore module-generation isolation in load_isolated_server (conftest). Benchmarks: - bench_hotpath recall-scaling section times only shipped code paths (recall_similar, _fetch_and_score_embedding_rows) via one async dispatch; README performance tables refreshed from a single run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds BM25 and embedding recall fusion with temporal fallback weighting, centralizes Windows managed-key ACL enforcement, introduces CLI smoke coverage, updates benchmark methodology, and releases version 2.29.0. ChangesHybrid Recall Fusion and Scoring
Windows Managed-Key ACL Hardening
Command Smoke Test Suite
Benchmark Integrity Rework
Version 2.29.0 Release Metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef3054836e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assert stop.returncode == 0, stop.stderr | ||
|
|
||
|
|
||
| @pytest.mark.smoke_destructive |
There was a problem hiding this comment.
Marking this as smoke_destructive does not by itself prevent plain python -m pytest tests/ (the documented test command and the publish workflow’s test step) from collecting and running it, so the default suite now executes the uninstall/reinstall smoke path instead of requiring scripts/test-scripts/smoke-commands.py --destructive. In environments without pypa/build it fails at python -m build, and with that dependency present it still performs the opt-in lifecycle during normal tests; gate it behind the smoke wrapper or exclude the marker from default pytest.
AGENTS.md reference: AGENTS.md:L61-L61
Useful? React with 👍 / 👎.
| return result.returncode == 0 | ||
|
|
||
|
|
||
| @pytest.mark.smoke_docker |
There was a problem hiding this comment.
This test is only marked smoke_docker, but normal pytest runs and scripts/run-tests.py do not deselect that marker (the runner filters the existing docker marker), so any machine with a Docker daemon will pull and start a real MARM container without passing --docker. Add the existing docker marker or an explicit skip/marker filter so Docker smoke remains opt-in.
AGENTS.md reference: AGENTS.md:L61-L61
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef3054836e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sys.executable, | ||
| "-m", | ||
| "build", | ||
| "--wheel", |
There was a problem hiding this comment.
When the opt-in destructive smoke suite runs in the documented local test setup, this subprocess fails before it ever reaches the uninstall/reinstall lifecycle because [dev] does not install the build module, so python -m build exits with No module named build. Either add build to the dev/test dependencies or make this test install/skip that prerequisite so smoke-commands.py --destructive is actually runnable.
AGENTS.md reference: AGENTS.md:L60-L61
Useful? React with 👍 / 👎.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
marm-mcp-server/marm_mcp_server/core/memory_recall.py (1)
334-389: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
retrieval_modemislabels results from the semantic-intent temporal fallback as"exact_fts"/"exact_like".
_recall_text_searchis now called both from_recall_exact(true exact lane,apply_temporal=False) and from the semantic-intent fallbacks at lines 191-197 and 315-321 (apply_temporal=True), but every result still hardcodesretrieval_modeto"exact_fts"/"exact_like". Per the docstring on_recall_exact(lines 51-53), this field exists so "callers and tests can confirm which path was taken" — that contract now breaks for the fallback callers, whose results are temporally-blended, not pure BM25/lexical order, yet get labeled identically to the deterministic exact lane.🏷️ Proposed fix: derive the label from `apply_temporal`
- "similarity": _blend_temporal(float(score), row["timestamp"]), - "project": row["project"], - "platform": row["platform"], - "retrieval_mode": "exact_fts", + "similarity": _blend_temporal(float(score), row["timestamp"]), + "project": row["project"], + "platform": row["platform"], + "retrieval_mode": "semantic_fallback_fts" if apply_temporal else "exact_fts", } for row, score in fts_rows"similarity": _blend_temporal(0.8, row[3]), "project": row[6], "platform": row[7], - "retrieval_mode": "exact_like", + "retrieval_mode": "semantic_fallback_like" if apply_temporal else "exact_like", } )Also applies to: 427-435
🤖 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 `@marm-mcp-server/marm_mcp_server/core/memory_recall.py` around lines 334 - 389, Update the result construction in _recall_text_search for both FTS and LIKE fallback paths so retrieval_mode is derived from apply_temporal: retain the exact labels for the non-temporal exact lane and use the semantic-intent temporal fallback labels when apply_temporal is true. Ensure both branches consistently identify which retrieval path produced the results.
🧹 Nitpick comments (1)
scripts/benchmarking/performance/bench_hotpath.py (1)
243-280: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSequential ordering may bias the speedup metric.
full_scanis always timed immediately beforeproduction_hybridon the same query, every iteration. If the full scan warms the SQLite page cache, the following hybrid call systematically benefits — a bias that isn't filtered out by taking the median, since it applies to every sample. This directly affects the "Speedup" number this rewrite is meant to report accurately.Consider alternating/randomizing which path runs first per iteration (or running each path in fully separate warm-up passes) so cache effects don't asymmetrically favor
production_hybrid.Example: alternate order per iteration
- # 1. Full semantic scan (production scorer, no pre-filter). - t0 = time.perf_counter() - await asyncio.to_thread( - _fetch_and_score_embedding_rows, mem.db_path, "bench", n, query_vec, 5 - ) - scan_samples.append((time.perf_counter() - t0) * 1000) - - # 2. Production hybrid recall (precomputed vector excludes encode) - t0 = time.perf_counter() - await mem.recall_similar( - query, session="bench", limit=5, query_vec=query_vec - ) - prod_samples.append((time.perf_counter() - t0) * 1000) + run_scan_first = iter_num % 2 == 0 + + async def _time_scan(): + t0 = time.perf_counter() + await asyncio.to_thread( + _fetch_and_score_embedding_rows, mem.db_path, "bench", n, query_vec, 5 + ) + return (time.perf_counter() - t0) * 1000 + + async def _time_hybrid(): + t0 = time.perf_counter() + await mem.recall_similar(query, session="bench", limit=5, query_vec=query_vec) + return (time.perf_counter() - t0) * 1000 + + if run_scan_first: + scan_samples.append(await _time_scan()) + prod_samples.append(await _time_hybrid()) + else: + prod_samples.append(await _time_hybrid()) + scan_samples.append(await _time_scan())🤖 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 `@scripts/benchmarking/performance/bench_hotpath.py` around lines 243 - 280, Update the benchmark loop around _fetch_and_score_embedding_rows and mem.recall_similar so the timed full_scan and production_hybrid paths alternate or randomize execution order per iteration, rather than always running full_scan first. Preserve each path’s existing timing and sample collection, ensuring neither path is systematically favored by SQLite cache warm-up before calculating the speedup.
🤖 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 `@marm-mcp-server/marm_mcp_server/config/settings.py`:
- Line 401: Check the boolean result from
restrict_windows_file_to_current_user(_MARM_ENV_PATH) in the auto-generated API
key save flow, and when it returns False, raise or log a warning using the
established “401: WARNING:...” pattern. Preserve the existing successful save
behavior.
In `@marm-mcp-server/marm_mcp_server/utils/security.py`:
- Around line 30-32: Update restrict_windows_file_to_current_user to avoid
spawning an unqualified whoami executable on Windows. Obtain the current
identity through the Windows identity API, or invoke whoami only via a validated
absolute SystemRoot\System32 path, while preserving the existing identity value
used by the icacls command.
In `@marm-mcp-server/tests/test_temporal_weighting.py`:
- Around line 199-209: Update the exact-lane test around recall_similar so it no
longer relies on SQLite’s undefined insertion order for equal BM25 scores.
Preserve coverage that exact mode does not re-rank by age, using a deterministic
assertion such as retrieval metadata or timestamp-invariant ordering; if
retaining order-based validation, make the fixture contents produce distinct
BM25 scores.
---
Outside diff comments:
In `@marm-mcp-server/marm_mcp_server/core/memory_recall.py`:
- Around line 334-389: Update the result construction in _recall_text_search for
both FTS and LIKE fallback paths so retrieval_mode is derived from
apply_temporal: retain the exact labels for the non-temporal exact lane and use
the semantic-intent temporal fallback labels when apply_temporal is true. Ensure
both branches consistently identify which retrieval path produced the results.
---
Nitpick comments:
In `@scripts/benchmarking/performance/bench_hotpath.py`:
- Around line 243-280: Update the benchmark loop around
_fetch_and_score_embedding_rows and mem.recall_similar so the timed full_scan
and production_hybrid paths alternate or randomize execution order per
iteration, rather than always running full_scan first. Preserve each path’s
existing timing and sample collection, ensuring neither path is systematically
favored by SQLite cache warm-up before calculating the speedup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59188776-90ed-41bc-aee7-fd32c1248782
📒 Files selected for processing (31)
.gitignoreAGENTS.mdCHANGELOG.mdREADME.mddocs/INSTALL-DOCKER.mddocs/INSTALL-LINUX.mddocs/INSTALL-PLATFORMS.mddocs/INSTALL-WINDOWS.mddocs/TECHNICAL-OVERVIEW.mdmarm-mcp-server/Dockerfilemarm-mcp-server/README.mdmarm-mcp-server/docker-compose.ymlmarm-mcp-server/marm-docs/README.mdmarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/core/memory_recall.pymarm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/marm_mcp_server/services/key_management.pymarm-mcp-server/marm_mcp_server/utils/security.pymarm-mcp-server/pyproject.tomlmarm-mcp-server/server.jsonmarm-mcp-server/tests/conftest.pymarm-mcp-server/tests/test_chunking.pymarm-mcp-server/tests/test_command_smoke.pymarm-mcp-server/tests/test_hybrid_search.pymarm-mcp-server/tests/test_runtime_cli.pymarm-mcp-server/tests/test_temporal_weighting.pyscripts/benchmarking/performance/bench_hotpath.pyscripts/test-scripts/README.mdscripts/test-scripts/smoke-commands.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
{marm-mcp-server/server.json,scripts/find-tools.py,README.md,docs/**/*.md,marm-mcp-server/marm-docs/**/*.md,marm-mcp-server/tests/**/*.py}
📄 CodeRabbit inference engine (AGENTS.md)
When adding or removing an MCP tool, update the tool manifest, canonical tool list, full tool-list documentation and counts, and tests covering both transports; then run
python scripts/find-tools.py.
Files:
docs/INSTALL-DOCKER.mddocs/TECHNICAL-OVERVIEW.mdmarm-mcp-server/marm-docs/README.mddocs/INSTALL-WINDOWS.mddocs/INSTALL-LINUX.mdmarm-mcp-server/server.jsonmarm-mcp-server/tests/test_chunking.pymarm-mcp-server/tests/conftest.pymarm-mcp-server/tests/test_runtime_cli.pydocs/INSTALL-PLATFORMS.mdmarm-mcp-server/tests/test_temporal_weighting.pymarm-mcp-server/tests/test_hybrid_search.pyREADME.mdmarm-mcp-server/tests/test_command_smoke.py
{marm-mcp-server/pyproject.toml,marm-mcp-server/server.json,marm-mcp-server/marm_mcp_server/__init__.py,marm-mcp-server/marm_mcp_server/config/settings.py,marm-mcp-server/marm_mcp_server/server.py,marm-mcp-server/Dockerfile,docker-compose.yml,README.md,marm-mcp-server/README.md,marm-mcp-server/marm-docs/README.md,docs/INSTALL-*.md}
📄 CodeRabbit inference engine (AGENTS.md)
When bumping the version, update every listed version source and audit the result with
python scripts/find-versions.py. Follow SemVer: major for breaking changes, minor for new tools/parameters/features, and patch for fixes or documentation updates.
Files:
docs/INSTALL-DOCKER.mdmarm-mcp-server/Dockerfilemarm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/README.mdmarm-mcp-server/marm-docs/README.mdmarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/pyproject.tomldocs/INSTALL-WINDOWS.mddocs/INSTALL-LINUX.mdmarm-mcp-server/server.jsondocs/INSTALL-PLATFORMS.mdmarm-mcp-server/marm_mcp_server/config/settings.pyREADME.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Only flag documentation issues that are materially wrong, misleading for installation/release behavior, or inconsistent with live MCP behavior. Skip style, phrasing, formatting, and wording preferences.
Files:
docs/INSTALL-DOCKER.mddocs/TECHNICAL-OVERVIEW.mdmarm-mcp-server/README.mdmarm-mcp-server/marm-docs/README.mdscripts/test-scripts/README.mdAGENTS.mddocs/INSTALL-WINDOWS.mddocs/INSTALL-LINUX.mdCHANGELOG.mddocs/INSTALL-PLATFORMS.mdREADME.md
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: Implement MARM server logic in Python undermarm-mcp-server/marm_mcp_server/, keeping endpoint logic inendpoints/and shared helpers incore/.
All memory writes must use the serialized asynchronous write queue; do not add bypass write paths.
A semantic-store failure duringmarm_log_entrymust never fail the corresponding log write.
Keep the memory database and isolated concept-graph database on separate connection pools; never share connections between them.
Graph and concept failures must not break the seven core memory tools; graph services should start lazily and operate degraded on failure.
marm_smart_recallmust keep primary memory ranking authoritative; graph enrichment is read-only, fail-open, bounded, and must be trimmed before primary results when enforcing response limits.
Writes must succeed when the embedding encoder is unavailable; embedding loading is lazy and serialized behind a lock.
Prefer the smallest change that solves the problem; avoid speculative abstractions and unrequested configuration flags.
Keep comments minimal and use them only to explain non-obvious reasons; do not narrate the next line.
Files:
marm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/marm_mcp_server/utils/security.pymarm-mcp-server/marm_mcp_server/services/key_management.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/marm_mcp_server/core/memory_recall.py
marm-mcp-server/marm_mcp_server/server.py
📄 CodeRabbit inference engine (AGENTS.md)
HTTP tools must be included in
MCP_TOOL_OPERATIONS; tools absent from this whitelist do not exist over HTTP.
Files:
marm-mcp-server/marm_mcp_server/server.py
marm-mcp-server/marm_mcp_server/{server.py,server_stdio.py,endpoints/**/*.py,services/stdio_graph_tools.py}
📄 CodeRabbit inference engine (AGENTS.md)
Never fork behavior between HTTP and STDIO transports; new tools must have matching implementations or service paths in both transports.
Files:
marm-mcp-server/marm_mcp_server/server.py
marm-mcp-server/{marm_mcp_server/endpoints/**/*.py,marm_mcp_server/server.py,marm_mcp_server/server_stdio.py}
📄 CodeRabbit inference engine (AGENTS.md)
When adding or removing an MCP tool, implement it in
endpoints/, add the HTTP route and whitelist entry, and add the matching STDIO wrapper or service path.
Files:
marm-mcp-server/marm_mcp_server/server.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
marm-mcp-server/marm_mcp_server/server.pyscripts/test-scripts/smoke-commands.pymarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/tests/test_chunking.pymarm-mcp-server/tests/conftest.pymarm-mcp-server/marm_mcp_server/utils/security.pymarm-mcp-server/tests/test_runtime_cli.pymarm-mcp-server/marm_mcp_server/services/key_management.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/tests/test_temporal_weighting.pymarm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/tests/test_hybrid_search.pyscripts/benchmarking/performance/bench_hotpath.pymarm-mcp-server/tests/test_command_smoke.pymarm-mcp-server/marm_mcp_server/core/memory_recall.py
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Run tests withpytestfrommarm-mcp-server/; prefer deep tests exercising real paths, and mock only when the mock matches real behavior with at least 95% fidelity.
Every new MARM Console API route requires at least one happy-path FastAPI response-contract test with the MCP adapter stubbed.
Do not add existence-check or coded-to-pass tests; usepytest.mark.skiponly for genuinely unavailable dependencies.
Files:
marm-mcp-server/tests/test_chunking.pymarm-mcp-server/tests/conftest.pymarm-mcp-server/tests/test_runtime_cli.pymarm-mcp-server/tests/test_temporal_weighting.pymarm-mcp-server/tests/test_hybrid_search.pymarm-mcp-server/tests/test_command_smoke.py
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path. Skip minor naming, comments, and layout preferences.
Files:
marm-mcp-server/tests/test_chunking.pymarm-mcp-server/tests/conftest.pymarm-mcp-server/tests/test_runtime_cli.pymarm-mcp-server/tests/test_temporal_weighting.pymarm-mcp-server/tests/test_hybrid_search.pymarm-mcp-server/tests/test_command_smoke.py
scripts/benchmarking/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Do not publish performance claims that cannot be supported by the repository's benchmark scripts.
Files:
scripts/benchmarking/performance/bench_hotpath.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Lyellr88/marm-memory
Timestamp: 2026-07-24T07:38:31.213Z
Learning: Keep changes surgical, match existing style, and preserve behavior during refactors.
Learnt from: CR
Repo: Lyellr88/marm-memory
Timestamp: 2026-07-24T07:38:31.213Z
Learning: Do not commit changes without an explicit user request.
🪛 ast-grep (0.44.1)
scripts/test-scripts/smoke-commands.py
[error] 61-61: Command coming from incoming request
Context: subprocess.run(command, cwd=PACKAGE_ROOT, check=False)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 61-61: Use of unsanitized data to create processes
Context: subprocess.run(command, cwd=PACKAGE_ROOT, check=False)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
marm-mcp-server/marm_mcp_server/utils/security.py
[error] 29-31: Command coming from incoming request
Context: subprocess.run(
["whoami"], check=False, capture_output=True, text=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 34-44: Command coming from incoming request
Context: subprocess.run(
[
"icacls",
str(path),
"/inheritance:r",
"/grant:r",
f"{identity}:(F)",
],
check=False,
capture_output=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
marm-mcp-server/tests/test_command_smoke.py
[warning] 236-238: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urllib.request.urlopen(
f"{base_url}/health", timeout=2
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(urlopen-unsanitized-data)
[error] 133-145: Command coming from incoming request
Context: subprocess.run(
[
str(python_executable or sys.executable),
"-c",
PRODUCT_ENTRYPOINT,
*arguments,
],
cwd=cwd or PACKAGE_ROOT,
env=environment,
capture_output=True,
text=True,
timeout=timeout,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 223-229: Command coming from incoming request
Context: subprocess.Popen(
[sys.executable, "-c", PRODUCT_ENTRYPOINT, "http"],
cwd=PACKAGE_ROOT,
env=environment,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 281-283: Command coming from incoming request
Context: subprocess.run(
["docker", "ps"], capture_output=True, text=True, timeout=20
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 329-344: Command coming from incoming request
Context: subprocess.run(
[
sys.executable,
"-m",
"build",
"--wheel",
"--no-isolation",
"--outdir",
str(wheel_dir),
],
cwd=PACKAGE_ROOT,
capture_output=True,
text=True,
env=pip_environment,
timeout=240,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 353-359: Command coming from incoming request
Context: subprocess.run(
[str(python), "-m", "pip", "install", "--no-deps", str(wheel)],
capture_output=True,
text=True,
env=pip_environment,
timeout=180,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 382-388: Command coming from incoming request
Context: subprocess.run(
[str(python), "-m", "pip", "install", "--no-deps", str(wheel)],
capture_output=True,
text=True,
env=pip_environment,
timeout=180,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🔇 Additional comments (38)
CHANGELOG.md (1)
5-27: LGTM!README.md (1)
8-8: LGTM!Also applies to: 176-215
docs/INSTALL-DOCKER.md (1)
5-5: LGTM!docs/INSTALL-LINUX.md (1)
5-5: LGTM!Also applies to: 323-323
docs/INSTALL-PLATFORMS.md (1)
1-1: LGTM!docs/INSTALL-WINDOWS.md (1)
5-5: LGTM!Also applies to: 297-297
docs/TECHNICAL-OVERVIEW.md (1)
3-3: LGTM!marm-mcp-server/Dockerfile (1)
76-76: LGTM!marm-mcp-server/README.md (1)
10-10: LGTM!marm-mcp-server/docker-compose.yml (1)
21-21: LGTM!marm-mcp-server/marm-docs/README.md (1)
1-1: LGTM!marm-mcp-server/marm_mcp_server/__init__.py (1)
17-20: LGTM!marm-mcp-server/marm_mcp_server/server.py (1)
8-8: LGTM!marm-mcp-server/server.json (1)
6-6: LGTM!Also applies to: 20-25
marm-mcp-server/marm_mcp_server/utils/security.py (1)
5-7: LGTM!Also applies to: 23-48
marm-mcp-server/marm_mcp_server/config/settings.py (1)
8-8: LGTM!Also applies to: 125-125
marm-mcp-server/marm_mcp_server/services/key_management.py (1)
5-10: LGTM!Also applies to: 36-47
marm-mcp-server/tests/test_runtime_cli.py (1)
223-245: LGTM!scripts/benchmarking/performance/bench_hotpath.py (2)
8-19: LGTM!Also applies to: 45-45, 206-215, 224-229
323-326: LGTM!Also applies to: 335-360
marm-mcp-server/tests/test_command_smoke.py (5)
290-323: 🎯 Functional Correctness | ⚡ Quick winAssert inside
finallycan mask the real failure.If the
trybody (e.g.run/status/logsassertions) fails, theassert stop.returncode == 0infinally(321-322) can raise and replace the originalAssertionError, hiding the actual root cause of a Docker lifecycle failure. Same pattern recurs in the destructive test below (see consolidated note).As per path instructions, tests should not be "incorrectly asserting behavior"; this pattern can obscure the true failure.🧪 Suggested fix: don't let cleanup assertions mask the original failure
finally: - stop = _run_product(("docker", "stop", "--name", container), environment) - assert stop.returncode == 0, stop.stderr + stop = _run_product(("docker", "stop", "--name", container), environment) + if stop.returncode != 0: + warnings.warn(f"docker stop cleanup failed: {stop.stderr}", stacklevel=2)Source: Path instructions
1-217: LGTM!
219-278: LGTM!
325-397: 🎯 Functional Correctness | ⚡ Quick winSame assert-in-
finallymasking pattern as the Docker test.Lines 383-390: if
result.returncode != 0in thetryblock, thefinallyblock'sassert reinstall.returncode == 0can raise and overwrite the original assertion, obscuring which step actually failed.As per path instructions, tests directory review should flag "incorrectly asserting behavior"; consolidating with the Docker-test instance of the same pattern below.
Source: Path instructions
79-96: 🩺 Stability & Availability
uninstallis safe inSAFE_DISPATCHES.Plain
uninstallreturns from the preview path when--yesis absent, so this smoke test does not remove the package from the test environment.scripts/test-scripts/smoke-commands.py (1)
1-67: LGTM!marm-mcp-server/pyproject.toml (2)
7-7: 📐 Maintainability & Code QualityConfirm version-bump audit covers all listed sources.
Verify
2.29.0was applied consistently across every guideline-listed version source and thatpython scripts/find-versions.pypasses clean.As per coding guidelines: "When bumping the version, update every listed version source and audit the result with
python scripts/find-versions.py."Source: Coding guidelines
112-115: LGTM!marm-mcp-server/tests/conftest.py (1)
60-93: LGTM!scripts/test-scripts/README.md (1)
18-34: LGTM!AGENTS.md (1)
61-61: LGTM!.gitignore (1)
34-34: LGTM!Also applies to: 121-122
marm-mcp-server/marm_mcp_server/core/memory_scoring.py (1)
7-21: LGTM!Also applies to: 85-127, 222-222, 233-266
marm-mcp-server/tests/test_chunking.py (1)
225-256: LGTM!marm-mcp-server/tests/test_hybrid_search.py (1)
385-388: LGTM!Also applies to: 428-428, 563-563, 601-682
marm-mcp-server/marm_mcp_server/core/memory_recall.py (2)
191-197: LGTM (temporal fallback wiring itself is correct — see theretrieval_modecontract issue flagged on_recall_text_searchat lines 334-389, which affects both of these call sites).Also applies to: 315-321
209-236: LGTM!Also applies to: 268-285
marm-mcp-server/tests/test_temporal_weighting.py (1)
212-253: LGTM!
| ) | ||
| except Exception: | ||
| pass | ||
| restrict_windows_file_to_current_user(_MARM_ENV_PATH) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg 'marm-mcp-server/marm_mcp_server/(config/settings|utils/.*/key_management|core/.*/key_management)\.py$|marm-mcp-server/marm_mcp_server/utils/.*/key_management\.py$|marm-mcp-server/marm_mcp_server/core/.*/key_management\.py$|key_management\.py$' || true
echo "== settings outline/lines =="
wc -l marm-mcp-server/marm_mcp_server/config/settings.py
sed -n '330,430p' marm-mcp-server/marm_mcp_server/config/settings.py
echo "== helper definitions/usages =="
rg -n "def restrict_windows_file_to_current_user|restrict_windows_file_to_current_user|_protect_key_file|protect.*key" marm-mcp-server/marm_mcp_server -g '*.py'
echo "== settings relevant imports/funcs names around api key =="
rg -n "MARM_API_KEY|MARM_ENV_PATH|generate|create|api_key|restrict_windows|key_file" marm-mcp-server/marm_mcp_server/config/settings.pyRepository: Lyellr88/marm-memory
Length of output: 5509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== security.py =="
sed -n '1,120p' marm-mcp-server/marm_mcp_server/utils/security.py
echo "== protect key usages =="
sed -n '25,85p' marm-mcp-server/marm_mcp_server/services/key_management.py
echo "== deterministic control-flow probe for settings branch =="
python3 - <<'PY'
from pathlib import Path
from unittest import mock
for restricted_return in (True, False):
print(f"restricted_return={restricted_return}")
with mock.patch("pathlib.Path.home", return_value=Path("/tmp/marmtest")):
with mock.patch("pathlib.Path.mkdir") as mkdir:
with mock.patch("pathlib.Path.write_text") as write:
with mock.patch("pathlib.Path.chmod") as chmod:
with mock.patch("marm_mcp_server.utils.security.restrict_windows_file_to_current_user") as rip:
rip.return_value = restricted_return
exec_globals = {}
exec(compile(open("marm-mcp-server/marm_mcp_server/config/settings.py").read().splitlines()[355:417][::-1][-1:][::-1], "<probe>", "exec"), exec_globals)
print("restrict_windows_file_to_current_user call kwargs:", rip.call_args_list)
print("outer warning:", not rip.return_value and "Could not restrict" in compile("", "<z", "exec").co_code)
PYRepository: Lyellr88/marm-memory
Length of output: 3776
Security Misconfiguration (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: Internal
Check the Windows permission result when auto-saving the API key.
restrict_windows_file_to_current_user() returns False on whoami/icacls failure, but the auto-generated key path ignores that result, leaving the file writable by inherited users. Raise or log a warning there with the same 401: WARNING:... pattern.
🤖 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 `@marm-mcp-server/marm_mcp_server/config/settings.py` at line 401, Check the
boolean result from restrict_windows_file_to_current_user(_MARM_ENV_PATH) in the
auto-generated API key save flow, and when it returns False, raise or log a
warning using the established “401: WARNING:...” pattern. Preserve the existing
successful save behavior.
CodeRabbit + Codex review of the v2.29.0 branch: - Gate smoke_docker/smoke_destructive behind MARM_SMOKE_DOCKER / MARM_SMOKE_DESTRUCTIVE at collection time so plain pytest and the publish test step no longer run the Docker or uninstall/reinstall lifecycle. run-tests.py and smoke-commands.py set the gates. - Add build>=1.2.2 to [dev] so the destructive wheel/reinstall smoke can actually run. - settings.py: warn when restrict_windows_file_to_current_user() fails instead of silently leaving the key file inheritable. - security.py: resolve whoami/icacls via absolute SystemRoot\System32 paths to avoid an untrusted-search-path hijack (CWE-426); test updated to match. - test_temporal_weighting.py: drop the undefined SQLite tie-order assumption on the exact lane; assert timestamp-invariant similarity and retrieval_mode instead. Benchmark: alternate which path runs first each iteration so neither gets a consistent warm-cache advantage; re-ran and refreshed README section 4 (numbers held: 39.4x at N=10,000). Scripts: --tests flag for check-file-length; clean-pytest-artifacts discovers .pytest-review-*/.pytest-smoke-*/marm-pytest-* dirs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
marm-mcp-server/marm_mcp_server/utils/security.py (1)
39-55: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: Internal
● Entry marm-mcp-server/marm_mcp_server/config/settings.py:400 restrict_windows_file_to_current_user │ ▼ ● Sink marm-mcp-server/marm_mcp_server/utils/security.pyReplace the full DACL, not only the current identity’s grant.
/inheritance:ronly removes inherited ACEs and/grant:r identity:(F)only replaces explicit permissions for that identity. Pre-existing explicit ACEs for other users/groups can remain, so returnFalseunless the DACL is rebuilt/verified to contain only the intended allow-list.🤖 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 `@marm-mcp-server/marm_mcp_server/utils/security.py` around lines 39 - 55, Update the ACL-setting flow around the icacls subprocess call to rebuild the full DACL rather than only replacing the current identity’s grant. Ensure pre-existing explicit ACEs for other users or groups are removed, and verify the resulting DACL contains only the intended allow-list before returning success; otherwise return False.
🤖 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 `@marm-mcp-server/marm_mcp_server/config/settings.py`:
- Around line 400-403: The Windows ACL failure warning in the settings
initialization block currently uses stdout and can corrupt the MCP STDIO
JSON-RPC stream. Replace the print call guarding
restrict_windows_file_to_current_user(_MARM_ENV_PATH) with stderr-directed
output, preserving the warning text and existing Windows-only behavior.
In `@marm-mcp-server/marm_mcp_server/utils/security.py`:
- Around line 31-38: Update the Windows security-path logic around the
SystemRoot, whoami, and icacls symbols to resolve the canonical Windows system
directory through the appropriate Windows API before constructing or executing
either binary. Do not trust os.environ["SystemRoot"] as the directory source;
validate or replace it with the API-resolved protected directory and preserve
the existing failure behavior when resolution is unavailable.
---
Outside diff comments:
In `@marm-mcp-server/marm_mcp_server/utils/security.py`:
- Around line 39-55: Update the ACL-setting flow around the icacls subprocess
call to rebuild the full DACL rather than only replacing the current identity’s
grant. Ensure pre-existing explicit ACEs for other users or groups are removed,
and verify the resulting DACL contains only the intended allow-list before
returning success; otherwise return False.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7051d827-599c-474b-b8d8-d7db29e1db58
📒 Files selected for processing (16)
.gitignoreREADME.mdmarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/core/memory_recall.pymarm-mcp-server/marm_mcp_server/utils/security.pymarm-mcp-server/pyproject.tomlmarm-mcp-server/tests/conftest.pymarm-mcp-server/tests/test_command_smoke.pymarm-mcp-server/tests/test_hybrid_search.pymarm-mcp-server/tests/test_runtime_cli.pymarm-mcp-server/tests/test_temporal_weighting.pyscripts/benchmarking/performance/bench_hotpath.pyscripts/check-file-length.pyscripts/clean-pytest-artifacts.pyscripts/run-tests.pyscripts/test-scripts/smoke-commands.py
🚧 Files skipped from review as they are similar to previous changes (8)
- marm-mcp-server/pyproject.toml
- marm-mcp-server/tests/test_runtime_cli.py
- marm-mcp-server/tests/test_temporal_weighting.py
- marm-mcp-server/tests/test_hybrid_search.py
- README.md
- marm-mcp-server/marm_mcp_server/core/memory_recall.py
- scripts/benchmarking/performance/bench_hotpath.py
- marm-mcp-server/tests/test_command_smoke.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,json}
📄 CodeRabbit inference engine (AGENTS.md)
Keep HTTP and STDIO transports in exact parity across all 14 public MCP tools.
Files:
scripts/check-file-length.pyscripts/run-tests.pyscripts/clean-pytest-artifacts.pymarm-mcp-server/tests/conftest.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/utils/security.pyscripts/test-scripts/smoke-commands.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
scripts/check-file-length.pyscripts/run-tests.pyscripts/clean-pytest-artifacts.pymarm-mcp-server/tests/conftest.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/utils/security.pyscripts/test-scripts/smoke-commands.py
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Tests must hit real FastAPI endpoints and real SQLite; mock only when it materially speeds testing and matches real behavior with at least 95% fidelity.
Every new MARM Console API route requires at least one happy-path FastAPI response-contract test with the MCP adapter stubbed.
Do not add existence-check or coded-to-pass tests; prefer deep tests exercising real paths.
Usepytest.mark.skiponly for genuinely unavailable dependencies, never for effort.
Files:
marm-mcp-server/tests/conftest.py
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path. Skip minor naming, comments, and layout preferences.
Files:
marm-mcp-server/tests/conftest.py
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: All memory writes must use the serialized async write queue; do not bypass it.
A semantic-store failure duringmarm_log_entrymust never fail the log write.
Never share database connections between the memory SQLite database and the isolated concept-graph database.
Graph and concept failures must never break the seven core memory tools.
marm_smart_recallmust keep primary memory ranking authoritative; graph enrichment is bounded, read-only, fail-open, and trimmed before primary results when enforcing limits.
Use one lazy-loaded, lock-serialized fastembed encoder with 512 dimensions, and allow writes to succeed when the encoder is unavailable.
Prefer the smallest change that solves the task; avoid speculative abstractions and unnecessary configuration flags.
Use minimal comments only for non-obvious rationale; do not narrate the next line.
Keep orchestration in its current owner file and extract modules only at real boundaries.
Files:
marm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/utils/security.py
{marm-mcp-server/pyproject.toml,marm-mcp-server/server.json,marm-mcp-server/marm_mcp_server/__init__.py,marm-mcp-server/marm_mcp_server/config/settings.py,marm-mcp-server/marm_mcp_server/server.py,marm-mcp-server/Dockerfile,docker-compose.yml,README.md,marm-mcp-server/README.md,marm-mcp-server/marm-docs/README.md,docs/INSTALL-*.md}
📄 CodeRabbit inference engine (AGENTS.md)
When bumping the version, update every listed version source, label, README heading, and installation-document version header, then audit with
python scripts/find-versions.py.
Files:
marm-mcp-server/marm_mcp_server/config/settings.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Lyellr88/marm-memory
Timestamp: 2026-07-24T09:16:24.977Z
Learning: Never commit changes without an explicit user request.
Learnt from: CR
Repo: Lyellr88/marm-memory
Timestamp: 2026-07-24T09:16:24.977Z
Learning: Semver uses MAJOR for breaking changes, MINOR for new tools, parameters, or features, and PATCH for fixes and documentation updates.
🪛 ast-grep (0.44.1)
marm-mcp-server/marm_mcp_server/utils/security.py
[error] 38-40: Command coming from incoming request
Context: subprocess.run(
[str(whoami)], check=False, capture_output=True, text=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
scripts/test-scripts/smoke-commands.py
[error] 67-69: Command coming from incoming request
Context: subprocess.run(
command, cwd=PACKAGE_ROOT, env=environment, check=False
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 67-69: Use of unsanitized data to create processes
Context: subprocess.run(
command, cwd=PACKAGE_ROOT, env=environment, check=False
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
🔇 Additional comments (9)
marm-mcp-server/tests/conftest.py (1)
37-52: LGTM!Also applies to: 77-87
scripts/run-tests.py (1)
97-102: LGTM!.gitignore (1)
114-115: LGTM!Also applies to: 125-125
marm-mcp-server/marm_mcp_server/config/settings.py (2)
8-8: LGTM!
125-125: 🎯 Functional CorrectnessVersion metadata is in sync.
scripts/check-file-length.py (2)
25-27: LGTM!
67-81: LGTM!scripts/clean-pytest-artifacts.py (1)
48-53: LGTM!scripts/test-scripts/smoke-commands.py (1)
63-69: LGTM!
Second CodeRabbit pass on PR #111: - security.py: replace the subprocess whoami/icacls approach with a native Win32 ctypes DACL replacement (OpenProcessToken -> token SID -> SetEntriesInAclW -> SetNamedSecurityInfoW). Spawns no external binaries, so the SystemRoot/untrusted-search-path exposure (CWE-426) is gone entirely. Verified on Windows: the file ACL collapses to a single owner-only full-control entry with inheritance removed. - settings.py: send the ACL-failure warning to stderr so it cannot corrupt the STDIO JSON-RPC stream. - test_runtime_cli.py: keep the delegation test and add a real Windows-gated test that runs the ctypes path and asserts the on-disk DACL is owner-only (skipped off win32). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@marm-mcp-server/marm_mcp_server/utils/security.py`:
- Around line 140-146: Update _set_windows_owner_only_dacl() to apply the
process-user SID as the file owner before installing the protected DACL,
including OWNER_SECURITY_INFORMATION and using the write handle with
WRITE_OWNER/WRITE_DAC where applicable. Ensure both ownership and DACL updates
succeed, and fail the operation if either security change fails.
In `@marm-mcp-server/tests/test_runtime_cli.py`:
- Around line 248-251: Update the DACL test setup around the before snapshot and
its “(I)” check to skip when the target does not have inherited ACEs, rather
than failing the test. Keep the ACL behavior assertions active when the
inheritance precondition is present.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c2e0a9d-a593-46c4-9d8e-f7fa8925287e
📒 Files selected for processing (3)
marm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/utils/security.pymarm-mcp-server/tests/test_runtime_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- marm-mcp-server/marm_mcp_server/config/settings.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: Route all memory writes through the serialized asynchronous write queue; do not add bypass write paths.
A semantic-store failure duringmarm_log_entrymust never fail the corresponding log write.
Never share SQLite connections between the memory database and the isolated concept-graph database.
Graph and concept failures must never break the seven core memory tools; graph services must start lazily and tolerate degraded operation.
marm_smart_recallmust keep primary memory ranking authoritative; graph enrichment is read-only, fail-open, bounded, and must be trimmed before primary results when enforcing response limits.
Use one lazily loaded, lock-serializedjinaai/jina-embeddings-v2-small-enencoder with 512 dimensions; writes must succeed when the encoder is unavailable.
Prefer the smallest change that solves the task; avoid speculative abstractions and unrequested configuration flags.
Keep comments minimal and explain only non-obvious rationale; do not narrate the next line.
Keep orchestration in its current owner file and extract modules only at real boundaries consistent with the existing endpoint split.
Files:
marm-mcp-server/marm_mcp_server/utils/security.py
**/*.{py,md,json,toml,yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
Follow SemVer: MAJOR for breaking changes, MINOR for new tools or parameters or features, and PATCH for fixes and documentation updates.
Files:
marm-mcp-server/marm_mcp_server/utils/security.pymarm-mcp-server/tests/test_runtime_cli.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
marm-mcp-server/marm_mcp_server/utils/security.pymarm-mcp-server/tests/test_runtime_cli.py
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Test both HTTP and STDIO transports when adding or changing an MCP tool.
Use real FastAPI endpoints and real SQLite in tests; mock only when it meaningfully speeds testing and matches real behavior with at least 95% fidelity.
Every new MARM Console API route must have at least one happy-path FastAPI response-contract test with the MCP adapter stubbed.
Do not write existence-check or coded-to-pass tests; prefer deep tests exercising real paths over broad shallow coverage.
Usepytest.mark.skiponly for genuinely unavailable dependencies, never for effort or convenience.
Files:
marm-mcp-server/tests/test_runtime_cli.py
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path. Skip minor naming, comments, and layout preferences.
Files:
marm-mcp-server/tests/test_runtime_cli.py
🪛 ast-grep (0.44.1)
marm-mcp-server/tests/test_runtime_cli.py
[error] 247-249: Command coming from incoming request
Context: subprocess.run(
["icacls", str(target)], capture_output=True, text=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 254-256: Command coming from incoming request
Context: subprocess.run(
["icacls", str(target)], capture_output=True, text=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 257-257: Command coming from incoming request
Context: subprocess.run(["whoami"], capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🔇 Additional comments (2)
marm-mcp-server/marm_mcp_server/utils/security.py (1)
3-7: LGTM!Also applies to: 25-157, 159-163
marm-mcp-server/tests/test_runtime_cli.py (1)
223-237: LGTM!
| advapi32.SetNamedSecurityInfoW( | ||
| str(path), | ||
| se_file_object, | ||
| dacl_security_information | protected_dacl_security_information, | ||
| None, | ||
| None, | ||
| dacl, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate security.py =="
fd -a 'security\.py$' . | sed 's#^\./##'
echo "== inspect relevant file outline =="
ast-grep outline marm-mcp-server/marm_mcp_server/utils/security.py --view compact || true
echo "== inspect relevant lines =="
wc -l marm-mcp-server/marm_mcp_server/utils/security.py
sed -n '1,230p' marm-mcp-server/marm_mcp_server/utils/security.py
echo "== search for managed key security call sites =="
rg -n "SetNamedSecurityInfoW|managed|key|protected_dacl_security_information|OWNER_SECURITY_INFORMATION|SetFileSecurity|Write" marm-mcp-server/marm_mcp_server -SRepository: Lyellr88/marm-memory
Length of output: 50377
🌐 Web query:
Windows SetNamedSecurityInfo OWNER_SECURITY_INFORMATION required to transfer owner owner NULL previous owner can modify DACL
💡 Result:
To transfer ownership of an object using SetNamedSecurityInfo with the OWNER_SECURITY_INFORMATION flag, a caller must have either WRITE_OWNER access to the object or the SE_TAKE_OWNERSHIP_NAME privilege enabled in their process token [1][2][3]. The process of transferring ownership and subsequently modifying the Discretionary Access Control List (DACL) is generally a two-step procedure [4]. The SE_TAKE_OWNERSHIP_NAME privilege allows you to change the owner of an object [4]. Once you successfully become the owner, you gain the right to modify the DACL (WRITE_DAC access) [4]. You can then perform a second call to SetNamedSecurityInfo using the DACL_SECURITY_INFORMATION flag to grant yourself the necessary permissions (such as GENERIC_WRITE) to access the object [5][4]. If you wish to set the owner to a SID that is not your own, you generally require the SE_RESTORE_NAME privilege [6][7][8]. The SE_RESTORE_NAME privilege is powerful: it grants the holder the ability to set any valid user or group SID as the owner of a file and provides write access control regardless of the existing ACL [6][8]. In summary: 1. To take ownership (set yourself as owner): You need WRITE_OWNER access or the SE_TAKE_OWNERSHIP_NAME privilege [1][2][4]. 2. To set an arbitrary owner (e.g., another user or group): You generally require the SE_RESTORE_NAME privilege [6][8]. 3. To modify the DACL after becoming the owner: Once ownership is transferred, the new owner (you) can use SetNamedSecurityInfo with DACL_SECURITY_INFORMATION to update the access control list [5][4].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setnamedsecurityinfoa
- 2: https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setnamedsecurityinfow
- 3: https://winapi.freetechsecrets.com/win32/WIN32SetNamedSecurityInfo_New__Window.htm
- 4: https://devblogs.microsoft.com/oldnewthing/20241030-00/?p=110440
- 5: https://learn.microsoft.com/en-us/windows/win32/secauthz/taking-object-ownership-in-c--
- 6: https://learn.microsoft.com/en-us/windows/win32/secauthz/privilege-constants
- 7: https://stackoverflow.com/questions/17479152/set-file-owner-to-non-existing-user-sid-in-windows
- 8: https://learn.microsoft.com/en-us/answers/questions/1792022/setnamedsecurityinfo()-returns-error-(1307)
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files under path =="
git ls-files 'marm-mcp-server/marm_mcp_server/**/*.py' | sed -n '1,200p'
echo "== exact security.py line count and slice =="
wc -l marm-mcp-server/marm_mcp_server/utils/security.py
sed -n '1,220p' marm-mcp-server/marm_mcp_server/utils/security.py
echo "== focused SetNamedSecurityInfoW and managed/key contexts =="
rg -n "SetNamedSecurityInfoW|create_managed_|managed|private_key|protected_dacl|OWNER_SECURITY_INFORMATION|WRITE_OWNER|WRITE_DAC|CreateFileW|SetFileSecurity" marm-mcp-server/marm_mcp_server -g '*.py'Repository: Lyellr88/marm-memory
Length of output: 19606
🌐 Web query:
Microsoft SetNamedSecurityInfo WIN32 error 1307 SET_SECURITY_OWNER_OWNER_DEFAULTED NULL owner DACL_OWNER_DEFAULTED previous owner modify DACL
💡 Result:
The Windows error 1307 (0x51B) is defined as ERROR_INVALID_OWNER, meaning "This security ID may not be assigned as the owner of this object" [1]. When encountered while using SetNamedSecurityInfo, this error indicates that the process lacks the necessary privileges or permissions to change the owner to the specified security identifier (SID) [2][3][4]. To successfully modify the owner of an object using SetNamedSecurityInfo, the process must adhere to specific security requirements enforced by the Windows operating system [5]. Key Causes and Solutions: 1. Privilege Requirements: You cannot typically assign an arbitrary SID as an object's owner unless your process has the SE_RESTORE_NAME privilege (often referred to as SeRestorePrivilege) enabled in its access token [2][3][4]. This privilege is designed to allow administrators to restore backed-up files and specifically grants the ability to set any valid user or group SID as the owner of an object [2][4]. 2. Administrative Context: Even if you possess the required privileges, ensure your process is running with elevated privileges (Run as Administrator) [2]. Without elevation, even if the privilege is present in your token, it is likely disabled by User Account Control (UAC) [2]. 3. Ownership Constraints: Under normal, non-privileged circumstances, an object's owner can only be set to an SID that is present in the caller's own token (specifically, an SID that represents a user or group associated with the process) [2][5][6]. If your goal is to modify the Discretionary Access Control List (DACL) while simultaneously modifying the owner, be aware that these are separate operations in the security descriptor [7][8]. If you call SetNamedSecurityInfo with OWNER_SECURITY_INFORMATION, the system will validate the new owner SID against your process privileges [4][5]. If you only intend to modify the DACL, ensure you pass NULL for the owner and group arguments and specify only DACL_SECURITY_INFORMATION in the SecurityInformation parameter [7][8]. Recommended Steps: - Verify that your application enables the SE_RESTORE_NAME privilege before calling SetNamedSecurityInfo [2][4][9]. - Confirm that the SID you are attempting to assign is a valid, well-formed SID [2]. - If you are not explicitly trying to change ownership, ensure you are not inadvertently including OWNER_SECURITY_INFORMATION in your flag bitmask [7][8].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/debug/system-error-codes--1300-1699-
- 2: https://learn.microsoft.com/en-us/answers/questions/1792022/setnamedsecurityinfo()-returns-error-(1307)
- 3: https://learn.microsoft.com/en-us/answers/questions/403989/(win32api)-how-to-change-file-owner
- 4: https://stackoverflow.com/questions/60509465/how-to-change-ownership-of-a-file-to-another-user-without-obtaining-serestorepri
- 5: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-adts/e42f988c-72a0-4f8d-a705-7235eac175d9
- 6: https://groups.google.com/g/comp.os.ms-windows.programmer.win32/c/pyC7ynoPeP8
- 7: https://learn.microsoft.com/en-us/windows/win32/secauthz/taking-object-ownership-in-c--
- 8: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-information
- 9: https://microsoft.public.platformsdk.security.narkive.com/G9VNVBQ3/setnamedsecurityinfo-failing-with-rc-1307
🌐 Web query:
DevBlogs oldnewthing SetNamedSecurityInfo owner DACL owner DACL_OWNER_DEFAULTED SetSecurityDescriptorOwner DACL_OWNER_DEFAULTED
💡 Result:
The flags DACL_OWNER_DEFAULTED and SE_OWNER_DEFAULTED are related to how Windows tracks whether security descriptor components (owner or DACL) were provided explicitly or by a default mechanism. SetNamedSecurityInfo and Security Descriptor Flags: SetNamedSecurityInfo is a common Win32 function used to modify security descriptors on securable objects (like files) [1][2]. While the function itself does not take parameters like DACL_OWNER_DEFAULTED directly, it operates on security information flags (such as OWNER_SECURITY_INFORMATION and DACL_SECURITY_INFORMATION) to determine which parts of the security descriptor to update [1][2]. SE_OWNER_DEFAULTED: This is a flag within the SECURITY_DESCRIPTOR_CONTROL structure [3][4]. It indicates that the owner SID was provided by a default mechanism rather than being explicitly specified [3][5]. This flag is managed by functions like SetSecurityDescriptorOwner, which accepts a boolean parameter (bOwnerDefaulted) to toggle the SE_OWNER_DEFAULTED flag [6][5]. DACL_OWNER_DEFAULTED: There is no standard Windows flag named DACL_OWNER_DEFAULTED. This appears to be a confusion of two distinct flags: 1. SE_OWNER_DEFAULTED: Indicates a default owner SID [3][7]. 2. SE_DACL_DEFAULTED: Indicates that the DACL was provided by a default mechanism [3]. Context in Technical Discussions: Technical discussions, such as those on The Old New Thing, frequently address the practical implications of security descriptors, particularly regarding ownership and access control [8][9][10]. A common theme is that simply "taking ownership" (using SE_TAKE_OWNERSHIP_NAME privilege) does not automatically grant full access [8][10]. Instead, it provides the caller with WRITE_DAC access, which then allows the user to modify the DACL to grant themselves the necessary permissions [8][10]. In summary, when working with security descriptors in C++ or other low-level Windows programming, you use SetSecurityDescriptorOwner to control the SE_OWNER_DEFAULTED flag [6], and SetNamedSecurityInfo to apply those changes to objects [1][2]. The term DACL_OWNER_DEFAULTED is not a valid constant; ensure you are referencing SE_OWNER_DEFAULTED or SE_DACL_DEFAULTED as required for your specific security descriptor manipulation [3].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setnamedsecurityinfow
- 2: https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setnamedsecurityinfoa
- 3: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-control
- 4: https://winapi.freetechsecrets.com/win32/WIN32SECURITYDESCRIPTORCONTROL.htm
- 5: https://networkdls.com/Win32Ref/SetSecurityDescriptorOwner.html
- 6: https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-setsecuritydescriptorowner
- 7: https://github.com/MicrosoftDocs/win32/blob/docs/desktop-src/SecAuthZ/security-descriptor-control.md
- 8: https://devblogs.microsoft.com/oldnewthing/20241030-00/?p=110440
- 9: https://devblogs.microsoft.com/oldnewthing/20050818-09/?p=34533
- 10: https://devblogs.microsoft.com/oldnewthing/20150610-00/?p=45411
🌐 Web query:
Microsoft DACL_OWNER_DEFAULTED previous owner can set owner DACL_SECURITY_INFORMATION ACE
💡 Result:
In Windows security architecture, the terms SE_OWNER_DEFAULTED and SE_DACL_DEFAULTED are control flags within a security descriptor, not an Access Control Entry (ACE) type [1][2]. SE_OWNER_DEFAULTED (0x0001) is a flag in the SECURITY_DESCRIPTOR_CONTROL structure indicating that the object's owner security identifier (SID) was provided by a default mechanism (such as the primary group or the creator's token) rather than being explicitly set by the object's creator [1][2][3]. Similarly, SE_DACL_DEFAULTED (0x0008) indicates that the Discretionary Access Control List (DACL) was assigned by a default mechanism [1][2][3]. These flags are used primarily by resource managers to track how security components were assigned and how they should be inherited [1][2][3]. Regarding the ability of a previous owner to set an owner or modify a DACL: 1. Ownership and Permissions: The owner of an object implicitly possesses WRITE_DAC and READ_CONTROL permissions [4]. This allows the owner to modify the object's DACL (i.e., change access control) [4]. 2. Changing Ownership: A user can only change the owner of an object if they have the WRITE_OWNER permission for that object, or if they possess the SE_RESTORE_NAME privilege [4]. Simply having been the previous owner does not grant a user the right to change the current owner unless they still retain those specific rights or privileges [4]. 3. Taking Ownership: Any user with the SE_TAKE_OWNERSHIP privilege enabled can take ownership of an object [4]. Additionally, members of the Administrators group have the inherent capability to take ownership of any object, regardless of who the current owner is [5]. DACL_SECURITY_INFORMATION is a flag used with functions like SetNamedSecurityInfo or SetSecurityInfo to specify that the caller intends to modify the object's DACL [6][7][8]. It is not an ACE. When using these functions, the system checks if the caller has the necessary permissions (typically WRITE_DAC) to perform the requested operation [6][7]. In summary, a previous owner does not retain special rights to set the owner or modify the DACL solely by virtue of their former status [4]. These actions are strictly governed by the current security descriptor's permissions (WRITE_DAC, WRITE_OWNER) and specific system privileges (e.g., SE_TAKE_OWNERSHIP, SE_RESTORE_NAME) [4][8].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-control
- 2: https://learn.microsoft.com/en-us/windows-hardware/drivers/ifs/security-descriptor-control
- 3: https://winapi.freetechsecrets.com/win32/WIN32SECURITYDESCRIPTORCONTROL.htm
- 4: https://learn.microsoft.com/en-us/windows/win32/secauthz/owner-of-a-new-object
- 5: https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-2000-server/cc961992(v=technet.10)
- 6: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-information
- 7: https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setsecurityinfo
- 8: https://learn.microsoft.com/en-us/windows-hardware/drivers/ifs/security-information
🌐 Web query:
"SetNamedSecurityInfo OWNER_SECURITY_INFORMATION SET_SECURITY_OWNER_OWNER_DEFAULTED"
💡 Result:
The flags referenced in your query, OWNER_SECURITY_INFORMATION and SET_SECURITY_OWNER_OWNER_DEFAULTED, pertain to different levels of the Windows security API and serve distinct purposes [1][2][3][4]. OWNER_SECURITY_INFORMATION is a bit flag used with the SetNamedSecurityInfo (and SetSecurityInfo) functions [1][3][5]. When calling SetNamedSecurityInfo, you must include this flag in the SecurityInfo parameter to indicate that the psidOwner argument contains a valid Security Identifier (SID) that should be applied as the new owner of the object [1][6][5]. SET_SECURITY_OWNER_OWNER_DEFAULTED (often represented as the SE_OWNER_DEFAULTED control flag) is not a flag used by SetNamedSecurityInfo [1][3][5]. Instead, this concept is associated with lower-level security descriptor functions such as SetSecurityDescriptorOwner or RtlSetOwnerSecurityDescriptor [2][4]. In those functions, a boolean parameter (often named bOwnerDefaulted) is used to specify whether the owner information was derived from a default mechanism [2][4]. If set to TRUE, the system sets the SE_OWNER_DEFAULTED flag in the security descriptor's control field [2][4]. In summary, SetNamedSecurityInfo provides a high-level interface that manages these details automatically [7]. If you need to explicitly control the SE_OWNER_DEFAULTED flag, you would typically manipulate the security descriptor directly using functions like SetSecurityDescriptorOwner, rather than through the SetNamedSecurityInfo function [2][4][7].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setnamedsecurityinfoa
- 2: https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-setsecuritydescriptorowner
- 3: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-information
- 4: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-rtlsetownersecuritydescriptor
- 5: https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setnamedsecurityinfow
- 6: https://github.com/MicrosoftDocs/sdk-api/blob/docs/sdk-api-src/content/aclapi/nf-aclapi-setnamedsecurityinfow.md
- 7: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-operations
🌐 Web query:
"SetNamedSecurityInfo DACL_SECURITY_INFORMATION DACL_OWNER_DEFAULTED previous owner DACL modified"
💡 Result:
When using the SetNamedSecurityInfo function, the behavior of security descriptor control flags, such as those related to the owner (e.g., SE_OWNER_DEFAULTED) or DACL (e.g., SE_DACL_DEFAULTED), is handled automatically by the system [1]. Specifically, when you use SetNamedSecurityInfo to modify a component of an object's security descriptor—such as providing a new DACL via the pDacl parameter with the DACL_SECURITY_INFORMATION flag—the system automatically updates the associated control bits in the security descriptor [1]. If you explicitly provide a DACL, the system effectively clears any "defaulted" status for that component because the DACL is no longer being supplied by a default mechanism (such as the creator's token), but rather by the explicit input provided to the function [2][3]. Key points regarding this behavior: 1. Automatic Management: Developers do not manually set these control flags when using SetNamedSecurityInfo; the function manages them as a side effect of applying the new security information [1]. 2. Defaulted Status: Flags like SE_DACL_DEFAULTED or SE_OWNER_DEFAULTED are designed to indicate whether a component was assigned by a default mechanism [2][3]. When an explicit security component is applied, these flags are updated to reflect that the component is no longer "defaulted" [1]. 3. Scope: The SetNamedSecurityInfo function is intended for setting security information by name and abstracts the underlying manipulation of the security descriptor control structure [4][1]. If fine-grained control over these specific bits is required, lower-level functions like SetSecurityDescriptorDacl or SetSecurityDescriptorOwner are typically used instead, though these are not used with the SetNamedSecurityInfo call itself [2][3]. In summary, you do not need to worry about the state of the owner-defaulted or DACL-defaulted flags when calling SetNamedSecurityInfo; the system performs the necessary logic to maintain the integrity of the security descriptor's control information based on the parameters you provide [1].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-operations
- 2: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-control
- 3: https://learn.microsoft.com/en-us/windows-hardware/drivers/ifs/security-descriptor-control
- 4: https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setnamedsecurityinfow
🌐 Web query:
Microsoft SetSecurityDescriptorSelf relative security descriptor DACL_OWNER_DEFAULTED DACL_OWNER_DEFAULTED true owner DACL modification ACE
💡 Result:
The terms you mentioned relate to managing security descriptors in Windows, specifically how system-provided default values are flagged versus explicit user-provided values. SetSecurityDescriptorOwner This Win32 API function sets the owner information in an absolute-format security descriptor [1][2]. The function signature includes a parameter, bOwnerDefaulted, which informs the system whether the provided owner SID was assigned by a default mechanism or explicitly specified [1][3]. SE_OWNER_DEFAULTED Flag When you call SetSecurityDescriptorOwner, passing TRUE for the bOwnerDefaulted parameter causes the system to set the SE_OWNER_DEFAULTED flag within the SECURITY_DESCRIPTOR_CONTROL structure [1][2]. This flag serves as a hint to resource managers or the system that the owner SID was derived from a default source (such as the creator's access token) rather than being explicitly set by an application [4][5]. DACL and ACE Modification It is important to distinguish between owner flags and DACL (Discretionary Access Control List) flags: 1. SE_DACL_DEFAULTED: Similar to the owner flag, this indicates a default DACL was applied (e.g., from an access token or parent object) [4][5]. It is set using the SetSecurityDescriptorDacl function [6][7]. 2. ACE Modification: These "defaulted" flags (SE_OWNER_DEFAULTED, SE_DACL_DEFAULTED) do not inherently prevent or modify individual Access Control Entries (ACEs). Instead, they are primarily used by the system to determine how to handle inheritance or subsequent modifications when an object is created [6][4]. For example, if a DACL is marked as defaulted, the system may treat it differently during inheritance operations compared to a DACL that was explicitly defined by a user [4][8]. Summary of Key Differences: - SE_OWNER_DEFAULTED: Managed via SetSecurityDescriptorOwner; relates to the object's owner [1][6]. - SE_DACL_DEFAULTED: Managed via SetSecurityDescriptorDacl; relates to the object's discretionary access list [6][5]. - Self-Relative Format: Note that SetSecurityDescriptorOwner, along with other low-level setter functions, typically requires an absolute-format security descriptor [1][9]. Self-relative descriptors are generally used for persistent storage or transmission and may require conversion before modification [10]. These flags do not modify the ACEs themselves but provide metadata that the operating system uses for authorization logic and inheritance behavior [4][5].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-setsecuritydescriptorowner
- 2: https://winapi.freetechsecrets.com/win32/WIN32SetSecurityDescriptorOwner.htm
- 3: https://networkdls.com/Win32Ref/SetSecurityDescriptorOwner.html
- 4: https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2003/cc781716(v=ws.10)
- 5: https://winapi.freetechsecrets.com/win32/WIN32SECURITYDESCRIPTORCONTROL.htm
- 6: https://github.com/MicrosoftDocs/win32/blob/docs/desktop-src/SecAuthZ/security-descriptor-control.md
- 7: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-control
- 8: https://learn.microsoft.com/en-us/windows/win32/adsi/iadssecuritydescriptor-property-methods
- 9: https://learn.microsoft.com/en-us/windows/win32/secauthz/low-level-security-descriptor-creation
- 10: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-string-format
🌐 Web query:
"Owning the object" "can always change its" "owner" "Ace" "default DACL"
💡 Result:
The statement that owning an object allows one to change its owner is not universally true in Windows; rather, it is a common misunderstanding of how the WRITE_OWNER permission and ownership rights interact [1][2]. In Windows security, the owner of an object possesses the inherent right to modify the object's discretionary access control list (DACL), allowing them to grant themselves or others the WRITE_DAC (Change Permissions) and WRITE_OWNER (Change Owner) permissions [3][4][5]. However, simply being the owner does not automatically grant the ability to perform every action on the security descriptor [1][2]. Key distinctions include: 1. Ownership vs. WRITE_OWNER: Being the owner of an object grants "implicit" control to modify permissions (DACL) [3][4][1]. However, the ability to change the owner of an object (setting a new owner) specifically requires the WRITE_OWNER permission [6][7][8]. While an owner can grant this right to themselves via the DACL, the privilege is not inherently absolute for all scenarios [3][2]. 2. Taking Ownership: A user can take ownership of an object if they have the TAKE_OWNERSHIP permission or the SeTakeOwnershipPrivilege (typically held by Administrators) [4][1]. Once a user successfully takes ownership, they effectively become the owner and can then modify the DACL to grant themselves further rights, such as WRITE_OWNER, if they do not already possess them [1][9]. 3. Limitations on Changing Owner: A standard user or even an object owner cannot necessarily "give" ownership to another user at will [2][8]. The WRITE_OWNER right generally allows a principal to set themselves as the new owner [2][8]. Changing the owner to a different user typically requires the SeRestorePrivilege, which is a powerful administrative privilege [1][2]. In summary, while the owner of an object has broad control, the act of changing ownership is a distinct operation regulated by the WRITE_OWNER permission and specific system privileges (e.g., SeRestorePrivilege), rather than being a direct, unconditional consequence of current ownership [1][2][6].
Citations:
- 1: https://helgeklein.com/blog/permissions-a-primer-or-dacl-sacl-owner-sid-and-ace-explained/
- 2: https://specterops.io/wp-content/uploads/sites/3/2022/06/an_ace_up_the_sleeve.pdf
- 3: https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2003/cc781716(v=ws.10)
- 4: https://flylib.com/books/en/2.922.1.136/1/
- 5: https://www.tenforums.com/tutorials/3587-owner-files-folders-change-windows-10-a.html
- 6: https://learn.microsoft.com/en-us/windows/win32/secauthz/standard-access-rights
- 7: https://learn.microsoft.com/en-us/windows/win32/secauthz/access-mask
- 8: https://github.com/CravateRouge/bloodyAD/wiki/Access-Control
- 9: https://learn.microsoft.com/en-us/windows/win32/secauthz/taking-object-ownership-in-c--
🌐 Web query:
"the DACL contains DACL_OWNER_DEFAULTED"
💡 Result:
The term you are referring to is SE_DACL_DEFAULTED (0x0008), which is a security descriptor control flag in the Windows operating system [1][2]. This flag indicates that a discretionary access control list (DACL) was provided by a default mechanism, rather than being explicitly specified by the creator of an object [1][3][2]. For example, if an object creator does not specify a DACL when creating a new securable object, the system may apply a default DACL from the creator's access token or from the object class schema [4][3][5]. When this happens, the SE_DACL_DEFAULTED flag is set to inform the system that this DACL was assigned automatically [3][2]. Key characteristics of this flag include: Effect on Inheritance: The presence of this flag can influence how the system handles access control entry (ACE) inheritance [1][2]. Dependency: The system ignores this flag if the SE_DACL_PRESENT flag (0x0004) is not set [1][2]. Not Physically Stored: While used to determine how the final DACL is computed, this flag is not stored physically in the security descriptor control of the actual securable object [1][2]. String Format: This flag is not included in security descriptor string representations (e.g., SDDL strings), unlike SE_DACL_PRESENT, which is represented by the D: token [6]. In summary, SE_DACL_DEFAULTED serves as a metadata indicator used by the system to distinguish between a manually defined DACL and one generated via default system processes [1][7][3].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-control
- 2: https://github.com/MicrosoftDocs/windows-driver-docs/blob/staging/windows-driver-docs-pr/ifs/security-descriptor-control.md
- 3: https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2003/cc781716(v=ws.10)
- 4: https://learn.microsoft.com/en-us/windows/win32/secauthz/dacl-for-a-new-object
- 5: https://learn.microsoft.com/en-us/windows/win32/ad/how-security-descriptors-are-set-on-new-directory-objects
- 6: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-string-format
- 7: https://learn.microsoft.com/en-us/previous-versions/windows/desktop/secrcw32prov/win32-securitydescriptor
🌐 Web query:
"SetEntriesInAcl SetEntriesInAclW DACL_OWNER_DEFAULTED DACL contains DACL_OWNER_DEFAULTED"
💡 Result:
The SetEntriesInAcl and SetEntriesInAclW functions are used to create or modify Access Control Lists (ACLs) by merging new Access Control Entries (ACEs) into an existing ACL [1][2][3]. The term DACL_OWNER_DEFAULTED is not a standard, standalone flag used in the context of DACLs. Instead, security descriptors (which contain DACLs) utilize specific control flags [4][5]. The flag corresponding to the concept of a default DACL is SE_DACL_DEFAULTED [4][5][6]. Similarly, there is an SE_OWNER_DEFAULTED flag, which relates to the owner SID of a security descriptor [4][5][6]. Regarding the interaction between these functions and flags: 1. Function Scope: SetEntriesInAcl and SetEntriesInAclW operate specifically on the structure of the ACL (the list of ACEs) itself [3][7]. They do not modify the security descriptor control flags directly [4][8]. 2. Flag Management: The SE_DACL_DEFAULTED flag is a control bit in the security descriptor, not a property of the DACL structure itself [4][5][6]. It is managed via functions like SetSecurityDescriptorDacl [4][8]. 3. Relationship: The SetEntriesInAcl/SetEntriesInAclW functions do not set or clear the SE_DACL_DEFAULTED flag [4][6]. This flag is typically set by the system (or by specific security descriptor management functions) when a DACL is provided by a default mechanism (e.g., from an access token) rather than being explicitly defined by the creator of the object [6]. In summary, SetEntriesInAcl and SetEntriesInAclW are limited to manipulating the list of ACEs [2][3]. They do not interact with or set the SE_DACL_DEFAULTED or SE_OWNER_DEFAULTED flags [4][6].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setentriesinaclw
- 2: https://learn.microsoft.com/en-us/windows/win32/secauthz/creating-or-modifying-an-acl
- 3: https://github.com/MicrosoftDocs/sdk-api/blob/docs/sdk-api-src/content/aclapi/nf-aclapi-setentriesinacla.md
- 4: https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-control
- 5: https://learn.microsoft.com/en-us/previous-versions/windows/desktop/secrcw32prov/win32-securitydescriptor
- 6: https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2003/cc781716(v=ws.10)
- 7: https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setentriesinacla
- 8: https://winapi.freetechsecrets.com/win32/WIN32SECURITYDESCRIPTORCONTROL.htm
Security Misconfiguration (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: External
● Entry
marm-mcp-server/marm_mcp_server/config/settings.py:400
restrict_windows_file_to_current_user
│
▼
● Sink
marm-mcp-server/marm_mcp_server/utils/security.py
Transfer ownership before installing the protected DACL
_set_windows_owner_only_dacl() installs a DACL-only update with None owner and no OWNER_SECURITY_INFORMATION, so the file’s previous owner can still use the retained implicit owner rights to restore a wider DACL after key initialization. Apply the process-user SID as the owner with OWNER_SECURITY_INFORMATION, ideally through the handle opened for the write path with WRITE_OWNER/WRITE_DAC, and fail if ownership or the DACL cannot both be secured.
🤖 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 `@marm-mcp-server/marm_mcp_server/utils/security.py` around lines 140 - 146,
Update _set_windows_owner_only_dacl() to apply the process-user SID as the file
owner before installing the protected DACL, including OWNER_SECURITY_INFORMATION
and using the write handle with WRITE_OWNER/WRITE_DAC where applicable. Ensure
both ownership and DACL updates succeed, and fail the operation if either
security change fails.
| before = subprocess.run( | ||
| ["icacls", str(target)], capture_output=True, text=True | ||
| ).stdout | ||
| assert "(I)" in before, "fixture file should start with inherited ACEs" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file metadata =="
wc -l marm-mcp-server/tests/test_runtime_cli.py || true
echo "== relevant code section =="
sed -n '210,280p' marm-mcp-server/tests/test_runtime_cli.py
echo "== test imports/top =="
sed -n '1,60p' marm-mcp-server/tests/test_runtime_cli.py
echo "== references to icacls and target fixture setup =="
rg -n "icacls|target|fixture|Inherited|inherited|\\(I\\)" marm-mcp-server/tests/test_runtime_cli.py || trueRepository: Lyellr88/marm-memory
Length of output: 5205
🌐 Web query:
Microsoft icacls command inherited ACEs I parenthesis documentation
💡 Result:
In the output of the Windows icacls command, the parenthesis (I) signifies that an Access Control Entry (ACE) is inherited from a parent container [1][2][3]. When you run icacls on a file or directory, the command displays the Access Control List (ACL) [3]. Each ACE in that list may include inheritance flags within parentheses to describe how the permission is applied and where it originated [1][2][4]. Key details regarding (I) and related inheritance flags include: (I) Inherited: This ACE was inherited from the parent container [1][3]. If an ACE entry does not contain (I), it is an explicit permission set directly on that specific object [4]. Other common inheritance flags (always used with parenthesis) include: (OI) Object Inherit: The ACE is inherited by files created within this container [1][3]. (CI) Container Inherit: The ACE is inherited by subfolders (subcontainers) created within this container [1][3]. (IO) Inherit Only: The ACE is inherited by child objects, but does not apply to the current object itself [1][3]. (NP) Don't Propagate Inherit: The ACE is inherited by children, but the inheritance does not continue (propagate) further down to nested containers [1][3]. These flags appear before the permission mask (e.g., F for Full Access, R for Read) in the icacls output [1][2][3]. For example, an entry like (I)(OI)(CI)(RX) indicates an inherited permission that applies to the current object, its files, and its subfolders with Read and Execute access [3][4].
Citations:
- 1: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/icacls
- 2: https://github.com/MicrosoftDocs/windowsserverdocs/blob/main/WindowsServerDocs/administration/windows-commands/icacls.md
- 3: https://superuser.com/questions/322423/explain-the-output-of-icacls-exe-line-by-line-item-by-item
- 4: https://zaur.it/icacls-command-windows/
Don’t fail the DACL test on ACL inheritance policy.
"(I)" in before makes the test dependent on the temp folder ACL policy; on a hardened environment with inheritance turned off or replaced by a managed ACL, the test can skip the real behavior under test. Skip only when the inheritance precondition is not set up, rather than asserting it hard.
🤖 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 `@marm-mcp-server/tests/test_runtime_cli.py` around lines 248 - 251, Update the
DACL test setup around the before snapshot and its “(I)” check to skip when the
target does not have inherited ACEs, rather than failing the test. Keep the ACL
behavior assertions active when the inheritance precondition is present.
Final CodeRabbit findings from PR #111, applied directly to main to close out the review loop: - security.py: SetNamedSecurityInfoW now also sets the process-user SID as owner (OWNER_SECURITY_INFORMATION) alongside the protected DACL. Without transferring ownership, a prior owner keeps implicit WRITE_DAC and could reopen the DACL after lockdown (CWE-732). Verified on Windows: owner and DACL both resolve to the current user only. - test_runtime_cli.py: the real DACL test now skips when the temp dir has no inherited ACEs instead of hard-asserting the precondition, so hardened environments don't turn a missing precondition into failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
v2.29.0
Retrieval:
Reliability:
Testing/tooling:
Benchmarks:
Summary by CodeRabbit