GITGUD

neiam neiam /gitgud

Git Gud

public · Issues · Pulls · Labels · Forks · Compare · Actions success · Badges · Packages

⭐ 1

Log in to mark this repository.

Add comprehensive security review documentation

67bd0f9 · gmorell · 2026-10-20 19:56

✓ build / android (push) ✓ build / build (push) ✓ build / cli (push) ✓ build / deb (push) ✓ hello / smoke (push)
9 files +852 -0
Message 
{commit_body(@commit)}

Files changed

added security-review/api-security.md
+135 −0
@@ -0,0 +1,135 @@
1 +# API Security & Rate Limiting
2 +
3 +## API Endpoints
4 +
5 +### Personal Access Tokens (PAT)
6 +**Scopes** (`lib/git_gud/accounts/personal_access_token.ex`)
7 +- Granular scope system (e.g., `issues:read`, `issues:write`, `repo:read`, `repo:write`)
8 +- Stored as hashes — cannot be recovered if lost
9 +- Explicit scope checking in mobile API controllers
10 +
11 +**Mobile API** (`lib/git_gud_web/controllers/api/mobile/`)
12 +- Auth via `x-api-key` or `Authorization: Bearer <token>` header
13 +- Per-action scope validation in controller actions
14 +- Endpoints:
15 + - `AuthController` — login, register, token refresh
16 + - `CiController` — CI/CD status, job logs
17 + - `IssueController` — issues CRUD
18 + - `LabelController` — labels CRUD
19 + - `PackageController` — package uploads/downloads
20 + - `PullRequestController` — PRs, reviews, comments
21 + - `RepositoryController` — repos, branches, commits, blobs
22 + - `SessionController` — session management
23 + - `WikiController` — wiki pages
24 +
25 +### Gitea-Compatible REST API
26 +**`lib/git_gud_web/controllers/api/v1/`**
27 +- `OrgController` — organization management
28 +- `RepositoryController` — repository CRUD, forks, settings
29 +- `WorkflowController` — CI workflow management
30 +- Auth: per-endpoint handling (public read, PAT for write, runner token for CI)
31 +
32 +### Runner Protocol (CI)
33 +**`lib/git_gud_web/controllers/runner_controller.ex`**
34 +- Twirp/protobuf endpoints at `/api/actions/_apis/runner.v1.*`
35 +- Methods: `Ping`, `Register`, `Declare`, `FetchTask`, `FetchSingleTask`, `UpdateTask`, `UpdateLog`
36 +- Auth: `x-runner-token` header or `Bearer` token
37 +- Runner registration uses one-shot token (`Runners.consume_registration_token/1`)
38 +- Per-job JWTs minted on task claim (`RunnerJwt.mint/3`)
39 +
40 +**Artifact/Cache APIs**
41 +- `ActionsArtifactController` — `/api/actions/_apis/pipelines/workflows/:run_id/artifacts/*`
42 +- `ActionsCacheController` — `/api/actions/_apis/artifactcache/*`
43 +- `WorkflowArtifactController` — `/api/actions/_apis/workflow-artifacts/*`
44 +- Auth: `RunnerJwtAuth` plug — verifies per-job JWT and job is active
45 +
46 +### OCI Registry API
47 +**`lib/git_gud_web/controllers/registry_controller.ex`**
48 +- Docker registry v2 protocol
49 +- Token-based auth at `/v2/token`
50 +- JWT tokens for push/pull operations
51 +- Token signing via `GitGud.Packages.Token` (RS256/HS256 configurable)
52 +
53 +### ActivityPub/WebFinger
54 +**`lib/git_gud_web/controllers/ap_actor_controller.ex`**
55 +- HTTP signatures (RSA-SHA256) with digest verification
56 +- Clock skew validation: 5-minute window
57 +- Content negotiation for JSON-LD / ActivityPub objects
58 +- WebFinger at `/.well-known/webfinger`
59 +
60 +**`lib/git_gud_web/plugs/ap_dispatcher.ex`**
61 +- Routes ActivityPub requests to inbox handlers
62 +- Validates signature chains for federated actors
63 +
64 +## Rate Limiting
65 +
66 +### Implemented Controls
67 +**Federation Inbox** (`lib/git_gud/federation/rate_limiter.ex`)
68 +- 60 requests/minute/source host
69 +- Allowlisted hosts: 5x multiplier
70 +- ETS-based counters with 60-second sweep interval
71 +- Prevents inbox flooding from remote instances
72 +
73 +**MFA Attempts** (`lib/git_gud/accounts/two_factor_attempts.ex`)
74 +- Tracks failed MFA verification attempts
75 +- Shared with login attempt tracking
76 +
77 +### Missing Rate Limiting
78 +- **Login/Register endpoints** — No rate limiting on authentication attempts
79 +- **Password reset/Magic links** — No throttling on email sending
80 +- **PAT token verification** — No brute-force protection on token endpoint
81 +- **API endpoints** — No rate limiting on mobile API, REST API, or runner endpoints
82 +- **Webhook delivery** — No rate limiting on outbound webhook retries
83 +- **Runner protocol** — No rate limiting on `FetchTask` calls
84 +
85 +## Security Observations
86 +
87 +### High
88 +
89 +**No rate limiting on authentication endpoints**
90 +- Login, registration, and magic link endpoints have no rate limiting
91 +- Vulnerable to credential stuffing and brute-force attacks
92 +- Recommendation: add IP-based rate limiting (e.g., 10 requests/minute for login, 5/hour for magic links)
93 +- Consider CAPTCHA or proof-of-work after N failed attempts
94 +
95 +**No rate limiting on API endpoints**
96 +- Mobile API, REST API, and runner protocol endpoints have no rate limiting
97 +- Can be abused for resource exhaustion or denial of service
98 +- Recommendation: add rate limiting via Plug (e.g., `Plug.RateLimit` or custom implementation)
99 +- Start with 100 requests/minute per authenticated user, 10/minute for unauthenticated
100 +
101 +### Medium
102 +
103 +**PAT scopes not uniformly validated**
104 +- Scoped token validation exists in mobile API but not consistently across all REST API endpoints
105 +- Recommendation: create a centralized plug for PAT scope validation
106 +- Apply to all API pipelines (`:api`, `:mobile_api`, `:browser_api`)
107 +
108 +**Runner registration token not expired**
109 +- `lib/git_gud_web/controllers/runner_controller.ex:73` — Registration uses one-shot token
110 +- No evidence of token expiry enforcement in the controller
111 +- If token is never consumed, it remains valid indefinitely
112 +- Recommendation: add expiry to registration tokens (e.g., 30 minutes)
113 +
114 +**API key exposure in URLs**
115 +- Verify API keys are never logged in query parameters
116 +- Recommendation: add logging middleware that redacts sensitive headers/parameters
117 +- Use `Authorization` header instead of query parameters for API keys
118 +
119 +**Webhook delivery no rate limiting**
120 +- `lib/git_gud/webhooks.ex` — Outbound delivery has no rate limiting
121 +- Malicious or misconfigured webhooks can flood external endpoints
122 +- Recommendation: add delivery rate limiting per webhook (e.g., 10 deliveries/minute)
123 +- Include retry backoff with exponential delay and maximum retry count
124 +
125 +### Low
126 +
127 +**OCI registry token TTL**
128 +- `config/config.exs:103` — Token TTL is 300 seconds (5 minutes)
129 +- Short TTL is good for security but increases token refresh overhead
130 +- Ensure token refresh logic in clients handles 401 responses gracefully
131 +
132 +**ActivityPub clock skew window**
133 +- 5-minute clock skew window for HTTP signatures may be too permissive
134 +- Recommendation: make clock skew configurable (e.g., `max_skew_seconds: 300`)
135 +- Default to 30 seconds for high-security deployments
added security-review/authentication.md
+92 −0
@@ -0,0 +1,92 @@
1 +# Authentication & Authorization
2 +
3 +## Implemented Controls
4 +
5 +### Multi-Factor Authentication
6 +- **TOTP** (`lib/git_gud/accounts/two_factor.ex`, `lib/git_gud/accounts/recovery_code.ex`)
7 + - RFC 6238 compliant time-based one-time passwords
8 + - 10 single-use recovery codes per user (bcrypt-hashed)
9 + - Instance-level enforcement configurable via `TwoFactor.enforcement_status/1`
10 + - Grace period support (`TwoFactor.enforced_at` vs `TwoFactor.grace_period_days`)
11 + - Attempt tracking via `TwoFactorAttempts` (shared with login)
12 +
13 +- **WebAuthn** (`lib/git_gud/accounts/webauthn.ex`, `lib/git_gud/accounts/webauthn_credential.ex`)
14 + - Passwordless or second-factor authentication
15 + - Ceremony managed via `Wax` library
16 + - Sudo-mode gating for credential changes
17 + - Credential management through `WebauthnController`
18 +
19 +- **Magic Links** — Email-based authentication via `UserNotifier`
20 + - Token-based login without password entry
21 + - Tokens stored server-side with expiry
22 +
23 +### Session Management
24 +- **Session tokens** stored in `users_tokens` table
25 + - 14-day expiry window
26 + - Server-side revocation capability
27 + - Renewal on activity (reissue after 7 days)
28 + - Session fixation prevention via `renew_session/2` on login
29 + - Cross-session invalidation via `disconnect_sessions/1`
30 +
31 +- **Remember-me cookies**
32 + - Signed cookies with 14-day persistence
33 + - Tied to user's `authenticated_at` timestamp
34 +
35 +### Personal Access Tokens
36 +- **Scoped tokens** (`lib/git_gud/accounts/personal_access_token.ex`)
37 + - Scope-based access control (e.g., `issues:read`, `repo:write`)
38 + - Stored as hashes (not plaintext)
39 + - Explicit scope checking in mobile API controller
40 + - Per-action validation in `api/mobile/` controllers
41 +
42 +- **Runner JWTs** (`lib/git_gud/workflows/runner_jwt.ex`)
43 + - Per-job short-lived tokens minted on `FetchTask`
44 + - Claims: `sub`, `jti`, `tid` (task ID), `rid` (run ID), `pid` (repo ID), `perm` (permissions)
45 + - Verified via `RunnerJwt.verify_active/1` which checks job is still running
46 + - 5-minute grace period after job completion (for post-job steps)
47 + - Permissions scoped to workflow `permissions:` block
48 +
49 +### Authorization Layers
50 +- **Repository visibility** via `RepoAuth` plug
51 + - Read-gate enforced at LiveView mount level
52 + - Visibility enforcement: `public`, `internal`, `private`
53 + - Preload-based checks to minimize queries
54 +
55 +- **Admin scope** via `Scope.for_user/1`
56 + - Gated through `require_admin` mount hook
57 + - Instance-level admin-only routes
58 +
59 +- **Sudo mode**
60 + - Password or second-factor re-authentication required
61 + - 10-minute validity window for sensitive operations
62 + - Credential changes gated behind re-authentication
63 +
64 +## Security Observations
65 +
66 +### Critical
67 +
68 +**Insecure default secrets in config** (`config/config.exs:141-149`)
69 +- Secrets master key default: `"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="` (all zeros)
70 +- Runner JWT secret default: `"dev-runner-jwt-secret-do-not-use-in-prod"`
71 +- If `runtime.exs` doesn't override these, all secrets are encrypted with a known key and tokens are forgeable
72 +- Mitigation: production deployment **must** override via environment variables
73 +
74 +### High
75 +
76 +**HS256-only JWT signing** (`lib/git_gud/workflows/runner_jwt.ex:174`)
77 +- Runner JWTs use symmetric HS256 signing
78 +- Same secret signs all tokens across the instance
79 +- If leaked from any single runner, attacker can mint arbitrary runner tokens
80 +- No key rotation at JWT layer (only app-level master key rotation for secrets)
81 +- Recommendation: consider asymmetric signing (RS256/ES256) or at minimum per-instance secret rotation
82 +
83 +### Medium
84 +
85 +**No explicit account lockout**
86 +- Only MFA attempts are tracked via `TwoFactorAttempts`
87 +- No lockout mechanism for failed login attempts
88 +- Recommendation: add progressive rate limiting or account lockout after N failed attempts
89 +
90 +**PAT scopes not uniformly validated**
91 +- Scoped token validation exists in mobile API but not consistently across all REST API endpoints
92 +- Recommendation: centralize PAT scope validation in a plug applied to all API pipelines
added security-review/csrf-xss.md
+100 −0
@@ -0,0 +1,100 @@
1 +# CSRF & XSS Prevention
2 +
3 +## CSRF Protection
4 +
5 +### Implemented Controls
6 +
7 +**Browser Pipeline** (`:browser` pipeline in router)
8 +- `protect_from_forgery` plug applied to all browser routes
9 + - Generates CSRF token per session
10 + - Token validated on POST/PUT/DELETE/PATCH requests
11 + - Token stored in session and compared against request
12 +
13 +**Token Transmission**
14 +- Form submissions: token included via hidden input field
15 +- AJAX/fetch requests: `x-csrf-token` header accepted
16 + - Phoenix automatically includes CSRF token in meta tag
17 + - JavaScript frameworks can read from `document.querySelector('meta[name="csrf-token"]').content`
18 +
19 +**Cookie Attributes**
20 +- SameSite=Lax on session cookies
21 + - Prevents cross-site cookie sending on top-level navigation
22 + - Blocks CSRF on GET requests and same-site navigation
23 + - Allows GET requests to cross-site via browser navigation (e.g., clicking a link)
24 +
25 +### Exclusions (Intentional)
26 +- **`:api` pipeline** — Used by Gitea-compatible REST API and runner endpoints
27 + - No CSRF protection — acceptable because endpoints use token authentication (PAT, runner JWT)
28 + - Token-authenticated clients don't automatically send cookies
29 +- **`:mobile_api` pipeline** — Mobile API endpoints
30 + - Uses personal access token authentication
31 + - No CSRF protection — acceptable for token-based auth
32 +
33 +## XSS Prevention
34 +
35 +### Implemented Controls
36 +
37 +**LiveView Auto-Escaping**
38 +- Phoenix LiveView automatically escapes all interpolated values in HEEx templates
39 + - `{@value}` — HTML-escaped interpolation
40 + - `<%= @value %>` — HTML-escaped interpolation in tag bodies
41 + - `{@value}` in attribute values — attribute-escaped
42 +- Template compilation provides compile-time escaping guarantees
43 +
44 +**Markdown Sanitization**
45 +- `lib/git_gud/markdown.ex` — HTML sanitization pipeline
46 + - Strips dangerous HTML elements (`<script>`, `<iframe>`, `<object>`, `<embed>`)
47 + - Strips event handler attributes (`onload`, `onclick`, etc.)
48 + - Strips `javascript:` URIs in href/src attributes
49 + - Allows safe subset: bold, italic, links, code blocks, tables, images
50 + - Processes user-generated content before rendering in templates
51 +
52 +**Content-Type Headers**
53 +- All responses include appropriate `Content-Type` headers
54 +- JSON responses: `application/json`
55 +- HTML responses: `text/html`
56 +- Prevents MIME-type confusion attacks
57 +
58 +## Security Observations
59 +
60 +### Medium
61 +
62 +**No Content-Security-Policy (CSP) header**
63 +- CSP provides defense-in-depth against XSS
64 +- Prevents inline script execution (except explicitly allowed)
65 +- Restricts sources for scripts, styles, images, fonts, etc.
66 +- Recommendation: add CSP header via Plug
67 + - Example: `default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline';`
68 + - Start with `Content-Security-Policy-Report-Only` for testing
69 +
70 +**No X-Frame-Options or X-Content-Type-Options headers**
71 +- `X-Frame-Options: DENY` — prevents clickjacking
72 +- `X-Content-Type-Options: nosniff` — prevents MIME-type sniffing
73 +- Recommendation: add via `put_secure_browser_headers` (already in `:browser` pipeline)
74 + - Verify these headers are not being stripped by reverse proxy
75 +
76 +**LiveDashboard accessible**
77 +- Phoenix LiveDashboard available at `/dashboard/*`
78 +- Exposes runtime metrics, process info, logger, IEx console
79 +- **Risk**: IEx console allows arbitrary code execution
80 +- Recommendation: disable `dev_routes` in production
81 + - `config :git_gud, GitGudWeb.Endpoint, dev_routes: false`
82 + - Or restrict to admin users only via plug
83 +
84 +**MailboxPreview accessible**
85 +- Swoosh MailboxPreview at `/dev/mailbox` in development
86 +- Exposes received emails (including magic links with tokens)
87 +- Recommendation: disable in production via `config :swoosh, local: false`
88 +
89 +### Low
90 +
91 +**Markdown sanitization needs periodic review**
92 +- XSS vectors evolve over time (e.g., SVG onload, iframe src, data URIs)
93 +- Recommendation: add security test suite with known XSS payloads
94 +- Test against latest OWASP XSS cheatsheet regularly
95 +- Consider using a well-maintained sanitization library (e.g., `bleach`-like Elixir port)
96 +
97 +**Inline JavaScript in templates**
98 +- If `phx-hook` or `colocated hooks` used in templates, inline scripts may be injected
99 +- Recommendation: ensure all JavaScript is loaded via assets bundle (app.js)
100 +- Use `phx-hook` with external hooks defined in `assets/js/` where possible
added security-review/error-handling-headers.md
+117 −0
@@ -0,0 +1,117 @@
1 +# Error Handling & Security Headers
2 +
3 +## Error Handling
4 +
5 +### Implemented Controls
6 +
7 +**Error Pages**
8 +- `lib/git_gud_web/controllers/error_html.ex` — HTML error pages (404, 500, etc.)
9 +- `lib/git_gud_web/controllers/error_json.ex` — JSON error responses
10 +- Consistent error format across HTML and JSON endpoints
11 +- Generic messages that don't leak implementation details
12 +
13 +**Authentication Errors**
14 +- Hook/auth failures log warnings with IP address
15 +- Runner protocol errors return Twirp-style error codes (e.g., `unauthenticated`, `invalid_argument`)
16 +- Generic messages for auth failures — no stack traces or internal details
17 +
18 +**Runner Protocol Error Handling**
19 +- `runner_controller.ex` — Twirp error responses with structured error codes
20 +- Protobuf error messages include `code` and `message` fields
21 +- Connect-Go client compatibility verified
22 +
23 +**Database Error Handling**
24 +- Ecto changeset errors returned to clients
25 +- No raw SQL errors exposed (Ecto catches and wraps errors)
26 +- Constraint violation errors handled gracefully
27 +
28 +## Security Headers
29 +
30 +### Implemented Controls
31 +
32 +**Browser Pipeline** (`:browser` pipeline)
33 +- `put_secure_browser_headers` plug
34 + - Sets standard security headers
35 + - Applied to all browser routes
36 + - Includes: `X-Frame-Options`, `X-Content-Type-Options`, `X-XSS-Protection`
37 +
38 +**Session Cookies**
39 +- SameSite=Lax on session cookies
40 +- Secure flag for HTTPS-only transmission
41 +- HttpOnly flag on session cookies (prevents JavaScript access)
42 +
43 +**Runner Protocol Responses**
44 +- `Connection: close` header on runner protocol responses
45 +- Prevents HTTP/1.1 keep-alive framing attacks
46 +- Bandit + forgejo-runner Connect-Go compatibility
47 +
48 +**Content-Type Headers**
49 +- All JSON responses: `application/json`
50 +- All HTML responses: `text/html`
51 +- Prevents MIME-type confusion attacks
52 +
53 +## Security Observations
54 +
55 +### Medium
56 +
57 +**No Content-Security-Policy (CSP) header**
58 +- CSP provides defense-in-depth against XSS and data injection
59 +- Restricts sources for scripts, styles, images, fonts, etc.
60 +- Recommendation: add CSP header via Plug
61 + - Start with `Content-Security-Policy-Report-Only` for testing
62 + - Example: `default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline';`
63 + - Gradually tighten as issues are resolved
64 +
65 +**X-Frame-Options not explicitly set**
66 +- `put_secure_browser_headers` may set this, but verify it's not being stripped
67 +- `X-Frame-Options: DENY` prevents clickjacking attacks
68 +- Recommendation: explicitly set header via Plug
69 + - `put_resp_header(conn, "x-frame-options", "deny")`
70 +
71 +**X-Content-Type-Options not explicitly set**
72 +- `put_secure_browser_headers` may set this, but verify it's not being stripped
73 +- `X-Content-Type-Options: nosniff` prevents MIME-type sniffing
74 +- Recommendation: explicitly set header
75 + - `put_resp_header(conn, "x-content-type-options", "nosniff")`
76 +
77 +**No CORS configuration**
78 +- Phoenix default applies (permissive)
79 +- API endpoints may be vulnerable to cross-origin attacks
80 +- Recommendation: add CORS headers for API endpoints
81 + - `cors_plug` library or custom Plug
82 + - Restrict to known origins for public APIs
83 + - Block all origins for private APIs
84 +
85 +**LiveDashboard accessible in development**
86 +- Phoenix LiveDashboard at `/dashboard/*`
87 +- Exposes runtime metrics, process info, logger, IEx console
88 +- **Critical**: IEx console allows arbitrary code execution
89 +- Recommendation: disable `dev_routes` in production
90 + - `config :git_gud, GitGudWeb.Endpoint, dev_routes: false`
91 + - Or restrict to admin users only via plug
92 +
93 +**MailboxPreview accessible in development**
94 +- Swoosh MailboxPreview at `/dev/mailbox`
95 +- Exposes received emails (including magic links with tokens)
96 +- Recommendation: disable in production
97 + - `config :swoosh, local: false`
98 +
99 +### Low
100 +
101 +**Error messages may leak implementation details**
102 +- Some error responses include `inspect(reason)` which may expose internal state
103 +- Example: `actions_artifact_controller.ex:99` — `Jason.encode!(%{"error" => inspect(reason)})`
104 +- Recommendation: use generic error messages for 4xx responses
105 +- Only expose detailed errors for 5xx responses in development mode
106 +
107 +**Stack traces in development**
108 +- Phoenix may display stack traces in development
109 +- Could leak sensitive information (file paths, database queries, etc.)
110 +- Recommendation: ensure `debug_errors: true` is disabled in production
111 + - `config :phoenix, :debug_errors, false`
112 +
113 +**No centralized error monitoring**
114 +- Sentry integration exists (`GitGud.SentryClient`) but not examined in detail
115 +- Verify error monitoring is configured in production
116 +- Recommendation: ensure Sentry DSN is configured via `SENTRY_DSN` environment variable
117 +- Add custom error handlers for critical paths
added security-review/file-upload-storage.md
+118 −0
@@ -0,0 +1,118 @@
1 +# File Upload & Storage Security
2 +
3 +## Implemented Controls
4 +
5 +### Storage Architecture
6 +**`lib/git_gud/storage/`**
7 +- `Storage.Disk` — Local filesystem storage
8 +- `Storage.Garage` — S3-compatible object storage
9 +- Unified interface for put/get/delete/presign operations
10 +- No multipart upload support — assembly happens in memory for chunked uploads
11 +
12 +### Storage Controller
13 +**`lib/git_gud_web/controllers/storage_controller.ex`**
14 +- Signed/capability tokens for file access
15 +- `Disk.verify_token/2` — validates token before file access
16 +- Token contains bucket + key binding (prevents path traversal)
17 +- Presigned URLs with configurable expiry
18 +
19 +### Upload Size Limits
20 +| Component | Limit | File/Line |
21 +|-----------|-------|-----------|
22 +| Runner proto body | 50MB | `runner_controller.ex:700` |
23 +| Artifact upload | 1GB | `actions_artifact_controller.ex:76` |
24 +| Cache chunk upload | 200MB | `actions_cache_controller.ex:91` |
25 +| Package upload | 200MB | `PackageRegistryController` |
26 +| LFS objects | Configurable | `LfsController` |
27 +
28 +### Runner Artifact Storage
29 +**`lib/git_gud_web/controllers/actions_artifact_controller.ex`**
30 +- Authenticated by runner JWT (`RunnerJwtAuth` plug)
31 +- Run-scoped: job only accesses its own run's artifacts (`own_run/2`)
32 +- Storage key sanitized: `String.replace(name, ~r/[^A-Za-z0-9._\-]/, "_")`
33 +- Storage path: `gitgud-artifacts/run-<run_id>/<sanitized_name>`
34 +
35 +### Runner Cache Storage
36 +**`lib/git_gud_web/controllers/actions_cache_controller.ex`**
37 +- Authenticated by runner JWT
38 +- Entry-scoped: only the job that reserved can upload (`reserved_by?/2`)
39 +- Upload expiry: 600 seconds (10 minutes)
40 +- Chunk validation: `Content-Range` header parsed and validated
41 +- Finalization: chunks assembled in offset order, no gaps allowed
42 +
43 +### Package Registry Uploads
44 +**`lib/git_gud_web/controllers/package_registry_controller.ex`**
45 +- Size limit: 200MB per upload
46 +- Content-type validation based on package format
47 +- Authentication via registry token or PAT
48 +- Format-specific parsing (`Formats.Deb`, `Formats.Rpm`, `Formats.Hex`, etc.)
49 +
50 +## Security Observations
51 +
52 +### High
53 +
54 +**No file type validation in storage controller**
55 +- `StorageController` relies on upstream validation
56 +- No magic byte checking (e.g., PE/ELF/Mach-O for binaries)
57 +- No MIME type verification against declared Content-Type
58 +- Recommendation: add content-type validation based on actual file content
59 +- Use `:erlang.md5/1` or similar to check magic bytes for known formats
60 +
61 +**No virus/malware scanning**
62 +- Uploaded artifacts, packages, and LFS objects not scanned
63 +- Malicious binaries can be distributed through the forge
64 +- Recommendation: integrate malware scanner (e.g., ClamAV)
65 +- Scan on upload, quarantine flagged files
66 +- Consider async scanning with upload allowed pending scan
67 +
68 +**Large body reads can cause memory exhaustion**
69 +- `actions_artifact_controller.ex:76` — 1GB read per upload
70 + - Multiple concurrent uploads can exhaust server memory
71 + - Recommendation: add per-user concurrent upload limits
72 + - Consider streaming uploads directly to object storage
73 +- `actions_cache_controller.ex:219` — Cache finalization assembles chunks in memory
74 + - Large cache entries loaded entirely into memory
75 + - Recommendation: implement streaming assembly or limit max cache entry size
76 +
77 +### Medium
78 +
79 +**Content-type spoofing in artifact downloads**
80 +- `actions_artifact_controller.ex:150` — Downloads return `artifact.content_type` from database
81 +- Attacker can set arbitrary `content_type` during artifact creation
82 +- If artifacts served inline in browser, could enable XSS via HTML/JS content types
83 +- Recommendation: validate and sanitize `content_type` field
84 +- Default to `application/octet-stream` unless explicitly set to safe MIME type
85 +- Add allowlist of allowed content types
86 +
87 +**Cache chunk upload not strictly validated**
88 +- `actions_cache_controller.ex:87-109` — Upload validates `Content-Range` header
89 +- No validation that chunk offset matches expected position
90 +- Recommendation: add server-side offset tracking per upload session
91 +- Reject chunks that don't match expected sequence
92 +
93 +**Presigned URL expiry too long**
94 +- `actions_cache.ex:46` — Download expiry: 300 seconds
95 +- `actions_cache.ex:47` — Upload expiry: 600 seconds
96 +- Long expiry increases window for URL theft/misuse
97 +- Recommendation: reduce download expiry to 60 seconds
98 +- Keep upload expiry at 300 seconds (already short)
99 +
100 +**No upload checksums**
101 +- Uploaded files not verified against client-provided checksums
102 +- Corruption during upload not detected
103 +- Recommendation: accept `Content-MD5` header from clients
104 +- Verify checksum after upload, return error on mismatch
105 +
106 +### Low
107 +
108 +**Upload URL construction**
109 +- `actions_artifact_controller.ex:184-187` — Upload URLs constructed via string interpolation
110 +- No validation that `endpoint.url()` is safe
111 +- Recommendation: use Phoenix URL helpers for URL generation
112 +- Prevent URL scheme injection
113 +
114 +**No rate limiting on uploads**
115 +- Upload endpoints have no rate limiting
116 +- Can be used for storage exhaustion attacks
117 +- Recommendation: add per-user upload limits (e.g., 10 uploads/hour)
118 +- Consider storage quota per repository/organization
added security-review/input-validation.md
+70 −0
@@ -0,0 +1,70 @@
1 +# Input Validation & Sanitization
2 +
3 +## Implemented Controls
4 +
5 +### SSRF Prevention
6 +**`lib/git_gud/outbound_url.ex`** — Comprehensive outbound URL validation
7 +
8 +Blocks the following categories of URLs when making outbound HTTP requests:
9 +- **Loopback addresses**: `127.0.0.0/8`, `::1`, `localhost`
10 +- **Link-local**: `169.254.0.0/16`
11 +- **Private ranges**: RFC 1918 (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`)
12 +- **Cloud metadata**: `169.254.169.254` (AWS, GCP, Azure metadata endpoints)
13 +- **IPv6 unique-local**: `fc00::/7`
14 +- **IPv6 multicast**: `ff00::/8`
15 +- **IPv6 reserved**: `::/128`, `::1/128`
16 +- **IPv4-mapped IPv6**: `::ffff:0:0/96`
17 +- **CGNAT**: `100.64.0.0/10`
18 +
19 +DNS rebinding protection included — resolves hostnames and validates resolved IP addresses.
20 +
21 +### Size Limits
22 +| Endpoint | Limit | File/Line |
23 +|----------|-------|-----------|
24 +| Pre-receive hooks | 1MB | `PreReceiveController` |
25 +| Package uploads | 200MB | `PackageRegistryController` |
26 +| Hook requests | 1MB body | `HookController` |
27 +| Runner proto | 50MB | `runner_controller.ex:700` |
28 +| Artifact uploads | 1GB | `actions_artifact_controller.ex:76` |
29 +| Cache chunk upload | 200MB | `actions_cache_controller.ex:91` |
30 +
31 +### Data Validation
32 +- **Ecto changesets** — all schema writes use `validate_required`, `validate_inclusion`, `unique_constraint`
33 +- **SHA validation** — pre-receive hooks validate 40-character hex SHA format
34 +- **Ref format parsing** — git ref updates validated against expected format
35 +- **Token format validation** — personal access tokens, runner tokens validated on use
36 +
37 +### HTML/Markdown Sanitization
38 +- **`lib/git_gud/markdown.ex`** — HTML sanitization before rendering user-generated content
39 + - Processes markdown input through sanitization pipeline
40 + - Strips dangerous elements (scripts, event handlers, javascript: URIs)
41 + - Uses `Floki` or `ex_html_sax` for DOM manipulation
42 + - Allows safe subset of HTML (bold, italic, links, code blocks, etc.)
43 +
44 +## Security Observations
45 +
46 +### Medium
47 +
48 +**Markdown sanitization rules need periodic review**
49 +- Sanitization configuration should be verified against latest OWASP XSS cheatsheet
50 +- New XSS vectors discovered regularly — ensure sanitization is current
51 +- Recommendation: add test cases for known XSS payloads (e.g., `<svg/onload=alert(1)>`, `<iframe src=javascript:...>`)
52 +
53 +**Size limits can cause memory exhaustion**
54 +- `actions_artifact_controller.ex:76` — `read_body/2` with `length: 1_000_000_000` (1 GB)
55 + - If multiple concurrent artifact uploads hit the limit, memory exhaustion is possible
56 + - Recommendation: add per-user or per-repo concurrent upload limits
57 +- `actions_cache_controller.ex:91` — 200MB per chunk upload
58 + - Consider chunking or streaming for very large caches
59 +
60 +**No file type validation in storage controller**
61 +- `StorageController` relies on upstream validation for file types
62 +- No virus/malware scanning pipeline
63 +- Recommendation: add content-type validation based on actual file content (magic bytes) not just headers
64 +- Consider adding malware scanning (e.g., ClamAV integration) for uploaded artifacts
65 +
66 +**No rate limiting on login/register endpoints**
67 +- Only MFA attempts tracked via `TwoFactorAttempts`
68 +- Login/register endpoints can be hammered for brute-force or credential stuffing
69 +- Recommendation: add IP-based rate limiting (e.g., 10 requests/minute per IP for login)
70 +- Recommendation: add CAPTCHA or proof-of-work after N failed attempts
added security-review/secrets-encryption.md
+146 −0
@@ -0,0 +1,146 @@
1 +# Secrets Management & Encryption
2 +
3 +## Secrets Storage
4 +
5 +### CI/Repository Secrets
6 +**`lib/git_gud/secrets/`**
7 +- `RepositorySecret` — Per-repository encrypted secrets
8 +- `OrganizationSecret` — Per-organization encrypted secrets
9 +- `RepositoryVariable` — Per-repository plaintext variables
10 +- `OrganizationVariable` — Per-organization plaintext variables
11 +- `ReservedNames` — Prevents overriding reserved names (e.g., `GITHUB_TOKEN`)
12 +
13 +**Encryption**
14 +- AES-256-GCM for all secrets (authenticated encryption)
15 +- Per-row initialization vector (IV) — prevents pattern analysis
16 +- Per-row key ID (`key_id`) supports master key rotation
17 +- `GitGud.Secrets` module provides encryption/decryption wrapper
18 +
19 +**Master Key Configuration**
20 +- `lib/git_gud/secrets.ex` — Encryption wrapper
21 +- Supports multiple `master_keys` for rotation
22 +- Primary key ID tracked per secret for automatic key identification
23 +- `resolve_for/1` — decrypts all secrets for a given repository/organization in memory
24 +
25 +### Session Tokens
26 +- `lib/git_gud/accounts/user_token.ex` — Phoenix Token signed
27 +- Tokens stored in `users_tokens` table
28 +- 14-day expiry with reissue logic
29 +- Types: `:confirm`, `:reset_password`, `update_email`, `:session`
30 +
31 +### Personal Access Tokens
32 +- Stored as bcrypt hashes — cannot be recovered
33 +- Scope metadata stored separately from hash
34 +- Token comparison via `Bcrypt.verify/2`
35 +
36 +### Runner JWTs
37 +- `lib/git_gud/workflows/runner_jwt.ex` — HS256 signed JWTs
38 +- Claims: `sub`, `jti`, `tid`, `rid`, `pid`, `perm`, `exp`, `iat`, `nbf`
39 +- TTL: 24 hours (`ttl_seconds: 86400` in config)
40 +- Grace period: 5 minutes after job completion for post-job steps
41 +- Verification via `RunnerJwt.verify_active/1` — checks job is still running
42 +
43 +### Webhook Secret
44 +- 32 random bytes generated at boot
45 +- Can be overridden via `GITGUD_HOOK_SECRET` environment variable
46 +- Used for `x-gitgud-hook-token` header validation
47 +
48 +### TOTP Secrets
49 +- Encrypted with AES-256-GCM (same as secrets)
50 +- Per-user IV and key_id
51 +- Stored in `users_two_factor` table
52 +
53 +### Registry Token Signing
54 +- `lib/git_gud/packages/token.ex` — OCI registry JWT signing
55 +- Configurable algorithm: RS256 (recommended) or HS256
56 +- Private key loaded from `GITGUD_REGISTRY_PRIVATE_KEY`
57 +- Public key published for validation (`/etc/zot/registry-pub.pem`)
58 +
59 +## Encryption Details
60 +
61 +### AES-256-GCM Implementation
62 +- Used for: CI secrets, organization secrets, TOTP secrets
63 +- Per-row IV ensures identical plaintext produces different ciphertexts
64 +- Key ID enables key rotation without re-encrypting all secrets
65 +- Tag size: 128 bits (default for GCM)
66 +
67 +### RSA-SHA256 for ActivityPub
68 +- Used for: HTTP signatures in federated requests
69 +- Private key for signing outbound requests
70 +- Public key verification for inbound requests
71 +- Digest verification: `SHA-256=<base64>` for request body integrity
72 +
73 +### Bcrypt for Passwords
74 +- Used for: user passwords, recovery codes
75 +- Cost factor: default (12 rounds as of Elixir 1.15+)
76 +- Recovery codes: 10 per user, single-use, hashed before comparison
77 +
78 +## Security Observations
79 +
80 +### Critical
81 +
82 +**Insecure default secrets master key** (`config/config.exs:142-143`)
83 +```elixir
84 +config :git_gud, GitGud.Secrets,
85 + primary_key_id: "default",
86 + master_key: "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="
87 +```
88 +- All zeros as default key — if production doesn't override, all secrets are encrypted with a known key
89 +- Recommendation: require `GITGUD_SECRETS_MASTER_KEY` environment variable in production
90 +- Fail closed: refuse to start if master key is all zeros or empty
91 +
92 +**Insecure default runner JWT secret** (`config/config.exs:148`)
93 +```elixir
94 +config :git_gud, GitGud.Workflows.RunnerJwt,
95 + secret: "dev-runner-jwt-secret-do-not-use-in-prod"
96 +```
97 +- Plaintext development secret — forgeable runner tokens if not overridden
98 +- `RunnerJwt.signing_secret/0` does raise on empty secret, but only if the value is empty string, not this default
99 +- Recommendation: require `GITGUD_RUNNER_JWT_SECRET` environment variable in production
100 +- Fail closed: refuse to start if secret matches development default
101 +
102 +### High
103 +
104 +**HS256-only JWT signing** (`lib/git_gud/workflows/runner_jwt.ex:174`)
105 +- Symmetric signing — same secret signs all tokens
106 +- If leaked from any runner, attacker can mint arbitrary tokens
107 +- No key rotation at JWT layer
108 +- Recommendation: consider asymmetric signing (RS256/ES256) for JWTs
109 +- At minimum: implement periodic secret rotation with graceful transition
110 +
111 +**Variables stored in plaintext**
112 +- `RepositoryVariable` and `OrganizationVariable` stored without encryption
113 +- Acceptable by design (variables are not secrets)
114 +- Access controls must be strict to prevent information leakage
115 +- Recommendation: audit access controls on variable endpoints
116 +
117 +**No automatic key rotation**
118 +- Master key rotation requires manual intervention
119 +- Old secrets must be re-encrypted with new key on access
120 +- Recommendation: implement background worker for key rotation
121 +- Rotate at least annually
122 +
123 +**Clock skew window too permissive**
124 +- 5-minute window for ActivityPub signature verification
125 +- Recommendation: make configurable (`max_skew_seconds: 300`)
126 +- Default to 30 seconds for high-security deployments
127 +
128 +### Medium
129 +
130 +**Secrets decrypted in memory**
131 +- `GitGud.Secrets.resolve_for/1` decrypts all secrets for a job in memory
132 +- Secrets visible in memory dumps, swap, or crash dumps
133 +- Recommendation: clear secret values from memory after use
134 +- Consider `:crypto.crypto.one_time` with explicit memory zeroing
135 +
136 +**No audit logging for secret operations**
137 +- Secrets can be created/updated/deleted via web UI
138 +- No audit trail for who accessed or modified secrets
139 +- Recommendation: log all secret operations (create, read, update, delete)
140 +- Include user ID, timestamp, and operation type
141 +
142 +**Erlang distribution cookie**
143 +- `rel/env.sh.eex` generates random node name based on `POD_IP`
144 +- Cookie loaded from K8s secret (`RELEASE_COOKIE`)
145 +- If cookie shared across environments, allows node hijacking
146 +- Recommendation: use environment-specific cookies with strict access controls
added security-review/session-management.md
+74 −0
@@ -0,0 +1,74 @@
1 +# Session Management
2 +
3 +## Implemented Controls
4 +
5 +### Server-Side Session Tokens
6 +- **`lib/git_gud/accounts/user_token.ex`** — Token schema with expiry
7 + - Tokens stored in `users_tokens` table (not client-side only)
8 + - 14-day expiry window from creation
9 + - Token types: `:confirm`, `:reset_password`, `:update_email`, `:session`
10 + - Reissue logic: tokens can be renewed before expiry
11 +
12 +### Cookie Sessions
13 +- **Signed cookies** with Phoenix Token
14 + - Session data stored in signed cookies (tamper-proof)
15 + - `signing_salt` configured in `endpoint.ex`
16 + - 14-day remember-me persistence
17 + - SameSite=Lax attribute on session cookies
18 + - Secure flag set for HTTPS-only transmission
19 +
20 +### Session Lifecycle
21 +- **`renew_session/2`** — Session fixation prevention
22 + - Called on login to issue new session token
23 + - Invalidates previous session token
24 + - Prevents session fixation attacks
25 +
26 +- **Session renewal on activity**
27 + - Tokens reissued after 7 days of inactivity
28 + - Extends session validity without requiring re-authentication
29 +
30 +- **`disconnect_sessions/1`** — Cross-session invalidation
31 + - Broadcasts disconnect event to all LiveView connections for a user
32 + - Terminates all browser tabs for invalidated tokens
33 + - Called when tokens are revoked (e.g., password change, admin action)
34 +
35 +### Sudo Mode
36 +- **Re-authentication** for sensitive operations
37 + - Requires current password or second-factor (TOTP/WebAuthn)
38 + - 10-minute validity window for sudo operations
39 + - Applied to: password changes, secret access, credential management
40 +
41 +## Security Observations
42 +
43 +### Medium
44 +
45 +**Hardcoded signing salt in endpoint.ex**
46 +- `config/config.exs:229` — `live_view: [signing_salt: "FRxtZcCV"]`
47 +- Salt is short (8 bytes) and appears to be a development default
48 +- While salts don't need to be secret, short salts reduce brute-force resistance
49 +- Recommendation: use environment-specific salts with at least 16 bytes
50 +- Example: `System.get_env("LIVE_VIEW_SALT", "default-change-me")`
51 +
52 +**No session binding to client fingerprint**
53 +- Sessions not bound to User-Agent or IP address
54 +- Stolen session tokens can be used from different clients
55 +- Recommendation: add optional IP binding or User-Agent validation for high-security deployments
56 +- Note: this conflicts with some legitimate use cases (IP rotation, proxy rotation)
57 +
58 +**Cookie signing salt not configurable**
59 +- `signing_salt` in `endpoint.ex` is hardcoded
60 +- If compromised, all signed cookies can be forged
61 +- Recommendation: make salt configurable via environment variable
62 +- Example: `config :git_gud, GitGudWeb.Endpoint, signing_salt: System.fetch_env!("PHOENIX_SIGNING_SALT")`
63 +
64 +### Low
65 +
66 +**Session renewal window may be too long**
67 +- 14-day expiry with 7-day renewal may be excessive for sensitive deployments
68 +- Recommendation: make expiry configurable via instance settings
69 +- Consider shorter lifetimes (e.g., 24 hours) for high-security instances
70 +
71 +**No concurrent session limit**
72 +- Users can have unlimited concurrent sessions
73 +- Recommendation: add optional concurrent session limit (e.g., 10 sessions per user)
74 +- Include UI for managing active sessions with revocation capability

Parents: 14fe8cf