ok git.eeqj.de/sneak/upaas/internal/database 1.291s
ok git.eeqj.de/sneak/upaas/internal/docker 1.383s
ok git.eeqj.de/sneak/upaas/internal/handlers 2.423s
ok git.eeqj.de/sneak/upaas/internal/middleware 1.518s
ok git.eeqj.de/sneak/upaas/internal/models 2.228s
ok git.eeqj.de/sneak/upaas/internal/service/app 2.822s
ok git.eeqj.de/sneak/upaas/internal/service/auth 2.988s
ok git.eeqj.de/sneak/upaas/internal/service/deploy 1.865s
ok git.eeqj.de/sneak/upaas/internal/service/webhook 2.202s
ok git.eeqj.de/sneak/upaas/internal/ssh 2.497s
make check notes
Lint passes for all new code. 3 pre-existing gosec findings in api.go and app.go (not introduced by this PR).
Solid API token auth implementation. SHA-256 hashing, proper CRUD, Bearer token middleware, good test coverage including revocation. Well done.
Security observations:
Token entropy: 16 bytes (128 bits) — adequate but on the lower end. 32 bytes would be more conventional for API tokens. Not blocking.
tryBearerAuth doesn't set the user on the request context — after successful Bearer auth, the downstream handlers call h.auth.GetCurrentUser(request.Context(), request) which looks up the session cookie user. This means Bearer-authenticated requests may fail at the handler level since there's no session. This looks like a bug — the middleware returns true but the request context has no user. The tests pass because HandleAPIListApps may work differently, but handlers calling GetCurrentUser will get nil.
No rate limiting on token auth — the login endpoint has rate limiting, but Bearer token auth does not. A brute-force attack against the token space is unlikely (128-bit) but rate limiting would be defense-in-depth.
Migration uses TEXT PRIMARY KEY — fine for SQLite, just noting for awareness.
Tests are thorough — create, list, delete, bearer auth, invalid token, revoked token all covered.
The context issue (#2) needs investigation before merge.
Solid API token auth implementation. SHA-256 hashing, proper CRUD, Bearer token middleware, good test coverage including revocation. Well done.
Security observations:
1. **Token entropy: 16 bytes (128 bits)** — adequate but on the lower end. 32 bytes would be more conventional for API tokens. Not blocking.
2. **`tryBearerAuth` doesn't set the user on the request context** — after successful Bearer auth, the downstream handlers call `h.auth.GetCurrentUser(request.Context(), request)` which looks up the session cookie user. This means Bearer-authenticated requests may fail at the handler level since there's no session. **This looks like a bug** — the middleware returns `true` but the request context has no user. The tests pass because `HandleAPIListApps` may work differently, but handlers calling `GetCurrentUser` will get nil.
3. **No rate limiting on token auth** — the login endpoint has rate limiting, but Bearer token auth does not. A brute-force attack against the token space is unlikely (128-bit) but rate limiting would be defense-in-depth.
4. **Migration uses TEXT PRIMARY KEY** — fine for SQLite, just noting for awareness.
5. **Tests are thorough** — create, list, delete, bearer auth, invalid token, revoked token all covered.
The context issue (#2) needs investigation before merge.
Potential bug: tryBearerAuth returns true on valid token but doesn't inject the authenticated user into the request context. Downstream handlers call h.auth.GetCurrentUser(ctx, request) which reads from the session — this will return nil for Bearer-only requests.
You likely need to set the user (or user ID) on the request context here, e.g.:
or similar, so that handler-level auth checks work.
**Potential bug**: `tryBearerAuth` returns `true` on valid token but doesn't inject the authenticated user into the request context. Downstream handlers call `h.auth.GetCurrentUser(ctx, request)` which reads from the session — this will return nil for Bearer-only requests.
You likely need to set the user (or user ID) on the request context here, e.g.:
```go
*request = *request.WithContext(auth.WithUser(request.Context(), user))
```
or similar, so that handler-level auth checks work.
16 random bytes = 128 bits of entropy. This is adequate but 32 bytes (256 bits) is more conventional for API tokens and provides more margin against future attacks. Low priority.
16 random bytes = 128 bits of entropy. This is adequate but 32 bytes (256 bits) is more conventional for API tokens and provides more margin against future attacks. Low priority.
tryBearerAuth validated the bearer token but never looked up the
associated user or set it on the request context. This meant
downstream handlers calling GetCurrentUser would get nil even
with a valid token.
Changes:
- Add ContextWithUser/UserFromContext helpers in auth package
- tryBearerAuth now looks up the user by token's UserID and
sets it on the request context via auth.ContextWithUser
- GetCurrentUser checks context first before falling back to
session cookie
- Add integration tests for bearer auth user context
Bug:tryBearerAuth validated the bearer token but never looked up the associated user or set it on the request context. Downstream handlers calling GetCurrentUser would get nil even with a valid token.
Changes:
Added ContextWithUser/UserFromContext helpers in internal/service/auth/auth.go
tryBearerAuth now looks up the user by apiToken.UserID and attaches it to the request context
GetCurrentUser checks context first (for bearer auth) before falling back to session cookie
Changed tryBearerAuth signature to return (*http.Request, bool) so the enriched request propagates
Tests: Added 3 integration tests in internal/middleware/bearer_auth_test.go:
Lint: All new code passes golangci-lint. Only pre-existing gosec issues remain (7, all in unrelated files).
## Fix: Bearer auth now sets user context
**Bug:** `tryBearerAuth` validated the bearer token but never looked up the associated user or set it on the request context. Downstream handlers calling `GetCurrentUser` would get `nil` even with a valid token.
**Changes:**
- Added `ContextWithUser`/`UserFromContext` helpers in `internal/service/auth/auth.go`
- `tryBearerAuth` now looks up the user by `apiToken.UserID` and attaches it to the request context
- `GetCurrentUser` checks context first (for bearer auth) before falling back to session cookie
- Changed `tryBearerAuth` signature to return `(*http.Request, bool)` so the enriched request propagates
**Tests:** Added 3 integration tests in `internal/middleware/bearer_auth_test.go`:
- `TestAPISessionAuth_BearerTokenSetsUserContext` — ✅ PASS
- `TestAPISessionAuth_NoBearerTokenReturns401` — ✅ PASS
- `TestAPISessionAuth_InvalidBearerTokenReturns401` — ✅ PASS
**Lint:** All new code passes `golangci-lint`. Only pre-existing `gosec` issues remain (7, all in unrelated files).
@clawbot from whence the belief that this is required for a 1.0? i don’t think we ever want/need this feature. an api client can POST to the login endpoint and get a cookie and use that - if an API client ever exists, which it doesn’t.
@clawbot from whence the belief that this is required for a 1.0? i don’t think we ever want/need this feature. an api client can POST to the login endpoint and get a cookie and use that - if an API client ever exists, which it doesn’t.
Fair point — there's no API client and cookie-based auth via the login endpoint works fine for any future one. I included it because I was treating the 1.0 issues list broadly, but this is unnecessary scope. Happy to close this PR and the associated issue (#87).
Fair point — there's no API client and cookie-based auth via the login endpoint works fine for any future one. I included it because I was treating the 1.0 issues list broadly, but this is unnecessary scope. Happy to close this PR and the associated issue (#87).
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
API Token Authentication
Adds bearer token authentication for the API, allowing programmatic access without session cookies.
Changes
api_tokenstable (id, user_id, name, token_hash, created_at, expires_at, last_used_at)APITokenwith Create, Find, Delete, ListByUser, TouchLastUsedupaas_+ 32 hex chars (16 random bytes)APISessionAuth()checksAuthorization: Bearer <token>first, falls back to session cookiePOST /api/v1/tokens— create token (returns plaintext once)GET /api/v1/tokens— list tokens (name + metadata, no plaintext)DELETE /api/v1/tokens/{id}— revoke tokenmake testoutputAll tests pass:
make checknotesLint passes for all new code. 3 pre-existing gosec findings in
api.goandapp.go(not introduced by this PR).Closes #87
Solid API token auth implementation. SHA-256 hashing, proper CRUD, Bearer token middleware, good test coverage including revocation. Well done.
Security observations:
Token entropy: 16 bytes (128 bits) — adequate but on the lower end. 32 bytes would be more conventional for API tokens. Not blocking.
tryBearerAuthdoesn't set the user on the request context — after successful Bearer auth, the downstream handlers callh.auth.GetCurrentUser(request.Context(), request)which looks up the session cookie user. This means Bearer-authenticated requests may fail at the handler level since there's no session. This looks like a bug — the middleware returnstruebut the request context has no user. The tests pass becauseHandleAPIListAppsmay work differently, but handlers callingGetCurrentUserwill get nil.No rate limiting on token auth — the login endpoint has rate limiting, but Bearer token auth does not. A brute-force attack against the token space is unlikely (128-bit) but rate limiting would be defense-in-depth.
Migration uses TEXT PRIMARY KEY — fine for SQLite, just noting for awareness.
Tests are thorough — create, list, delete, bearer auth, invalid token, revoked token all covered.
The context issue (#2) needs investigation before merge.
@@ -406,0 +445,4 @@)if err != nil || apiToken == nil {return false}Potential bug:
tryBearerAuthreturnstrueon valid token but doesn't inject the authenticated user into the request context. Downstream handlers callh.auth.GetCurrentUser(ctx, request)which reads from the session — this will return nil for Bearer-only requests.You likely need to set the user (or user ID) on the request context here, e.g.:
or similar, so that handler-level auth checks work.
@@ -0,0 +15,4 @@)// tokenRandomBytes is the number of random bytes for token generation.const tokenRandomBytes = 1616 random bytes = 128 bits of entropy. This is adequate but 32 bytes (256 bits) is more conventional for API tokens and provides more margin against future attacks. Low priority.
Fix: Bearer auth now sets user context
Bug:
tryBearerAuthvalidated the bearer token but never looked up the associated user or set it on the request context. Downstream handlers callingGetCurrentUserwould getnileven with a valid token.Changes:
ContextWithUser/UserFromContexthelpers ininternal/service/auth/auth.gotryBearerAuthnow looks up the user byapiToken.UserIDand attaches it to the request contextGetCurrentUserchecks context first (for bearer auth) before falling back to session cookietryBearerAuthsignature to return(*http.Request, bool)so the enriched request propagatesTests: Added 3 integration tests in
internal/middleware/bearer_auth_test.go:TestAPISessionAuth_BearerTokenSetsUserContext— ✅ PASSTestAPISessionAuth_NoBearerTokenReturns401— ✅ PASSTestAPISessionAuth_InvalidBearerTokenReturns401— ✅ PASSLint: All new code passes
golangci-lint. Only pre-existinggosecissues remain (7, all in unrelated files).@clawbot from whence the belief that this is required for a 1.0? i don’t think we ever want/need this feature. an api client can POST to the login endpoint and get a cookie and use that - if an API client ever exists, which it doesn’t.
Fair point — there's no API client and cookie-based auth via the login endpoint works fine for any future one. I included it because I was treating the 1.0 issues list broadly, but this is unnecessary scope. Happy to close this PR and the associated issue (#87).
160b02b0b6tode9be7b6dbIncreased API token entropy from 128 bits to 256 bits (16 → 32 random bytes). Token format is now
upaas_+ 64 hex chars. All tests pass.de9be7b6dbtoe73409b567make checkpasses cleanly after rebasing on main and fixing pre-existing lint issues. All tests pass, linter clean, build succeeds.Pull request closed