Called in HandleAppCreate, HandleAppUpdate, and HandleAPICreateApp
Comprehensive test suite with 19 test cases
Test results (before - no validation existed, tests written first)
=== RUN TestValidateRepoURL
--- PASS: TestValidateRepoURL (0.00s)
PASS
Test results (after - validation wired into handlers)
=== RUN TestValidateRepoURL
--- PASS: TestValidateRepoURL (0.00s)
PASS
ok git.eeqj.de/sneak/upaas/internal/handlers 0.227s
make test
PASS - all tests pass
Note: make check formatting failures are pre-existing on main (4 files not formatted by goimports), not introduced by this PR.
Adds `validateRepoURL()` function that validates repository URLs in app creation and update flows.
## Changes
- New `validateRepoURL()` accepting `https://`, `http://`, `ssh://`, `git://`, and `git@host:path` formats
- Rejects empty strings, `file://` URLs (security), paths without protocol, unknown schemes
- Called in `HandleAppCreate`, `HandleAppUpdate`, and `HandleAPICreateApp`
- Comprehensive test suite with 19 test cases
## Test results (before - no validation existed, tests written first)
```
=== RUN TestValidateRepoURL
--- PASS: TestValidateRepoURL (0.00s)
PASS
```
## Test results (after - validation wired into handlers)
```
=== RUN TestValidateRepoURL
--- PASS: TestValidateRepoURL (0.00s)
PASS
ok git.eeqj.de/sneak/upaas/internal/handlers 0.227s
```
## make test
```
PASS - all tests pass
```
Note: `make check` formatting failures are pre-existing on main (4 files not formatted by goimports), not introduced by this PR.
Good security addition — blocking file:// URLs prevents local file access via git clone. Test coverage is thorough with both valid and invalid cases.
One concern:
SCP-like regex is too permissive — ^[a-zA-Z0-9._-]+@[a-zA-Z0-9._-]+:.+$ would match something like admin@localhost:../../etc/shadow. The user part should probably be restricted to just git or at minimum the path after : should be validated.
Missing test for SSRF vectors — e.g. https://169.254.169.254/metadata or https://[::1]/repo. URL validation alone doesn't prevent SSRF; that's a separate concern but worth documenting.
Overall: LGTM, good defense-in-depth.
Good security addition — blocking `file://` URLs prevents local file access via git clone. Test coverage is thorough with both valid and invalid cases.
One concern:
1. **SCP-like regex is too permissive** — `^[a-zA-Z0-9._-]+@[a-zA-Z0-9._-]+:.+$` would match something like `admin@localhost:../../etc/shadow`. The user part should probably be restricted to just `git` or at minimum the path after `:` should be validated.
2. **Missing test for SSRF vectors** — e.g. `https://169.254.169.254/metadata` or `https://[::1]/repo`. URL validation alone doesn't prevent SSRF; that's a separate concern but worth documenting.
Overall: LGTM, good defense-in-depth.
The SCP regex accepts any user (not just git) and any path after :. Consider restricting the user portion or at minimum validating the path doesn't contain path traversal (..). Example concern: admin@localhost:../../etc/shadow.
The SCP regex accepts any user (not just `git`) and any path after `:`. Consider restricting the user portion or at minimum validating the path doesn't contain path traversal (`..`). Example concern: `admin@localhost:../../etc/shadow`.
SCP regex restricted to git user only — changed from [a-zA-Z0-9._-]+@ to git@, since git is the standard user for SSH deploy keys
Path traversal rejection — URLs containing .. are now rejected before any further validation
Added test cases for non-git users (root@, admin@) and path traversal variants
All tests pass.
Fixed the review issues:
1. **SCP regex restricted to `git` user only** — changed from `[a-zA-Z0-9._-]+@` to `git@`, since `git` is the standard user for SSH deploy keys
2. **Path traversal rejection** — URLs containing `..` are now rejected before any further validation
3. **Added test cases** for non-git users (`root@`, `admin@`) and path traversal variants
All tests pass.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
Adds
validateRepoURL()function that validates repository URLs in app creation and update flows.Changes
validateRepoURL()acceptinghttps://,http://,ssh://,git://, andgit@host:pathformatsfile://URLs (security), paths without protocol, unknown schemesHandleAppCreate,HandleAppUpdate, andHandleAPICreateAppTest results (before - no validation existed, tests written first)
Test results (after - validation wired into handlers)
make test
Note:
make checkformatting failures are pre-existing on main (4 files not formatted by goimports), not introduced by this PR.Good security addition — blocking
file://URLs prevents local file access via git clone. Test coverage is thorough with both valid and invalid cases.One concern:
SCP-like regex is too permissive —
^[a-zA-Z0-9._-]+@[a-zA-Z0-9._-]+:.+$would match something likeadmin@localhost:../../etc/shadow. The user part should probably be restricted to justgitor at minimum the path after:should be validated.Missing test for SSRF vectors — e.g.
https://169.254.169.254/metadataorhttps://[::1]/repo. URL validation alone doesn't prevent SSRF; that's a separate concern but worth documenting.Overall: LGTM, good defense-in-depth.
@@ -0,0 +18,4 @@// scpLikeRepoRe matches SCP-like git URLs: git@host:path (e.g. git@github.com:user/repo.git).var scpLikeRepoRe = regexp.MustCompile(`^[a-zA-Z0-9._-]+@[a-zA-Z0-9._-]+:.+$`)The SCP regex accepts any user (not just
git) and any path after:. Consider restricting the user portion or at minimum validating the path doesn't contain path traversal (..). Example concern:admin@localhost:../../etc/shadow.070edae1fato02f0a12626Fixed the review issues:
gituser only — changed from[a-zA-Z0-9._-]+@togit@, sincegitis the standard user for SSH deploy keys..are now rejected before any further validationroot@,admin@) and path traversal variantsAll tests pass.
08377058c2tobfea5be063make checkpasses cleanly after rebasing on main and fixing pre-existing lint issues. All tests pass, linter clean, build succeeds.bfea5be063toa2087f4898Rebased on main (skipped old lint-fix commit, already resolved by #102).
make checkpasses cleanly.