Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent password values from being rendered into the frontend DOM for ui.text(kind="password"), while still keeping the Python-side value intact and making the password field editable without immediately overwriting the backend value.
Changes:
- Backend: mask password
initial_valuein rendered HTML and add apassword-has-valueflag. - Frontend: add masked-placeholder behavior for password inputs and remount logic keyed off
random-id. - Tests: add a unit test for password masking behavior and a new smoke test app for manual verification.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
marimo/_plugins/ui/_impl/input.py |
Sends an empty initial_value to the frontend for password inputs with a non-empty default and adds a password-has-value arg. |
frontend/src/plugins/impl/TextInputPlugin.tsx |
Uses passwordHasValue to show a masked placeholder and suppress value propagation until the user types; forces remount on cell rerun. |
tests/_plugins/ui/_impl/test_input.py |
Adds a unit test ensuring password defaults aren’t embedded in the HTML output. |
marimo/_smoke_tests/inputs/password_masking.py |
Adds a smoke test app covering multiple debounce modes for password inputs. |
| is_password_with_value = kind == "password" and bool(value) | ||
| super().__init__( | ||
| component_name=text._name, | ||
| initial_value=value, | ||
| initial_value="" if is_password_with_value else value, | ||
| label=label, |
There was a problem hiding this comment.
Masking passwords by passing initial_value="" changes the UIElement's frontend initial value and therefore breaks behaviors that depend on the element’s initial frontend value (notably cloning via _clone() used by .form(), .batch(), ui.array, etc.). A cloned password input will initialize with an empty value (losing the provided default), and a form submit without edits would submit an empty string and overwrite the backend’s stored password. Consider implementing masking at HTML serialization / frontend protocol level (e.g., separate “frontend_initial_value” from the backend value, or a sentinel meaning “unchanged”) so clones/forms preserve the backend value without embedding it in the DOM.
There was a problem hiding this comment.
I think this is valid @dmadisetti . If I do
form = mo.ui.text(value="Hello!", kind="password").form()
formform.valueThe initial value doesn't appear
| const [valueOnBlur, setValueOnBlur] = useState(props.value); | ||
| const valueToValidate = valueOnBlur == null ? value : valueOnBlur; | ||
| const isValid = validate(props.kind, valueToValidate); |
There was a problem hiding this comment.
valueOnBlur is initialized from props.value but never updated when props.debounce === true (the OnBlurredInput path doesn’t call setValueOnBlur). As a result, isValid can remain stuck on the initial value and the invalid border state won’t reflect user-entered email/url values after blur/Enter. Updating valueOnBlur when props.value changes (or deriving valueToValidate directly from the displayed/internal value for the OnBlurredInput case) would fix this.
| # HTML should NOT contain the real password | ||
| assert "secret123" not in t.text | ||
| # data-initial-value should be empty string | ||
| assert "data-initial-value" in t.text | ||
|
|
There was a problem hiding this comment.
The test comment says data-initial-value should be an empty string, but the assertion only checks that the attribute exists. This would still pass if the initial value were accidentally populated (as long as it doesn’t contain the exact substring secret123). Consider asserting the actual initial value is empty (e.g., via parsing the data-initial-value attribute) to make the test match the intended guarantee.
| const value = masked ? "" : props.value; | ||
| const placeholder = masked ? MASK_PLACEHOLDER : props.placeholder; | ||
| const setValue: Setter<T> = masked | ||
| ? (v) => { | ||
| if (!hasTyped.current) { | ||
| return; | ||
| } | ||
| setMasked(false); | ||
| props.setValue(v); | ||
| } | ||
| : props.setValue; |
There was a problem hiding this comment.
The masking flow sets value to an empty string while masked is true and suppresses setValue until typing begins. With the backend now holding a non-empty password that the frontend doesn’t know, any higher-level components that read the UI registry/initial value (e.g., forms) can treat the value as empty and overwrite the stored password on submit unless the protocol supports an “unchanged password” sentinel. It may be worth extending the contract so “password has value but not revealed” round-trips safely through form submit / buffered states without clearing the backend value.
| is_password_with_value = kind == "password" and bool(value) | ||
| super().__init__( | ||
| component_name=text._name, | ||
| initial_value=value, | ||
| initial_value="" if is_password_with_value else value, | ||
| label=label, |
There was a problem hiding this comment.
I think this is valid @dmadisetti . If I do
form = mo.ui.text(value="Hello!", kind="password").form()
formform.valueThe initial value doesn't appear
|
Nice catch @Light2Dark ! Screen.Recording.2026-03-26.at.3.25.12.PM.mov |
Light2Dark
left a comment
Light2Dark
left a comment
There was a problem hiding this comment.
I tested a different approach, if you do want to check it out
#8892
📝 Summary
The PR prevents the backend from passing passwords to the frontend. There's a bit of additional logic to make the text box editable in a meaningful way.