|
6 | 6 | import pytest |
7 | 7 |
|
8 | 8 | from specify_cli.agents import CommandRegistrar |
| 9 | +from specify_cli._utils import relative_extension_path_violation |
9 | 10 |
|
10 | 11 |
|
11 | 12 | TRAVERSAL_PAYLOADS = [ |
@@ -135,8 +136,185 @@ def test_rejects_traversal_names(self, tmp_path, bad_name): |
135 | 136 | _assert_no_stray_files(tmp_path, Path(bad_name).name.replace("/", "")) |
136 | 137 |
|
137 | 138 |
|
138 | | -class TestSafeRegistration: |
139 | | - """Positive regression — well-formed names continue to register.""" |
| 139 | +ABS_OUTSIDE = "__ABS_OUTSIDE__" |
| 140 | + |
| 141 | +FILE_FIELD_PAYLOADS = [ |
| 142 | + "../outside.txt", |
| 143 | + "../../outside.txt", |
| 144 | + "commands/../../outside.txt", |
| 145 | + "C:outside.txt", |
| 146 | + ABS_OUTSIDE, |
| 147 | +] |
| 148 | + |
| 149 | + |
| 150 | +def _resolve_payload(bad_file: str, outside_file: Path) -> str: |
| 151 | + """Map the absolute-path sentinel to the real, existing outside file. |
| 152 | +
|
| 153 | + Using the temp file's own absolute path (instead of ``/etc/passwd``) |
| 154 | + guarantees the file exists on every platform — so the test fails if the |
| 155 | + absolute-path guard regresses, rather than passing because the target |
| 156 | + happens not to exist (e.g. on Windows runners). |
| 157 | + """ |
| 158 | + return str(outside_file) if bad_file == ABS_OUTSIDE else bad_file |
| 159 | + |
| 160 | + |
| 161 | +def _assert_no_marker_leak(project: Path, marker: str) -> None: |
| 162 | + """Fail if ``marker`` content was written into any file under ``project``.""" |
| 163 | + leaked = [ |
| 164 | + p for p in project.rglob("*") |
| 165 | + if p.is_file() and marker in p.read_text(encoding="utf-8", errors="ignore") |
| 166 | + ] |
| 167 | + assert leaked == [], f"Outside file leaked into generated command: {leaked}" |
| 168 | + |
| 169 | + |
| 170 | +class TestCommandFileTraversal: |
| 171 | + """The manifest ``file`` field must not read files outside source_dir. |
| 172 | +
|
| 173 | + Regression for GHSA-w5fv-7w9x-7fc5: ``register_commands`` read |
| 174 | + ``source_dir / cmd_file`` with no containment check, so a manifest with |
| 175 | + a traversal (``file: ../../../outside.txt``) or an absolute path read an |
| 176 | + arbitrary host file verbatim into the generated agent command. |
| 177 | + """ |
| 178 | + |
| 179 | + @pytest.mark.parametrize("bad_file", FILE_FIELD_PAYLOADS) |
| 180 | + def test_claude_skips_traversal_in_file_field(self, tmp_path, bad_file): |
| 181 | + project, ext_dir = _project_and_source(tmp_path) |
| 182 | + (project / ".claude" / "skills").mkdir(parents=True) |
| 183 | + |
| 184 | + outside_file = tmp_path / "outside.txt" |
| 185 | + outside_file.write_text("OUTSIDE-FILE-MARKER", encoding="utf-8") |
| 186 | + |
| 187 | + registrar = CommandRegistrar() |
| 188 | + registered = registrar.register_commands( |
| 189 | + "claude", |
| 190 | + [{"name": "speckit.myext.hello", "file": _resolve_payload(bad_file, outside_file), "aliases": []}], |
| 191 | + "myext", |
| 192 | + ext_dir, |
| 193 | + project, |
| 194 | + ) |
| 195 | + |
| 196 | + assert registered == [] |
| 197 | + _assert_no_marker_leak(project, "OUTSIDE-FILE-MARKER") |
| 198 | + |
| 199 | + @pytest.mark.parametrize("bad_file", FILE_FIELD_PAYLOADS) |
| 200 | + def test_gemini_skips_traversal_in_file_field(self, tmp_path, bad_file): |
| 201 | + project, ext_dir = _project_and_source(tmp_path) |
| 202 | + (project / ".gemini" / "commands").mkdir(parents=True) |
| 203 | + |
| 204 | + outside_file = tmp_path / "outside.txt" |
| 205 | + outside_file.write_text("OUTSIDE-FILE-MARKER", encoding="utf-8") |
| 206 | + |
| 207 | + registrar = CommandRegistrar() |
| 208 | + registered = registrar.register_commands( |
| 209 | + "gemini", |
| 210 | + [{"name": "speckit.myext.hello", "file": _resolve_payload(bad_file, outside_file), "aliases": []}], |
| 211 | + "myext", |
| 212 | + ext_dir, |
| 213 | + project, |
| 214 | + ) |
| 215 | + |
| 216 | + assert registered == [] |
| 217 | + _assert_no_marker_leak(project, "OUTSIDE-FILE-MARKER") |
| 218 | + |
| 219 | + @pytest.mark.parametrize("bad_value", [None, 123, "", ["x"]]) |
| 220 | + def test_non_string_file_is_skipped(self, tmp_path, bad_value): |
| 221 | + """A non-string/empty ``file`` must be skipped, not raise TypeError.""" |
| 222 | + project, ext_dir = _project_and_source(tmp_path) |
| 223 | + (project / ".gemini" / "commands").mkdir(parents=True) |
| 224 | + |
| 225 | + registrar = CommandRegistrar() |
| 226 | + registered = registrar.register_commands( |
| 227 | + "gemini", |
| 228 | + [{"name": "speckit.myext.hello", "file": bad_value, "aliases": []}], |
| 229 | + "myext", |
| 230 | + ext_dir, |
| 231 | + project, |
| 232 | + ) |
| 233 | + |
| 234 | + assert registered == [] |
| 235 | + |
| 236 | + def test_dotdot_rejected_even_when_target_is_in_bounds(self, tmp_path): |
| 237 | + """An in-bounds ``..`` payload is rejected by the ``..`` check itself. |
| 238 | +
|
| 239 | + ``commands/../cmd.md`` resolves to ``ext_dir/cmd.md`` — inside |
| 240 | + source_dir — so the resolve()/relative_to() containment backstop would |
| 241 | + allow it. Creating that target file ensures the command is skipped |
| 242 | + because of the ``..`` rejection, not merely because the file is absent. |
| 243 | + """ |
| 244 | + project, ext_dir = _project_and_source(tmp_path) |
| 245 | + (project / ".gemini" / "commands").mkdir(parents=True) |
| 246 | + (ext_dir / "cmd.md").write_text( |
| 247 | + "---\ndescription: test\n---\n\nbody\n", encoding="utf-8" |
| 248 | + ) |
| 249 | + |
| 250 | + registrar = CommandRegistrar() |
| 251 | + registered = registrar.register_commands( |
| 252 | + "gemini", |
| 253 | + [{"name": "speckit.myext.hello", "file": "commands/../cmd.md", "aliases": []}], |
| 254 | + "myext", |
| 255 | + ext_dir, |
| 256 | + project, |
| 257 | + ) |
| 258 | + |
| 259 | + assert registered == [] |
| 260 | + |
| 261 | + |
| 262 | +class TestRelativeExtensionPathPolicy: |
| 263 | + """Unit tests for the shared ``relative_extension_path_violation`` policy.""" |
| 264 | + |
| 265 | + @pytest.mark.parametrize( |
| 266 | + "value", |
| 267 | + [ |
| 268 | + "commands/hello.md", |
| 269 | + "hello.md", |
| 270 | + "a/b/c/hello.md", |
| 271 | + ], |
| 272 | + ) |
| 273 | + def test_safe_relative_paths_have_no_violation(self, value): |
| 274 | + assert relative_extension_path_violation(value) is None |
| 275 | + |
| 276 | + @pytest.mark.parametrize( |
| 277 | + "value", |
| 278 | + [ |
| 279 | + None, |
| 280 | + 123, |
| 281 | + ["x"], |
| 282 | + "", |
| 283 | + " ", |
| 284 | + " hello.md", |
| 285 | + "hello.md ", |
| 286 | + "/abs/outside.md", |
| 287 | + "/etc/passwd", |
| 288 | + "C:foo.md", |
| 289 | + "C:\\Windows\\system32", |
| 290 | + "\\\\server\\share\\x.md", |
| 291 | + "../escape.md", |
| 292 | + "commands/../../escape.md", |
| 293 | + ], |
| 294 | + ) |
| 295 | + def test_unsafe_values_report_violation(self, value): |
| 296 | + assert relative_extension_path_violation(value) is not None |
| 297 | + |
| 298 | + |
| 299 | +class TestReadSkipWarning: |
| 300 | + """Unregisterable but in-bounds files warn instead of failing silently.""" |
| 301 | + |
| 302 | + def test_unreadable_target_warns_and_skips(self, tmp_path): |
| 303 | + project, ext_dir = _project_and_source(tmp_path) |
| 304 | + (project / ".gemini" / "commands").mkdir(parents=True) |
| 305 | + (ext_dir / "cmd.md").write_bytes(b"\xff\xfe\x00\x80bad") |
| 306 | + |
| 307 | + registrar = CommandRegistrar() |
| 308 | + with pytest.warns(UserWarning): |
| 309 | + registered = registrar.register_commands( |
| 310 | + "gemini", |
| 311 | + [{"name": "speckit.myext.hello", "file": "cmd.md", "aliases": []}], |
| 312 | + "myext", |
| 313 | + ext_dir, |
| 314 | + project, |
| 315 | + ) |
| 316 | + |
| 317 | + assert registered == [] |
140 | 318 |
|
141 | 319 | def test_symlinked_subdir_under_commands_dir_is_preserved(self, tmp_path): |
142 | 320 | """Lexical check must not block legitimately symlinked sub-directories. |
|
0 commit comments