fix(security): normalize MEMPALACE_PALACE_PATH env var with abspath+expanduser#1166
Merged
igorls merged 3 commits intoMemPalace:developfrom Apr 24, 2026
Merged
Conversation
…xpanduser MEMPALACE_PALACE_PATH (and legacy MEMPAL_PALACE_PATH) read from the environment was returned as-is from Config.palace_path, while the sibling --palace CLI path gets os.path.abspath() applied at mcp_server.py:62. That inconsistency means env-var callers can end up with literal '~' or unresolved '..' segments in the path, which (a) breaks user intuition and (b) lets a caller who can set env vars on the target user's session redirect palace storage to an unexpected location. Apply os.path.abspath(os.path.expanduser(...)) to the env-var branch so both code paths converge on the same resolved absolute path. Closes MemPalace#1163
arnoldwender
requested review from
bensig,
igorls and
milla-jovovich
as code owners
The new abspath+expanduser normalization means /env/palace no longer round-trips literally on Windows (abspath prepends the current drive, producing D:\env\palace). Rewrite the env-var tests to compare against os.path.abspath(os.path.expanduser(raw)) instead of hardcoded Unix strings, and build raw paths with os.path.join so backslash-vs-slash differences don't leak into assertions. Covers test_env_override, the three new tests, and the legacy-alias test in test_config_extra.
Windows 8.3 short paths legitimately contain tildes (e.g. the CI runner's USERPROFILE resolves to C:\Users\RUNNER~1\...), so asserting "~" is absent from the expanded path fails on Windows even when expanduser worked correctly. The equality check against os.path.abspath(os.path.expanduser()) is authoritative; drop the redundant absence heuristic.
jphein
added a commit
to jphein/mempalace
that referenced
this pull request
Apr 24, 2026
… state Three stale sections updated: - Fork change queue: row 8 (.blob_seq_ids_migrated marker) struck through → FILED as MemPalace#1177. Two new rows added for segfault fixes discovered today (MemPalace#1171 concurrent-write lock, MemPalace#1173 quarantine in make_client) that weren't in the queue because the bugs surfaced today, not during the original 2026-04-21 triage. - Open upstream PRs: was showing 3 of 10 PRs. Now shows all 10 with current CI/review state. All rebased onto current upstream/develop and MERGEABLE as of today. - Merged since v3.3.1: added v3.3.3 release (2026-04-24) with its constituent merges — MemPalace#942, MemPalace#833, MemPalace#1097, MemPalace#1145, MemPalace#1147, MemPalace#1148/1150/1157 entity-detection overhaul (via @igorls's MemPalace#1175 stacked-PR rescue), MemPalace#1166 palace-path security, MemPalace#340/MemPalace#1093 install regression, plus MemPalace#851 from the 2026-04-22 batch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and Why
MEMPALACE_PALACE_PATH(and legacyMEMPAL_PALACE_PATH) read from the environment was returned as-is fromConfig.palace_path, while the sibling--palaceCLI path getsos.path.abspath()applied atmcp_server.py:62. The inconsistency lets env-var callers end up with a literal~or unresolved..segments in the stored path, which breaks user intuition and lets a caller who can set env vars on the target user's session redirect palace storage somewhere the user didn't expect.Root Cause
mempalace/config.py:167-172— the env-var branch of thepalace_pathproperty short-circuits and returnsenv_valwithout running it throughos.path.expanduser()/os.path.abspath().Change Summary
mempalace/config.py— expand~and collapse..on the env-var branch so both code paths converge on the same resolved absolute path. A 2-line inline comment notes the parity with the CLI code path.tests/test_config.py— three new tests covering~expansion,..collapse, and the legacyMEMPAL_PALACE_PATHalias getting the same treatment.Test Plan
pytest tests/test_config.py -v— 25/25 pass (3 new tests green)ruff check— cleanruff format --check— formattedCloses #1163