Merge pull request #5057 from psrsingh/fix-github-return-url-validation

fix: validate github return url metadata

Author: lukaszgryglicki

cla-backend-go/github/github_repository.go

@@ -2597,28 +2597,76 @@ func GetReturnURL(ctx context.Context, installationID, repositoryID int64, pullR
 		"pullRequestID":  pullRequestID,
 	}
 
-	client, err := NewGithubAppClient(installationID)
+	if installationID <= 0 {
+		err := errors.New("invalid installation ID")
+		log.WithFields(f).WithError(err).Warn("invalid installation ID")
+		return "", err
+	}
+	if repositoryID <= 0 {
+		err := errors.New("invalid repository ID")
+		log.WithFields(f).WithError(err).Warn("invalid repository ID")
+		return "", err
+	}
+	if pullRequestID <= 0 {
+		err := errors.New("invalid pull request ID")
+		log.WithFields(f).WithError(err).Warn("invalid pull request ID")
+		return "", err
+	}
 
+	client, err := NewGithubAppClient(installationID)
 	if err != nil {
 		log.WithFields(f).WithError(err).Warn("unable to create Github client")
 		return "", err
 	}
 
 	log.WithFields(f).Debugf("getting github repository by id: %d", repositoryID)
-	repo, _, err := client.Repositories.GetByID(ctx, repositoryID)
+	repo, resp, err := client.Repositories.GetByID(ctx, repositoryID)
 	if err != nil {
+		if ok, wrapped := CheckAndWrapForKnownErrors(resp, err); ok {
+			log.WithFields(f).WithError(wrapped).Warnf("unable to get repository by ID: %d", repositoryID)
+			return "", wrapped
+		}
 		log.WithFields(f).WithError(err).Warnf("unable to get repository by ID: %d", repositoryID)
 		return "", err
 	}
+	if repo == nil {
+		err := fmt.Errorf("missing repository for repository ID %d", repositoryID)
+		log.WithFields(f).WithError(err).Warn("invalid repository metadata")
+		return "", err
+	}
+
+	owner := repo.GetOwner().GetLogin()
+	name := repo.GetName()
+	if owner == "" || name == "" {
+		err := fmt.Errorf("invalid repository owner/name for repository ID %d", repositoryID)
+		log.WithFields(f).WithError(err).Warn("invalid repository metadata")
+		return "", err
+	}
 
 	log.WithFields(f).Debugf("getting pull request by id: %d", pullRequestID)
-	pullRequest, _, err := client.PullRequests.Get(ctx, *repo.Owner.Login, *repo.Name, pullRequestID)
+	pullRequest, resp, err := client.PullRequests.Get(ctx, owner, name, pullRequestID)
 	if err != nil {
+		if ok, wrapped := CheckAndWrapForKnownErrors(resp, err); ok {
+			log.WithFields(f).WithError(wrapped).Warnf("unable to get pull request by ID: %d", pullRequestID)
+			return "", wrapped
+		}
 		log.WithFields(f).WithError(err).Warnf("unable to get pull request by ID: %d", pullRequestID)
 		return "", err
 	}
+	if pullRequest == nil {
+		err := fmt.Errorf("missing pull request %d/%s/%s", pullRequestID, owner, name)
+		log.WithFields(f).WithError(err).Warn("invalid pull request metadata")
+		return "", err
+	}
+
+	htmlURL := pullRequest.GetHTMLURL()
+	if htmlURL == "" {
+		err := fmt.Errorf("missing html url for pull request %d/%s/%s", pullRequestID, owner, name)
+		log.WithFields(f).WithError(err).Warn("invalid pull request metadata")
+		return "", err
+	}
 
-	log.WithFields(f).Debugf("returning pull request html url: %s", *pullRequest.HTMLURL)
+	log.WithFields(f).Debugf("returning pull request html url: %s", htmlURL)
 
-	return *pullRequest.HTMLURL, nil
+	return htmlURL, nil
 }

cla-backend-go/v2/sign/service.go

@@ -1550,6 +1550,22 @@ func getUserName(user *v1Models.User) string {
 	return ""
 }
 
+// metadataStringValue extracts a string value from metadata and validates it.
+func metadataStringValue(metadata map[string]interface{}, key string) (string, error) {
+	if metadata == nil {
+		return "", fmt.Errorf("missing %s in metadata", key)
+	}
+	v, ok := metadata[key]
+	if !ok || v == nil {
+		return "", fmt.Errorf("missing %s in metadata", key)
+	}
+	s := strings.TrimSpace(fmt.Sprintf("%v", v))
+	if s == "" || s == "<nil>" {
+		return "", fmt.Errorf("missing %s in metadata", key)
+	}
+	return s, nil
+}
+
 func (s *service) getIndividualSignatureCallbackURLGitlab(ctx context.Context, userID string, metadata map[string]interface{}) (string, error) {
 	f := logrus.Fields{
 		"functionName": "sign.getIndividualSignatureCallbackURLGitlab",
@@ -1569,16 +1585,14 @@ func (s *service) getIndividualSignatureCallbackURLGitlab(ctx context.Context, u
 		}
 	}
 
-	if found, ok := metadata["repository_id"].(string); ok {
-		repositoryID = found
-	} else {
+	repositoryID, err = metadataStringValue(metadata, "repository_id")
+	if err != nil {
 		log.WithFields(f).WithError(err).Warnf("unable to get repository ID for user: %s", userID)
 		return "", err
 	}
 
-	if found, ok := metadata["merge_request_id"].(string); ok {
-		mergeRequestID = found
-	} else {
+	mergeRequestID, err = metadataStringValue(metadata, "merge_request_id")
+	if err != nil {
 		log.WithFields(f).WithError(err).Warnf("unable to get merge request ID for user: %s", userID)
 		return "", err
 	}
@@ -1599,6 +1613,7 @@ func (s *service) getIndividualSignatureCallbackURLGitlab(ctx context.Context, u
 	}
 
 	if gitlabOrg.OrganizationID == "" {
+		err = errors.New("missing organization ID for GitLab repository")
 		log.WithFields(f).WithError(err).Warnf("unable to get organization ID for repository ID: %s", repositoryID)
 		return "", err
 	}
@@ -1628,18 +1643,16 @@ func (s *service) getIndividualSignatureCallbackURL(ctx context.Context, userID
 		}
 	}
 
-	if found, ok := metadata["repository_id"].(string); ok {
-		repositoryID = found
-	} else {
+	repositoryID, err = metadataStringValue(metadata, "repository_id")
+	if err != nil {
 		log.WithFields(f).WithError(err).Warnf("unable to get repository ID for user: %s", userID)
 		return "", err
 	}
 
 	log.WithFields(f).Debugf("found repository ID: %s", repositoryID)
 
-	if found, ok := metadata["pull_request_id"].(string); ok {
-		pullRequestID = found
-	} else {
+	pullRequestID, err = metadataStringValue(metadata, "pull_request_id")
+	if err != nil {
 		log.WithFields(f).WithError(err).Warnf("unable to get pull request ID for user: %s", userID)
 		return "", err
 	}