Fix Zip-Slip arbitrary file write bypass via startswith #157
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a Zip-Slip path validation bypass in the extension extraction utility, and also extends scan feedback handling to persist model/ruleset version metadata derived from stored scan payloads.
Changes:
- Harden zip-slip protection in
extract_extension_crx()by switching fromstartswith()toos.path.commonpath()comparisons. - Persist
model_versionin generated executive summaries and persistruleset_versioninto stored scansummarypayloads. - Enhance
/api/feedbackto resolve and storemodel_version/ruleset_version, with new endpoint tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/extension_shield/utils/extension.py |
Replaces fragile prefix-string validation with commonpath-based zip-slip validation during extraction. |
src/extension_shield/api/main.py |
Adds version-extraction helpers; augments stored scan summary with ruleset_version; persists versions when saving feedback. |
src/extension_shield/core/summary_generator.py |
Ensures executive summary payloads default model_version to the model used. |
tests/api/test_feedback_endpoint.py |
Adds tests asserting feedback persistence includes version metadata (and handles missing metadata). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| target_path = os.path.join(extract_dir, member.filename) | ||
| abs_target = os.path.abspath(target_path) | ||
| abs_extract = os.path.abspath(extract_dir) | ||
| if not abs_target.startswith(abs_extract): | ||
| if os.path.commonpath([abs_extract, abs_target]) != abs_extract: | ||
| raise ValueError(f"Zip-slip attempt detected: {member.filename}") | ||
| zip_ref.extract(member, extract_dir) |
There was a problem hiding this comment.
os.path.commonpath() can raise ValueError on Windows when paths are on different drives (e.g., a crafted member filename like D:\...). That would turn a zip-slip attempt into an unhandled exception rather than a clean rejection. Consider wrapping the commonpath call in try/except ValueError and treating it as a zip-slip violation (raise the same ValueError you raise now).
| target_path = os.path.join(extract_dir, member.filename) | ||
| abs_target = os.path.abspath(target_path) | ||
| abs_extract = os.path.abspath(extract_dir) | ||
| if not abs_target.startswith(abs_extract): | ||
| if os.path.commonpath([abs_extract, abs_target]) != abs_extract: | ||
| raise ValueError(f"Zip-slip attempt detected: {member.filename}") | ||
| zip_ref.extract(member, extract_dir) |
There was a problem hiding this comment.
Same commonpath check is used in the .zip extraction branch and has the same potential ValueError edge case on Windows (different drive letters). Handle ValueError consistently here as well so zip-slip attempts are rejected deterministically.
| target_path = os.path.join(extract_dir, member.filename) | ||
| abs_target = os.path.abspath(target_path) | ||
| abs_extract = os.path.abspath(extract_dir) | ||
| if not abs_target.startswith(abs_extract): | ||
| if os.path.commonpath([abs_extract, abs_target]) != abs_extract: | ||
| raise ValueError(f"Zip-slip attempt detected: {member.filename}") | ||
| zip_ref.extract(member, extract_dir) |
There was a problem hiding this comment.
The zip-slip fix is security-critical, but there isn’t currently a unit test that would have caught the original .startswith() prefix-bypass (e.g., extracting to a sibling dir like .../extracted_x_hacked). There are already extraction tests in tests/test_extension_extract.py (zip-bomb), so adding a focused regression test for this path-prefix case would help prevent reintroductions.
| def _coerce_json_dict(value: Any) -> Dict[str, Any]: | ||
| """Convert JSON-ish values to a dict for metadata lookups.""" | ||
| if isinstance(value, dict): | ||
| return value | ||
| if isinstance(value, str): | ||
| try: | ||
| parsed = json.loads(value) | ||
| except (TypeError, ValueError): | ||
| return {} | ||
| return parsed if isinstance(parsed, dict) else {} | ||
| return {} | ||
|
|
||
|
|
||
| def _extract_feedback_versions(payload: Optional[Dict[str, Any]]) -> tuple[Optional[str], Optional[str]]: | ||
| """Extract model and ruleset versions from a scan payload when available.""" | ||
| if not isinstance(payload, dict): | ||
| return None, None | ||
|
|
||
| metadata = _coerce_json_dict(payload.get("metadata")) | ||
| summary = _coerce_json_dict(payload.get("summary")) | ||
| report_view_model = _coerce_json_dict(payload.get("report_view_model")) |
There was a problem hiding this comment.
This PR’s description is focused on the zip-slip validation change, but it also introduces new feedback-version extraction logic and modifies summary generation to persist model_version/ruleset_version. Please update the PR description to reflect these additional behavior changes, or split them into a separate PR to keep the security fix isolated and easier to review/release.
| def _resolve_feedback_versions(scan_id: str) -> tuple[Optional[str], Optional[str]]: | ||
| """Look up the related scan payload and extract version metadata safely.""" | ||
| payload = scan_results.get(scan_id) | ||
| if isinstance(payload, dict): | ||
| return _extract_feedback_versions(payload) | ||
|
|
||
| db_payload = db.get_scan_result(scan_id) | ||
| return _extract_feedback_versions(db_payload) | ||
|
|
There was a problem hiding this comment.
_resolve_feedback_versions() calls db.get_scan_result(scan_id) which (in both SQLite and Supabase implementations) fetches the full scan row (SELECT *). Since feedback only needs a couple metadata fields, this adds potentially heavy read I/O per feedback submission. Consider adding a narrower DB method/query (e.g., fetch only summary/metadata/scoring_v2) or storing the needed version fields in a dedicated column so this endpoint doesn’t need to load the full payload.
| executive_summary.setdefault( | ||
| "ruleset_version", | ||
| scoring_v2_payload.get("weights_version") or scoring_v2_payload.get("scoring_version"), | ||
| ) |
There was a problem hiding this comment.
executive_summary is copied and augmented with ruleset_version before being stored in raw_results["summary"], but build_report_view_model_safe() is still fed analysis_results with executive_summary sourced from final_state (without this augmentation). That can lead to inconsistencies between summary and report_view_model for the same scan. Consider passing the updated executive_summary into the analysis_results dict used for report_view_model generation as well.
| ) | |
| ) | |
| analysis_results = dict(analysis_results or {}) | |
| analysis_results["executive_summary"] = executive_summary |
|
@Th-Shivam please fix the failing checks! |
|
@sapnilbiswas ok i will fix it |
github-actions
Bot
added
the
area: backend
|
@Th-Shivam Any updates? |
📝 WalkthroughWalkthroughChanges establish a local Supabase CLI wrapper script, add version tracking utilities to the feedback workflow, enhance zip-slip protection in extension extraction, and introduce a comprehensive feedback endpoint test suite. No breaking changes to public APIs or user-facing behavior. Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Main
participant DB as Database
participant SummaryGen as Summary Generator
Client->>API: POST /feedback (scan_id, feedback_data)
API->>API: _extract_feedback_versions(payload)
alt Versions in payload
API->>API: Parse model_version from summary
API->>API: Extract ruleset_version from scoring_v2_payload
else Versions not in payload
API->>DB: get_scan_result(scan_id)
DB-->>API: scan_result
API->>API: _resolve_feedback_versions(scan_id)
end
API->>DB: save_feedback(model_version, ruleset_version, ...)
DB-->>API: Feedback saved
API->>SummaryGen: generate_summary()
SummaryGen->>SummaryGen: Augment with model_version from model_name
SummaryGen-->>API: enriched_summary
API-->>Client: HTTP 200 + summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
area: docs
![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
🧹 Nitpick comments (2)
src/extension_shield/utils/extension.py (1)
239-247: Optional: centralize the containment guard to avoid logic drift.The same zip-slip check is duplicated in both branches. A small helper would keep behavior consistent if this evolves later.
♻️ Suggested refactor
+def _assert_within_extract_dir(extract_dir: str, member_name: str) -> None: + target_path = os.path.join(extract_dir, member_name) + abs_target = os.path.abspath(target_path) + abs_extract = os.path.abspath(extract_dir) + if os.path.commonpath([abs_extract, abs_target]) != abs_extract: + raise ValueError(f"Zip-slip attempt detected: {member_name}") + ... - target_path = os.path.join(extract_dir, member.filename) - abs_target = os.path.abspath(target_path) - abs_extract = os.path.abspath(extract_dir) - if os.path.commonpath([abs_extract, abs_target]) != abs_extract: - raise ValueError(f"Zip-slip attempt detected: {member.filename}") + _assert_within_extract_dir(extract_dir, member.filename) zip_ref.extract(member, extract_dir) ... - target_path = os.path.join(extract_dir, member.filename) - abs_target = os.path.abspath(target_path) - abs_extract = os.path.abspath(extract_dir) - if os.path.commonpath([abs_extract, abs_target]) != abs_extract: - raise ValueError(f"Zip-slip attempt detected: {member.filename}") + _assert_within_extract_dir(extract_dir, member.filename) zip_ref.extract(member, extract_dir)Also applies to: 260-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/extension_shield/utils/extension.py` around lines 239 - 247, Duplicate zip-slip containment logic around the zip extraction should be centralized into a small helper to avoid drift: create a helper function (e.g., _ensure_within_extract_dir(abs_extract, target_path) or _is_within_extract_dir(extract_dir, member_filename)) and replace the duplicated checks in the extraction loops (the code that computes target_path, abs_target, abs_extract and compares os.path.commonpath) with a single call to that helper before calling zip_ref.extract(member, extract_dir); ensure the helper raises the same ValueError message ("Zip-slip attempt detected: {member.filename}") or returns False so callers enforce the same behavior.package.json (1)
4-12: Consider preserving a pinned Supabase CLI for reproducible tooling.With
supabaseremoved fromdevDependencies, script behavior now depends on each machine’s globally installed CLI version. If deterministic dev/CI behavior is required, keep a pinned local CLI and run throughnpx --no-install(or equivalent local-bin resolution).#!/bin/bash # Verify current package.json Supabase script wiring and whether Supabase is pinned locally. python - <<'PY' import json, pathlib pkg = json.loads(pathlib.Path("package.json").read_text()) deps = pkg.get("devDependencies", {}) print("devDependencies.supabase:", deps.get("supabase")) print("\nScripts using run-supabase.sh:") for name, cmd in pkg.get("scripts", {}).items(): if "run-supabase.sh" in cmd: print(f" {name}: {cmd}") PYExpected:
devDependencies.supabaseisNone/missing, and all db/supabase scripts route viarun-supabase.sh.Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 4 - 12, Add a pinned supabase CLI to devDependencies and ensure scripts use the local CLI resolver rather than relying on a global install: add a fixed version entry for "supabase" under devDependencies, and update the script entries that reference run-supabase.sh (e.g., the "supabase", "db:diff", "db:push", "db:pull", "db:reset", "db:start", "db:stop", "db:migration:up", "db:migration:new" scripts) to invoke the CLI via local-bin resolution such as using "npx --no-install supabase ..." or the package-manager's equivalent so CI/dev environments use the pinned version while still keeping the helper run-supabase.sh behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/extension_shield/api/main.py`:
- Around line 1000-1030: _extract_feedback_versions can return non-string values
even though its signature is tuple[Optional[str], Optional[str]]; coerce the
resolved model_version and ruleset_version to either a stripped string or None
before returning so callers (e.g., db.save_feedback) never receive non-string
types. Locate the variables model_version and ruleset_version in
_extract_feedback_versions and replace the final return with normalization that:
if the value is None leave it None, otherwise convert to str(value).strip() and
treat empty strings as None.
- Around line 1033-1040: The DB read in _resolve_feedback_versions can raise and
crash /api/feedback; wrap the call to db.get_scan_result(scan_id) in a
try/except and on any exception return (None, None) so feedback persists; locate
_resolve_feedback_versions and ensure exceptions from db.get_scan_result are
caught and handled (still call _extract_feedback_versions only when db_payload
is valid) and return (None, None) on failure.
In `@src/extension_shield/core/summary_generator.py`:
- Line 566: The current use of summary.setdefault("model_version", model_name)
only fills the fallback when the key is missing, not when the value is
None/empty; change the logic in the summary generation path (the code using
summary.setdefault and the variables summary and model_name) to explicitly check
for falsy/empty values (e.g., if not summary.get("model_version"):
summary["model_version"] = model_name) so that None or empty-string values are
replaced with model_name.
---
Nitpick comments:
In `@package.json`:
- Around line 4-12: Add a pinned supabase CLI to devDependencies and ensure
scripts use the local CLI resolver rather than relying on a global install: add
a fixed version entry for "supabase" under devDependencies, and update the
script entries that reference run-supabase.sh (e.g., the "supabase", "db:diff",
"db:push", "db:pull", "db:reset", "db:start", "db:stop", "db:migration:up",
"db:migration:new" scripts) to invoke the CLI via local-bin resolution such as
using "npx --no-install supabase ..." or the package-manager's equivalent so
CI/dev environments use the pinned version while still keeping the helper
run-supabase.sh behavior.
In `@src/extension_shield/utils/extension.py`:
- Around line 239-247: Duplicate zip-slip containment logic around the zip
extraction should be centralized into a small helper to avoid drift: create a
helper function (e.g., _ensure_within_extract_dir(abs_extract, target_path) or
_is_within_extract_dir(extract_dir, member_filename)) and replace the duplicated
checks in the extraction loops (the code that computes target_path, abs_target,
abs_extract and compares os.path.commonpath) with a single call to that helper
before calling zip_ref.extract(member, extract_dir); ensure the helper raises
the same ValueError message ("Zip-slip attempt detected: {member.filename}") or
returns False so callers enforce the same behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebb3e6e6-6641-419e-9763-93f2f460dae9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.codexdocs/GET_STARTED.mdpackage.jsonscripts/README.mdscripts/run-supabase.shscripts/supabase_push_env.shsrc/extension_shield/api/main.pysrc/extension_shield/core/summary_generator.pysrc/extension_shield/utils/extension.pytests/api/test_feedback_endpoint.py
| def _extract_feedback_versions(payload: Optional[Dict[str, Any]]) -> tuple[Optional[str], Optional[str]]: | ||
| """Extract model and ruleset versions from a scan payload when available.""" | ||
| if not isinstance(payload, dict): | ||
| return None, None | ||
|
|
||
| metadata = _coerce_json_dict(payload.get("metadata")) | ||
| summary = _coerce_json_dict(payload.get("summary")) | ||
| report_view_model = _coerce_json_dict(payload.get("report_view_model")) | ||
| report_meta = _coerce_json_dict(report_view_model.get("meta")) | ||
|
|
||
| scoring_v2 = _coerce_json_dict(payload.get("scoring_v2")) | ||
| if not scoring_v2: | ||
| scoring_v2 = _coerce_json_dict(summary.get("scoring_v2")) | ||
|
|
||
| model_version = ( | ||
| summary.get("model_version") | ||
| or metadata.get("model_version") | ||
| or report_meta.get("model_version") | ||
| or summary.get("llm_model") | ||
| or metadata.get("llm_model") | ||
| ) | ||
|
|
||
| ruleset_version = ( | ||
| summary.get("ruleset_version") | ||
| or metadata.get("ruleset_version") | ||
| or report_meta.get("ruleset_version") | ||
| or scoring_v2.get("weights_version") | ||
| or scoring_v2.get("scoring_version") | ||
| ) | ||
|
|
||
| return model_version, ruleset_version |
There was a problem hiding this comment.
Normalize extracted version fields to strings to match declared return type.
_extract_feedback_versions is annotated as tuple[Optional[str], Optional[str]], but current selection logic can return non-string values from payloads. Normalize/strip before returning to avoid type drift into db.save_feedback.
🔧 Proposed fix
+def _coerce_optional_str(value: Any) -> Optional[str]:
+ if isinstance(value, str):
+ value = value.strip()
+ return value or None
+ return None
+
def _extract_feedback_versions(payload: Optional[Dict[str, Any]]) -> tuple[Optional[str], Optional[str]]:
@@
- return model_version, ruleset_version
+ return _coerce_optional_str(model_version), _coerce_optional_str(ruleset_version)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/extension_shield/api/main.py` around lines 1000 - 1030,
_extract_feedback_versions can return non-string values even though its
signature is tuple[Optional[str], Optional[str]]; coerce the resolved
model_version and ruleset_version to either a stripped string or None before
returning so callers (e.g., db.save_feedback) never receive non-string types.
Locate the variables model_version and ruleset_version in
_extract_feedback_versions and replace the final return with normalization that:
if the value is None leave it None, otherwise convert to str(value).strip() and
treat empty strings as None.
| def _resolve_feedback_versions(scan_id: str) -> tuple[Optional[str], Optional[str]]: | ||
| """Look up the related scan payload and extract version metadata safely.""" | ||
| payload = scan_results.get(scan_id) | ||
| if isinstance(payload, dict): | ||
| return _extract_feedback_versions(payload) | ||
|
|
||
| db_payload = db.get_scan_result(scan_id) | ||
| return _extract_feedback_versions(db_payload) |
There was a problem hiding this comment.
Guard feedback version lookup against DB read failures.
At Line 1039, an exception from db.get_scan_result(scan_id) will bubble up and fail /api/feedback. This path should degrade to (None, None) so feedback still persists.
🛡️ Proposed fix
def _resolve_feedback_versions(scan_id: str) -> tuple[Optional[str], Optional[str]]:
"""Look up the related scan payload and extract version metadata safely."""
payload = scan_results.get(scan_id)
if isinstance(payload, dict):
return _extract_feedback_versions(payload)
- db_payload = db.get_scan_result(scan_id)
- return _extract_feedback_versions(db_payload)
+ try:
+ db_payload = db.get_scan_result(scan_id)
+ except Exception as exc:
+ logger.warning("Failed to resolve feedback versions for scan_id=%s: %s", scan_id, exc)
+ return None, None
+ return _extract_feedback_versions(db_payload)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/extension_shield/api/main.py` around lines 1033 - 1040, The DB read in
_resolve_feedback_versions can raise and crash /api/feedback; wrap the call to
db.get_scan_result(scan_id) in a try/except and on any exception return (None,
None) so feedback persists; locate _resolve_feedback_versions and ensure
exceptions from db.get_scan_result are caught and handled (still call
_extract_feedback_versions only when db_payload is valid) and return (None,
None) on failure.
| ) | ||
|
|
||
|
|
||
| summary.setdefault("model_version", model_name) |
There was a problem hiding this comment.
model_version fallback should handle empty/null values, not just missing keys.
At Line 566, setdefault skips fallback when "model_version" exists but is None/empty, which can still propagate missing version metadata.
🔧 Proposed fix
- summary.setdefault("model_version", model_name)
+ if not isinstance(summary.get("model_version"), str) or not summary.get("model_version").strip():
+ summary["model_version"] = model_name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| summary.setdefault("model_version", model_name) | |
| if not isinstance(summary.get("model_version"), str) or not summary.get("model_version").strip(): | |
| summary["model_version"] = model_name |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/extension_shield/core/summary_generator.py` at line 566, The current use
of summary.setdefault("model_version", model_name) only fills the fallback when
the key is missing, not when the value is None/empty; change the logic in the
summary generation path (the code using summary.setdefault and the variables
summary and model_name) to explicitly check for falsy/empty values (e.g., if not
summary.get("model_version"): summary["model_version"] = model_name) so that
None or empty-string values are replaced with model_name.
Description:
Fixes : #156
This PR patches an incomplete Zip-slip validation in the file extraction logic within src/extension_shield/utils/extension.py.
Fix details:
The original implementation used if not abs_target.startswith(abs_extract): to prevent extracting files outside of extract_dir. Since .startswith() matches prefixes exactly as strings, an extract_dir of /extensions/extracted_123 would falsely allow writing to /extensions/extracted_123_hacked/malicious_file.py because the path string technically still starts with /extensions/extracted_123.
This has been updated to use os.path.commonpath([abs_extract, abs_target]) != abs_extract which robustly compares the actual file system paths correctly, enforcing that extracted files reside strictly within the intended extract_dir directory structure.
Summary by CodeRabbit
Release Notes
Documentation
Chores