Skip to content

Fix critical deployment and security issues from 2026-09 codebase review - #27

Merged
mickume merged 6 commits into
mainfrom
claude/hub-codebase-review-o6x2zt
Sep 9, 2026
Merged

Fix critical deployment and security issues from 2026-09 codebase review#27
mickume merged 6 commits into
mainfrom
claude/hub-codebase-review-o6x2zt

Conversation

@mickume

@mickume mickume commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

This PR addresses critical findings from a comprehensive codebase review conducted in September 2026. The review identified deployment blockers, security vulnerabilities, carry-patch correctness issues, audit subsystem gaps, and git subprocess handling problems across the hub.

Summary of Changes

This is a large, multi-faceted fix addressing 5 critical, 27 high, 28 medium, and 8 low severity issues organized into five logical commit groups:

Deployment Blockers

  • Container image fixes: Added git binary and libstdc++ to runtime image (was completely missing, breaking all git operations)
  • Config discovery: Fixed bundled git config not being found at runtime
  • Push size limits: Removed 1 MB limit that broke large patch pushes
  • Makefile: Corrected volume mount paths for local development

Authorization & Injection

  • Workspace ownership enforcement: All workspace-scoped endpoints now enforce ownership checks (non-owners get 404, admins bypass)
  • Credential redaction: Git error messages and job records no longer leak embedded credentials in URLs
  • Ref name validation: Added ValidateRefName() to prevent git option injection via branch/tag names
  • Environment isolation: Git subprocess now strips redirect variables (GIT_DIR, GIT_WORK_TREE, GIT_CONFIG_PARAMETERS) and defaults identity
  • Path encoding: CLI now percent-encodes all user-supplied path segments (slugs, keys, ids) before placing in URLs
  • Credential helper: Fixed to only answer for the hub's own host, preventing credential leakage to other services

Carry-Patch Correctness

  • Upstream branch tracking: New upstream package ensures rebuilds, syncs, and previews all use the same upstream default branch
  • Cherry-pick safety: Added --end-of-options to prevent commit SHAs from being parsed as options
  • Rerere by ID: Changed rerere list/forget to use conflict hash IDs instead of paths (git doesn't track paths in rr-cache)
  • Soft-delete positions: Fixed UNIQUE constraint collision when soft-deleting patches mid-stack
  • Auth method threading: Updated Fetch and ResolveAuth signatures to properly thread transport.AuthMethod through rebuild pipeline
  • Workspace locking: New wslock package provides per-workspace mutual exclusion for trunk mutations

Audit Subsystem

  • Session metadata round-trip: Fixed metadata serialization/deserialization through DuckDB
  • Hub event timestamps: Added timestamp field to hub events; unified query now sorts by timestamp + event ID
  • Client timestamp validation: Added NormalizeTimestamp() to validate and normalize RFC 3339 timestamps from clients
  • Session filtering: Unified query now filters by session_id when provided
  • Retention guards: Orphaned audit data cleanup now respects grace period before deletion
  • Metrics wiring: Fixed Prometheus metrics initialization and collection
  • Workspace filtering: Audit read endpoints now enforce workspace ownership via allowlist

Git & CLI Robustness

  • Subprocess process groups: Unix builds now place git in its own process group for clean cancellation
  • Timeout handling: Improved context cancellation and timeout behavior
  • Wait command improvements: Added merge wait and rebuild wait subcommands; fixed polling logic
  • Job status polling: Commands now poll immediately then at --poll-interval; exit 0 only on completion, non-zero on failure
  • Error handling: Commands now exit with code 1 for API/network errors, 2 for usage errors
  • Ref validation: Added validation to checkout, branch, merge, cherry, cherry-pick, merge-base, config operations

Notable Implementation Details

  • Ownership checks: Implemented via new workspace.AuthorizeWorkspace() helper that returns workspace state for precondition checks
  • Credential redaction: Uses regex-based URL parsing to mask userinfo in https://user:token@host/... patterns
  • Workspace locking: Simple in-memory map with sync.Mutex; prevents concurrent mutations of shared trunk working tree
  • Upstream tracking: Fetches upstream remote and

https://claude.ai/code/session_01Gsa6jQ2Du2qW3PX2HFc88o

@claude
fix: unblock the hub container and large git pushes
44c3fa9 
- Install git-core in the runtime image: internal/gitcmd shells out to git for
  merge, rebase, and every carry-patch operation, and ubi-minimal ships
  without it, so those features failed with "failed to run git --version".
- Point XDG_CONFIG_HOME/XDG_DATA_HOME at /config/af-hub and /data/af-hub so
  the bundled config.toml is found by default and the image matches
  deploy/deployment.yaml. Adjust `make hub-runc` mounts and the docs.
- Exempt /git/ routes from apikit's instance-wide request body limit via a
  Pre middleware that stashes the original body, and stream the receive-pack
  body instead of buffering it. A push larger than max_body_size (1MB by
  default) was rejected with 413 before reaching the git server.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gsa6jQ2Du2qW3PX2HFc88o
@claude
fix: enforce workspace ownership and harden git argument handling
b74398c 
Authorization:
- Add workspace.AuthorizeWorkspace and apply the owner-or-admin rule (404
  for non-owners, matching workspace CRUD) to sync, reclone, all patch
  endpoints, every carry-patch endpoint (rebuild submit/list/get/cancel/
  requeue/rollback, preview, patch-status, rerere list/forget, the sync
  hook), and every merge endpoint (submit/list/get/cancel/requeue, batch
  rebase). Any authenticated API key could previously act on any workspace.
- Audit: POST /sessions requires access to the target workspace and no
  longer returns another tenant's session on an id collision; per-run read
  endpoints and the transcript require workspace ownership; GET /audit and
  the SSE stream restrict non-admin callers to workspaces they own.
- Git server: reject credentials of blocked users, as apikit's REST
  middleware already does.
- Secrets/vars: admin tokens are rejected on user-scoped endpoints instead
  of operating on a phantom empty-owner scope; the resolved-vars endpoint
  only includes the org tier when the workspace owner is an org member.

Input hardening:
- gitcmd: pass --end-of-options (or "--") in every typed helper, make
  RevParse use --verify, add ValidateRefName, and redact URL userinfo in
  GitError messages. A ref such as "--detach" or "--exec=<cmd>" previously
  passed the branch-exists check and reached git checkout/rebase as an
  option.
- Validate ref names at the API boundary: merge target/source, batch
  rebase refs, patch branch names, and integration_branch.
- Merge CHECK_COMMAND no longer inherits the hub process environment.
- afc credential-helper only answers for the hub's own scheme and
  host:port, and honours ENDPOINT_URL/API_KEY like every other command.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gsa6jQ2Du2qW3PX2HFc88o
@claude
fix: make carry-patch rebuilds, merges, and syncs safe on the shared …
d081e6f 
…trunk

Upstream base:
- Fetch the upstream remote via go-git with the workspace's upstream
  credentials (the git CLI fetch had none, so private upstreams failed) and
  record the upstream default branch under refs/remotes/upstream/HEAD.
  Rebuild, sync, and preview now resolve that ref as the base; FETCH_HEAD
  after `git fetch upstream` is the alphabetically first branch of the
  remote, so multi-branch upstreams were rebased onto the wrong base, and
  preview used the trunk's local HEAD.
- Post-clone setup fetches upstream so the integration branch starts at the
  upstream base.

Rebuild executor:
- Clear stale state (in-progress cherry-pick/merge, dirty tree, leftover
  temporary branch) before starting, create the temp branch with -B, and
  restore the originally checked-out branch afterwards instead of leaving
  the trunk on a detached HEAD.
- Replay only non-merge commits whose content is not already upstream
  (--no-merges --right-only --cherry-pick) and skip cherry-picks that
  become empty instead of treating them as conflicts.
- Capture REBUILD_FAIL_MODE (not just the strategy) at enqueue time for
  auto-rebuilds triggered by sync and push; match the squash-merge PR
  number only as a subject suffix so reverts do not mark patches merged.
- Opt-in REBUILD_PUSH_INTEGRATION_BRANCH=true pushes the rebuilt
  integration branch to origin.

Merge jobs:
- Check out the target branch before deleting the source branch; the job
  deleted the checked-out branch, leaving HEAD dangling and breaking every
  subsequent git operation on the workspace. Restore the original checkout
  on exit, reset in-progress state before rebasing, abort a rebase that a
  cancelled context interrupted, and fetch with the job context.
- Surface CHECK_COMMAND output in the job record, fail closed when the
  variable store errors, and filter the merge list by workspace in SQL.

Concurrency and data safety:
- Add internal/wslock, a per-workspace lock held by rebuild and merge jobs
  and tried by sync, reclone, archive, rollback, rerere forget, batch
  rebase, and the git push worktree reset (409 workspace_busy for
  handlers).
- Claim sync_status='syncing' with a conditional UPDATE.
- Archive pushes local branches to origin with the stored credentials and
  keeps the clone when the push fails; reclone pushes with credentials.
  Archiving previously deleted the clone without pushing, losing branches
  that only existed on the hub.
- Park soft-deleted patches at unique negative positions so compaction
  after a rebuild no longer violates UNIQUE(workspace_slug, position).
- Rerere: list rr-cache entries by id with a resolved flag (git does not
  record paths in the cache, so path-based listing was always empty) and
  allow forgetting by id.
- Job progress updates only apply to running jobs.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gsa6jQ2Du2qW3PX2HFc88o
@claude
fix: correct audit session metadata handling, timestamps, and retention
0aa783e 
- Scan agent_sessions.metadata into an untyped value and decode it from
  either JSON text or the already-decoded map go-duckdb returns for JSON
  columns; previously every session created with metadata failed to read
  back ("unsupported Scan ... map[string]interface {} into *string").
- Populate hub_audit_events.timestamp from the emitter (microsecond UTC)
  and normalise both sources of the unified audit query to epoch
  microseconds so hub and agent events order and paginate consistently;
  legacy rows with an empty timestamp fall back to ingested_at.
- Validate client-supplied timestamps on all ingestion endpoints
  (events, outcomes, tool calls/errors, traces, postmortems, batches)
  and cursors, returning 400 instead of a DuckDB conversion 500; values
  are normalised to UTC.
- Apply the session_id filter in QueryAuditEvents/QuerySessionOutcomes.
- Retention: step 1 ages session_outcomes/tool_calls/tool_errors along
  with agent_audit_events; step 8 no longer purges workspace-less hub
  events as orphans; env overrides below 1 fall back to defaults.
- Wire afhub_sse_connections and afhub_audit_events_total through a
  package-level metrics sink (audit.SetMetrics) used by the SSE manager,
  emitter and ingestion handlers.
- Unified query tests now run against the production schema.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gsa6jQ2Du2qW3PX2HFc88o
@claude
fix: harden git subprocess handling and make afc scriptable
d6bc6ba 
gitcmd:
- Run git in its own process group and kill the whole group on context
  cancellation (with a bounded WaitDelay) so hung helpers cannot pin a
  workspace lock; contexts without a deadline get DefaultTimeout (10m).
- Strip repository-redirecting and config-injecting variables
  (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_CONFIG_PARAMETERS, ...)
  from the inherited environment, force LC_ALL=C for parseable output,
  disable the editor/pager, and default the author/committer identity to
  af-hub <af-hub@localhost> when none is configured. Without an identity
  git refuses to commit inside the container, which broke carry-patch
  rebuilds and merge jobs.

afc:
- --wait (and the new `merge wait` / `rebuild wait` subcommands) poll
  immediately and exit non-zero when a job ends failed, dead_letter or
  cancelled, or a clone ends failed; the final record is still printed.
- Every user-supplied path segment (slugs, keys, ids, rerere entries) is
  percent-encoded before it is put in a request path.
- `secrets create` / `vars create` accept several KEY=VALUE arguments;
  create and update accept `--from-stdin` so values stay out of shell
  history; `credential set --from-stdin` reads the PAT or password.
- `credential set` requires username and password together.
- `patch update` rejects a call without any change flag.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gsa6jQ2Du2qW3PX2HFc88o
@claude
docs: document review outcomes, ownership, workspace lock, and CLI ch…
7204371 
…anges

- api.md / openapi.yaml / permissions.md: ownership is enforced on every
  workspace-scoped endpoint (404 for non-owners), workspace_busy error
  type and the per-workspace lock, archive push-before-delete, rerere
  entries by id with `resolved`, patch status=deleted rejected, rebuild
  job `upstream_head_sha` / `integration_branch_pushed`, audit timestamp
  validation and per-owner audit visibility, admin tokens on /user/*,
  org-tier variable resolution, and the CHECK_COMMAND sensitivity of
  vars:write.
- cli.md: exit codes for --wait, `merge wait` / `rebuild wait`,
  `--from-stdin`, multiple KEY=VALUE arguments, credential pairing,
  credential helper environment and scheme matching, path encoding.
- carry_patch_workflow.md: upstream base resolution, executor recovery,
  REBUILD_PUSH_INTEGRATION_BRANCH, rerere ids, updated limitations.
- configuration.md: git identity defaults and hardened git environment.
- New: docs/review_2026_09_codebase.md (findings, fixes, follow-ups),
  docs/apikit_issues.md (issue drafts for txsvc/apikit), errata for
  rerere-by-id and ownership enforcement.
- jobqueue: goroutine-count tests wait for the expected delta instead of
  sampling the noisy process-wide count once.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gsa6jQ2Du2qW3PX2HFc88o
@mickume
mickume merged commit 09ed6a8 into main Sep 9, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mickume @claude