fix: normalize env palace_path and validate KG date fields#817
Open
Kesshite wants to merge 4 commits intoMemPalace:developfrom
Open
fix: normalize env palace_path and validate KG date fields#817Kesshite wants to merge 4 commits intoMemPalace:developfrom
Kesshite wants to merge 4 commits intoMemPalace:developfrom
Conversation
Kesshite
requested review from
bensig,
igorls and
milla-jovovich
as code owners
igorls
added
area/kg
Kesshite
force-pushed
the
fix/security-config-validation
branch
from
68c2d86 to
59fc0dd
Compare
Collaborator
|
hey @Kesshite — this conflicts with develop now. pls rebase and we'll get it in. thanks! |
Collaborator
|
@Kesshite pls check conflicts |
config.py: - Apply os.path.abspath(os.path.expanduser()) to MEMPALACE_PALACE_PATH and MEMPAL_PALACE_PATH env vars, preventing path traversal via crafted values like '../../tmp/evil'. Consistent with --palace CLI arg which already uses abspath in mcp_server.py. mcp_server.py: - Add _validate_date() helper accepting YYYY-MM or YYYY-MM-DD format - Apply to tool_kg_query (as_of), tool_kg_add (valid_from), and tool_kg_invalidate (ended) at the MCP boundary - Invalid dates now return a clear error instead of silently breaking temporal filtering (queries returning empty when facts exist) Tests: - Path traversal normalization (../../ evil resolved) - Tilde expansion (~/ resolved to home dir) - Existing env override test updated for abspath behavior - Invalid date rejection (not-a-date, 2026-99-99, yesterday) - Valid date acceptance (YYYY-MM-DD, YYYY-MM) Refs: MemPalace#809
…ce arg - config.py palace_path: normalize all sources (env var, config file, default) with expanduser+abspath, not just env vars — a crafted config.json with "../../evil" was not being resolved - mcp_server.py: add expanduser before abspath on --palace CLI arg, so --palace ~/path expands correctly before being stored in env var - test_config.py: wrap test_env_override in try/finally to prevent env var leak on assertion failure; update test_config_from_file to handle Windows drive letter prefix from abspath Refs: MemPalace#809
Kesshite
force-pushed
the
fix/security-config-validation
branch
from
59fc0dd to
016d424
Compare
Contributor
Author
|
Rebased on latest develop — resolved the 3 conflicts in mcp_server.py where |
Formatting-only changes on 7 files that arrived from develop during rebase and were not formatted upstream. No logic changes.
Reformatted 6 upstream test files with ruff 0.4.x (matching CI pinned version) to pass ruff format --check in CI pipeline.
Kesshite
force-pushed
the
fix/security-config-validation
branch
from
4d44054 to
618d324
Compare
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
3 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.
Summary
MEMPALACE_PALACE_PATHenv var was used withoutexpanduser()orabspath(), allowing path traversal via../../tmp/evil. Now normalized consistently with the--palaceCLI arg.as_of,valid_from,endeddate params in KG MCP tools reached SQLite without format validation. Invalid strings silently broke temporal filtering (queries returned empty instead of matching facts). Now validated as ISO-8601 at the MCP boundary.Refs: #809 (Findings 7, 8)
What changed
mempalace/config.pypalace_pathproperty — appliesos.path.abspath(os.path.expanduser())to env var valuesmempalace/mcp_server.py_validate_date()— new helper acceptingYYYY-MMorYYYY-MM-DD, raisesValueErrorotherwisetool_kg_query()— validatesas_oftool_kg_add()— validatesvalid_fromtool_kg_invalidate()— validatesendedtests/test_config.pytest_env_override— updated to assert normalized absolute pathtests/test_config_extra.pytest_env_palace_path_normalizes_traversal— verifies../resolvedtest_env_palace_path_expands_tilde— verifies~/expandedtests/test_mcp_server.pytest_kg_query_rejects_invalid_datetest_kg_query_accepts_valid_datetest_kg_query_accepts_year_monthtest_kg_add_rejects_invalid_valid_fromtest_kg_invalidate_rejects_invalid_endedTest plan
pytest tests/test_config.py tests/test_config_extra.py -v— 23/23 passedpytest tests/test_mcp_server.py::TestDateValidation -v— 5/5 passedpytest tests/ -v --ignore=tests/benchmarks— 693 passed, 2 failed (pre-existing version mismatch)ruff check— all checks passedruff format --check— all files formatted