fix: preserve gitlab namespace hierarchy in mirrors#11
Merged
Conversation
📝 WalkthroughWalkthroughReplaces repo.Name with repo.FullPath across mirroring and outputs, adds OS-aware path validations (including Windows reserved names and MAX_PATH checks), enforces cloning into BaseDir/<repo.FullPath>, introduces multi-group CLI mirroring, and adds unit tests for path and nesting cases. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (ztigit)
participant Auth as Token/Keychain
participant Provider as Provider API (GitLab/GitHub)
participant Mirror as Mirror (internal/mirror)
participant FS as Filesystem
CLI->>Auth: load token (keychain/env/config)
CLI->>Provider: validate provider type / resolve base URL
CLI->>Mirror: start mirror (groups or single target)
Mirror->>Provider: list repositories (include FullPath)
Mirror->>Mirror: validatePath(repo.FullPath)
Mirror->>Mirror: validateFullPathLength(BaseDir, repo.FullPath)
alt path valid
Mirror->>FS: create BaseDir/<repo.FullPath> directories
Mirror->>FS: git clone or git fetch into BaseDir/<repo.FullPath>
Mirror->>CLI: report "cloned"/"updated" using repo.FullPath
else invalid path
Mirror->>CLI: return descriptive error (invalid path / too long)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/mirror/mirror_path_test.go`:
- Around line 1-7: The import block in mirror_path_test.go is not
gofmt-formatted; run gofmt (or goimports) on the file and fix the import
grouping for package mirror so standard library imports (path/filepath, testing)
are grouped together and the external import
(github.com/zsoftly/ztigit/internal/provider) is separated with a blank line,
then save the file to resolve the CI gofmt violations.
In `@internal/mirror/mirror.go`:
- Around line 208-211: The code currently joins m.options.BaseDir with
repo.FullPath directly in Mirror.mirrorRepo, which allows path traversal; fix by
validating/sanitizing repo.FullPath before join: 1) canonicalize repo.FullPath
with filepath.Clean and reject if filepath.IsAbs(cleaned) or it equals "." or
begins with ".." or contains ".." path elements; 2) construct repoDir from the
cleaned relative path and then compute absolute paths for both m.options.BaseDir
and repoDir and verify filepath.Rel(baseAbs, repoDirAbs) does not start with
".." (reject if it does); 3) on rejection return a failure Result (same error
handling path as other validation errors) rather than proceeding to use repoDir.
Use the symbols mirrorRepo, repo.FullPath, m.options.BaseDir, and repoDir when
making these changes.
ahmedb244
force-pushed
the
issues/10-gitlab-nesting
branch
from
39158a1 to
bcc061c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/mirror/mirror_path_test.go`:
- Line 59: Replace the hard-coded "/etc/passwd" absolute-path test entry in
mirror_path_test.go (the {"Absolute Path", "/etc/passwd", true} case) with a
platform-aware path: call filepath.Abs on a relative path (and handle the error)
or construct one via filepath.Join(string(filepath.Separator), "etc", "passwd")
so filepath.IsAbs returns true on all OSes; alternatively gate the test via
runtime.GOOS or build tags to provide a Windows-specific absolute path—update
the test table entry to use the computed path variable and keep the expected
boolean true.
- Around line 76-86: The test dereferences result.Error without a nil-check
causing potential panics; update the test in mirror_path_test.go (the block
referencing result.Error, tt.isError and tt.fullpath) to first check if
result.Error != nil before calling result.Error.Error(), and invert logic
appropriately (e.g. if tt.isError assert result.Error != nil and contains the
"invalid repository path" substring; if not tt.isError assert result.Error ==
nil or does not contain that substring). Also strengthen the security-path check
to handle OS differences by normalizing/inspecting absolute paths in a
cross-platform way (use filepath.IsAbs semantics or test both "/" and
drive-letter/Windows absolute forms) so paths like "/etc/passwd" on Windows CI
are evaluated correctly.
♻️ Duplicate comments (1)
internal/mirror/mirror_path_test.go (1)
3-9: Fix import grouping to satisfy gofmt.Standard library imports should be grouped separately from external imports with a blank line between them.
🧹 Proposed fix
import ( "context" - "github.com/zsoftly/ztigit/internal/provider" "path/filepath" "strings" "testing" + + "github.com/zsoftly/ztigit/internal/provider" )
🧹 Nitpick comments (1)
internal/mirror/mirror_path_test.go (1)
34-42: Test doesn't exercise actual mirror code.This test only verifies
filepath.Joinbehavior, not the actualmirrorRepopath construction or security guard. Consider removing this test sinceTestPathTraversalBlockingalready exercises the real code path with valid inputs.
When mirroring GitLab repositories, the local directory structure now correctly preserves the full namespace path, including groups and any nested subgroups. Previously, repositories from all subgroups were cloned into a single, flat directory, causing namespace collisions and loss of context. The cloning logic was using the project's base name instead of its 'path_with_namespace'. This fix changes the mirroring logic to use the full 'FullPath' (path_with_namespace) when constructing the clone destination. All logging and reporting have also been updated to display the full path, improving clarity. A test case has been added to verify that repositories from subgroups are cloned into the correct nested directory structure.
ditahkk
force-pushed
the
issues/10-gitlab-nesting
branch
from
66793db to
fa73c4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/mirror/mirror_test.go`:
- Around line 1-10: The file in package "mirror"
(internal/mirror/mirror_test.go) is not formatted per gofmt which is causing CI
to fail; run "gofmt -s -w" on this file (and any other files reported by `gofmt
-s -l .`) to apply standard formatting changes to the imports/whitespace so the
test file compiles and CI passes. Ensure you stage the updated file(s) before
committing.
- Around line 45-52: The test in mirror_test.go creates a provider.Repository
(repo) that points at an external GitHub URL via CloneURL/SSHUrl which
introduces network flakiness; replace that external clone with a local bare repo
fixture (create a temporary bare repo at test startup and set repo.CloneURL to
its file:// path or its filesystem path) or alternatively wrap the test in a
testing.Short() check to skip when network/integration tests are undesired;
update the setup/teardown to init the bare repo before using the repository
value and ensure repo.CloneURL/SSHUrl reference the local fixture rather than
the GitHub URL.
Comment on lines
+45
to
+52
| // This repo mimics a GitLab project in a nested subgroup | ||
| repo := provider.Repository{ | ||
| Name: "my-project", | ||
| FullPath: "my-group/my-subgroup/my-project", | ||
| // Use a public, lightweight repo for the clone test | ||
| CloneURL: "https://github.com/git-fixtures/basic.git", | ||
| SSHUrl: "git@github.com:git-fixtures/basic.git", | ||
| } |
There was a problem hiding this comment.
Avoid network dependency in unit test.
This test clones a public GitHub repo, which makes CI flaky/offline-sensitive and slow. Prefer a local bare repo fixture (or mark as integration/testing.Short() skip).
💡 Example: create a local bare repo fixture and use its path
import (
"context"
"os"
+ "os/exec"
"path/filepath"
"testing"
"github.com/zsoftly/ztigit/internal/provider"
)
+
+func createBareRepo(t *testing.T, dir string) string {
+ t.Helper()
+ if _, err := exec.LookPath("git"); err != nil {
+ t.Skip("git not available; skipping clone test")
+ }
+ repoDir := filepath.Join(dir, "fixture.git")
+ cmd := exec.Command("git", "init", "--bare", repoDir)
+ if out, err := cmd.CombinedOutput(); err != nil {
+ t.Fatalf("git init --bare failed: %v\n%s", err, out)
+ }
+ return repoDir
+}
func TestMirrorRepo_GitLabSubgroup(t *testing.T) {
// 1. Setup
tempDir, err := os.MkdirTemp("", "ztigit-test-*")
if err != nil {
t.Fatalf("Failed to create temp dir: %v", err)
}
defer os.RemoveAll(tempDir)
// This repo mimics a GitLab project in a nested subgroup
+ localRepoPath := createBareRepo(t, tempDir)
repo := provider.Repository{
Name: "my-project",
FullPath: "my-group/my-subgroup/my-project",
- // Use a public, lightweight repo for the clone test
- CloneURL: "https://github.com/git-fixtures/basic.git",
- SSHUrl: "git@github.com:git-fixtures/basic.git",
+ CloneURL: localRepoPath,
+ SSHUrl: localRepoPath,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // This repo mimics a GitLab project in a nested subgroup | |
| repo := provider.Repository{ | |
| Name: "my-project", | |
| FullPath: "my-group/my-subgroup/my-project", | |
| // Use a public, lightweight repo for the clone test | |
| CloneURL: "https://github.com/git-fixtures/basic.git", | |
| SSHUrl: "git@github.com:git-fixtures/basic.git", | |
| } | |
| // This repo mimics a GitLab project in a nested subgroup | |
| localRepoPath := createBareRepo(t, tempDir) | |
| repo := provider.Repository{ | |
| Name: "my-project", | |
| FullPath: "my-group/my-subgroup/my-project", | |
| CloneURL: localRepoPath, | |
| SSHUrl: localRepoPath, | |
| } |
🤖 Prompt for AI Agents
In `@internal/mirror/mirror_test.go` around lines 45 - 52, The test in
mirror_test.go creates a provider.Repository (repo) that points at an external
GitHub URL via CloneURL/SSHUrl which introduces network flakiness; replace that
external clone with a local bare repo fixture (create a temporary bare repo at
test startup and set repo.CloneURL to its file:// path or its filesystem path)
or alternatively wrap the test in a testing.Short() check to skip when
network/integration tests are undesired; update the setup/teardown to init the
bare repo before using the repository value and ensure repo.CloneURL/SSHUrl
reference the local fixture rather than the GitHub URL.
…ssues/10-gitlab-nesting-extra-tests Resolve conflicts by: - Keeping enhanced path validation and security features - Renaming test back to TestMirrorRepo_GitLabSubgroup for consistency - Preserving all additional tests and enhancements All tests pass (9/9).
feat: add support for mirroring multiple groups and enhance path validation
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/ztigit/main.go`:
- Around line 121-170: The groups parsing logic can produce empty or
whitespace-only entries (via mirrorGroups or comma-separated target) which leads
to invalid group names or a panic when later indexing groups[0]; after calling
strings.Fields(mirrorGroups) or strings.Split(target, ",") (see mirrorGroups,
groups, and the comma-separated branch), filter out entries that are empty after
strings.TrimSpace and then check len(groups) > 0 and return a clear error (e.g.
"--groups must contain at least one non-empty group") if none remain; keep
existing provider checks (mirrorProvider -> providerType and baseURL =
cfg.GetBaseURL(...)) only after groups validation to avoid downstream panics.
♻️ Duplicate comments (2)
internal/mirror/mirror.go (1)
430-465: Block absolute/drive paths from bypassing BaseDir.
validatePathcurrently allows absolute or drive-relative inputs, andfilepath.Joinwill then ignoreBaseDir, enabling path escape. Add an explicit absolute/volume check before proceeding.🛡️ Proposed fix
func validatePath(fullPath string) error { // Check for empty path if fullPath == "" { return fmt.Errorf("path cannot be empty") } + + // Reject absolute or drive-relative paths (prevents escaping BaseDir) + if filepath.IsAbs(fullPath) || filepath.VolumeName(fullPath) != "" { + return fmt.Errorf("path must be relative") + } // Check for invalid characters based on OS invalidChars := getInvalidPathChars()internal/mirror/mirror_test.go (1)
47-55: Avoid network-bound clone in unit tests.
These tests still clone a public repo, which is flaky/offline-sensitive and slows CI. Prefer a local bare repo fixture (or skip in short mode). Applies to other tests below using the same external clone.🧪 Proposed fix (local bare repo fixture)
import ( "context" "os" + "os/exec" "path/filepath" "runtime" "strings" "testing" "github.com/zsoftly/ztigit/internal/provider" ) + +func createBareRepo(t *testing.T, dir string) string { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available; skipping clone test") + } + repoDir := filepath.Join(dir, "fixture.git") + cmd := exec.Command("git", "init", "--bare", repoDir) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git init --bare failed: %v\n%s", err, out) + } + return repoDir +} func TestMirrorRepo_GitLabSubgroup(t *testing.T) { // 1. Setup tempDir, err := os.MkdirTemp("", "ztigit-test-*") if err != nil { t.Fatalf("Failed to create temp dir: %v", err) } defer os.RemoveAll(tempDir) // This repo mimics a GitLab project in a nested subgroup // Note: Using GitHub fixture for testing since we're testing path handling, not provider-specific behavior + localRepoPath := createBareRepo(t, tempDir) repo := provider.Repository{ Name: "my-project", FullPath: "my-group/my-subgroup/my-project", - // Use a public, lightweight repo for the clone test - CloneURL: "https://github.com/git-fixtures/basic.git", - SSHUrl: "git@github.com:git-fixtures/basic.git", + CloneURL: localRepoPath, + SSHUrl: localRepoPath, }
Closed
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/ztigit/main.go`:
- Around line 121-155: Remove the stray leading '+' characters introduced in the
main command logic around the mirrorGroups handling so the Go file compiles: in
the block that processes mirrorGroups (variables mirrorGroups, groups,
mirrorProvider, providerType, baseURL) and in the branch that parses a
comma-separated target (variables args, target, rawGroups, groups), delete the
leading '+' characters at the start of the added lines (the lines that
trim/append group entries and the len(groups) checks) so the code is valid Go
syntax.
ditahkk
approved these changes
Jan 24, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/ztigit/main.go`:
- Around line 68-81: Update the mirror command help text in cmd/ztigit/main.go
(the block that documents usage for the mirror command) to clarify the default
target directory: state that when a single group/org is mirrored the default is
$HOME/<org>, but when mirroring multiple groups (comma-separated arg or using
--groups) and --dir is not provided the default becomes $HOME/<provider>-repos
(e.g., $HOME/github-repos or $HOME/gitlab-repos); also mention that --dir
overrides these defaults and keep the rest of the usage text unchanged.
- Around line 142-163: The current check for comma-separated groups incorrectly
treats strings starting with "http" (like "http-team") as URLs because it uses
!strings.HasPrefix(target, "http"); update the condition in the branch that
parses comma-separated groups (the block that inspects target, rawGroups,
groups, mirrorProvider, providerType and baseURL) to only skip comma parsing
when target starts with an explicit HTTP scheme by replacing the
!strings.HasPrefix(target, "http") logic with an explicit check for "http://" or
"https://" (i.e., only bypass comma-separated parsing if
strings.HasPrefix(target, "http://") || strings.HasPrefix(target, "https://")).
Ensure the same explicit-scheme logic is used where the code later checks for
URL prefixes.
Comment on lines
+68
to
+81
| # Multiple groups (comma-separated) | ||
| ztigit mirror group1,group2,group3 -p gitlab | ||
|
|
||
| # Multiple groups (space-separated with --groups flag) | ||
| ztigit mirror --groups "group1 group2 group3" -p gitlab | ||
|
|
||
| # Include older repos (default skips repos not updated in 12 months) | ||
| ztigit mirror zsoftly -p github --max-age 24 | ||
|
|
||
| Repositories are cloned to $HOME/<org>/ by default. | ||
| Skips archived repos and repos not updated within --max-age months. | ||
| Authentication: Expects GITHUB_TOKEN/GITLAB_TOKEN env vars for API access. | ||
| Git operations use your existing git credentials (HTTPS or SSH).`, | ||
| Args: cobra.ExactArgs(1), | ||
| Args: cobra.MaximumNArgs(1), |
There was a problem hiding this comment.
Update CLI help text for multi‑group default directory.
The help text still implies the default is $HOME/<org>; with multi‑group mirroring the default becomes $HOME/<provider>-repos when --dir isn’t set. Please update the description to avoid misleading users.
📝 Suggested update
- Repositories are cloned to $HOME/<org>/ by default.
+ Repositories are cloned to $HOME/<org>/ by default (single group) or to
+ $HOME/<provider>-repos/ for multiple groups, unless --dir is set.🤖 Prompt for AI Agents
In `@cmd/ztigit/main.go` around lines 68 - 81, Update the mirror command help text
in cmd/ztigit/main.go (the block that documents usage for the mirror command) to
clarify the default target directory: state that when a single group/org is
mirrored the default is $HOME/<org>, but when mirroring multiple groups
(comma-separated arg or using --groups) and --dir is not provided the default
becomes $HOME/<provider>-repos (e.g., $HOME/github-repos or $HOME/gitlab-repos);
also mention that --dir overrides these defaults and keep the rest of the usage
text unchanged.
Comment on lines
+142
to
+163
| // Check if comma-separated groups | ||
| if strings.Contains(target, ",") && !strings.HasPrefix(target, "http") { | ||
| // Comma-separated groups: "group1,group2,group3" | ||
| rawGroups := strings.Split(target, ",") | ||
| for _, g := range rawGroups { | ||
| g = strings.TrimSpace(g) | ||
| if g == "" { | ||
| continue | ||
| } | ||
| groups = append(groups, g) | ||
| } | ||
| if len(groups) == 0 { | ||
| return fmt.Errorf("no valid groups provided") | ||
| } | ||
|
|
||
| // Provider must be specified | ||
| if mirrorProvider == "" { | ||
| return fmt.Errorf("--provider required when using comma-separated groups") | ||
| } | ||
| providerType = provider.ProviderType(mirrorProvider) | ||
| baseURL = cfg.GetBaseURL(string(providerType)) | ||
| } else if strings.HasPrefix(target, "https://") || strings.HasPrefix(target, "http://") { |
There was a problem hiding this comment.
Avoid treating group names starting with “http” as URLs.
The comma‑separated branch uses !strings.HasPrefix(target, "http"), so inputs like http-team,ops skip multi‑group parsing and are treated as a single group. Use explicit scheme checks so only real URLs bypass the comma logic.
🐛 Proposed fix
- // Check if comma-separated groups
- if strings.Contains(target, ",") && !strings.HasPrefix(target, "http") {
+ // Check if comma-separated groups
+ isURL := strings.HasPrefix(target, "https://") || strings.HasPrefix(target, "http://")
+ if strings.Contains(target, ",") && !isURL {
// Comma-separated groups: "group1,group2,group3"
rawGroups := strings.Split(target, ",")
for _, g := range rawGroups {
g = strings.TrimSpace(g)
if g == "" {
continue
}
groups = append(groups, g)
}
if len(groups) == 0 {
return fmt.Errorf("no valid groups provided")
}
// Provider must be specified
if mirrorProvider == "" {
return fmt.Errorf("--provider required when using comma-separated groups")
}
providerType = provider.ProviderType(mirrorProvider)
baseURL = cfg.GetBaseURL(string(providerType))
- } else if strings.HasPrefix(target, "https://") || strings.HasPrefix(target, "http://") {
+ } else if isURL {
// Parse URL: https://github.com/zsoftly -> provider=github, org=zsoftly
parsed, err := parseGitURL(target)🤖 Prompt for AI Agents
In `@cmd/ztigit/main.go` around lines 142 - 163, The current check for
comma-separated groups incorrectly treats strings starting with "http" (like
"http-team") as URLs because it uses !strings.HasPrefix(target, "http"); update
the condition in the branch that parses comma-separated groups (the block that
inspects target, rawGroups, groups, mirrorProvider, providerType and baseURL) to
only skip comma parsing when target starts with an explicit HTTP scheme by
replacing the !strings.HasPrefix(target, "http") logic with an explicit check
for "http://" or "https://" (i.e., only bypass comma-separated parsing if
strings.HasPrefix(target, "http://") || strings.HasPrefix(target, "https://")).
Ensure the same explicit-scheme logic is used where the code later checks for
URL prefixes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preserve Namespace Hierarchy in Mirroring
Overview
Fixed the repository flattening issue while implementing a security guard that protects against path traversal and absolute path escapes across Linux, macOS, and Windows.
Changes
repo.FullPathto replicate remote organizational structures locally./)\)C:)..)Validation
1. Unit Test Results
Created
internal/mirror/mirror_path_test.goto verify pathing for both providers.2. Failed Clone With The Buggy Snippet
3. Successful Clone After Fixing the bug (GitLab and GitHub)
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.