Repository navigation
fix(settings): the email whitelist validates what it stores and can remove whatever it holds (#3455, #3456) - #3479
Merged
Conversation
…any row (#3455, #3456) #3455 — POST /api/settings/email-whitelist only lower-cased its input, so `a@`, `@b.com` and a 10 000-character string were stored as "emails" (and a body that was not a JSON object was a 500). The field is type=email but sits outside a <form>, so the browser's own check never ran either. - `db_models.EmailWhitelistAdd` trims and lower-cases `email` and refuses anything that is not ONE address: empty, over 254 characters, containing whitespace or control characters, or not exactly one `@` with text on both sides. The whitelist is matched by exact lower-cased address — there is no domain or wildcard entry form — so nothing supported is refused. The check is deliberately the minimal one ScheduleCreate uses (intranet and internationalised addresses stay valid); `email-validator` is available but its special-use-domain rules would refuse `user@localhost`. - The route answers 422 with a one-sentence string `detail` naming the reason (never Pydantic's error array, never the echoed value). - The same rule runs client-side (`utils/emailWhitelist.js`) inside a new settings-store action, which rejects before any request; Settings.vue shows the reason — the client rule's or the server's — beside the field with the InlineError primitive. The address cell wraps instead of stretching the table, so a pre-existing oversized row keeps its Remove button in reach. #3456 — DELETE /api/settings/email-whitelist/{email} had no path converter. The router decodes %2F before matching, so a stored value containing `/` (a legal local-part character, and present in rows that predate validation) matched no route and could never be removed. The parameter is now `{email:path}`; nothing else is registered under /email-whitelist/, so it claims no sibling (Invariant #4). The client already sent the value as one encoded segment; that now lives in `whitelistEntryUrl` and is pinned. `test_1028_settings_package.py` records the path-string change explicitly in `_RESHAPED_SINCE_SPLIT` rather than editing the frozen fork-point literal. Mutation-checked — each fix reverted, tests red, restored byte-identical: - validator removed -> 15 red in tests/unit/test_3455_email_whitelist_validation.py (test_a_value_that_is_not_one_address_is_refused_with_a_named_reason, …) - router 422 mapping removed -> 19 red (same file, incl. test_a_malformed_body_is_a_422_not_a_500) - `:path` removed -> 9 red (test_a_stored_value_is_removable_when_sent_url_encoded, test_delete_keeps_its_admin_gate) - store pre-check removed -> 9 red in src/frontend/tests/unit/emailWhitelist.spec.js (store "does not POST …" and the mounted "an invalid entry … sends nothing") - encodeURIComponent removed -> 7 red (incl. mounted "Remove on a row containing a slash DELETEs its encoded value") - Settings.vue posting directly / dropping InlineError / reporting to the page banner -> 6 red each (mounted Settings.vue tests) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 9, 2026
8 tasks done
vybe
added a commit
that referenced
this pull request
Oct 9, 2026
…e-apply default_role through settingsStore.addWhitelistEmail after #3479)
3 tasks
webmixgamer
added a commit
that referenced
this pull request
Oct 10, 2026
…erges) into the review round The remote PR branch carried two merge-train merges of dev on top of 37829f0 (#3412, #3428, #3479 and others). One conflict, Settings.vue: - the whitelist add now goes through #3479's settingsStore.addWhitelistEmail, which returns { email, existingAccountRole } so the form can still name an existing account's role (emailWhitelist.spec updated, pass-through case added); - the add's refusals use #3455's InlineError under the form, beside the account notice, and the page-wide error is no longer set (the eyeball of d9473d3: a 409 landed at the bottom of Settings); pinned in workspaceOnlyWiring.spec; - #3479's guarded workspaceOnlyUsers computed, plus this round's filter reset. test_3455's whitelist fake gains the two reads the route now makes. Re-run on the merge: targeted backend 3983 passed (134 files incl. every unit test the merges changed), frontend 5501 passed (3 known host-only files), MCP 829/831 (2 = the host's pnpm layout), CLI suites green. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
2 participants
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.
Description
EmailWhitelistAddtrims, lowercases and refuses anything that is not a single address (empty, over 254 characters, whitespace or control characters, not exactly one@with text on both sides); the route answers 422 with a one-sentence reason that never echoes the value, and a malformed body is 422 instead of 500. The same rule runs client-side before any request and the reason shows inline under the field./email-whitelist/{email:path}, so a stored value containing a slash can be removed, including rows that predate validation.test_1028_settings_package.pyrecords the reshaped path without touching the frozen fork-point literal.Known limit: a stored value of exactly
.or..may still be unremovable from a browser (untested).Part of the UI sweep epic #3471.
Related Issue
Fixes #3455
Fixes #3456
Journey Impact
Journey Impact: none: bug fix to existing behaviour found by the UI sweep; no journey promise is added or changed
Type of Change
Testing
Mutation:
tests/unit/test_3455_email_whitelist_validation.py— 15 red with the validator removed, 19 with the 422 mapping removed, 9 with:pathremoved.emailWhitelist.spec.js— 9 red with the store pre-check removed, 7 withencodeURIComponentremoved, 6 with Settings.vue posting directly, 6 with the inline error dropped. Each restored byte-identical.UI verification: on an isolated stack built from this change:
a@and a 298-character entry show an inline reason with no POST; a padded mixed-case address is stored trimmed and lowercased;DELETE /api/settings/email-whitelist/team%2Fops%40example.comreturns 200 and the row disappears; a 220-character row wraps with Remove in reach at 1440 and 768. The inline error was also checked read-only on a live local instance in light and dark.Checklist
🤖 Generated with Claude Code