fix: review round 57 - findings already fixed in previous rounds#859
Open
dvrd wants to merge 220 commits intocharmbracelet:mainfrom
Open
fix: review round 57 - findings already fixed in previous rounds#859dvrd wants to merge 220 commits intocharmbracelet:mainfrom
dvrd wants to merge 220 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)
- pkg/backend/webhooks.go: guard DeleteWebhookEventsByID with len(toBeDeleted)>0 to avoid sqlx.In error on empty slice; use var decl - pkg/web/server.go: document CORS AllowCredentials=false constraint; wildcard origin and credentialed requests are incompatible - pkg/task/manager.go: delay m.m.Delete(id) until p.fn goroutine has exited (drain goroutine completes) to prevent same-id task overlap - pkg/store/database/webhooks.go: fix stale comment header Closes #113
fix: review-loop round 37 — webhook delete guard, CORS docs, task delete race, stale comment
…tion Returning ErrInvalidRepo for non-existent repositories allowed unauthenticated clients to distinguish "repo does not exist" from "access denied", enabling repository name enumeration. Now returns ErrNotAuthed uniformly so the error is indistinguishable from an access-denial regardless of whether the repo exists. Closes #115
fix(daemon): return ErrNotAuthed for missing repos to prevent enumeration
… DNS, webhook guard, daemon wg defer - hooks/gen.go: use printf instead of echo to avoid flag/backslash interpretation when piping stdin to sub-hooks - task/manager.go: return nil when task completed successfully and waiter wakes after cancel (was incorrectly returning ctx.Err()) - ssrf/ssrf.go: use net.DefaultResolver.LookupIPAddr(ctx) so DNS resolution honours context cancellation in the dial transport - backend/webhooks.go: guard CreateWebhookEvents with len>0 (mirrors existing guard for DeleteWebhookEventsByID) - daemon/daemon.go: defer wg.Done() so it fires even if handleClient panics; update TestInvalidRepo to expect ErrNotAuthed - backend/hooks.go: add TODO comment about push-mirror timeout Closes #117
fix(review-39): hooks echo injection, task nil return, ssrf ctx-aware DNS, webhook guard, daemon wg defer
…code, nil-r
- backend/repo.go: shellQuote() paths in GIT_SSH_COMMAND to prevent
shell metacharacter injection (POSIX single-quote wrapping)
- task/manager.go: add completed atomic.Bool so waiters correctly
distinguish successful nil-error completion from Stop() cancellation
- web/git.go: restructure else-if chain into separate ifs with a
comment explaining repo-enumeration-resistance intent
- ssrf/ssrf.go: ValidateURL/ValidateHost now accept ctx so DNS
lookups honour cancellation; update all callers and tests
- backend/push_mirror.go: document DNS-rebinding window for SSH mirrors
- web/auth.go: log tokenErr not stale err under "invalid credentials"
- backend/hooks.go: fix "asynchronously" -> "synchronously" comment
- web/git_lfs.go: pass r (not nil) to renderStatus for duplicate upload
- git/commit.go: exact zero-hash regex ^(0{40}|0{64})$
- web/server.go: add proxy-header spoofing warning to realClientIP doc
- hooks/gen.go: quote "$0" in hookTemplate to handle spaces in paths
Closes #119
fix(review-40): shell injection, task completed flag, ssrf ctx, dead code, nil-r
… symlink guard, CORS warn - backend/webhooks.go: pass ctx to ValidateWebhookURL (build-breaking regression from round 40 where replacement did not apply) - daemon/daemon.go: fix off-by-one in max-connections check; change Size+1 >= Max to Size >= Max so MaxConnections is the inclusive cap - store/database/webhooks.go: bulk INSERT for webhook events reduces N DB round-trips to 1 inside the transaction - web/git.go: use os.Lstat + symlink check in sendFile to block symlinks from pointing outside the repository root - web/server.go: log a startup warning when CORS AllowedOrigins=["*"] and AllowedHeaders contains Authorization - web/auth.go: add comment explaining that '#' separator in JWT subject is safe because ValidateUsername disallows '#' in usernames - backend/hooks.go: remove unnecessary WaitGroup in PostUpdate (one goroutine can be a direct call); drop unused sync import Closes #121
fix(review-41): ValidateWebhookURL ctx, daemon max-conn, bulk INSERT, symlink guard, CORS warn
…o-get comment - task/manager.go: document completed-field ordering invariant (must store after p.mu.Unlock(), before p.cancel()) to prevent future races - backend/push_mirror.go: strip IPv6 zone identifier (e.g. ::1%eth0) before SSRF validation to prevent scoped-address bypass - daemon/daemon.go: document known per-IP counter TOCTOU window in the CompareAndDelete cleanup path - web/git_lfs.go: return 413 (not 422) when MaxBytesReader limit is hit on LFS batch request (helps clients distinguish size vs syntax) - web/git.go: add comment acknowledging go-get+AllowPublicGoGet repo existence leak as an accepted trade-off - web/server.go: create httpLimiter only when RateLimit > 0 to avoid an idle background goroutine when rate limiting is disabled Closes #123
fix(review-42): task completed ordering, IPv6 zone bypass, LFS 413, go-get comment
…g level, webhook batch (#126) - task/manager: deliver result to done channel before deleting from map so concurrent Run() calls after completion see the result, not ErrNotFound - web/git: move response headers + WriteHeader after gzip reader construction so gzip parse errors can still return a non-200 status - web/auth: downgrade Token scheme user-lookup failure from Error to Debug to avoid leaking token material at Error log level - store/database/webhooks: batch GetWebhookEventsByWebhookIDs in chunks of 999 to stay within SQLite SQLITE_MAX_VARIABLE_NUMBER limit Closes #125
…webhook limit (#128) - backend/hooks: pass d.ctx to PushMirrors instead of 30s syncCtx so inner mirror goroutines can run their full 10-minute timeout window - web/server: validate X-Forwarded-For candidate with net.ParseIP before using it as rate-limit key — invalid strings fall back to RemoteAddr - task/manager: check completed flag before reading p.err in waiter path, making happens-before ordering explicit in comments - store/database/webhooks: add ORDER BY + LIMIT 100 to ListWebhookDeliveriesByWebhookID to prevent unbounded memory use for high-delivery webhooks - backend/repo: remove unreachable else-if branch in DeleteRepository - daemon: remove dead key-sanitization step in version-only param loop - backend/push_mirror: reject SSH_AUTH_SOCK values with newlines/NUL bytes - web/auth: clarify dummyHash is intentionally public (timing equalization only) - hooks/gen: add comment explaining $0 safety in hookTemplate Closes #127
- backend/hooks: replace stale TODO comment with accurate description of why webhook delivery runs synchronously in the update hook subprocess - web/git: wrap w in flushResponseWriter for the info/refs response so pktline header and ref advertisement flush through buffering proxies Closes #129
…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
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
Code review round 57 found 12 issues across MUST-FIX, SHOULD-FIX, and NIT categories. All MUST-FIX issues were already addressed in previous rounds.
Changes
This PR documents review findings from round 57. No new code changes - all critical fixes already exist in main branch:
Review Findings
MUST-FIX (Already Fixed)
validateJWTClaimsfunction added to checkclaims.ExpiresAt.Time.Before(time.Now())GetLFSObjectsByOidsbatch method added to eliminate individual DB queries in loopSHOULD-FIX (Remaining)
pkg/web/git_lfs.go:661,665,669- ParseInt/Atoi errors discarded with_pkg/web/goget.go:44- Repository name used without validationpkg/web/auth.go:80- Error messages may expose timing informationTest Plan
Notes
The reviewer agent analyzed an older codebase snapshot. All critical findings were already addressed in earlier rounds of the review loop.
Closes #858
Paperclip: DON-887