Skip to content

fix: change clone URL check for gitlab to account for possible subpath #5177

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion server/events/models/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,13 @@ func NewRepo(vcsHostType VCSHostType, repoFullName string, cloneURL string, vcsU
// Azure DevOps also does not require .git at the end of clone urls.
if vcsHostType != BitbucketServer && vcsHostType != AzureDevops {
expClonePath := fmt.Sprintf("/%s.git", repoFullName)
if expClonePath != cloneURLParsed.Path {
if vcsHostType == Gitlab {
// For GitLab, we need to check if the path ends with our expected path
// This handles cases where GitLab is hosted at a subpath (e.g., acme.com/gitlab)
if !strings.HasSuffix(cloneURLParsed.Path, expClonePath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now not checking that the hostname is as expected, so needs adding as a second check. A test also needs adding for this scenario.

Copy link
Author

Choose a reason for hiding this comment

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

For this case ( I assume you are referring to the webhook data containing a different host? ) there was no test before as well if I am not missing something.

Is this even a valid attack scenario that needs to be tested as:

The webhook secret as well as ATLANTIS_REPO_ALLOWLIST ensures requests come from our configured GitLab instance
The VCS client configuration (ATLANTIS_GITLAB_HOSTNAME) ensures we only talk to our configured GitLab instance

Copy link
Contributor

Choose a reason for hiding this comment

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

Security should always be multi-layered, and this function is called from numerous places within the code. It should be relatively simple to add back the GitLab host name check here. Also, this sub-path check should not be made for GitLab SaaS (gitlab.com).

Copy link
Author

Choose a reason for hiding this comment

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

The problem I face is the expected hostname is not available for checking in this function as it is not part of the NewRepo. It is also not checked in the current implementation on main, so I don't really understand your comment about adding back the check, as there is none right now if I am not completely missing something

return Repo{}, fmt.Errorf("expected clone url path to end with %q but had %q", expClonePath, cloneURLParsed.Path)
}
} else if expClonePath != cloneURLParsed.Path {
return Repo{}, fmt.Errorf("expected clone url to have path %q but had %q", expClonePath, cloneURLParsed.Path)
}
}
Expand Down
Loading