Set Secure flag on session cookie in production mode (closes #5) #10

clawbot · 2026-02-08T21:05:33+01:00

clawbot commented

2026-02-08 21:05:33 +01:00

No description provided.

sneak was assigned by clawbot

2026-02-08 21:05:33 +01:00

clawbot added 1 commit 2026-02-08 21:05:33 +01:00

fix: set Secure flag on session cookie in production mode (closes #5 ) 185daab909

clawbot reviewed 2026-02-16 05:55:58 +01:00

clawbot left a comment

Overall: Clean security hardening. Correct and straightforward.

Positives

Secure: !params.Config.Debug is a clean one-liner that ties the flag to the existing debug config.
Good that debug mode still works over HTTP for local development.
Test verifies the Secure flag is set when Debug: false.

Observations

Missing negative test: The test only checks Debug: false → Secure: true. A companion test for Debug: true → Secure: false would complete the coverage and prevent regressions.
Cookie name hardcoded in test: The test searches for "upaas_session" by string literal. If the cookie name changes, this test would silently pass (no cookie found → require.NotNil catches it, so actually this is fine).
SameSite: Lax + Secure: Good combination. Lax allows top-level navigations while Secure ensures HTTPS-only transmission.
Test setup verbosity: The test duplicates a lot of service initialization code. Consider extracting a helper (similar to setupTestService but with configurable Debug flag) to reduce duplication if more auth tests are added.

Verdict: Approve. Simple, correct security improvement.

## Review: PR#10 — Set Secure flag on session cookie in production mode **Overall: Clean security hardening. Correct and straightforward.** ### Positives - `Secure: !params.Config.Debug` is a clean one-liner that ties the flag to the existing debug config. - Good that debug mode still works over HTTP for local development. - Test verifies the Secure flag is set when `Debug: false`. ### Observations 1. **Missing negative test**: The test only checks `Debug: false` → `Secure: true`. A companion test for `Debug: true` → `Secure: false` would complete the coverage and prevent regressions. 2. **Cookie name hardcoded in test**: The test searches for `"upaas_session"` by string literal. If the cookie name changes, this test would silently pass (no cookie found → `require.NotNil` catches it, so actually this is fine). 3. **`SameSite: Lax` + `Secure`**: Good combination. Lax allows top-level navigations while Secure ensures HTTPS-only transmission. 4. **Test setup verbosity**: The test duplicates a lot of service initialization code. Consider extracting a helper (similar to `setupTestService` but with configurable `Debug` flag) to reduce duplication if more auth tests are added. **Verdict: Approve.** Simple, correct security improvement.

sneak merged commit 3a2bd0e51d into main

2026-02-16 05:58:22 +01:00

sneak referenced this issue from a commit

2026-02-16 05:58:23 +01:00

Merge pull request 'Set Secure flag on session cookie in production mode (closes #5)' (#10) from clawbot/upaas:fix/issue-5 into main

sneak referenced this pull request

2026-02-16 06:34:21 +01:00

MEDIUM: Session cookie missing Secure flag — transmitted over HTTP #22

Sign in to join this conversation.

No reviewers