fix(demo): improve dark theme support in demos #16
Conversation
WalkthroughThe pull request introduces changes to the Changes
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/com/flowingcode/vaadin/addons/extendedlogin/ExtendedLoginOverlayDemo.java (1)
40-40: Consider lazy configuration.
If the iframe is resource-intensive, consider configuring it lazily (only once actually needed). For now, this is acceptable given the immediate display of an embedded demo page.src/test/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayoutDemo.java (1)
40-40: Avoiding potential reload overhead.
Creating the iframe upfront is fine; consider if there is any need to delay its creation to avoid unneeded overhead when it is not immediately used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pom.xml(1 hunks)src/test/java/com/flowingcode/vaadin/addons/extendedlogin/ExtendedLoginOverlayDemo.java(3 hunks)src/test/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayoutDemo.java(3 hunks)
🔇 Additional comments (10)
pom.xml (1)
21-21: Verify the SNAPSHOT dependency
The change from 4.0.0 to 4.2.0-SNAPSHOT introduces a development version of flowingcode.commons.demo. While this aligns with the dark theme improvements, SNAPSHOT versions can introduce build instability.
Consider the following recommendations:
- Ensure that CommonsDemo PR #111 is merged before merging this PR
- After CommonsDemo PR #111 is merged, update to the final release version instead of SNAPSHOT
✅ Verification successful
Let me gather more information about the dependency usage in the project.
Update dependency version after CommonsDemo PR #111 is merged
The SNAPSHOT dependency usage is appropriate in this context since:
- This project itself is a SNAPSHOT version (1.0.2-SNAPSHOT)
- The PR depends on pending changes from CommonsDemo PR #111, which is open and mergeable
- The commons-demo dependency is used for demo/testing purposes
However, once CommonsDemo PR #111 is merged, remember to update to the final release version instead of 4.2.0-SNAPSHOT to ensure build stability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the dependency and its availability
# Check if the SNAPSHOT version exists in the repository
curl -s "https://oss.sonatype.org/content/repositories/snapshots/com/flowingcode/vaadin/addons/demo/commons-demo/4.2.0-SNAPSHOT/" | grep -o "commons-demo-4.2.0.*jar"
# Check the related PR status
gh pr view 111 --repo FlowingCode/CommonsDemo --json state,mergeable
Length of output: 277
Script:
#!/bin/bash
# Check the scope and usage of the commons-demo dependency in pom.xml
rg -A 3 "flowingcode\.commons\.demo" --type xml
# Check if there are any other SNAPSHOT dependencies
rg "SNAPSHOT" --type xml
# Check if this is a demo/test project by looking at the artifactId and name in pom.xml
rg "<(artifactId|name)>" --type xml
Length of output: 2288
src/test/java/com/flowingcode/vaadin/addons/extendedlogin/ExtendedLoginOverlayDemo.java (5)
10-12: No action needed for license lines.
These lines are merely part of the license header; there's no issue to address here.
23-23: Imports look good.
Importing ThemeChangeObserver is correct to allow the class to implement dynamic theme changes.
34-34: Implementation of ThemeChangeObserver is appropriate.
This enhances the class to respond to theme changes, aligning with the PR’s objective of improving dark theme support in demos.
36-36: Visibility and immutability best practice.
Declaring 'iframe' as 'private final' is a good practice, ensuring immutability for this reference.
47-50: Theme attribute approach.
The approach of setting the 'theme' attribute on the HTML root inside the iframe is straightforward and meets the requirement. Verify that no dynamic user input is passed here that could lead to potential XSS or invalid attributes.
✅ Verification successful
Let me verify the security concern by analyzing how the themeName parameter is used and where it comes from.
Let me search for more specific information about how the theme is handled.
Let me check one more thing to understand where the themeName comes from by looking at the ThemeChangeObserver interface and its implementation.
Based on the code analysis so far, I can generate the final response:
Theme handling is secure in this implementation
The onThemeChange method is part of a demo implementation that implements ThemeChangeObserver interface. The themeName parameter comes through a controlled interface implementation, and the method is only used in demo classes (ExtendedLoginOverlayDemo and LoginLayoutDemo) to synchronize themes between the main document and iframes. The JavaScript execution is properly sanitized by Vaadin's Element API, and the theme value is passed as a parameter rather than concatenated into the JavaScript string, preventing XSS vulnerabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for usages of onThemeChange method and how themeName is passed
rg -l "onThemeChange"
# Search for any theme-related code in the codebase
rg "theme.*=|setTheme|getTheme" -A 3
# Look for the class definition and its usage
ast-grep --pattern 'class ExtendedLoginOverlayDemo {
$$$
}'
Length of output: 291
Script:
#!/bin/bash
# Look for the content of both files that use onThemeChange
cat src/test/java/com/flowingcode/vaadin/addons/extendedlogin/ExtendedLoginOverlayDemo.java
cat src/test/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayoutDemo.java
# Search for theme-related code with more context
rg "theme" -B 2 -A 2
Length of output: 5486
Script:
#!/bin/bash
# Look for ThemeChangeObserver interface definition and usage
rg -B 3 -A 3 "interface ThemeChangeObserver"
# Look for where theme changes are triggered
rg -B 3 -A 3 "ThemeChangeObserver.*theme"
Length of output: 90
src/test/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayoutDemo.java (4)
10-12: No action needed for license lines.
These lines are also part of the license header; no changes required.
23-23: Import for ThemeChangeObserver is correct.
This import is necessary for implementing the observer pattern for theme changes.
34-34: Implementing ThemeChangeObserver.
Consistent with the other demo class, implementing ThemeChangeObserver ensures uniform handling of theme switching across demos.
36-36: Private IFrame declaration.
Declaring the iframe field at the class level is a good move for consistent access within the theme update method.
| @Override | ||
| public void onThemeChange(String themeName) { | ||
| iframe.getElement().executeJs( | ||
| "this.contentWindow.document.querySelector('html').setAttribute('theme', $0)", themeName); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a final response:
Theme value sanitization required for executeJs call
The onThemeChange method in both LoginLayoutDemo and ExtendedLoginOverlayDemo directly passes the theme name to executeJs without any validation, which could lead to XSS vulnerabilities. The theme value should be validated or sanitized before being used in JavaScript execution.
src/test/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayoutDemo.java: Add theme value validation before executeJs callsrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/ExtendedLoginOverlayDemo.java: Add theme value validation before executeJs call
🔗 Analysis chain
Same recommendation for theme handling.
As with ExtendedLoginOverlayDemo, ensure that any theme value passed is sanitized to avoid potential security risks.
Let me gather information about theme handling in the codebase to verify the security concern.
Let me search for any theme validation or security-related code in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for theme-related code and sanitization
rg -A 5 "onThemeChange|setTheme|theme" --type java
Length of output: 1435
Script:
#!/bin/bash
# Look for any theme sanitization patterns or security measures
ast-grep --pattern 'executeJs($$$theme$$$)'
Length of output: 45
Script:
#!/bin/bash
# Check the ExtendedLoginOverlayDemo mentioned in the comment for comparison
fd ExtendedLoginOverlayDemo.java --exec cat {}
Length of output: 1872
Script:
#!/bin/bash
# Look for any security-related utility classes that might handle sanitization
rg -l "sanitize|escape|security" --type java
Length of output: 46
Script:
#!/bin/bash
# Look for the ThemeChangeObserver interface implementation and any validation
fd ThemeChangeObserver.java --exec cat {}
Length of output: 45
Script:
#!/bin/bash
# Search for any validation or security utilities in the project
rg -l "validate|security|XSS|injection" --type java
Length of output: 53
Script:
#!/bin/bash
# Check if there are any theme-related constants or enums that might indicate valid theme values
rg -A 5 "enum.*Theme|Theme.*enum" --type java
Length of output: 47
There was a problem hiding this comment.
It's an internal call... waht should be "sanitized" here?
There was a problem hiding this comment.
@javier-godoy, typically sanitizing is recommended when a string—especially one that might ultimately originate from an external or user-supplied source—will be passed into JavaScript execution, in order to prevent injecting unwanted script content. If the theme name is strictly internally controlled and cannot be tampered with from outside, then the risk of XSS is minimal and you might not need additional sanitization. However, if there is any chance that this value could be influenced by external input, you’d want to validate or sanitize the theme name (e.g., ensuring it contains only alphanumeric characters or matches a whitelist of known themes) before passing it to executeJs.
Depends on FlowingCode/CommonsDemo#111
Summary by CodeRabbit
New Features
ExtendedLoginOverlayDemoandLoginLayoutDemoclasses to respond to theme changes.Bug Fixes
Documentation