fix(repository): support per-submodule git credentials - #4136
Open
Arielgordon123 wants to merge 2 commits into
Open
fix(repository): support per-submodule git credentials#4136Arielgordon123 wants to merge 2 commits into
Arielgordon123 wants to merge 2 commits into
Conversation
Submodule clones previously reused only the main repository's single access key, so submodules hosted on a different server/credentials than the main repo failed with "could not read Username ... terminal prompts disabled" (semaphoreui#4134). This affected both git client backends (cmd_git and go_git). Adds a per-repository list of host -> access key mappings (RepositorySubmoduleCredential) that both git clients consult when cloning/updating each submodule individually, falling back to the repository's own key when a submodule's host has no explicit mapping. Wired through the full HA remote-runner path so it also works when tasks are dispatched to remote runners, plus a new "Submodule Credentials" section on the Repository edit form.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds repository-scoped submodule credential storage and management. Credentials flow through inventory and task execution into command-line Git and go-git clients. The repository form supports credential configuration. ChangesSubmodule credential persistence and API
Task credential propagation
Credential-aware Git operations
Repository configuration UI
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RepositoryForm
participant RepositorySubmoduleCredentials
participant API
participant TaskRunner
participant LocalExecutor
participant GitClient
RepositoryForm->>RepositorySubmoduleCredentials: configure host and access key
RepositorySubmoduleCredentials->>API: persist repository submodule credential
TaskRunner->>LocalExecutor: pass decrypted credentials
LocalExecutor->>GitClient: clone or pull repository
GitClient->>GitClient: resolve credential by submodule host
GitClient-->>LocalExecutor: update recursive submodules
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
db_lib/git_url_host_test.go (1)
39-71: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for the host-plus-port mismatch.
resolveSubmoduleAccessKeymatcheshost[:port]exactly. A credential configured forgitserverdoes not match a submodule URL ofhttps://gitserver:8443/.... Add a subtest for that case to lock in the intended behavior.♻️ Proposed test addition
t.Run("falls back to main key when no credentials configured", func(t *testing.T) { got := resolveSubmoduleAccessKey(mainKey, nil, "https://gitserver/group/submodule1") assert.Equal(t, mainKey.ID, got.ID) }) + + t.Run("does not match when the url carries a port and the credential does not", func(t *testing.T) { + got := resolveSubmoduleAccessKey(mainKey, creds, "https://gitserver:8443/group/submodule1") + assert.Equal(t, mainKey.ID, got.ID) + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db_lib/git_url_host_test.go` around lines 39 - 71, Add a subtest to TestResolveSubmoduleAccessKey covering a credential configured for “gitserver” and a submodule URL using “gitserver:8443”; assert that resolveSubmoduleAccessKey returns the main key because host-plus-port matching is exact.db_lib/submodule_credentials_test.go (1)
172-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the cause of the failure, not only that a failure happened.
Both "unmatched" tests accept any error. A broken fixture, a missing
gitbinary, or a wrong branch name also produces an error, so the tests can pass while the reproduction no longer holds. Assert that the error text names the submodule path or an authentication failure.Also applies to: 229-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db_lib/submodule_credentials_test.go` around lines 172 - 173, Update both unmatched-credentials tests around gitRepo.Clone() to assert that the returned error identifies the expected submodule path or indicates an authentication failure, rather than only checking assert.Error. Preserve the existing failure assertion while validating the error cause in each affected test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db_lib/CmdGitClient.go`:
- Around line 298-309: Limit recursive submodule traversal in both
updateSubmodules implementations by threading a depth counter through each
recursive call and returning an error when a fixed maximum depth is reached.
Apply this change in db_lib/CmdGitClient.go at lines 298-309 and
db_lib/GoGitClient.go at lines 183-208, preserving normal submodule updates
below the limit.
- Around line 286-296: Update the submodule URL configuration in the loop around
resolveSubmoduleAccessKey and runInDir so git config always receives a
credential-free URL. Move authentication to a short-lived GIT_ASKPASS helper or
temporary credential file scoped to the init/update operation, ensuring
credentials are not persisted in .git/config and are cleaned up afterward.
In `@db_lib/git_url_host.go`:
- Around line 48-58: Correct the doc comment for resolveSubmoduleAccessKey to
remove the inaccurate claim that credentials are never sent to unauthorized
hosts. Document that unmatched or hostless submodule URLs fall back to mainKey,
while preserving the existing implementation and its recursive-clone behavior.
In `@db_lib/submodule_credentials_test.go`:
- Around line 47-54: Update lastCommitHash to remove the trailing newline from
git rev-parse HEAD output using strings.TrimSpace instead of slicing to 40
bytes. Preserve the full hash for both SHA-1 and SHA-256 repositories and avoid
panics on shorter output; add the required strings import.
In `@db/RepositorySubmoduleCredential.go`:
- Around line 31-38: Update RepositorySubmoduleCredential.Validate to accept
only non-empty hostname-only Host values: reject URLs, paths, ports, queries,
fragments, wildcard values, and any leading or trailing whitespace. Preserve the
existing AccessKeyID validation, and add API tests covering each invalid host
form.
In `@services/tasks/local_executor.go`:
- Around line 1100-1106: Update GetRepositoryPlaybooks and cloneInventoryRepo to
load the repository’s corresponding credential mappings and set
SubmoduleCredentials when constructing GitRepository values. Ensure every
Clone/Pull path receives credentials, while leaving checkoutRepository unchanged
because it does not perform submodule updates.
In `@web/src/components/RepositorySubmoduleCredentials.vue`:
- Around line 150-152: Update RepositorySubmoduleCredentials so errors assigned
by loadCredentials() and deleteCredential() remain visible when editDialog is
closed: render formError in an alert near the credential list or route it
through the existing global notification mechanism, while preserving the current
dialog error display for errors occurring within the editor.
- Around line 22-27: Update the host validation in
RepositorySubmoduleCredentials to require a canonical hostname rather than
merely a non-empty value, rejecting schemes, paths, and other URL-form inputs
such as https://gitserver.example.com/org/repo. Normalize the accepted hostname
consistently with the API before saving so credential mappings match the exact
hostname extracted from submodule Git URLs.
---
Nitpick comments:
In `@db_lib/git_url_host_test.go`:
- Around line 39-71: Add a subtest to TestResolveSubmoduleAccessKey covering a
credential configured for “gitserver” and a submodule URL using
“gitserver:8443”; assert that resolveSubmoduleAccessKey returns the main key
because host-plus-port matching is exact.
In `@db_lib/submodule_credentials_test.go`:
- Around line 172-173: Update both unmatched-credentials tests around
gitRepo.Clone() to assert that the returned error identifies the expected
submodule path or indicates an authentication failure, rather than only checking
assert.Error. Preserve the existing failure assertion while validating the error
cause in each affected test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 53912cbb-d567-4a77-845f-1f584a27dcc5
📒 Files selected for processing (26)
api/projects/repository_submodule_credential.goapi/projects/repository_submodule_credential_test.goapi/router.goapi/runners/runners.godb/Migration.godb/RepositorySubmoduleCredential.godb/Store.godb/sql/migrations/v2.20.2.err.sqldb/sql/migrations/v2.20.2.sqldb/sql/repository_submodule_credential.godb_lib/CmdGitClient.godb_lib/GitRepository.godb_lib/GoGitClient.godb_lib/git_url_host.godb_lib/git_url_host_test.godb_lib/submodule_credentials_test.goservices/runners/executor_factory.goservices/runners/types.goservices/tasks/TaskPool.goservices/tasks/TaskRunner.goservices/tasks/executor.goservices/tasks/local_executor.goservices/tasks/local_executor_provider.goweb/src/components/RepositoryForm.vueweb/src/components/RepositorySubmoduleCredentials.vueweb/src/lang/en.js
Comment on lines
+286
to
+296
| for _, sm := range submodules { | ||
| key := resolveSubmoduleAccessKey(r.Repository.SSHKey, r.SubmoduleCredentials, sm.URL) | ||
|
|
||
| // Override the submodule's local URL before init/update runs, so an | ||
| // HTTPS credential is embedded exactly like the main repository's | ||
| // GetGitURL(false) does -- never touching the tracked .gitmodules file. | ||
| effectiveURL := gitURLWithCredentials(sm.URL, key) | ||
| if err := c.runInDir(r, dir, r.Repository.SSHKey, | ||
| "config", "submodule."+sm.Name+".url", effectiveURL); err != nil { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how LogCmd renders arguments and whether any redaction exists.
rg -nP -C 8 'func .*LogCmd' --type=go
rg -n -e 'redact' -e 'GIT_ASKPASS' -e 'credential' --type=go -g '!**/*_test.go' | head -n 40Repository: semaphoreui/semaphore
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(CmdGitClient|.*Logger|.*log.*)\.(go)$' | head -n 80
printf '%s\n' '--- symbols and credential handling ---'
rg -n -C 12 'LogCmd|runInDir|gitURLWithCredentials|updateSubmodules|submodule\..*\.url|effectiveURL|GIT_ASKPASS|credential' --type=go .
printf '%s\n' '--- target file sections ---'
wc -l db_lib/CmdGitClient.go
sed -n '90,155p;270,370p' db_lib/CmdGitClient.goRepository: semaphoreui/semaphore
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -e
rg -n -C 15 'LogCmd' --glob '*.go' .
rg -n -C 10 'type .*Logger|interface .*Logger|func .*Log|LogCmd' --glob '*.go' .
rg -n -C 8 'func \(.*\) run|func .*runInDir|exec\.Command|CommandContext|CombinedOutput|Stdout|Stderr' db_lib --glob '*.go'Repository: semaphoreui/semaphore
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -e
rg -n -C 12 'updateSubmodules\(|Clone\(|Pull\(|GetFullPath\(|SubmoduleCredentials|SSHKey' --glob '*.go' .
rg -n -C 15 'func gitURLWithCredentials|func resolveSubmoduleAccessKey|type .*Submodule|SubmoduleCredentials' --glob '*.go' .Repository: semaphoreui/semaphore
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- repository layout and target references ---'
git ls-files db_lib | head -n 80
printf '%s\n' '--- target implementation ---'
sed -n '1,160p' db_lib/CmdGitClient.go
sed -n '160,380p' db_lib/CmdGitClient.goRepository: semaphoreui/semaphore
Length of output: 11464
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target implementation ---'
sed -n '1,160p' db_lib/CmdGitClient.go
sed -n '160,380p' db_lib/CmdGitClient.go
printf '%s\n' '--- logger and command execution references ---'
rg -n -C 12 'LogCmd|runInDir|exec\.Command|CommandContext|CombinedOutput' --glob '*.go' .Repository: semaphoreui/semaphore
Length of output: 47130
🏁 Script executed:
#!/bin/bash
set -e
sed -n '90,155p;270,370p' db_lib/CmdGitClient.go
rg -n -C 12 'LogCmd|runInDir|exec\.Command|CommandContext' --glob '*.go' .
rg -n -C 8 'gitURLWithCredentials|resolveSubmoduleAccessKey|updateSubmodules|GetFullPath' --glob '*.go' .Repository: semaphoreui/semaphore
Length of output: 50377
Do not persist the password in .git/config.
effectiveURL contains HTTPS credentials, and git config submodule.<name>.url stores them in the shared working copy. Keep the configured URL credential-free and use a short-lived GIT_ASKPASS helper or credential file instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@db_lib/CmdGitClient.go` around lines 286 - 296, Update the submodule URL
configuration in the loop around resolveSubmoduleAccessKey and runInDir so git
config always receives a credential-free URL. Move authentication to a
short-lived GIT_ASKPASS helper or temporary credential file scoped to the
init/update operation, ensuring credentials are not persisted in .git/config and
are cleaned up afterward.
Comment on lines
1100
to
+1106
| func (t *LocalExecutor) updateRepository() error { | ||
| repo := db_lib.GitRepository{ | ||
| Logger: t.Logger, | ||
| TemplateID: t.Template.ID, | ||
| Repository: t.Repository, | ||
| Client: db_lib.CreateDefaultGitClient(t.KeyInstaller), | ||
| Logger: t.Logger, | ||
| TemplateID: t.Template.ID, | ||
| Repository: t.Repository, | ||
| SubmoduleCredentials: t.SubmoduleCredentials, | ||
| Client: db_lib.CreateDefaultGitClient(t.KeyInstaller), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List all GitRepository composite literals and check for the SubmoduleCredentials field.
ast-grep run --pattern 'GitRepository{$$$}' --lang go .
rg -nP -C 6 '\bGitRepository\s*\{' --type=go -g '!**/*_test.go'Repository: semaphoreui/semaphore
Length of output: 3865
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- GitRepository constructions ---'
rg -n -P -C 8 '\b(?:db_lib\.)?GitRepository\s*\{' --type go -g '!**/*_test.go' .
printf '%s\n' '--- LocalExecutor methods and GitRepository references ---'
rg -n -P -C 10 '\bGitRepository\b|SubmoduleCredentials|func \(t \*LocalExecutor\)' services/tasks/local_executor.go
printf '%s\n' '--- GitRepository definition and credential use ---'
rg -n -P -C 12 'type GitRepository struct|SubmoduleCredentials|func .*Update|func .*Checkout|func .*Clone' db_lib --type go -g '!**/*_test.go'Repository: semaphoreui/semaphore
Length of output: 37879
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- checkout and repository operation flow ---'
sed -n '1136,1174p' services/tasks/local_executor.go
sed -n '104,220p' db_lib/GoGitClient.go
sed -n '146,320p' db_lib/CmdGitClient.go
printf '%s\n' '--- inventory executor context and initialization ---'
sed -n '1,105p' services/tasks/local_executor_inventory.go
rg -n -P -C 8 'SubmoduleCredentials|LocalExecutor\{|Inventory\s*:' --type go -g '!**/*_test.go' .
printf '%s\n' '--- relevant changed-file summary ---'
git diff --stat
git diff -- services/tasks/local_executor.go services/tasks/local_executor_inventory.go db_lib/GitRepository.go db_lib/GoGitClient.go db_lib/CmdGitClient.goRepository: semaphoreui/semaphore
Length of output: 50035
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- API repository operation bodies ---'
sed -n '55,135p' api/projects/repository.go
sed -n '1,120p' services/schedules/SchedulePool.go
printf '%s\n' '--- Static operation classification for every non-test literal ---'
python3 - <<'PY'
from pathlib import Path
import re
files = [p for p in Path('.').rglob('*.go') if '_test.go' not in p.name]
literal = re.compile(r'\b(?:db_lib\.)?GitRepository\s*\{')
func = re.compile(r'func\s+(?:\([^)]*\)\s*)?([A-Za-z_]\w*)\s*\(')
for path in files:
text = path.read_text(errors='replace')
for m in literal.finditer(text):
start = m.start()
line = text.count('\n', 0, start) + 1
# Find the enclosing function by the last function declaration.
fm = list(func.finditer(text, 0, start))
name = fm[-1].group(1) if fm else '<package scope>'
# Extract the composite literal with a brace counter.
brace = text.find('{', m.start())
depth = 0
end = brace
while end < len(text):
if text[end] == '{':
depth += 1
elif text[end] == '}':
depth -= 1
if depth == 0:
end += 1
break
end += 1
literal_text = text[m.start():end]
# Inspect the enclosing function body for direct calls on the conventional repo variable.
body_start = text.find('{', fm[-1].start()) if fm else 0
body_end = len(text)
if fm:
depth = 0
for i in range(body_start, len(text)):
if text[i] == '{':
depth += 1
elif text[i] == '}':
depth -= 1
if depth == 0:
body_end = i
break
body = text[body_start:body_end]
ops = sorted(set(re.findall(r'\brepo\.(Clone|Pull|Checkout|GetLastRemoteCommitHash|GetRemoteBranches|GetLastCommitHash|GetLastCommitMessage|CanBePulled)\s*\(', body)))
print(f'{path}:{line} function={name} credentials={"yes" if "SubmoduleCredentials:" in literal_text else "no"} operations={",".join(ops) or "none"}')
PYRepository: semaphoreui/semaphore
Length of output: 6154
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- RepositoryController dependencies and construction ---'
sed -n '1,65p' api/projects/repository.go
rg -n -P -C 8 'NewRepositoryController|RepositoryController\{' api --type go
printf '%s\n' '--- Git operations that update submodules ---'
rg -n -P -C 5 'func \(c (GoGitClient|CmdGitClient)\) (Clone|Pull|Checkout)|updateSubmodules\(' db_lib/GoGitClient.go db_lib/CmdGitClient.go
printf '%s\n' '--- Repository credential model and inventory repository loading ---'
rg -n -P -C 8 'type RepositorySubmoduleCredential|type Inventory struct|Repository\s+\*?db?\.Repository|Inventory\.Repository|GetRepositorySubmoduleCredentials' db services api --type go -g '!**/*_test.go'Repository: semaphoreui/semaphore
Length of output: 35306
Propagate credentials to every Clone/Pull path
GetRepositoryPlaybooks and cloneInventoryRepo construct GitRepository values without credentials, although Clone and Pull always update submodules. Load mappings for the corresponding repository and pass them to these constructions. checkoutRepository only checks out or reads commits, so it does not require the field.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/tasks/local_executor.go` around lines 1100 - 1106, Update
GetRepositoryPlaybooks and cloneInventoryRepo to load the repository’s
corresponding credential mappings and set SubmoduleCredentials when constructing
GitRepository values. Ensure every Clone/Pull path receives credentials, while
leaving checkoutRepository unchanged because it does not perform submodule
updates.
…#4136 - Stop persisting embedded submodule HTTPS credentials in .git/config; apply them as a process-local `git -c` override instead. - Bound submodule recursion depth to prevent unbounded traversal/disk fill from a cyclic submodule graph. - Validate submodule credential Host as a bare hostname (optionally with a port), both server-side and in the Vue form. - Propagate submodule credentials to the playbook-browsing and inventory-repository clone/pull paths, including full HA remote-runner wiring for the inventory case. - Fix doc comment, SHA truncation in a test helper, and surface load/delete errors outside the credentials dialog.
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
1 participant
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.
Problem Description
When a repository's
.gitmodulesreferences a submodule hosted on a different git server/repo that needs different credentials than the main repository, cloning fails with:Root cause:
db.Repository/db.AccessKeyonly support one credential per repository, and both git client backends (cmd_gitandgo_git) reuse that single credential for the entire recursive clone.cmd_gitrunsgit clone --recursive/git submodule update --init --recursive, andgo_git'sCloneOptions.RecurseSubmodulespasses the same top-levelAuthinto every submodule update internally — so switchingSEMAPHORE_GIT_CLIENTbetween the two does not fix this.Closes #4134
Solution Implemented
RepositorySubmoduleCredentialentity: a per-repository list of{host, access_key}mappings, with a new migration (v2.20.2), store methods, and REST endpoints under/api/project/:id/repositories/:repoId/submodule_credentials. Creating/updating a mapping validates the access key belongs to the same project (no cross-project IDOR).CmdGitClientandGoGitClientnow clone/update each submodule individually (instead of relying on--recursive/RecurseSubmodules), resolving credentials per submodule by exact hostname match. A submodule whose host has no explicit mapping falls back to the repository's own key — today's behavior is unchanged for existing repos.JobData, access-key hydration,ExecutorProvider), so it also works when tasks are dispatched to remote runners, not just local execution.Verification
db_lib/git_url_host_test.go,db_lib/submodule_credentials_test.go) reproduce the exact reported failure and prove the fix end-to-end for both git backends against a real authenticated git-over-HTTP server.api/projects/repository_submodule_credential_test.go) cover CRUD and the cross-project IDOR rejection.go build ./...,go vet ./..., andgo test ./...all pass; frontend builds and lints clean.Summary by CodeRabbit