-
Notifications
You must be signed in to change notification settings - Fork 0
feat: session ownership, role fix, SSRF fix, E2E suite #56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| DROP INDEX IF EXISTS idx_sessions_owner_id; | ||
| ALTER TABLE sessions DROP COLUMN IF EXISTS owner_id; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| -- Add owner_id to sessions for multi-session support. | ||
| -- Each session is owned by a user identified by a persistent uid cookie. | ||
| -- Existing sessions get empty owner_id (orphaned — invisible to new users). | ||
| ALTER TABLE sessions ADD COLUMN owner_id TEXT NOT NULL DEFAULT ''; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ambiguous Default Value for NOT NULL Column Using Recommended solution: ALTER TABLE sessions ADD COLUMN owner_id TEXT DEFAULT NULL;Or, if orphan marker is needed: ALTER TABLE sessions ADD COLUMN owner_id TEXT NOT NULL DEFAULT 'orphan'; |
||
|
|
||
| CREATE INDEX idx_sessions_owner_id ON sessions(owner_id, updated_at DESC); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-portable Index Definition ( The use of Recommended solution: CREATE INDEX idx_sessions_owner_id ON sessions(owner_id, updated_at); |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,22 +2,30 @@ | |
| -- Generated code will be in internal/sqlc/sessions.sql.go | ||
|
|
||
| -- name: CreateSession :one | ||
| INSERT INTO sessions (title) | ||
| VALUES ($1) | ||
| INSERT INTO sessions (title, owner_id) | ||
| VALUES ($1, sqlc.arg(owner_id)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent Parameter Usage in
|
||
| RETURNING *; | ||
|
|
||
| -- name: Session :one | ||
| SELECT id, title, created_at, updated_at | ||
| SELECT id, title, owner_id, created_at, updated_at | ||
| FROM sessions | ||
| WHERE id = $1; | ||
|
|
||
| -- name: Sessions :many | ||
| SELECT id, title, created_at, updated_at | ||
| SELECT id, title, owner_id, created_at, updated_at | ||
| FROM sessions | ||
| WHERE owner_id = sqlc.arg(owner_id) | ||
| ORDER BY updated_at DESC | ||
| LIMIT sqlc.arg(result_limit) | ||
| OFFSET sqlc.arg(result_offset); | ||
|
|
||
| -- name: SessionByIDAndOwner :one | ||
| -- Verify session exists and is owned by the given user. | ||
| -- Used for ownership checks without a separate query + comparison. | ||
| SELECT id, title, owner_id, created_at, updated_at | ||
| FROM sessions | ||
| WHERE id = sqlc.arg(session_id) AND owner_id = sqlc.arg(owner_id); | ||
|
|
||
| -- name: UpdateSessionUpdatedAt :exec | ||
| UPDATE sessions | ||
| SET updated_at = NOW() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,24 +84,20 @@ func (h *chatHandler) send(w http.ResponseWriter, r *http.Request) { | |
| return | ||
| } | ||
|
|
||
| // Resolve session from context (set by session middleware from cookie) | ||
| sessionID, ok := sessionIDFromContext(r.Context()) | ||
| if !ok { | ||
| WriteError(w, http.StatusBadRequest, "session_required", "session ID required", h.logger) | ||
| if req.SessionID == "" { | ||
| WriteError(w, http.StatusBadRequest, "session_required", "sessionId is required", h.logger) | ||
| return | ||
| } | ||
|
|
||
| // If body also specifies a session, verify it matches (defense-in-depth) | ||
| if req.SessionID != "" { | ||
| parsed, err := uuid.Parse(req.SessionID) | ||
| if err != nil { | ||
| WriteError(w, http.StatusBadRequest, "invalid_session", "invalid session ID", h.logger) | ||
| return | ||
| } | ||
| if parsed != sessionID { | ||
| WriteError(w, http.StatusForbidden, "forbidden", "session access denied", h.logger) | ||
| return | ||
| } | ||
| sessionID, err := uuid.Parse(req.SessionID) | ||
| if err != nil { | ||
| WriteError(w, http.StatusBadRequest, "invalid_session", "invalid session ID", h.logger) | ||
| return | ||
| } | ||
|
|
||
| if !h.sessionAccessAllowed(r, sessionID) { | ||
| WriteError(w, http.StatusForbidden, "forbidden", "session access denied", h.logger) | ||
| return | ||
| } | ||
|
|
||
| msgID := uuid.New().String() | ||
|
Comment on lines
84
to
103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ambiguous error reporting for session manager misconfiguration If the session manager is misconfigured (e.g., Recommendation: if h.sessions == nil || h.sessions.store == nil {
h.logger.Warn("session manager misconfigured", "sessionId", req.SessionID)
WriteError(w, http.StatusForbidden, "forbidden", "session access denied", h.logger)
return
} |
||
|
|
@@ -118,6 +114,30 @@ func (h *chatHandler) send(w http.ResponseWriter, r *http.Request) { | |
| }, h.logger) | ||
| } | ||
|
|
||
| // sessionAccessAllowed checks whether the request may access the session. | ||
| // Returns true when no session manager is configured (unit tests, CLI mode). | ||
| // When configured, verifies the session belongs to the authenticated user. | ||
| func (h *chatHandler) sessionAccessAllowed(r *http.Request, sessionID uuid.UUID) bool { | ||
| if h.sessions == nil { | ||
| return true // no session manager → allow (test/CLI mode) | ||
| } | ||
| if h.sessions.store == nil { | ||
| return false // configured but no store → deny | ||
| } | ||
|
|
||
| userID, ok := userIDFromContext(r.Context()) | ||
| if !ok || userID == "" { | ||
| return false | ||
| } | ||
|
|
||
| sess, err := h.sessions.store.Session(r.Context(), sessionID) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| return sess.OwnerID == userID | ||
| } | ||
|
|
||
| // stream handles GET /api/v1/chat/stream — SSE endpoint with JSON events. | ||
| func (h *chatHandler) stream(w http.ResponseWriter, r *http.Request) { | ||
| msgID := r.URL.Query().Get("msgId") | ||
|
|
@@ -129,14 +149,13 @@ func (h *chatHandler) stream(w http.ResponseWriter, r *http.Request) { | |
| return | ||
| } | ||
|
|
||
| // Verify session ownership | ||
| parsedID, err := uuid.Parse(sessionID) | ||
| if err != nil { | ||
| WriteError(w, http.StatusBadRequest, "invalid_session", "invalid session ID", h.logger) | ||
| return | ||
| } | ||
| ctxID, ok := sessionIDFromContext(r.Context()) | ||
| if !ok || ctxID != parsedID { | ||
|
|
||
| if !h.sessionAccessAllowed(r, parsedID) { | ||
| WriteError(w, http.StatusForbidden, "forbidden", "session access denied", h.logger) | ||
| return | ||
| } | ||
|
Comment on lines
149
to
161
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing logging for denied session access When session access is denied in the Recommendation: if !h.sessionAccessAllowed(r, parsedID) {
h.logger.Warn("session access denied", "sessionId", sessionID, "msgId", msgID)
WriteError(w, http.StatusForbidden, "forbidden", "session access denied", h.logger)
return
} |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential schema inconsistency:
Dropping the
owner_idcolumn without first removing any dependent constraints (e.g., foreign keys, triggers) may result in errors or leave the database in an inconsistent state. Ensure all dependencies are explicitly dropped before removing the column.Recommended solution:
Add statements to drop any foreign key constraints or triggers related to
owner_idprior to dropping the column: