-
Notifications
You must be signed in to change notification settings - Fork 287
feat: Allow port in git repository url #1193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Allow port in git repository url #1193
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubic found 1 issue across 1 file. Review it in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai
to give specific feedback.
@@ -64,6 +64,7 @@ def validate_origin(cls, v: str) -> str: | |||
if not re.match( | |||
r"^git\+ssh://git@" # Protocol and user prefix | |||
r"[-a-zA-Z0-9.]+" # Hostname | |||
r"(:[0-9]{1,5})?" # Port |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex accepts any 1-5 digit number, so ports like 0 or 99999—outside the valid 1-65535 range—are treated as valid. This can let users configure repositories that will later fail to connect or open a security hole by silently accepting invalid endpoints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CL0Pinette looks good but just a small suggestion here ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I just pushed a new version with a more restrictive regex for ports
@CL0Pinette running the tests now. Have you dealt with the case where port is NOT required? Would it be possible to add 1-2 tests as well. Once that's through I think it's good to go! |
Yes, if port is not set, it is defaulted to 22. Sure, I will add test at the end of this week |
I believe it should be no port set at all as default. |
Hi! |
Thank you @CL0Pinette for taking a first shot at this, help us a lot! I'm going to close this PR and take it on / open it up to others. Looks like it's a bigger priority for folks. |
@Kilroy1337 @CL0Pinette tracking here: #1356. I'm working on it now. Will be out this week as part of 0.43.0 |
Checklist
uv run pytest tests
)?pre-commit run --all-files
)?Description
This PR allows the user to have a git repository with SSH server on a non default port.
Steps to QA
Add a new repository
git+ssh://git@host/org/repo.git
Add a new repository
git+ssh://git@host:1234/org/repo.git
Both repositories should sync well
Summary by cubic
Added support for specifying a port in git repository SSH URLs.