Toggle password #206
Open
Toggle password #206
Conversation
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
Reviewer's GuideThis pull request implements a password visibility toggle by introducing a Symfony form type extension, registering it as a service, wiring in a Stimulus controller, updating Twig and SCSS assets, enhancing translations, updating documentation, and removing the previous UX package dependency. Sequence diagram for password visibility toggle interactionsequenceDiagram
actor User
participant PasswordInput as Password Input Field
participant ToggleButton as Toggle Button
participant StimulusController as Stimulus Controller
User->>ToggleButton: Clicks toggle button
ToggleButton->>StimulusController: Event: click
StimulusController->>PasswordInput: Change type (password/text)
StimulusController->>ToggleButton: Update label and icon
Class diagram for TogglePasswordTypeExtensionclassDiagram
class TogglePasswordTypeExtension {
- TranslatorInterface translator
+ __construct(translator: TranslatorInterface|null)
+ getExtendedTypes(): iterable
+ configureOptions(resolver: OptionsResolver): void
+ buildView(view: FormView, form: FormInterface, options: array): void
- translateLabel(label: string|TranslatableMessage|null, translationDomain: string|null): string|null
}
TogglePasswordTypeExtension --|> AbstractTypeExtension
Class diagram for Stimulus toggle_password_controller.jsclassDiagram
class TogglePasswordController {
+ visibleLabelValue: String
+ visibleIconValue: String
+ hiddenLabelValue: String
+ hiddenIconValue: String
+ buttonClassesValue: Array
- isDisplayed: Boolean
- visibleIcon: String
- hiddenIcon: String
+ connect(): void
+ createButton(): HTMLButtonElement
+ toggle(event): void
+ dispatchEvent(name: String, payload: Object): void
}
TogglePasswordController --|> Controller
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
button.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
toggleButtonElement.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- The service definition for TogglePasswordTypeExtension reuses the ‘framework.collection_type_extension’ ID and will override your collection extension; give it a unique service ID (e.g. ‘framework.toggle_password_type_extension’) and tag it appropriately.
- Using bare translation keys like “Show” and “Hide” risks collisions in large apps; consider namespacing them (e.g. ‘toggle_password.show’, ‘toggle_password.hide’) in your message files.
- In the docs snippet, the array entry 'use_toggle_form_theme' => false is missing a trailing comma—adding it will prevent a PHP parse error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The service definition for TogglePasswordTypeExtension reuses the ‘framework.collection_type_extension’ ID and will override your collection extension; give it a unique service ID (e.g. ‘framework.toggle_password_type_extension’) and tag it appropriately.
- Using bare translation keys like “Show” and “Hide” risks collisions in large apps; consider namespacing them (e.g. ‘toggle_password.show’, ‘toggle_password.hide’) in your message files.
- In the docs snippet, the array entry 'use_toggle_form_theme' => false is missing a trailing comma—adding it will prevent a PHP parse error.
## Individual Comments
### Comment 1
<location> `assets-public/controllers/toggle_password_controller.js:44` </location>
<code_context>
+ const button = document.createElement('button')
+ button.type = 'button'
+ button.classList.add(...this.buttonClassesValue)
+ button.setAttribute('tabindex', '-1')
+ button.addEventListener('click', this.toggle.bind(this))
+ button.innerHTML = `${this.visibleIcon} ${this.visibleLabelValue}`
</code_context>
<issue_to_address>
Setting tabindex to -1 prevents keyboard navigation to the toggle button.
Users who rely on keyboard navigation will be unable to access the toggle button. To maintain accessibility, use tabindex="0" or omit it.
</issue_to_address>
## Security Issues
### Issue 1
<location> `assets-public/controllers/toggle_password_controller.js:46` </location>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Issue 2
<location> `assets-public/controllers/toggle_password_controller.js:46` </location>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `button.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Issue 3
<location> `assets-public/controllers/toggle_password_controller.js:56` </location>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Issue 4
<location> `assets-public/controllers/toggle_password_controller.js:56` </location>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `toggleButtonElement.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const button = document.createElement('button') | ||
| button.type = 'button' | ||
| button.classList.add(...this.buttonClassesValue) | ||
| button.setAttribute('tabindex', '-1') |
There was a problem hiding this comment.
issue: Setting tabindex to -1 prevents keyboard navigation to the toggle button.
Users who rely on keyboard navigation will be unable to access the toggle button. To maintain accessibility, use tabindex="0" or omit it.
| button.classList.add(...this.buttonClassesValue) | ||
| button.setAttribute('tabindex', '-1') | ||
| button.addEventListener('click', this.toggle.bind(this)) | ||
| button.innerHTML = `${this.visibleIcon} ${this.visibleLabelValue}` |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| button.classList.add(...this.buttonClassesValue) | ||
| button.setAttribute('tabindex', '-1') | ||
| button.addEventListener('click', this.toggle.bind(this)) | ||
| button.innerHTML = `${this.visibleIcon} ${this.visibleLabelValue}` |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a button.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
Comment on lines
+56
to
+58
| toggleButtonElement.innerHTML = this.isDisplayed | ||
| ? `${this.hiddenIcon} ${this.hiddenLabelValue}` | ||
| : `${this.visibleIcon} ${this.visibleLabelValue}` |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
Comment on lines
+56
to
+58
| toggleButtonElement.innerHTML = this.isDisplayed | ||
| ? `${this.hiddenIcon} ${this.hiddenLabelValue}` | ||
| : `${this.visibleIcon} ${this.visibleLabelValue}` |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a toggleButtonElement.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
tijsverkoyen
reviewed
Sep 2, 2025
| export default class extends Controller { | ||
| static values = { | ||
| visibleLabel: { type: String, default: 'Show' }, | ||
| visibleIcon: { type: String, default: 'Default' }, |
There was a problem hiding this comment.
Voor mij moeten die icons niet configureerbaar zijn. Is enkel maar bloated code. Tenzij anderen daar een andere mening over hebben natuurlijk.
| const button = document.createElement('button') | ||
| button.type = 'button' | ||
| button.classList.add(...this.buttonClassesValue) | ||
| button.setAttribute('tabindex', '-1') |
| 'use_toggle_form_theme' => false | ||
|
|
||
| // Customizing labels and icons | ||
| 'hidden_label' => 'Masquer', |
There was a problem hiding this comment.
Eventueel ook echt met translation service in voorbeeld steken. Zodat mensen het kunnen kopiëren.
|
|
||
| ## Password Visibility Toggle | ||
|
|
||
| This works automatically when you use the `PasswordType` or `RepeatedPasswordStrengthType` in your form type. |
There was a problem hiding this comment.
Zet een voorbeeld zodat mensen het eenvoudig kunnen kopiëren .
|
|
||
| 'Text copied!': 'Test copied!' | ||
| 'Postcode and municipality': Postcode and municipality | ||
| Show: Show |
There was a problem hiding this comment.
Misschien beter Show password en Hide password gebruiken als labels. Die zijn minder generiek.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by Sourcery
Add a configurable password visibility toggle to Symfony forms by implementing a PasswordType extension, Stimulus controller, accompanying assets, translations, and documentation, replacing the previous UX dependency
New Features:
Enhancements:
Build:
Documentation: