Skip to content

Commit bfef075

Browse files
jet-pangclaude
andcommitted
fix: address code review issues across frontend, backend, and tests
Fix security, correctness, and robustness issues identified by deep code review, plus expand test coverage and fix OSGi test version mismatch. Frontend: track rAF IDs with disconnect cleanup, fix collaboration listener leak, replace isApiChange boolean with counter, sanitize editorId/CSS values, convert O(n*m) plugin filter to Set-based O(n), normalize URL check. Backend: sync ContentManager on setHtmlSanitizer, remove duplicate ValueChangeEvent, fix mixed CJK word count, add defensive copies for ToolbarStyle/StyleDefinition/toolbar array, allow relative upload URLs, add builder build-once guard, reject null MentionFeed, block full 127.x loopback range, remove dead code in PremiumPlugin. Tests: fix wrong default expectations, fix flaky upload test with CountDownLatch, add ~35 new tests for config round-trips, premium dependencies, autosave boundaries, word count, and build-once guard. Use dynamic project.version in OSGi test to prevent version mismatch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 21f11f6 commit bfef075

16 files changed

+449
-53
lines changed

src/main/java/com/wontlost/ckeditor/CKEditorConfig.java

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -436,8 +436,9 @@ private void validateUploadUrl(String uploadUrl) {
436436
URI uri = new URI(uploadUrl);
437437
String scheme = uri.getScheme();
438438

439+
// Allow relative URLs (no scheme) - common in Vaadin apps (e.g. "/api/upload")
439440
if (scheme == null) {
440-
throw new IllegalArgumentException("Upload URL must have a protocol scheme");
441+
return;
441442
}
442443

443444
String protocol = scheme.toLowerCase(Locale.ROOT);
@@ -474,7 +475,7 @@ private boolean isPrivateNetworkAddress(String host) {
474475
String lowerHost = host.toLowerCase(Locale.ROOT);
475476

476477
// IPv4 localhost and private addresses
477-
if (lowerHost.equals("localhost") || lowerHost.equals("127.0.0.1") ||
478+
if (lowerHost.equals("localhost") || lowerHost.startsWith("127.") ||
478479
lowerHost.startsWith("192.168.") || lowerHost.startsWith("10.") ||
479480
isPrivateClassBAddress(lowerHost) ||
480481
lowerHost.endsWith(".local") || lowerHost.endsWith(".internal")) {
@@ -588,7 +589,7 @@ private boolean isSIITIPv6PrivateAddress(String host) {
588589
* Extracts common logic to avoid duplication.
589590
*/
590591
private boolean isPrivateIPv4String(String ipv4) {
591-
return ipv4.equals("127.0.0.1") ||
592+
return ipv4.startsWith("127.") ||
592593
ipv4.startsWith("192.168.") ||
593594
ipv4.startsWith("10.") ||
594595
isPrivateClassBAddress(ipv4) ||
@@ -1337,7 +1338,7 @@ public static class MentionFeed {
13371338

13381339
public MentionFeed(String marker, String[] feed, int minimumCharacters) {
13391340
this.marker = marker;
1340-
this.feed = feed;
1341+
this.feed = java.util.Objects.requireNonNull(feed, "Mention feed items must not be null");
13411342
this.minimumCharacters = minimumCharacters;
13421343
}
13431344

@@ -1448,7 +1449,7 @@ public String getElement() {
14481449
* Get the CSS classes
14491450
*/
14501451
public String[] getClasses() {
1451-
return classes;
1452+
return classes != null ? classes.clone() : null;
14521453
}
14531454

14541455
/**
@@ -1575,7 +1576,9 @@ private ToolbarStyle(Builder builder) {
15751576
this.buttonOnBackground = builder.buttonOnBackground;
15761577
this.buttonOnColor = builder.buttonOnColor;
15771578
this.iconColor = builder.iconColor;
1578-
this.buttonStyles = builder.buttonStyles;
1579+
this.buttonStyles = builder.buttonStyles != null
1580+
? java.util.Collections.unmodifiableMap(new java.util.LinkedHashMap<>(builder.buttonStyles))
1581+
: null;
15791582
}
15801583

15811584
public static Builder builder() {
@@ -1619,7 +1622,7 @@ public String getIconColor() {
16191622
}
16201623

16211624
public Map<String, ButtonStyle> getButtonStyles() {
1622-
return buttonStyles;
1625+
return buttonStyles != null ? buttonStyles : java.util.Collections.emptyMap();
16231626
}
16241627

16251628
/**

src/main/java/com/wontlost/ckeditor/VaadinCKEditor.java

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -307,13 +307,14 @@ public void setValue(String value) {
307307
protected void setModelValue(String value, boolean fromClient) {
308308
String oldValue = this.editorData;
309309
String newValue = value != null ? value : "";
310-
// Only fire event when value actually changes
310+
// Only update when value actually changes
311311
if (java.util.Objects.equals(oldValue, newValue)) {
312312
return;
313313
}
314+
// super.setModelValue already fires ValueChangeEvent via Vaadin's AbstractField,
315+
// so we must not call fireEvent again to avoid duplicate events
314316
super.setModelValue(newValue, fromClient);
315317
this.editorData = newValue;
316-
fireEvent(new ComponentValueChangeEvent<>(this, this, oldValue, fromClient));
317318
}
318319

319320
@ClientCallable
@@ -657,6 +658,8 @@ public ErrorHandler getErrorHandler() {
657658
*/
658659
public void setHtmlSanitizer(HtmlSanitizer sanitizer) {
659660
this.htmlSanitizer = sanitizer;
661+
// Update ContentManager so getSanitizedValue() uses the new sanitizer
662+
this.contentManager = new com.wontlost.ckeditor.internal.ContentManager(sanitizer);
660663
}
661664

662665
/**
@@ -850,7 +853,7 @@ private void cancelUploadFromClient(String uploadId) {
850853
void setEditorDataInternal(String data) { this.editorData = data != null ? data : ""; }
851854
void setReadOnlyInternal(boolean readOnly) { this.readOnly = readOnly; }
852855
void setLicenseKeyInternal(String licenseKey) { this.licenseKey = licenseKey; }
853-
void setToolbarInternal(String[] toolbar) { this.toolbar = toolbar; }
856+
void setToolbarInternal(String[] toolbar) { this.toolbar = toolbar != null ? toolbar.clone() : null; }
854857
void setConfigInternal(CKEditorConfig config) { this.config = config; }
855858
CKEditorConfig getConfigInternal() { return this.config; }
856859

src/main/java/com/wontlost/ckeditor/VaadinCKEditorBuilder.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ public class VaadinCKEditorBuilder {
9797
private static final String TOOLBAR_SEPARATOR = "|";
9898

9999
private final VaadinCKEditor editor;
100+
private boolean built = false;
100101
private CKEditorPreset preset;
101102
private final Set<CKEditorPlugin> additionalPlugins = new LinkedHashSet<>();
102103
private final Set<CKEditorPlugin> removedPlugins = new HashSet<>();
@@ -539,6 +540,11 @@ public VaadinCKEditorBuilder withLicenseKey(String licenseKey) {
539540
* and plugin dependencies are not satisfied
540541
*/
541542
public VaadinCKEditor build() {
543+
if (built) {
544+
throw new IllegalStateException("Builder has already been used. Create a new builder for additional editors.");
545+
}
546+
built = true;
547+
542548
// Collect initial plugins
543549
Set<CKEditorPlugin> initialPlugins = new LinkedHashSet<>();
544550
if (preset != null && !editor.hasPlugins()) {

src/main/java/com/wontlost/ckeditor/VaadinCKEditorPremium.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ public enum PremiumPlugin {
249249

250250
PremiumPlugin(String pluginName, String... toolbarItems) {
251251
this.pluginName = pluginName;
252-
this.toolbarItems = toolbarItems != null ? toolbarItems : new String[0];
252+
this.toolbarItems = toolbarItems;
253253
}
254254

255255
/**

src/main/java/com/wontlost/ckeditor/internal/ContentManager.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,13 @@ public int getWordCount(String html) {
143143
cjkChars++;
144144
}
145145
}
146-
count += cjkChars > 0 ? cjkChars : 1;
146+
if (cjkChars > 0) {
147+
// CJK chars each count as 1 word, plus 1 for any non-CJK portion
148+
boolean hasNonCjk = word.length() > cjkChars;
149+
count += cjkChars + (hasNonCjk ? 1 : 0);
150+
} else {
151+
count += 1;
152+
}
147153
}
148154
}
149155
return count;

src/main/resources/META-INF/frontend/vaadin-ckeditor/plugin-resolver.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -531,15 +531,16 @@ export class PluginResolver {
531531
this.logger.debug(`Removed ${removed.length} conflicting/unavailable plugins:`, removed);
532532
}
533533

534-
// Separate plugins by type
534+
// Separate plugins by type (use Set for O(1) lookup)
535+
const filteredSet = new Set(filtered);
535536
const standardPluginsToLoad = pluginConfigs.filter(
536-
p => filtered.includes(p.name) && !p.premium && !p.importPath
537+
p => filteredSet.has(p.name) && !p.premium && !p.importPath
537538
);
538539
const premiumPluginsToLoad = pluginConfigs.filter(
539-
p => filtered.includes(p.name) && p.premium && !p.importPath
540+
p => filteredSet.has(p.name) && p.premium && !p.importPath
540541
);
541542
const customPluginsToLoad = pluginConfigs.filter(
542-
p => filtered.includes(p.name) && p.importPath
543+
p => filteredSet.has(p.name) && p.importPath
543544
);
544545

545546
// Load standard plugins from registry

0 commit comments

Comments
 (0)