Only offer expiry options the instance accepts - #139
Merged
Conversation
Setting MAX_EXPIRY on a real deployment broke note creation. The API enforces
it per request (as of the earlier hardening work), but the picker still listed
every option, so choosing "30 days" on an instance capped at 7 failed with a
raw 400 — `Maximum expiry is 604800 seconds`, in English, whatever the locale.
Any operator tightening retention hits this. The front simply had no idea what
the server would accept: it builds its config from the environment and never
knew about MAX_EXPIRY at all.
MAX_EXPIRY now flows into ServerConfig, the picker lists only what fits, and
the default lands on the longest allowed option instead of a hardcoded 24h that
a tighter instance would reject. A selection carried over from a looser config
snaps back into range.
Two things found while testing this:
- `MAX_FILE_SIZE=20MB` produced NaN, and `file.size > NaN` is always false —
size validation silently stopped happening. `MAX_FILE_SIZE=` produced 0,
rejecting every non-empty file. Parsing now falls back on anything unusable,
in a pure module (`$lib/server-config.ts`) that the coverage gate measures;
the route loader was excluded from it, which is why this went unnoticed.
- A blank value aborted API startup: `FOO=` in a .env, or an unset `${FOO}` in
a compose file, arrives as "" and `Number("")` is 0, which failed the
positive-integer checks with a message that never mentioned the blank. Blank
now means "use the default" for every numeric setting, while a value that is
present but invalid is still rejected.
docker-compose.dev.yml passes MAX_EXPIRY/MAX_FILE_SIZE/MAX_FILES_PER_NOTE
through, so `MAX_EXPIRY=3600 make dev` exercises a tightened instance locally.
Verified that way in a browser: the picker drops to 5 min / 1 hour, defaults to
1 hour, and note creation succeeds instead of 400-ing.
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.
Setting
MAX_EXPIRYon a real deployment broke note creation. Found by deploying, not by reading.The bug
The API enforces
MAX_EXPIRYper request since the hardening work. The picker kept listing every option regardless, so on an instance capped at 7 days, choosing "30 days" failed with:Raw, in English, whatever the user's locale. Any operator tightening retention hits this — and it is a natural thing to configure.
The root cause is that the front had no idea what the server would accept: it builds its config from the environment and never knew
MAX_EXPIRYexisted. It was indocker-compose.yml, applied by the API, and invisible to the UI.The fix
MAX_EXPIRYflows intoServerConfig, the picker lists only what fits, and the default lands on the longest allowed option rather than a hardcoded 24h a tighter instance would reject. A selection carried over from a looser config snaps back into range.Verified in a browser against
MAX_EXPIRY=3600:{"selected":"3600","options":["300=5 minutes","3600=1 heure"]}…and creating a note then succeeds, where it previously 400'd.
Two bugs found while testing this
Size validation could silently switch off.
MAX_FILE_SIZE=20MBproducedNaN, andfile.size > NaNis always false — so no file was ever rejected.MAX_FILE_SIZE=produced0, rejecting every non-empty file instead. Parsing now falls back on anything unusable.The parsing moved into
$lib/server-config.ts, a pure module the coverage gate measures. It previously lived in the route loader, which is excluded from the gate — that exclusion is exactly why this went unnoticed.A blank value aborted API startup.
FOO=in a.env, or an unset${FOO}in a compose file, arrives as"", andNumber("")is0— which failed the positive-integer checks and stopped the server with a message that never mentioned the blank. Blank now means "use the default" for every numeric setting; a value that is present but invalid is still rejected.I hit this one by adding the pass-through to
docker-compose.dev.ymland watching the API refuse to boot.Testing locally
docker-compose.dev.ymlnow forwardsMAX_EXPIRY,MAX_FILE_SIZEandMAX_FILES_PER_NOTE, so a tightened instance is one command away:876 tests at 100% coverage; lint, typecheck, build and the SDK package check all pass.