Fix: sign-in was impossible on plain-HTTP self-hosted installs - #6
Merged
Conversation
Two bugs, both reported from an actual phone on a home network. Neither was caught by the test suite, and both broke the project's primary deployment path while the app still looked fine. 1. The session cookie was marked Secure whenever NODE_ENV was production. Browsers silently discard a Secure cookie on an insecure origin, so every install reached over plain HTTP - http://192.168.1.20:3000, which is exactly how someone runs this on a NAS or a Pi - could render pages but never hold a session. The symptom is bizarre: pages load, then every action reports 'Not signed in'. The flag now follows the request's real protocol via x-forwarded-proto, with COOKIE_SECURE to override. 2. output: 'standalone' was set unconditionally for the Docker image, and it silently breaks 'next start'. So 'npm run build && npm start' gave everyone an app that booted, served pages, and failed on every Server Action. Next prints a warning about this that is easy to miss in build output. Standalone is now opt-in via BUILD_STANDALONE, which the Dockerfile sets and nobody else needs. The cookie decision is extracted into isSecureRequest() so it can be tested, covering the LAN case, proxy chains and the override. That is the part worth guarding: getting it wrong makes every self-hosted install unusable in a way that looks like a bug anywhere except here. Also adds scripts/demo-seed.mjs, which is what made this reproducible. Verified against a real device over LAN HTTP: joined a household, loaded the agenda, and posted both the complete and skip actions. Completion credited the right person and updated the ledger; skip rescheduled without crediting anyone. Server log clean. 119 tests passing.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs, both found by running the app from a phone on a home network. Neither was caught by the test suite, and both broke the project's primary deployment path while leaving the app looking perfectly healthy.
1. The session cookie was
Secureon plain HTTPsecure: process.env.NODE_ENV === 'production'.Browsers silently discard a
Securecookie on an insecure origin. The primary way anyone runs chorely is a container on their home network, reached at something likehttp://192.168.1.20:3000— so in production, over HTTP, the session cookie was set and immediately thrown away.The symptom is genuinely confusing: pages render fine, then every single action reports Not signed in. Nothing points at cookie flags.
NODE_ENVwas the wrong thing to key off. The flag now follows the request's actual protocol viax-forwarded-proto, so it's off on a home network and on behind a TLS-terminating proxy, withCOOKIE_SECURE=1|0to override for proxies that don't send the header.2.
output: 'standalone'silently brokenext startPR #4 set this unconditionally to keep the Docker image small. Next warns about the incompatibility, but the line is easy to lose in build output — and the failure mode is the same shape as bug 1: the server boots, serves pages, and then fails on Server Actions.
That meant
npm run build && npm starthanded everyone a broken app. Standalone is now opt-in behindBUILD_STANDALONE, which the Dockerfile sets and nobody else needs.Why this matters more than the fix
Both bugs were invisible to CI. The tests exercise the domain and the service layer, and the Docker job checks that the container serves
/— which it did, because rendering was never broken. What nothing covered was a real session doing a real mutation over a real network.The cookie decision is now extracted into
isSecureRequest()and tested directly: the bare LAN case, proxy chains likehttps,http, casing, and the override. It's a handful of trivial-looking assertions guarding a failure that makes every self-hosted install unusable.Verified
Not just "tests pass". Against a real device over LAN HTTP:
done, credited to the right person, effort awarded, ledger updatedskipped, nobody credited, next occurrence rescheduledAlso adds
scripts/demo-seed.mjs— a three-person household with a few weeks of deliberately uneven history. It's what made this reproducible, and it's what anyone evaluating the project needs, since an empty app doesn't demonstrate a balance score.119 tests passing, lint clean, typecheck clean.