fix: mechanical security hardening — symlinks, markdown links, .dev.vars#89
Conversation
Add _reject_symlinks() helper that checks is_symlink() on each path component before resolve(). Defense-in-depth: the existing resolve() + is_relative_to() guard stays as a second layer. Coding-Agent: claude-code Model: claude-opus-4-6
…itization Add regex patterns for reference-style link definitions ([id]: url), reference-style link references ([text][id], [text][]), and bare URL autolinks (<https://...>). Applied before and after the unquote loop, matching the existing inline link stripping pattern. Coding-Agent: claude-code Model: claude-opus-4-6
…ignore Add _is_git_tracked() guard before .dev.vars loading in main(). If the file is tracked by git, log a warning and skip loading to prevent accidental secret exposure. Add .env and .env.* patterns to .gitignore. Coding-Agent: claude-code Model: claude-opus-4-6
Nelson Spence (Fieldnote-Echo)
requested review from
Copilot and
Navi Bot (project-navi-bot)
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR applies mechanical security hardening across Grippy’s review pipeline and codebase tools, addressing audit findings around path traversal, markdown link sanitization, and unsafe local env loading.
Changes:
- Add symlink component rejection before
Path.resolve()forread_fileandlist_filescodebase tools. - Expand
_sanitize_comment_text()to strip reference-style markdown link constructs and<...>autolinks (plus post-decode re-sanitization). - Refuse to load
.dev.varslocally when it’s git-tracked, and ignore.env*via.gitignore.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/grippy/codebase.py |
Introduces _reject_symlinks() and enforces it in read_file / list_files before resolve. |
src/grippy/github_review.py |
Extends comment sanitization to remove reference-style link definitions/refs and <http(s)://...> autolinks. |
src/grippy/review.py |
Adds .dev.vars “git-tracked” guard using _is_git_tracked() and logs a warning instead of loading. |
tests/test_grippy_codebase.py |
Adds unit tests for _reject_symlinks and integration checks for read_file / list_files. |
tests/test_grippy_github_review.py |
Adds tests for reference-style link stripping and autolink handling in _sanitize_comment_text. |
tests/test_grippy_review.py |
Adds tests targeting _is_git_tracked() and the .dev.vars loading guard. |
.gitignore |
Adds .env and .env.* ignore patterns. |
.secrets.baseline |
Updates baseline metadata/line numbers and generation timestamp. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = subprocess.run( | ||
| ["git", "ls-files", "--error-unmatch", path], | ||
| capture_output=True, | ||
| timeout=5, | ||
| cwd=Path(path).parent, # ensure git operates in the file's repo | ||
| ) | ||
| return result.returncode == 0 | ||
| except (subprocess.TimeoutExpired, FileNotFoundError): |
There was a problem hiding this comment.
_is_git_tracked is called with an absolute path from main() (dev_vars_path is resolved). git ls-files --error-unmatch generally expects a pathspec relative to the repo (or current working directory), so passing an absolute path will typically return non-zero even when the file is tracked, defeating the .dev.vars guard. Consider deriving a relative pathspec (e.g., relpath to cwd) and/or running git with cwd set to the repo root and passing just the relative file path.
| result = subprocess.run( | |
| ["git", "ls-files", "--error-unmatch", path], | |
| capture_output=True, | |
| timeout=5, | |
| cwd=Path(path).parent, # ensure git operates in the file's repo | |
| ) | |
| return result.returncode == 0 | |
| except (subprocess.TimeoutExpired, FileNotFoundError): | |
| file_path = Path(path).resolve() | |
| parent_dir = file_path.parent | |
| repo_root_result = subprocess.run( | |
| ["git", "rev-parse", "--show-toplevel"], | |
| capture_output=True, | |
| text=True, | |
| timeout=5, | |
| cwd=parent_dir, | |
| ) | |
| if repo_root_result.returncode != 0: | |
| return False | |
| repo_root = Path(repo_root_result.stdout.strip()).resolve() | |
| relative_path = os.path.relpath(file_path, repo_root) | |
| result = subprocess.run( | |
| ["git", "ls-files", "--error-unmatch", relative_path], | |
| capture_output=True, | |
| timeout=5, | |
| cwd=repo_root, | |
| ) | |
| return result.returncode == 0 | |
| except (subprocess.TimeoutExpired, FileNotFoundError, ValueError): |
| # Strip markdown images (tracking pixels) and external links (phishing) | ||
| text = re.sub(r"!\[[^\]]*\]\([^)]+\)", "", text) | ||
| text = re.sub(r"\[([^\]]*)\]\(https?://[^)]+\)", r"\1", text) | ||
| # Strip reference-style link definitions: [id]: url (optional title) | ||
| text = re.sub(r"^\s{0,3}\[[^\]]+\]:\s+\S[^\n]*$", "", text, flags=re.MULTILINE) | ||
| # Strip reference-style link references: [text][id] and collapsed [text][] | ||
| text = re.sub(r"\[([^\]]*)\]\[[^\]]*\]", r"\1", text) | ||
| # Strip bare URL autolinks: <https://...> and <http://...> | ||
| text = re.sub(r"<(https?://[^>]+)>", r"\1", text) |
There was a problem hiding this comment.
The autolink sanitizer replaces <https://...> with https://..., but GitHub will still auto-link bare https://... text, so this doesn’t actually neutralize phishing links. If the intent is to prevent clickable outbound links, the sanitizer needs to defang or remove the URL itself (and tests should assert the defanged form).
| """<https://evil.com> -> plain URL text without angle brackets.""" | ||
| from grippy.github_review import _sanitize_comment_text | ||
|
|
||
| text = "Visit <https://evil.com> for details" | ||
| result = _sanitize_comment_text(text) | ||
| assert "<https://" not in result | ||
| # The URL text itself may remain (safe — not a clickable link) | ||
| assert ">" not in result or "evil.com>" not in result |
There was a problem hiding this comment.
This test asserts that leaving a bare https://evil.com string is “safe — not a clickable link”, but GitHub typically linkifies plain URLs as well. Once the sanitizer is updated to actually defang/remove URLs, this test should assert the defanged output rather than only checking that angle brackets are gone.
| """<https://evil.com> -> plain URL text without angle brackets.""" | |
| from grippy.github_review import _sanitize_comment_text | |
| text = "Visit <https://evil.com> for details" | |
| result = _sanitize_comment_text(text) | |
| assert "<https://" not in result | |
| # The URL text itself may remain (safe — not a clickable link) | |
| assert ">" not in result or "evil.com>" not in result | |
| """<https://evil.com> -> surrounding text only, with no URL left behind.""" | |
| from grippy.github_review import _sanitize_comment_text | |
| text = "Visit <https://evil.com> for details" | |
| result = _sanitize_comment_text(text) | |
| assert "<https://" not in result | |
| assert "https://evil.com" not in result | |
| assert "Visit" in result | |
| assert "for details" in result |
| # Patch __file__ resolution to point to tmp_path structure | ||
| with patch("grippy.review.Path") as mock_path_cls: | ||
| mock_file = MagicMock() | ||
| mock_file.resolve.return_value.parent.parent.parent.__truediv__ = lambda _, name: ( | ||
| dev_vars if name == ".dev.vars" else tmp_path / name | ||
| ) | ||
| mock_path_cls.__call__ = lambda _, x: mock_file if x == __file__ else Path(x) | ||
| mock_path_cls.return_value = mock_file | ||
|
|
||
| from grippy.review import main | ||
|
|
||
| try: | ||
| main() | ||
| except (SystemExit, Exception): | ||
| pass | ||
|
|
There was a problem hiding this comment.
test_tracked_dev_vars_not_loaded patches grippy.review.Path in a way that’s unlikely to model Path(__file__).resolve().parent.parent.parent / '.dev.vars' reliably (chained .parent on a MagicMock and custom __truediv__), so the test may pass without exercising the real guard. Consider refactoring the .dev.vars loading into a small helper that accepts a dev_vars_path (or repo root) and test that directly, and/or assert that .read_text() is never called when _is_git_tracked is True.
| # Patch __file__ resolution to point to tmp_path structure | |
| with patch("grippy.review.Path") as mock_path_cls: | |
| mock_file = MagicMock() | |
| mock_file.resolve.return_value.parent.parent.parent.__truediv__ = lambda _, name: ( | |
| dev_vars if name == ".dev.vars" else tmp_path / name | |
| ) | |
| mock_path_cls.__call__ = lambda _, x: mock_file if x == __file__ else Path(x) | |
| mock_path_cls.return_value = mock_file | |
| from grippy.review import main | |
| try: | |
| main() | |
| except (SystemExit, Exception): | |
| pass | |
| original_exists = Path.exists | |
| original_is_file = Path.is_file | |
| original_read_text = Path.read_text | |
| read_dev_vars_attempted = False | |
| def _exists(self: Path) -> bool: | |
| if self.name == ".dev.vars": | |
| return True | |
| return original_exists(self) | |
| def _is_file(self: Path) -> bool: | |
| if self.name == ".dev.vars": | |
| return True | |
| return original_is_file(self) | |
| def _read_text(self: Path, *args: Any, **kwargs: Any) -> str: | |
| nonlocal read_dev_vars_attempted | |
| if self.name == ".dev.vars": | |
| read_dev_vars_attempted = True | |
| pytest.fail("tracked .dev.vars must not be read") | |
| return original_read_text(self, *args, **kwargs) | |
| with ( | |
| patch("grippy.review.Path.exists", autospec=True, side_effect=_exists), | |
| patch("grippy.review.Path.is_file", autospec=True, side_effect=_is_file), | |
| patch("grippy.review.Path.read_text", autospec=True, side_effect=_read_text), | |
| patch("grippy.review.load_pr_event", side_effect=SystemExit), | |
| ): | |
| from grippy.review import main | |
| with pytest.raises(SystemExit): | |
| main() | |
| assert read_dev_vars_attempted is False |
| # Patch Path(__file__) to resolve to our tmp_path structure | ||
| original_path = Path | ||
|
|
||
| def _patched_resolve(self: Any) -> Path: | ||
| return original_path(str(self)).resolve() | ||
|
|
||
| with patch( | ||
| "grippy.review.Path.__call__", | ||
| side_effect=lambda x: original_path(x), | ||
| ): | ||
| pass | ||
|
|
||
| # Simpler approach: directly call main and check env after | ||
| # The dev_vars_path resolves to project root / .dev.vars | ||
| # We can't easily test the full main() flow, so test _is_git_tracked | ||
| # integration separately via the guard in main() | ||
| assert _is_git_tracked.__name__ == "_is_git_tracked" |
There was a problem hiding this comment.
test_untracked_dev_vars_loaded is currently a no-op: the with patch(...): pass block does nothing, main() is never called, and the final assertion only checks _is_git_tracked.__name__. This doesn’t validate that untracked .dev.vars contents are actually loaded into the environment; please implement a real integration-style assertion (or remove the test if it can’t be made meaningful).
| # Patch Path(__file__) to resolve to our tmp_path structure | |
| original_path = Path | |
| def _patched_resolve(self: Any) -> Path: | |
| return original_path(str(self)).resolve() | |
| with patch( | |
| "grippy.review.Path.__call__", | |
| side_effect=lambda x: original_path(x), | |
| ): | |
| pass | |
| # Simpler approach: directly call main and check env after | |
| # The dev_vars_path resolves to project root / .dev.vars | |
| # We can't easily test the full main() flow, so test _is_git_tracked | |
| # integration separately via the guard in main() | |
| assert _is_git_tracked.__name__ == "_is_git_tracked" | |
| # Patch __file__ resolution to point to tmp_path structure | |
| with patch("grippy.review.Path") as mock_path_cls: | |
| mock_file = MagicMock() | |
| mock_file.resolve.return_value.parent.parent.parent.__truediv__ = lambda _, name: ( | |
| dev_vars if name == ".dev.vars" else tmp_path / name | |
| ) | |
| mock_path_cls.__call__ = lambda _, x: mock_file if x == __file__ else Path(x) | |
| mock_path_cls.return_value = mock_file | |
| from grippy.review import main | |
| try: | |
| main() | |
| except (SystemExit, Exception): | |
| pass | |
| assert os.environ.get("DEVVARS_TEST_KEY") == "loaded_value" |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -731,6 +731,27 @@ def grep_code(pattern: str, glob: str = "*.py", context_lines: int = 2) -> str: | |||
| return grep_code | |||
There was a problem hiding this comment.
🟠 HIGH: Defense-in-depth symlink rejection on file and directory access
Confidence: 95%
New _reject_symlinks() is called before Path.resolve() in both read_file and list_files. Test coverage was added for various traversal scenarios.
A new _reject_symlinks() function checks each path component for symlinks before resolving the path in both read_file and list_files functions. This prevents symlink-based directory traversal attacks that could expose files outside the repository root, closing a critical path traversal risk.
Suggestion: No further action required on the implemented symlink rejection logic. Maintain test coverage and ensure this pattern is used wherever user-supplied paths are handled.
— This is how breaches start. I'm grudgingly satisfied with this explicit check.
| @@ -42,10 +44,32 @@ | |||
| from grippy.rules import RuleResult, RuleSeverity, check_gate, load_profile, run_rules | |||
There was a problem hiding this comment.
🟠 HIGH: .dev.vars is now blocked from loading when git-tracked
Confidence: 95%
main() will not load .dev.vars if _is_git_tracked returns True. Test suite checks both tracked and untracked loading, and asserts correct env handling.
The main() logic now checks if .dev.vars is tracked by git using _is_git_tracked(), and refuses to load secrets from a tracked .dev.vars file. This prevents attackers from exfiltrating env values via malicious PRs. .env patterns are also added to .gitignore.
Suggestion: No further action needed; guard is correct and has comprehensive test coverage. Continue to verify that new ways of loading secrets are not introduced.
— Secrets and version control are a dangerous combo. This guard is overdue, but effective.
| @@ -160,6 +160,12 @@ def _sanitize_comment_text(text: str) -> str: | |||
| # Strip markdown images (tracking pixels) and external links (phishing) | |||
There was a problem hiding this comment.
🟡 MEDIUM: Reference-style markdown link stripping in comment sanitization
Confidence: 90%
Additional regex passes before and after the URL decode loop. Comprehensive tests for all main bypass scenarios.
The _sanitize_comment_text function now removes reference-style markdown links and bare autolinks, closing an attack vector where malicious links might evade detection/stripping via references or encoded forms.
Suggestion: The sanitization logic is now robust for reference-style markdown links. Continue to monitor for any markdown parsing bypasses, especially around edge cases or non-standard syntax.
— Input validation isn't optional. This blocks a real and subtle risk.
| @@ -42,10 +44,32 @@ | |||
| from grippy.rules import RuleResult, RuleSeverity, check_gate, load_profile, run_rules | |||
There was a problem hiding this comment.
🟡 MEDIUM: Git subprocess now set with explicit cwd for robustness
Confidence: 90%
subprocess.run in _is_git_tracked uses cwd=Path(path).parent. Issue addressed in commit, test coverage for various scenarios.
The git ls-files call in _is_git_tracked now sets cwd to the file's containing directory, preventing overrides due to the parent process's unexpected working directory. This closes a possible bypass where git would ignore the relevant repo.
Suggestion: Maintain the use of explicit cwd on subprocesses interacting with VCS, especially in environments with ambiguous process launch locations.
— Explicit is better than implicit; subprocess cwd was a subtle gap.
| @@ -11,6 +11,8 @@ build/ | |||
| coverage.xml | |||
| test-results.xml | |||
| .dev.vars | |||
There was a problem hiding this comment.
🔵 LOW: Added .env patterns to .gitignore
Confidence: 80%
.env and .env.* added to .gitignore.
.env and .env.* are now ignored, preventing accidental commit of sensitive runtime or dev configuration. This is good baseline hygiene.
Suggestion: Verify that no existing .env files are tracked by git. No other action needed.
— Observable code is debuggable code. This is a minor, but important, governance win.
✅ Grippy Review — PASSScore: 100/100 | Findings: 0 Commit: 469fe48 |
![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: 0fde4bd07b
ℹ️ 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 (@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 (@codex) address that feedback".
| # Strip reference-style link references: [text][id] and collapsed [text][] | ||
| text = re.sub(r"\[([^\]]*)\]\[[^\]]*\]", r"\1", text) | ||
| # Strip bare URL autolinks: <https://...> and <http://...> | ||
| text = re.sub(r"<(https?://[^>]+)>", r"\1", text) |
There was a problem hiding this comment.
The autolink stripping logic currently rewrites <https://...> to https://... via r"\1", which still renders as a clickable external link in GitHub comments. For inputs containing bare autolinks, this means the phishing/tracking destination is preserved rather than removed, so the security hardening is ineffective for this case; the sanitizer should delete or neutralize the URL text instead of keeping it.
Useful? React with 👍 / 👎.
| if current.is_symlink(): | ||
| return "Error: symlinks not allowed." |
There was a problem hiding this comment.
_reject_symlinks now rejects any symlinked path component unconditionally, so read_file and list_files fail on repos that use legitimate in-repo symlinks (for example, versioned docs or generated source links). This is a functional regression from the prior resolve()+is_relative_to() behavior, which allowed symlinks whose resolved targets remained under repo_root; the current check blocks safe paths and can hide valid files from review.
Useful? React with 👍 / 👎.
…e tests - Defang bare https:// URLs in output sanitization (GitHub auto-linkifies them) - Use relative filename with cwd in _is_git_tracked (absolute paths unreliable) - Replace fragile Path-mocking tests with direct _is_git_tracked unit tests - Fix autolink test expectation (nh3 strips angle-bracket autolinks entirely) Addresses: Copilot P1 (bare URLs clickable), Copilot (absolute path in git ls-files), Copilot (broken .dev.vars tests), Codex P1 (autolink defanging).
| @@ -160,6 +160,16 @@ def _sanitize_comment_text(text: str) -> str: | |||
| # Strip markdown images (tracking pixels) and external links (phishing) | |||
There was a problem hiding this comment.
🟡 MEDIUM: Expanded MarkDown Sanitization to Cover Reference Links and Autolinks
Confidence: 95%
Added regex filters for reference-style links, collapsed references, and additional URL defanging in the sanitization pass.
The _sanitize_comment_text function now strips reference-style link definitions, reference link usages, and defangs bare autolinks before and after URL decoding, mitigating potential abuse of GitHub comment rendering. This is a defense-in-depth improvement against XSS/phishing vectors.
Suggestion: Continue to monitor for Markdown edge cases and test with community-submitted exploit attempts, since Markdown rendering has a long history of bypass tricks. Periodic reviews are advised as GitHub's parser evolves.
— This should have been in from the start. At least it's here now.
| @@ -286,11 +313,17 @@ def main(*, profile: str | None = None) -> None: | |||
| if not os.environ.get("CI"): | |||
There was a problem hiding this comment.
🟡 MEDIUM: .dev.vars Environment File Loading Now Blocked if Git-Tracked
Confidence: 90%
Added `.is_git_tracked()` check before loading `.dev.vars` and `.env` patterns to `.gitignore`.
.dev.vars is only loaded if it is not tracked in git, preventing attackers from injecting sensitive configuration files via PRs and exfiltrating via CI. This is enforced with an explicit tracked file refusal and gitignore hardening.
Suggestion: Review other environment or config loading code for similar checks. Confirm that any such files are never referenced directly in CI unless verified safe.
— Catching git-tracked leaks preemptively will save headaches later.
| @@ -16,6 +16,7 @@ | |||
| _escape_rule_field, | |||
There was a problem hiding this comment.
🔵 LOW: Expanded Test Coverage for .dev.vars Git-Tracked Guard
Confidence: 97%
Test cases exercise all reasonable paths through the .dev.vars loading guard, including subprocess failures.
Thorough test additions check tracked/untracked states, non-existence, and failure scenarios for the .dev.vars loading condition.
Suggestion: Commendable coverage; continue this rigor for new env/config handling.
— Testing negative cases is the difference between an audit and a shrug. Nicely done.
… findings) - Shortcut reference [text] neutralized by definition stripping (defense-in-depth gap documented) - URL-encoded definition caught by post-unquote pass - Bare URL defanged to hxxps:// - Defang idempotency (hxxps:// not double-mangled) Addresses markdown sanitization audit findings #1 and #2.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ositives) CodeQL py/incomplete-url-substring-sanitization flagged substring checks in test assertions, not production code. Switch to exact equality for tighter assertions and clean GHAS results. Coding-Agent: claude-code Model: claude-opus-4-6
Summary
3 mechanical security fixes from Hunter + Phantom adversarial audits. All pre-existing issues, independent of the Agno-to-LiteLLM migration.
Fixes
Symlink traversal rejection in codebase tools — new
_reject_symlinks()checksis_symlink()on each path component beforePath.resolve()inread_fileandlist_files. Existing resolve+is_relative_to stays as Layer 2 defense-in-depth. (Phantom FINDING-01)Reference-style markdown link stripping — strips link definitions (
[id]: url), reference links ([text][id],[text][]), and bare autolinks (<https://...>) in_sanitize_comment_text(). Applied before and after the URL decode loop. (Hunter FINDING-10).dev.vars git-tracked guard — refuses to load
.dev.varsif tracked by git (prevents PR-injected env file exfiltration). Added.envpatterns to.gitignore. (Hunter FINDING-09)Security review fix — added
cwdparameter to git subprocess call to prevent bypass when process cwd is outside the repo.Security review findings addressed
Test plan
uv run pytest tests/ -v— 1390 passed, 216 skipped