fix: validate git refs before archive URL (#53)#244
Conversation
|
🎉 Thank you @Kirti391 for submitting a Pull Request! We're excited to review your contribution. Before Review✅ Ensure all CI checks pass ⚡ Want faster reviews and contributor support? Join our Discord community: 🔗 https://discord.gg/FcXuyw2Rs Maintainers and mentors are active there and can help resolve blockers quickly. Happy Contributing! 🚀 |
github-actions
Bot
added
backend
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
arpit2006
added
Medium
arpit2006
left a comment
arpit2006
left a comment
There was a problem hiding this comment.
Fix CI and Quality Issue!
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
Hi @arpit2006 , |
|
@Kirti391 , Currently as I can see your Code is not passing Quality Gates please resolve it then do ping me for review. |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
Hi @arpit2006 , I addressed the requested changes and the quality gates are now passing. Could you please review the PR again? Thanks! |
|
@Kirti391 , Please Resolve issues. |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
@Kirti391 , Merge Confict! |
arpit2006
left a comment
arpit2006
left a comment
There was a problem hiding this comment.
Requesting changes — the intent is correct but there are blocking issues before this can merge.
Thanks for tackling this — ref validation before building the archive URL is exactly the right approach. That said, the current implementation has a few issues that need to be resolved:
🔴 validate_git_ref always returns True for non-existent refs
git ls-remote exits with code 0 even when no refs match — it simply returns empty stdout. Your CalledProcessError catch never fires for a missing branch; it only fires if git itself errors out (e.g., repo not found). You need to check whether the output is non-empty:
output = subprocess.check_output(["git", "ls-remote", repo_url, ref])
return bool(output.strip())🔴 Blocking the event loop
subprocess.check_output is synchronous. Calling it directly inside an async def endpoint blocks the entire event loop for the duration of the network round-trip to GitHub. Wrap it with run_in_threadpool:
is_valid = await run_in_threadpool(validate_git_ref, repo_url, ref)🔴 Wrong HTTP status code
The endpoint's own OpenAPI spec (line 699) documents 404 for "Repository or branch not found." Returning 400 for an invalid ref contradicts the documented contract. Please use status_code=404 here.
🔴 No input sanitization on ref
The ref value from the user is passed directly to subprocess and embedded in a URL with no allowlist check. A ref like ../../etc/passwd or one containing shell metacharacters could cause unexpected behavior. Add a guard before calling validate_git_ref:
if not re.match(r'^[a-zA-Z0-9_.\-/]+$', ref):
raise HTTPException(status_code=400, detail="Ref contains invalid characters.")🟡 Secondary issues worth fixing in this PR:
github_zip_urlhardcodesrefs/heads/, so validated tags (e.g.,v1.0.0) and commit SHAs will produce broken download URLs even after passing validation.- The org-scan workflow (
main.py:1363) callsgithub_zip_urlwith an unvalidated ref — the fix is incomplete. - Existing tests in
test_scan_url.pydon't mockvalidate_git_ref, so they'll now make real network calls. A test for the invalid-ref rejection path is also missing entirely.
Fix issues 1–4 and the tests, and this will be in good shape to merge. Happy to discuss any of these points further.
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
…pass, blocking call, tag/SHA URL bug)
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
@Kirti391 , Merge Conflicts! |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
2 participants
Linked issue
Closes #53
What this PR does
Fixes a bug in the scan_url workflow where archive URLs were constructed without validating the provided Git ref. Invalid or non‑existent refs previously caused CI failures and broken downloads. This PR introduces a validation step that checks the existence of the ref before building the archive URL, ensuring reliable scans and clearer error handling.
Type of change
ML tier (if applicable)
Stack affected
Changes
Backend
Added validate_git_ref helper function.
Updated scan_url to validate refs before calling github_zip_url.
Improved error handling with descriptive HTTPException messages.
Frontend
No frontend changes
New dependencies
None
Database / schema changes
None
Testing
How did you test this?
Ran the application locally.
Tested with valid refs (e.g., main) → scan succeeded.
Tested with invalid refs (e.g., wrongbranch) → clear error returned.
Verified CI no longer fails on invalid refs.
Checklist
console.erroror unhandled Python exceptions introducedrequirements.txt/package.jsonupdated if new dependencies added.pkl,.pt, etc.) are gitignored, not committedAnything reviewers should focus on
Please review the validation logic in scan_url and confirm that error handling is consistent with existing patterns.
Screenshots (if UI changed)
N/A (Backend‑only changes)