Skip to content

Validate git refs before constructing archive URL#235

Open
saitejasviputta wants to merge 6 commits into
ionfwsrijan:mainfrom
saitejasviputta:fix/validate-git-ref
Open

Validate git refs before constructing archive URL#235
saitejasviputta wants to merge 6 commits into
ionfwsrijan:mainfrom
saitejasviputta:fix/validate-git-ref

Conversation

@saitejasviputta

@saitejasviputta saitejasviputta commented Jun 28, 2026

Copy link
Copy Markdown

Linked issue

Closes #227

What this PR does

Adds validation for the user-supplied ref parameter used by the /scan-url endpoint before constructing the GitHub archive download URL. The change rejects malformed Git references and URL-encodes validated references before generating the archive URL.

Type of change

  • Bug fix
  • New feature
  • ML model / training pipeline
  • Refactor (no behaviour change)
  • Documentation
  • Tests only

ML tier (if applicable)

  • Tier 1 — Triage
  • Tier 2 — Predictive
  • Tier 3 — Autonomous
  • Not ML-related

Stack affected

  • Backend
  • Frontend
  • Both

Changes

Backend

  • Added validate_git_ref() to validate Git reference names using an allowlist.
  • Rejects malformed references containing invalid path components or traversal patterns.
  • Validates the ref parameter before constructing the GitHub archive URL.
  • URL-encodes the validated reference before inserting it into the archive URL.

Frontend

  • No changes.

New dependencies

  • None.

Database / schema changes

  • None.

Testing

How did you test this?

  • Verified that valid Git references such as main and feature/login continue to construct valid archive URLs.
  • Verified that malformed references (for example ../main, ./main, and references containing backslashes) return a 400 Bad Request.

Checklist

  • Tested locally end-to-end (upload ZIP or GitHub URL → scan → findings returned correctly)
  • New ML model falls back gracefully when model file is absent
  • No new console.error or unhandled Python exceptions introduced
  • Added or updated tests where applicable
  • requirements.txt / package.json updated if new dependencies added
  • New model files (.pkl, .pt, etc.) are gitignored, not committed

Anything reviewers should focus on

Please review the validation rules to ensure they are appropriately restrictive while continuing to support valid Git branch names such as feature/login.

Screenshots (if UI changed)

Not applicable.

@github-actions

Copy link
Copy Markdown

🎉 Thank you @saitejasviputta for submitting a Pull Request!

We're excited to review your contribution.

Before Review

✅ Ensure all CI checks pass
✅ Complete the PR template
✅ Link the related issue

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

Copy link
Copy Markdown

PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

1 similar comment
@github-actions

Copy link
Copy Markdown

PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

@github-actions

Copy link
Copy Markdown

PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

@github-actions github-actions Bot removed the needs-work Work needed label Jun 28, 2026
@github-actions github-actions Bot requested a review from arpit2006 June 28, 2026 10:42
@github-actions

Copy link
Copy Markdown

PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

@saitejasviputta

Copy link
Copy Markdown
Author

Hello @arpit2006, I tried by best to clear those CI fails. Please review it again. If it still fails I will try a different approach.
Thank you!

@arpit2006 arpit2006 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @saitejasviputta ! I won't be able to review this PR today due to other commitments. I'll go through the changes thoroughly tomorrow and, if everything looks good, I'll leave my approval then. Thanks for your patience!

@github-actions

Copy link
Copy Markdown

PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

@saitejasviputta

Copy link
Copy Markdown
Author

Hi @arpit2006, thank you for your patience! I've addressed the CI issue, and the checks are now passing on my latest changes. Please take your time to review it whenever you're available.
Thanks again!

@arpit2006 arpit2006 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saitejasviputta !
Thanks for the contribution!

It looks like the required GitHub Actions checks are currently failing, so I won't be able to complete the review or approve the PR just yet.

Could you please investigate and resolve the failing CI checks (particularly the Backend Python 3.10 workflow)? Once the CI is passing, I'll review the changes in detail and proceed with approval if everything looks good.

@github-actions github-actions Bot requested a review from arpit2006 June 29, 2026 12:47
@github-actions

Copy link
Copy Markdown

PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

@saitejasviputta

Copy link
Copy Markdown
Author

Hi @arpit2006 ,
Thank you for your patience! I've addressed the CI issues, and all the required GitHub Actions checks are now passing. Please review the PR whenever you have the time.
Thank you!

@arpit2006 arpit2006 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Approve ✅ — with one requested follow-up

The core fix is correct. validate_git_ref() uses an allowlist regex (^[A-Za-z0-9._/-]+$) which is the right approach — it correctly permits feature/login-style branches while blocking traversal patterns like ../main and ./main. The validation is called before URL construction, and quote(ref, safe="/") encodes it correctly so GitHub receives the expected path format.

One thing I'd ask before merging:
test_scan_url.py has no test cases for the ref parameter at all. The checklist says tests were added "where applicable" — I'd argue they're applicable here. Please add a few parametrized cases, for example:

  • main → should pass
  • feature/login → should pass
  • ../main → should return 400
    • ./main → should return 400
  • main\hack → should return 400

Minor nit (no blocker): The forbidden tuple checks for \\ (backslash), but the allowlist regex already excludes it — that branch is dead code. Worth a comment or removal for clarity.

Happy to approve once the tests are added. The security intent is solid.

@arpit2006

Copy link
Copy Markdown
Collaborator

@saitejasviputta , All good just solve the minor changes. Once the changes are done PR is ready to APPROVE.

@github-actions github-actions Bot requested a review from arpit2006 July 3, 2026 12:55
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

@saitejasviputta

Copy link
Copy Markdown
Author

Hello @arpit2006 ,
Thanks for the review! I’ve added the tests covering valid (main, feature/login, release-1.0) and invalid (../main, ./main, main\hack) ref values as requested. Please let me know if you'd like any additional test coverage.
Thank you!

@github-actions

Copy link
Copy Markdown

PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security] Validate ref Parameter When Constructing GitHub Archive URLs

2 participants