CBG-4966 create a one time session id for blipsync #7845

torcolvin · 2025-10-30T21:56:24Z

CBG-4966 create a one time session id for blipsync

Create a one_time: true parameter for public /db/_session end point that will return a one time session in one_time_session_id property
If handler is /ks/_blipsync check Sec-WebSocket-Protocol header for a value SyncGatewaySession={sessionID}. If found, use this for auth. Modify http.Request.Header to remove this header to remove this from logging https://github.com/couchbase/go-blip/blob/main/context.go#L256. It isn't harmful to keep it in, it will just get logged if a client connects to blip that doesn't speak any of the supported protocols. At that point the session will be invalid.

I'm not sure I like the refactoring I did in handleSessionPOST, but getUserFromSessionRequestBody was hiding a lot of errors, including guest auth. There might be a less risky way to do that, and I'm open to going over it again.

Blocks https://github.com/couchbase/couchbase-lite-js/pull/67

Pre-review checklist

Removed debug logging (fmt.Print, log.Print, ...)
Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

Link upstream PRs
Update Go module dependencies when merged

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGatewayIntegration/0000/

github-actions · 2025-10-30T21:56:33Z

Redocly previews

Copilot

Pull Request Overview

This PR implements a one-time session ID mechanism for BlipSync authentication. The feature allows clients to create single-use session credentials that expire after first use or 5 minutes, whichever comes first.

Key changes:

Added one_time parameter to the /db/_session endpoint that generates a one-time session with a 5-minute TTL
Implemented WebSocket protocol header authentication for BlipSync that extracts session IDs from Sec-WebSocket-Protocol headers
Enhanced session management to support one-time sessions that are automatically deleted upon successful authentication

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
rest/session_api.go	Refactored session creation to support one-time sessions with dedicated TTL and response formatting
rest/handler.go	Added WebSocket token extraction and one-time session authentication for BlipSync connections
rest/blip_sync.go	Defined constants for WebSocket protocol header and session ID prefix
rest/blip_sync_test.go	Added test coverage for one-time session BlipSync authentication flow
rest/oidc_api.go	Updated OIDC session creation to specify non-one-time session type
auth/session.go	Extended session model with OneTime field and implemented AuthenticateOneTimeSession method
auth/session_test.go	Added comprehensive test coverage for one-time session creation and authentication
docs/api/paths/public/db-_session.yaml	Updated OpenAPI specification with one_time parameter documentation

rest/handler.go

rest/session_api.go

auth/session.go

docs/api/paths/public/db-_session.yaml

Co-authored-by: Copilot <[email protected]>

adamcfraser

Generally looks good - a few comments/questions.

auth/session.go

adamcfraser · 2025-10-30T22:28:52Z

auth/session.go

+	if err != nil {
+		return nil, base.HTTPErrorf(http.StatusUnauthorized, "Session Invalid")
+	}
+	err = auth.datastore.Delete(auth.DocIDForSession(sessionID))


It feels like we should be checking for session.OneTime actually being set in the session here before deleting the session.

Should we let the session authenticate? I'm inclined to, but I could be flexible.

rest/blip_sync_test.go

auth/session.go

adamcfraser · 2025-10-31T16:31:22Z

rest/handler.go

+	var outputHeaders []string
+	var sessionID string
+	for _, header := range strings.Split(protocolHeaders, ",") {
+		header := strings.TrimSpace(header)


The TrimSpace here means that outputHeaders might not exactly match the incoming headers. I don't see how this would cause a problem, but it would be preferable to avoid changing anything other than the blipSessionIDPrefix header here?

adamcfraser · 2025-10-31T16:43:57Z

rest/session_api.go


-	// NOTE: handleSessionPOST doesn't handle creating users from OIDC - checkPublicAuth calls out into AuthenticateUntrustedJWT.
-	// Therefore, if by this point `h.user` is guest, this isn't creating a session from OIDC.
-	if h.db.Options.DisablePasswordAuthentication && (h.user == nil || h.user.Name() == "") {


This early return (and the associated comment) looks like it should be valid under the changes in this PR. Should we leave as-is to minimize risk of changes in behaviour?

I can put this code back, but I had trouble with this case in unit tests which primarily existed to check the case where you could fall through and accidentally validate a user as a guest after removal of user, err := h.getUserFromSessionRequestBody()

I'm pretty confident this case is checked below because it is checked when params.Name != "" is checked but I don't mind keeping this either.

adamcfraser · 2025-10-31T17:00:42Z

rest/session_api.go

+	}
+	user := h.user
+	if len(body) > 0 {
+		err := base.JSONUnmarshal(body, &params)


What's the reason to not use readJSONInto here, as was previously done?

readJSONInto will return an EOF error if there's no body - I want to read the body once.

But I'll switch to using ReadJSONFromMime to accomplish the same thing.

I couldn't even catch EOF error since

sync_gateway/rest/multipart.go

Line 48 in e886b08

err = base.HTTPErrorf(http.StatusBadRequest, "Bad JSON: %s", err.Error())

will drop the underlying error and the underlying error isn't even io.EOF anyway.

adamcfraser · 2025-10-31T17:33:55Z

rest/session_api.go

+	}
+	user := h.user
+	if len(body) > 0 {
+		err := ReadJSONFromMIME(h.rq.Header, io.NopCloser(bytes.NewReader(body)), &params)


Can we just use h.requestBody here? (and avoid reading the body above)

We can't detect if the body was empty or malformed using this because I can't identify the error from

sync_gateway/rest/multipart.go

Line 48 in e886b08

err = base.HTTPErrorf(http.StatusBadRequest, "Bad JSON: %s", err.Error())

whether it is invalid JSON or empty json - even if this error were wrapped (which it isn't), it isn't an io.EOF.

Co-authored-by: Adam Fraser <[email protected]>

CBG-4966 create a one time session id for blipsync

a0f3411

Copilot AI review requested due to automatic review settings October 30, 2025 21:56

Copilot AI reviewed Oct 30, 2025

View reviewed changes

rest/handler.go Outdated Show resolved Hide resolved

rest/session_api.go Outdated Show resolved Hide resolved

auth/session.go Show resolved Hide resolved

docs/api/paths/public/db-_session.yaml Outdated Show resolved Hide resolved

torcolvin and others added 4 commits October 30, 2025 17:57

Update rest/session_api.go

d638990

Co-authored-by: Copilot <[email protected]>

Update docs/api/paths/public/db-_session.yaml

2bc0c2f

Co-authored-by: Copilot <[email protected]>

remove wrong error check

591db1d

fix debug logging

1d0f1ff

adamcfraser requested changes Oct 30, 2025

View reviewed changes

torcolvin added 2 commits October 30, 2025 22:07

add missing tests and fix the multi value blip header

8b5c27a

fix lint

4189ce2

torcolvin requested a review from adamcfraser October 31, 2025 13:00

torcolvin assigned adamcfraser Oct 31, 2025

torcolvin added 2 commits October 31, 2025 12:00

Allow non one time sesssion to auth

c5bccd6

Switch to character supported by websocket protocol

8998924

adamcfraser requested changes Oct 31, 2025

View reviewed changes

address comments

1b54b8f

adamcfraser requested changes Oct 31, 2025

View reviewed changes

torcolvin and others added 8 commits October 31, 2025 13:39

Apply suggestion from @adamcfraser

84a42a3

Co-authored-by: Adam Fraser <[email protected]>

Use query param for one_time

069669e

remove dead code

3e52290

remove debug

e624c6d

return user object

255e00e

always delete one time sessions

8ddaff1

Ensure one time session is removed with cookie auth

11e9bc0

Merge remote-tracking branch 'origin/main' into CBG-4966

d6546c7

adamcfraser approved these changes Oct 31, 2025

View reviewed changes

torcolvin enabled auto-merge (squash) October 31, 2025 21:36

torcolvin merged commit f4f2e44 into main Oct 31, 2025
46 of 47 checks passed

torcolvin deleted the CBG-4966 branch October 31, 2025 21:40

CBG-4966 create a one time session id for blipsync #7845

CBG-4966 create a one time session id for blipsync #7845

Uh oh!

Conversation

torcolvin commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-review checklist

Dependencies (if applicable)

Uh oh!

github-actions bot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Redocly previews

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamcfraser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

torcolvin commented Oct 30, 2025 •

edited

Loading

github-actions bot commented Oct 30, 2025 •

edited

Loading