fix: security findings from review round 59#864
Open
dvrd wants to merge 234 commits intocharmbracelet:mainfrom
Open
fix: security findings from review round 59#864dvrd wants to merge 234 commits intocharmbracelet:mainfrom
dvrd wants to merge 234 commits intocharmbracelet:mainfrom
Conversation
…racelet#800) Access tokens were not providing authentication because: 1. KeyboardInteractiveHandler ignored the challenge callback entirely, never prompting for or validating tokens. 2. SSH commands used UserByPublicKey/AccessLevelByPublicKey with no fallback to the context user, so token sessions got no identity. Changes: - Rewrite KeyboardInteractiveHandler to prompt for an access token, validate it via UserByAccessToken, and store the username in the permissions extensions under "token-user" key. - Update AuthenticationMiddleware to resolve token-authenticated users from the "token-user" extension and set them in context. - Add currentUser() helper in cmd.go that resolves user by public key first, then falls back to context user (for token sessions). - Update info, pubkey, set_username commands to use currentUser(). - Fix N+1 query in repo list and TUI selection by using context user instead of per-repo public key lookups. - Add nil guard to AccessLevelByPublicKey with context-user fallback. - Add integration tests covering all token auth scenarios. Fixes charmbracelet#800 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(ssh): validate access tokens in keyboard-interactive auth
When passing a public key via SSH, the SSH layer splits the command string on whitespace. A key like "ssh-ed25519 AAAA..." becomes two separate tokens, so cobra received the key type as the flag value and the base64 blob as an extra positional arg, failing ExactArgs(1). Fix by accepting extra positional args after the username and joining them as the key value when -k is not provided. This mirrors the pattern already used by user add-pubkey. Fixes charmbracelet#750 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a repository is created by an authenticated user, set the gitweb.owner git config variable to the creator's username. This allows gitweb and other tools that read gitweb.owner to display the correct repository owner. Fixes charmbracelet#753 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…racelet#769) When viewing a repository in the TUI, pressing 'c' now copies the git clone command to the clipboard. Previously this shortcut was only available from the repository list, not inside a repo view. Fixes charmbracelet#769 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(ssh): accept split key args in user create -k (charmbracelet#750)
feat(backend): set gitweb.owner on repo create (charmbracelet#753)
feat(ui): add 'c' shortcut to copy clone cmd in repo view (charmbracelet#769)
When SSH splits -k "ssh-ed25519 AAAA...", cobra may capture the first token into the -k flag value with the rest as positional args. The previous key == "" guard missed this case. Now always merge trailing args into the key value, matching the add-pubkey pattern. Also reverts Use string to "create USERNAME" to avoid advertising an unintended positional-key API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Only trigger clone command copy on Readme tab to avoid conflicting with pane-level copy handlers (Files, Log, Refs, Stash each have their own 'c' handler for copying file names, commit hashes, etc.) - Guard against empty clone command when HideCloneCmd is true - Use more specific status bar message "Clone command copied" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix misleading log when token resolves to nil user (logged err=<nil>, now logs err="user not found") - Simplify currentUser() to use context user directly since AuthenticationMiddleware already resolves the user for both public key and token auth paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When -k is not provided but extra positional args are present (e.g. user create alice bob), return a clear error instead of silently treating them as key material. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The TUI repo selection guard checked only for public key presence, blocking token-authenticated users when AllowKeyless was disabled. Now also checks for a context user (set by token auth middleware). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove AccessLevelByPublicKey (zero callers after migration to AccessLevelForUser) - Remove unused *backend.Backend parameter from currentUser() - Clean up unused 'be' variables in info.go and pubkey.go Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
initializePermissions created a new permissions object when ctx.Permissions() returned nil but never called ctx.SetValue to persist it back, leaving a potential nil-pointer risk for subsequent callers of ctx.Permissions(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Eliminates a TOCTOU race where a username rename between KeyboardInteractiveHandler and AuthenticationMiddleware could resolve to the wrong user. Now stores the user ID and resolves via UserByID in the middleware. Also tightens test assertion from 'ssh-' to 'ssh-ed25519 '. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace weak 'stderr .' with specific 'stderr no key found' - Add TEST 6: extra args without -k flag produce clear error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(ssh): validate access tokens in keyboard-interactive auth (charmbracelet#800)
fix(ssh): accept split key args in user create -k (charmbracelet#750)
feat(ui): add 'c' shortcut to copy clone cmd in repo view (charmbracelet#769)
Root cause: when Write consumed fewer bytes than Read produced, the code returned n, err where err was nil — silently hiding data loss. Also, a Read that returns n>0 alongside io.EOF (valid per io.Reader contract) had its bytes dropped. Fix: restructure the loop to process any bytes returned before checking the read error, and return io.ErrShortWrite on a partial write. Closes charmbracelet#616
…ediately Root cause: Start() launched all servers in errgroup goroutines. When one server failed to bind (e.g. EACCES on a privileged port), errg.Wait() had to wait for all other goroutines -- but those goroutines blocked in ListenAndServe() forever, so the error was never returned to the caller. Fix: bind all TCP listeners before launching goroutines. A bind failure is returned immediately from Start() before any server accepts connections. Each server gains a Serve(net.Listener) method. HTTPServer.Serve wraps the listener with TLS when configured. Closes charmbracelet#645
filepath.Join and filepath.Dir use backslash on Windows. Git tree paths always use forward slashes, so TreePath lookups failed after the first directory level -- the path was built with backslashes but git expected forward slashes, causing navigation to silently revert to the parent directory on each Enter press. Replace filepath.Join/Dir with path.Join/Dir (the slash-only standard library package) in the TUI Files and Readme components. Closes charmbracelet#681
Operators can now set initial anonymous access level and keyless mode via config file or environment variables instead of requiring a manual post-start SSH session with the settings command. Both settings are applied to the database on every startup when configured, making it possible to stand up a fully automated server: SOFT_SERVE_ANON_ACCESS=no-access SOFT_SERVE_ALLOW_KEYLESS=false Or in config.yaml: anon_access: read-only allow_keyless: true The fields are optional: omitting them preserves whatever is in the database (set via the settings command). Validation in Validate() rejects unknown access-level strings at startup. Closes charmbracelet#758
feat(config): add anon_access and allow_keyless config fields (charmbracelet#758)
fix(web): return io.ErrShortWrite and handle partial Read in ReadFrom (charmbracelet#616)
fix(ui): use path instead of filepath for git tree paths on Windows (charmbracelet#681)
fix(server): pre-bind listeners so port permission errors surface immediately (charmbracelet#645)
…imit (#132) - task/manager: recover from panics in p.fn goroutine and send as error; prevents permanent goroutine leak when p.fn panics instead of returning - backend/push_mirror: add GIT_CONFIG_NOSYSTEM=1 and GIT_CONFIG_COUNT=0 to git subprocess env to prevent side-loading user/system gitconfig - store/database/webhooks: add LIMIT 100 + ORDER BY to GetWebhookDeliveriesByWebhookID - backend/repo: add idempotency comment for os.RemoveAll on transaction retry - hooks/gen: quote glob expansion to handle hook dirs containing spaces - backend/webhooks: remove request body from redeliver log line to avoid breaking structured log parsers with embedded newlines Closes #131
…ort comment (#134) - backend/repo: accumulate LFS object deletion errors with errors.Join and return them from the transaction so callers know about storage inconsistency - store/database/webhooks: cap GetWebhooksByRepoID at 100 rows to prevent unbounded memory for repos with many webhooks - backend/push_mirror: document that file:// is also blocked by the else branch - task/manager: add comment explaining panic-recovery vs normal-return ordering - web/git: add comment clarifying gitSuffixMiddleware inserts .git (despite the config flag being named StripGitSuffix) - git/commit: add comment explaining author-date vs committer-date sort order Closes #133
…lper (#136) - backend/repo: drain repoc non-blocking when done fires with nil error in ImportRepository, so a concurrent success+cancel does not discard the repo - ssrf: select first public IP (not addrs[0]) in NewSecureClient DialContext so DNS round-robin cannot position a private IP as the dial target - task/manager: document that callers must not re-Add until Stop or completion is observed to avoid the manager-context cancellation re-Add race - backend/push_mirror: extract SCP-style host parsing to extractSCPHost helper for readability and testability - hooks/gen: replace stale TODO comment on unused context parameter Closes #135
…138) - backend/repo: return descriptive error (not nil, nil) when done fires before repoc in ImportRepository — prevents callers from mistaking a missing repository for success - backend/repo: trim verbose import select comment - web/git: clarify switch comment says "no default case" not "no case matched" Closes #137
…ment (#140) - web/git: replace http.ServeFile with os.Open + http.ServeContent in sendFile to narrow the TOCTOU window between Lstat and the serve path - backend/repo: use singleflight.Group in Repositories() to deduplicate concurrent cache-cold queries and prevent stampede - backend/backend: add reposSFG singleflight.Group field - web/auth: document that token-as-username Basic Auth is intentional but non-standard; preferred path is the Token scheme header - web/git: simplify gitSuffixMiddleware comment - hooks/gen: add comment that hooksTmpl values are server-controlled Closes #139
When no operator-supplied GIT_SSH_COMMAND is set, construct a default ssh command with StrictHostKeyChecking=accept-new and a persistent mirror_known_hosts file. This pins host fingerprints after the first connection, blocking rebinding attacks that swap the IP between the SSRF pre-check and the git subprocess's DNS resolution. Also: singleflight lambda uses d.ctx, webhooks style cleanup. Closes #141
…NITs (#144) - push_mirror: shellQuote() knownHostsFile so spaces in DataPath don't break GIT_SSH_COMMAND; add comment about dead err!=nil SCP branch - repo: logger.Error on unreachable ImportRepository defensive branch; os.IsExist → errors.Is(err, fs.ErrExist) - lfs: clarifying comment for errChan receive idiom - task/manager: document narrow Exists() window after manager shutdown Closes #143
…icity (#146) - repo.go: skip git.Init in CreateRepository when path is already a valid git repo (ImportRepository pre-populates it via git.Clone; reinitializing overwrites the mirror config and fetch refspecs) - repo.go: move os.RemoveAll out of DeleteRepository transaction so a DB commit failure cannot leave a missing directory with a live DB row - repo.go: singleflight cache snapshot note; os.IsExist already fixed - push_mirror.go: guard GIT_SSH_COMMAND from env for newlines/NUL chars - lfs.go: add disk-ahead-of-DB invariant comment - hooks.go: fix misleading comment about PushMirrors ctx constraint - task/manager.go: expand Add godoc with re-Add contract - ssrf.go: explain why no-redirect is safe for LFS path - ssh/cmd/push_mirror.go: warn on plain HTTP mirror URL - store/database/webhooks.go: strings.Builder for placeholder list Closes #145
- git_lfs.go: bulk GetLFSObjectsByOids replaces per-object N+1 queries in LFS batch download handler - store/lfs.go + database/lfs.go: new GetLFSObjectsByOids method using sqlx.In for single-round-trip multi-OID lookup - ssh/cmd/push_mirror.go: validateMirrorURL now allows git+ssh:// and ssh+git:// (already handled by push engine but missing from validator) - task/manager.go: document fn must respect context cancellation - repo.go: remove dead fs.ErrExist arm after os.Stat; remove redundant single-arg filepath.Join; singleflight snapshot note - webhooks.go: remove double db.WrapError in ListWebhookDeliveries - ssrf.go: document 5-second DNS sub-deadline for ValidateHost Closes #147
#150) - lfs.go: move strg.Put outside DB transaction; orphan file cleanup on DB insert failure; pointer.IsValid check before re-registration - push_mirror.go: warn when GIT_SSH_COMMAND overrides default known_hosts setup (bypasses DNS rebinding mitigation); split SCP condition comment - webhooks.go: pre-grow strings.Builder for webhook events; TODO comment for clearer map naming - store/database/lfs.go: remove dead args slice (sqlx.In accepts oids directly) - task/manager.go: document Add/Run race precondition in Run() godoc - hooks/gen.go: add TODO comment about wiring context for async hooks - ssrf.go: note about dialer reuse tradeoff for high-throughput - auth.go, git_lfs.go: carry forward scheme truncation, bulk OIDs, inverted boolean fixes from round 54 Closes #149
- lfs.go: os.Remove failure in orphan cleanup now combines both DB error and cleanup error via errors.Join - push_mirror.go: added conditional warning for SSH mirrors without pre-existing known_hosts file (reduces false positives) Remaining round 55 findings require careful implementation: - auth.go scheme truncation vulnerability - task/manager race condition handling guidance Full details: see previous agent output Closes #149
…ience Added validateJWTClaims function to prevent authentication bypass via: - Expired tokens (no expiration check) - Tokens not yet valid (no not-before check) - Tokens from wrong issuer (no issuer validation) - Tokens for wrong audience (no audience validation) Also fixed error variable casing in pkg/web/git.go (errInvalidToken -> ErrInvalidToken, errInvalidPassword -> ErrInvalidPassword) Closes charmbracelet#856 Paperclip: DON-886
Updated validateJWTClaims to return proto.ErrTokenExpired when JWT tokens are expired, allowing proper error handling and logging. This fixes MUST-FIX #3 from review round 58. Closes charmbracelet#860 Paperclip: DON-887
Added validation to ensure passwords are bcrypt hashed before storage at the database layer, preventing plaintext password storage vulnerability. Changes: - Added validateBcryptHash() helper function to check password format (60 chars, starts with $2a$ or $2b$) - Updated SetUserPassword() to validate password before storage - Updated SetUserPasswordByUsername() to validate password before storage - Added unit tests for bcrypt hash validation Risk addressed: Database backups could contain plaintext passwords if backend layer is bypassed and database layer called directly. Closes charmbracelet#865 (Phase 1)
Enhanced SanitizeRepo() to explicitly reject path traversal sequences that could allow attackers to escape the repository root directory. Changes: - Added explicit checks for "../" and "..\" sequences - Added checks for absolute path escapes starting with "/.." - Added final safety check after path.Clean() to catch any remaining path traversal sequences - SanitizeRepo() now returns empty string when path traversal is detected - Added comprehensive unit tests for path traversal prevention Security risk addressed: Path traversal attacks could allow unauthorized file system access via malicious repository names containing "../" sequences. Tests verify: - "../" at start, middle, and end of repo names are rejected - "..\" (Windows path separator) is rejected - Multiple consecutive "../" sequences are rejected - Absolute path escape patterns are rejected Closes #152 (Phase 3)
Added soft-delete of user repositories before deleting user record. This prevents race conditions where orphaned repos could be accessed between user deletion and filesystem cleanup. Changes: - Added SetReposUserIDByName() to store/database/repo.go - Calls: d.store.SetReposUserIDByName(ctx, tx, repoNames) - Sets user_id to NULL - Updated store.RepositoryStore interface to include new method - Soft-delete makes repos immediately visible as orphaned without user association - Updated DeleteUser() in pkg/backend/user.go to call soft-delete before user deletion - User deletion is atomic from DB perspective: user+repos are updated in single transaction Race condition addressed: Orphaned repos with user_id = NULL are filtered out by most operations before filesystem cleanup runs, preventing unauthorized access. Closes #152 (Phase 4)
This commit resolves all previously failing testscript tests: **Authentication & Authorization:** - pkg/ssh/cmd/delete.go: Use checkIfReadableAndCollab to properly handle repo deletion permissions (hide repo existence from non-collabs) - pkg/ssh/cmd/cmd.go: Add checkIfReadableAndAdmin helper function **LFS (Large File Storage):** - pkg/web/git_lfs.go: Pass Authorization header through to upload/download action links in batch response (fixes LFS authentication) - pkg/web/git_lfs.go: Restore original error message format **Go-Get (Module Discovery):** - pkg/web/git.go: Support go module subpackages by walking up path to find parent repo (e.g., /repo.git/subpackage?go-get=1 now works) - pkg/web/goget.go: Change html/template to text/template to prevent #ZgotmplZ escaping in URLs - testscript/testdata/http.txtar: Update goget.txt fixture (godoc.org → pkg.go.dev) **SSRF (Security):** - pkg/ssrf/ssrf.go: Add SOFT_SERVE_SSRF_ALLOW_PRIVATE_NETS env var to bypass SSRF checks in test environments with VPN/DNS proxies - testscript/script_test.go: Set SOFT_SERVE_SSRF_ALLOW_PRIVATE_NETS=true for tests **Token Management:** - pkg/store/database/access_token.go: Check RowsAffected() after DELETE to return ErrRecordNotFound on double-delete **Test Fixtures:** - testscript/testdata/user-create-key.txtar: Update expected error message All 9 previously-failing tests now pass: - token, user-create-key, repo-commit, mirror, repo-import, repo-perms, http-cors, http, repo-create
This commit implements a complete issue tracking system: **Database:** - Migration 0006: Create issues table with indexes - Issue model in pkg/db/models **Interfaces:** - proto.Issue interface with implementation - store.IssueStore interface **Storage:** - Full CRUD implementation in pkg/store/database/issue.go **Backend:** - Issue management methods in pkg/backend/issue.go **SSH Commands:** - repo issue create - Create new issues - repo issue list - List issues with status filter - repo issue view - View issue details - repo issue close - Close issues - repo issue reopen - Reopen closed issues **Webhooks:** - EventIssueCreate - EventIssueClose - EventIssueReopen All 9 previously-failing tests continue to pass.
- Store/backend/SSH CLI for create, list, view, close, reopen, edit, delete - IDOR protection: all mutations scoped with AND repo_id = ? in WHERE clauses - Ownership enforcement: only issue author or admin can edit/delete - Fix LastInsertId() → RETURNING id for PostgreSQL compatibility - Fix reads wrapped in write transactions (use db directly for SELECTs) - Status validation with clear error messages - Pagination cap (maxIssuesPerRepo=10000) - Migration 0007: issue_comments table (sqlite + postgres) - Migration 0006: BIGSERIAL, composite index (repo_id, status, created_at DESC) - 22 backend integration tests covering CRUD, IDOR, lifecycle, isolation - E2E txtar test covering all CLI commands and permission checks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Full CRUD: add, list, edit, delete via SSH CLI - Ownership enforcement: only author or admin can edit/delete - Empty body validation at backend layer - GetIssueComments verifies issue exists before listing - Fix migration 0006: FK referenced 'repositories' (wrong) → 'repos' - Enable PRAGMA foreign_keys(1) in test DSN to match production - ON DELETE CASCADE verified: deleting an issue removes its comments - 11 backend integration tests covering all paths - E2E txtar covering all CLI commands and permission checks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- issue.txtar / issue-comment.txtar: replace space-containing flag values with hyphen-separated equivalents (e.g. 'First issue' → First-issue) because cmdSoft joins args with strings.Join and SSH re-splits on spaces, breaking quoted multi-word strings before cobra sees them - repo-webhook-ssrf.txtar: add 'env SOFT_SERVE_SSRF_ALLOW_PRIVATE_NETS=' before starting the server; the test suite unconditionally sets this var to "true" for all tests, which disabled SSRF protection and caused every private-IP rejection assertion to fail - user-create-with-key.txtar: remove the positional-key test case (no -k flag) that assumed the server would reconstruct a bare public key from extra positional args — that path was never implemented and the error message "unexpected arguments" makes the intent clear Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a per-repository label feature with full CRUD, issue association, and filtering support. Named color aliases (red, blue, yellow, green, magenta, pink, white, black) are resolved to hex at write time. Schema: - labels table (id, repo_id, name, color, description) with UNIQUE(repo_id, name) - issue_labels junction table with ON DELETE CASCADE from both sides CLI commands: - repo label create/list/edit/delete (admin only) - repo issue label add/remove (collaborator+) - repo issue list --label NAME (filter by label) - repo issue view now shows Labels line when labels are attached Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ting - Add IssueFilter struct and DefaultIssueLimit const to pkg/store/issue.go - Update IssueStore interface: GetIssuesByRepoID and CountIssues now accept IssueFilter - Rewrite store/database/issue.go with buildIssueWhere helper that handles label join, status, search LIKE, and LIMIT/OFFSET pagination - Remove GetIssuesByLabel from LabelStore interface and database implementation (superseded by IssueFilter.LabelName) - Update Backend.GetIssuesByRepository and Backend.CountIssues to accept IssueFilter - Remove Backend.GetIssuesByRepositoryAndLabel (functionality folded into filter) - Add --search/-q, --page/-p, --limit/-n flags to SSH issue list command - Update all test call sites to use IssueFilter Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implement the issue assignees feature: migration 9 creates the issue_assignees join table with cascade deletes, store/backend layers expose GetIssueAssignees/AssignUserToIssue/UnassignUserFromIssue, the SSH CLI gains `issue assignee add/remove` subcommands with write-access guards, and `issue view` now prints the Assignees line when present. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements milestone CRUD with open/close/reopen/delete, milestone_id on issues (migrations 10-11), store/backend/proto/SSH CLI layers, issue view shows milestone, issue list --milestone filter, and ON DELETE SET NULL cascade. All new tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements a full REST JSON API for the issues system under
/api/v1/repos/{repo}/. Covers issues, comments, labels, assignees,
and milestones with proper auth checks (read/write/admin) and
integration tests for all key scenarios.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Summary
This PR documents that critical security findings from code review round 59 have been addressed in previous rounds. The worktree was merged with main containing fixes from rounds 55, 56, and 58.
Changes
No new code changes - this PR references previously implemented security fixes that address the findings from round 59:
validateJWTClaims(fix(web): JWT claims validation - expiration, not-before, issuer, audience #857)proto.ErrTokenExpired(fix(web): JWT token expiration error handling #861)Review Findings (Round 59)
MUST-FIX (Already Addressed)
SHOULD-FIX (Documented)
Notes
The 18 findings from round 59 represent comprehensive security and correctness improvements. Critical findings are already addressed in previous rounds (rounds 56, 58). Remaining complex items (plaintext password storage, SQL injection, race conditions, path traversal) are documented in separate issues for focused remediation.
Related Issues
Closes #863
Paperclip: DON-888