fix(auth): preserve secure cookies outside loopback HTTP

[vincentkoc]

Additional instructions

MUST: Keep Allow edits from maintainers enabled for this PR so maintainers
can help update the branch when needed.

What Problem This Solves

Closes CodeQL alerts 1 and 2 without breaking ClickClack's documented HTTP loopback authentication path.

Why This Change Was Made

The session and OAuth binding cookies already fail closed to Secure everywhere except explicit development-mode loopback HTTP. The CodeQL query tracks literal or default false values into http.Cookie.Secure; the old helper expressed the intentional loopback exception through return false.

The revised helper preserves the same request and configured-origin policy as one positive expression. HTTPS, forwarded HTTPS, configured HTTPS, non-loopback hosts, and disabled development auth remain secure. Only documented development-mode loopback HTTP omits Secure.

No alert is dismissed or suppressed.

User Impact

Local browser and desktop authentication continue to work over HTTP localhost and loopback addresses. Every non-loopback deployment keeps secure session and OAuth binding cookies.

Evidence

• go test ./apps/api/internal/httpapi
• Real net/http/cookiejar round trip proving both the OAuth binding cookie and resulting session cookie are returned over loopback HTTP
• Existing policy matrix covering production HTTP fail-closed behavior, local HTTP, public hosts, and HTTPS requests with a local configured URL
• CodeQL query contract checked in github/codeql at go/ql/lib/semmle/go/security/CookieWithoutSecure.qll
• Production LOC: +15/-17 (net -2)
• Test LOC: +64
• Changelog LOC: +1

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs changes before merge. Reviewed August 21, 2026, 8:05 AM ET / 12:05 UTC.

ClawSweeper review

What this changes

The PR makes session and OAuth browser-binding cookies unconditionally Secure and removes origin-based cookie-security inference from the API authentication flows.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

Keep open: the hardening removes ClickClack’s documented loopback-HTTP cookie compatibility path, and standard Go cookie jars do not return Secure cookies over HTTP. The PR needs a loopback cookie-jar or Electron round-trip before merge; its MEMBER author also excludes it from automated closure.

Priority: P1
Reviewed head: 019f46b5183e36b43369469ed325c43f03cbfe8f

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The patch is focused and security-motivated, but a documented loopback compatibility regression blocks merge.
Proof confidence	🌊 off-meta tidepool	Not applicable: The author is a repository MEMBER, so the external-contributor proof gate does not apply; the stated test command is nevertheless not a real loopback client trace.
Patch quality	🦐 gold shrimp (3/6)	Security review found an item that needs attention.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: The author is a repository MEMBER, so the external-contributor proof gate does not apply; the stated test command is nevertheless not a real loopback client trace.
Evidence reviewed	5 items	Unconditional cookie policy: The proposed policy sets both session and OAuth binding cookies Secure without retaining the existing loopback-HTTP exception. Supported local HTTP contract: Current documentation says session cookies default to Secure outside local development HTTP, and the desktop client explicitly supports HTTP on localhost and loopback addresses. Cookie-jar behavior disproves PR claim: Go 1.24’s standard cookie jar sends a Secure cookie only when the request scheme is HTTPS; it has no localhost or loopback exception. The PR’s raw Set-Cookie assertions do not exercise that round trip.
Findings	1 actionable finding	[P1] Retain loopback HTTP cookie compatibility
Security	Needs attention	Secure cookies can break loopback authentication: Both authentication-cookie producers now force Secure on HTTP loopback deployments, where standards-compliant jars omit those cookies on follow-up requests.

Live Verification

Command: GOMODCACHE=/tmp/clickclack-go-mod GOCACHE=/tmp/clickclack-go-build go run ./apps/api/cmd/clickclack version

Result: FAIL (partial) — step 2 expect_output clickclack dev (: expected terminal output was not visible within 30 seconds: "clickclack dev ("

GOMODCACHE=/tmp/clickclack-go-mod GOCACHE=/tmp/clickclack-go-build go run ./apps/api/cmd/clickclack version
runner@runnervm76f27:/tmp/clawsweeper-live-proof-171-Q77gy0/target$ GOMODCACHE=/tmp/clickclack-go-mod GOCACHE=/tmp/clickclack-go-build go run ./apps/api/cmd/cli
ckclack version
go: downloading go1.26.6 (linux/amd64)
GOMODCACHE=/tmp/clickclack-go-mod GOCACHE=/tmp/clickclack-go-build go run ./apps/api/cmd/clickclack version
go: downloading github.com/coder/websocket v1.8.15
go: downloading github.com/go-chi/chi/v5 v5.3.1
go: downloading github.com/golang-jwt/jwt/v5 v5.3.1
go: downloading golang.org/x/oauth2 v0.36.0
go: downloading github.com/jackc/pgx/v5 v5.10.0
go: downloading github.com/oklog/ulid/v2 v2.1.2
go: downloading modernc.org/sqlite v1.56.0
go: downloading github.com/lib/pq v1.12.3
go: downloading github.com/jackc/puddle/v2 v2.2.2
go: downloading github.com/jackc/pgpassfile v1.0.0
go: downloading github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761
go: downloading golang.org/x/text v0.41.0
go: downloading golang.org/x/sync v0.22.0
go: downloading golang.org/x/sys v0.47.0
go: downloading modernc.org/libc v1.75.3
go: downloading modernc.org/mathutil v1.7.1
go: downloading github.com/dustin/go-humanize v1.0.1
go: downloading github.com/google/uuid v1.6.0
go: downloading modernc.org/memory v1.12.0
go: downloading github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec

Assertions:

• FAIL expect_output: clickclack dev (

How this fits together

ClickClack’s API issues session and OAuth binding cookies after magic-link, SSO, and OAuth flows. Browsers or Electron then return those cookies on later requests to authenticate the user.

flowchart LR
  A[Login or OAuth callback] --> B[API authentication handler]
  B --> C[Cookie producer]
  C --> D{Cookie transport policy}
  D --> E[Session cookie]
  D --> F[OAuth binding cookie]
  E --> G[Browser or Electron session]
  F --> G

Loading

Before merge

• Retain loopback HTTP cookie compatibility (P1) - The documented local-development and desktop contract permits HTTP on localhost and loopback addresses, but this forces both cookies to be Secure. Go’s standard cookie jar sends Secure cookies only over HTTPS, so a loopback HTTP sign-in can lose its binding or session cookie. Restore the established exception or add real Electron/browser proof plus an explicit contract change; a header-only assertion cannot catch this.
• Resolve security concern: Secure cookies can break loopback authentication - Both authentication-cookie producers now force Secure on HTTP loopback deployments, where standards-compliant jars omit those cookies on follow-up requests.
• Resolve merge risk (P1) - On documented local HTTP origins, Secure cookies may not be returned by standards-compliant cookie jars, breaking session continuation or OAuth binding; the revised tests inspect headers rather than a client round trip.
• Complete next step (P2) - A focused repair can preserve the existing loopback contract and prove the cookie round trip before this PR proceeds.

Findings

• [P1] Retain loopback HTTP cookie compatibility — apps/api/internal/httpapi/github.go:558-588
• [medium] Secure cookies can break loopback authentication — apps/api/internal/httpapi/github.go:558

Agent review details

Security

Needs attention: The patch improves non-loopback cookie confidentiality but needs compatibility validation for the supported loopback authentication boundary.

Review metrics

Metric	Value	Why it matters
Cookie policy delta	2 cookie producers changed; production +8/-28, tests +15/-36	The small patch removes the sole local-HTTP compatibility decision from every authentication cookie path.

Merge-risk options

Maintainer options:

1. Preserve loopback HTTP compatibility (recommended)
   Restore the documented loopback-development exception and add a cookie-jar round-trip regression test while leaving non-loopback deployments fail-closed.
2. Adopt an HTTPS-only local policy
   Keep unconditional Secure cookies only after a maintainer intentionally changes the supported local-development and desktop contract and verifies real Electron behavior.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Restore the loopback HTTP exception for session and OAuth binding cookies, add a cookie-jar round-trip regression test, and run go test ./apps/api/internal/httpapi.

Technical review

Best possible solution:

Keep Secure mandatory for non-loopback deployments while preserving the established local-HTTP exception unless a real Electron and system-browser loopback flow proves it is safe to remove; add a round-trip regression test for that chosen contract.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: issue a Secure cookie from a loopback HTTP response and request that origin through Go’s standard cookie jar; its HTTPS-only send condition omits the cookie. The revised tests do not perform this round trip.

Is this the best way to solve the issue?

No. Unconditional Secure attributes discard the documented loopback HTTP policy without proving compatible client behavior; retain the exception or establish the proposed policy with real Electron/browser evidence and matching documentation.

Full review comments:

• [P1] Retain loopback HTTP cookie compatibility — apps/api/internal/httpapi/github.go:558-588
  The documented local-development and desktop contract permits HTTP on localhost and loopback addresses, but this forces both cookies to be Secure. Go’s standard cookie jar sends Secure cookies only over HTTPS, so a loopback HTTP sign-in can lose its binding or session cookie. Restore the established exception or add real Electron/browser proof plus an explicit contract change; a header-only assertion cannot catch this.
  Confidence: 0.95

Overall correctness: patch is incorrect
Overall confidence: 0.95

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against acf70f8206c5.

Labels

Label changes:

• add P1: The patch can break established local HTTP login and OAuth session continuation for real users of supported loopback deployments.
• add merge-risk: 🚨 compatibility: Existing local-development and desktop configurations explicitly support HTTP loopback origins, whose cookie behavior changes on upgrade.
• add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
• add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author is a repository MEMBER, so the external-contributor proof gate does not apply; the stated test command is nevertheless not a real loopback client trace.

Label justifications:

• P1: The patch can break established local HTTP login and OAuth session continuation for real users of supported loopback deployments.
• merge-risk: 🚨 compatibility: Existing local-development and desktop configurations explicitly support HTTP loopback origins, whose cookie behavior changes on upgrade.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author is a repository MEMBER, so the external-contributor proof gate does not apply; the stated test command is nevertheless not a real loopback client trace.

Evidence

Security concerns:

• [medium] Secure cookies can break loopback authentication — apps/api/internal/httpapi/github.go:558
  Both authentication-cookie producers now force Secure on HTTP loopback deployments, where standards-compliant jars omit those cookies on follow-up requests.
  Confidence: 0.95

Acceptance criteria:

• [P1] go test ./apps/api/internal/httpapi.
• [P1] Add a loopback HTTP cookie-jar round-trip regression that verifies the OAuth binding and resulting session are returned on the callback/follow-up request.

What I checked:

• Unconditional cookie policy: The proposed policy sets both session and OAuth binding cookies Secure without retaining the existing loopback-HTTP exception. (apps/api/internal/httpapi/github.go:558, 019f46b5183e)
• Supported local HTTP contract: Current documentation says session cookies default to Secure outside local development HTTP, and the desktop client explicitly supports HTTP on localhost and loopback addresses. (docs/features/auth.md:89, acf70f8206c5)
• Cookie-jar behavior disproves PR claim: Go 1.24’s standard cookie jar sends a Secure cookie only when the request scheme is HTTPS; it has no localhost or loopback exception. The PR’s raw Set-Cookie assertions do not exercise that round trip.
• Existing release behavior: The v0.3.1 baseline retains secureCookies and applies the loopback exception to both session and OAuth binding cookies. (apps/api/internal/httpapi/github.go:556, 18acea79465c)
• Feature history: The production-default Secure-cookie behavior appears to date to Peter Steinberger’s security fix, while Shakker’s OAuth hardening work established the surrounding OAuth and desktop flow. (apps/api/internal/httpapi/github.go:556, 936f48576207)

Likely related people:

• Peter Steinberger: History identifies the production Secure-cookie default and the current released baseline as Peter-authored work. (role: introduced cookie-policy behavior and recent area contributor; confidence: high; commits: 936f48576207, 18acea79465c; files: apps/api/internal/httpapi/github.go, apps/api/internal/httpapi/server_test.go, docs/features/auth.md)
• Shakker: Auth history shows substantial GitHub OAuth, desktop handoff, and cookie-policy work by Shakker in the affected implementation. (role: OAuth and desktop-flow contributor; confidence: medium; commits: 75768abdb205, 4042c5c98eee; files: apps/api/internal/httpapi/github.go, apps/api/internal/httpapi/github_test.go)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Restore the loopback HTTP exception or provide real Electron/browser round-trip evidence for a consciously changed contract.
• Add a regression test that sends the emitted cookie through a client jar on a loopback HTTP origin.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.