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 maintainer review before merge. Reviewed August 23, 2026, 5:08 AM ET / 09:08 UTC.

ClawSweeper review

What this changes

The branch rewrites the session and OAuth cookie-security predicate to preserve the documented loopback-HTTP exception without a literal false return path.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

Keep open for normal maintainer review. The member-authored draft resolves the prior loopback-cookie concern while preserving the current default branch’s policy; current main and v0.3.1 still use the alert-triggering predicate form.

Priority: P1
Reviewed head: 82d0a6d8a2560af1156a5a5ee1bf4280008ba1f7

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused, policy-preserving authentication fix with appropriate regression coverage and no actionable correctness finding.
Proof confidence	🌊 off-meta tidepool	Not applicable: The contributor is a repository member, so the external-contributor proof gate does not apply; the PR body nevertheless describes a real cookie-jar round trip matching the added regression test.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: The contributor is a repository member, so the external-contributor proof gate does not apply; the PR body nevertheless describes a real cookie-jar round trip matching the added regression test.
Evidence reviewed	5 items	Current-main implementation: Current main still contains the earlier multi-return predicate, including the literal false loopback branch that this PR replaces. Policy equivalence: The branch retains the same precedence for configured HTTPS, request HTTPS, configured loopback HTTP, and request loopback HTTP, expressed as one boolean return. Existing contract and coverage: Desktop documentation permits plain HTTP only on loopback, and the existing main test matrix verifies production HTTP fails closed, local HTTP works, public hosts remain secure, and HTTPS is not downgraded.
Findings	None	None.
Security	None	None.

Live Verification

Command: go test ./apps/api/internal/httpapi -run TestAuthenticationCookiesRoundTripOnLoopbackHTTP -count=1

Result: FAIL (failed) — execution before step 1 run: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.20.0.tgz

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.20.0.tgz
node:internal/assert/utils:77
    throw err;
    ^

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert(!this.paused)

    at Parser.finish (node:internal/deps/undici/undici:7388:9)
    at TLSSocket.onHttpSocketEnd (node:internal/deps/undici/undici:7827:34)
    at TLSSocket.emit (node:events:521:24)
    at endReadableNT (node:internal/streams/readable:1736:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:90:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '==',
  diff: 'simple'
}

Node.js v24.19.0

Assertions:

• FAIL expect_output: ok

How this fits together

ClickClack’s API authentication flow issues session and OAuth browser-binding cookies. The shared cookie predicate combines configured public origins and request transport details to decide whether those cookies require HTTPS.

flowchart LR
A[Browser or desktop request] --> B[API authentication flow]
B --> C[Cookie security policy]
C --> D{Documented loopback HTTP?}
D -->|yes| E[Local HTTP cookies]
D -->|no| F[Secure cookies]
E --> G[Browser cookie jar]
F --> G

Loading

Before merge

• Complete next step (P2) - This member-authored draft needs normal maintainer sign-off; no concrete automated repair remains.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Policy and regression scope	production +15/-17; tests +64; changelog +1	The implementation is a small shared authentication-policy refactor backed by a real cookie-jar regression scenario.

Technical review

Best possible solution:

Land the focused predicate rewrite after normal member-PR review, retaining the documented loopback-only exception and the existing secure-cookie policy matrix.

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

Yes, by source inspection: current main contains the literal loopback false branch, while the branch adds a real net/http cookie-jar round trip that exercises both issued cookies over loopback HTTP.

Is this the best way to solve the issue?

Yes. The shared predicate is the narrowest maintainable place to preserve the established local-HTTP contract while retaining Secure cookies for every other origin.

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

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

Labels

Label changes:

• add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The contributor is a repository member, so the external-contributor proof gate does not apply; the PR body nevertheless describes a real cookie-jar round trip matching the added regression test.
• remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.
• remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
• remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.

Label justifications:

• P1: The PR affects browser and desktop authentication cookie delivery on the documented local-development path.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The contributor is a repository member, so the external-contributor proof gate does not apply; the PR body nevertheless describes a real cookie-jar round trip matching the added regression test.

Evidence

What I checked:

• Current-main implementation: Current main still contains the earlier multi-return predicate, including the literal false loopback branch that this PR replaces. (apps/api/internal/httpapi/github.go:570, e617cb5f1842)
• Policy equivalence: The branch retains the same precedence for configured HTTPS, request HTTPS, configured loopback HTTP, and request loopback HTTP, expressed as one boolean return. (apps/api/internal/httpapi/github.go:570, 82d0a6d8a256)
• Existing contract and coverage: Desktop documentation permits plain HTTP only on loopback, and the existing main test matrix verifies production HTTP fails closed, local HTTP works, public hosts remain secure, and HTTPS is not downgraded. (docs/desktop.md:60, e617cb5f1842)
• Release check: The latest release v0.3.1 retains the earlier predicate, so this security-analysis remediation is neither merged nor released. (apps/api/internal/httpapi/github.go:570, 18acea79465c)
• Feature history: History identifies prior local-auth and split-origin work as the closest ownership trail. (apps/api/internal/httpapi/auth.go:174, 04402c823df1)

Likely related people:

• Peter Steinberger: History includes the split frontend/API origin work and local-development authentication hardening that define this cookie-policy boundary. (role: historical authentication and origin-policy contributor; confidence: medium; commits: 04402c823df1, 0ada413abfb4; files: apps/api/internal/httpapi/auth.go, apps/api/internal/httpapi/origins.go, apps/api/internal/httpapi/github.go)
• Shakker: Recent authentication history includes OAuth cookie ambiguity and desktop OAuth hardening adjacent to the changed flow. (role: OAuth security contributor; confidence: medium; commits: 25d3ddc8a8d6, fcc653158ca1; files: apps/api/internal/httpapi/github.go, apps/api/internal/httpapi/github_test.go)

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.

History

Review history (1 earlier review cycle)

• reviewed 2026-08-21T12:05:34.538Z sha 019f46b :: needs changes before merge. :: [P1] Retain loopback HTTP cookie compatibility