fix: address critical code review issues for 1.0 release #32

leoafarias · 2026-01-28T14:09:15Z

T1/T2: Align SDK constraints (>=3.9.0) and fix mix version conflict (1.x vs 2.x)
T4: Fix watch task lifecycle bug - tasks were disposed after every build,
causing reuse of disposed tasks in watch mode. Added DeckBuilder.dispose()
T10: Fix path validation false positives - now checks for '..' as path
segment, not substring (allows '..config.png', rejects '../secret')
T12: Fix browser initialization race condition - concurrent calls to
_getBrowser() now share initialization future instead of launching
multiple browsers
Add code review fixes plan to .planning/

https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

- T1/T2: Align SDK constraints (>=3.9.0) and fix mix version conflict (1.x vs 2.x) - T4: Fix watch task lifecycle bug - tasks were disposed after every build, causing reuse of disposed tasks in watch mode. Added DeckBuilder.dispose() - T10: Fix path validation false positives - now checks for '..' as path segment, not substring (allows '..config.png', rejects '../secret') - T12: Fix browser initialization race condition - concurrent calls to _getBrowser() now share initialization future instead of launching multiple browsers - Add code review fixes plan to .planning/ https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

Mark completed tasks (T1, T2, T4, T10, T12) and document remaining work. https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

- Remove mix/mix_generator from root pubspec (mix_generator has no 2.x version; melos.yaml bootstrap handles dependency overrides for packages) - Fix uri_validator path traversal check to use raw string splitting instead of parsed URI segments, since Uri.parse() resolves '..' before we can detect it (e.g., file:///../../../etc/passwd -> file:///etc/passwd) - Also catches URL-encoded traversal (%2e%2e) All tests pass: 1380 passed, 11 skipped, 0 failures Analyze: no issues across all 5 packages https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

Keep mix: ^2.0.0-rc.0 in root pubspec as intended. The earlier bootstrap failure was due to Flutter SDK not being initialized; now resolves correctly via FVM symlink. mix_generator remains removed (no 2.x version exists). https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

docs-page · 2026-01-28T14:09:20Z

To view this pull requests documentation preview, visit the following URL:

docs.page/btwld/superdeck~32

Documentation is deployed and generated using docs.page.

https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

Copilot

Pull request overview

This PR addresses critical code review issues identified for the 1.0 release, focusing on SDK constraints, lifecycle management, security improvements, and concurrency fixes.

Changes:

Updated SDK constraints to >=3.9.0 and upgraded mix package from 1.7.0 to 2.0.0-rc.0
Fixed watch mode resource leaks by adding DeckBuilder.dispose() method and proper cleanup
Enhanced path traversal validation to check '..' as path segments rather than substrings
Resolved browser initialization race condition in MermaidGenerator using shared futures

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
pubspec.yaml	Updated SDK constraints and mix package version from 1.x to 2.x
pubspec.lock	Updated lockfile reflecting new dependencies and SDK constraints
packages/superdeck/lib/src/utils/uri_validator.dart	Improved path traversal detection to check segments, allowing filenames like '..config.png'
packages/core/lib/src/deck_configuration.dart	Enhanced path validation to split by path separators and check segments
packages/cli/lib/src/commands/build_command.dart	Fixed watch mode by moving builder creation earlier and adding proper disposal
packages/builder/lib/src/deck_builder.dart	Added dispose() method to clean up task resources
packages/builder/lib/src/assets/mermaid_generator.dart	Fixed race condition using shared initialization future
demo/macos/Flutter/GeneratedPluginRegistrant.swift	Auto-generated file updated (removed unused path_provider registration)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-28T14:24:30Z

packages/superdeck/lib/src/utils/uri_validator.dart

+      final rawSegments = trimmed.split('/');
+      if (rawSegments.any((s) => s == '..' || s == '%2e%2e' || s == '%2E%2E')) {


The new path validation logic correctly checks for '..' as a path segment rather than as a substring. However, there are no test cases covering the new behavior that allows filenames like '..config.png' or 'foo..bar.txt'. Consider adding test coverage for these edge cases to ensure the fix works as intended and to prevent regressions.

Copilot · 2026-01-28T14:24:30Z

packages/core/lib/src/deck_configuration.dart

+    final segments = pathValue.split(RegExp(r'[/\\]'));
+    if (segments.contains('..')) {


The path validation logic has been updated to check for '..' as a path segment rather than as a substring. However, there are no test cases in the test file that verify this new behavior works correctly for filenames containing '..' (like '..config.png' or 'foo..bar.txt'). Consider adding test coverage to ensure this fix works as intended.

Copilot · 2026-01-28T14:24:31Z

packages/builder/lib/src/assets/mermaid_generator.dart

+    // Start initialization and store the future for concurrent callers
+    _browserInitFuture = _launchBrowser();
+    try {
+      _browser = await _browserInitFuture!;


After successful browser initialization, the _browserInitFuture should be cleared (set to null) to indicate initialization is complete. While this doesn't cause a bug (since the _browser != null check returns early), it's cleaner to clear the future once initialization succeeds. Add _browserInitFuture = null; after line 279.

Suggested change

_browser = await _browserInitFuture!;

_browser = await _browserInitFuture!;

_browserInitFuture = null;

Copilot · 2026-01-28T14:24:31Z

packages/superdeck/lib/src/utils/uri_validator.dart

+      if (rawSegments.any((s) => s == '..' || s == '%2e%2e' || s == '%2E%2E')) {
        throw FormatException('Path traversal detected');


The updated path traversal check on trimmed only rejects segments equal to '..', '%2e%2e' or '%2E%2E', which leaves exploitable gaps for encoded traversal patterns like '..%2F', '%2e%2F' or mixed-case/partial encodings that become '../' after percent-decoding. Because UriValidator is explicitly used for untrusted markdown, an attacker can craft a file: or relative URI using these encodings to bypass this check and cause the app to load images from arbitrary paths (e.g. sensitive files) on the viewer's filesystem. To harden this, normalize/percent-decode each non-network path segment before validation and reject any segment that is or becomes '..' (including encoded or partially encoded variants), or use a canonicalized filesystem path and ensure it stays within an allowed base directory.

Suggested change

if (rawSegments.any((s) => s == '..' || s == '%2e%2e' || s == '%2E%2E')) {

throw FormatException('Path traversal detected');

for (final segment in rawSegments) {

// Percent-decode each segment and look for decoded ".." components,

// including cases like "..%2F" that become "../" after decoding.

final decoded = Uri.decodeComponent(segment);

final decodedParts = decoded.split(RegExp(r'[\/\\]'));

if (decodedParts.any((part) => part == '..')) {

throw FormatException('Path traversal detected');

}

claude added 4 commits January 28, 2026 12:40

docs: update code review plan with implementation progress

1ce243e

Mark completed tasks (T1, T2, T4, T10, T12) and document remaining work. https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

chore: remove code review plan file

b972840

https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

leoafarias requested a review from Copilot January 28, 2026 14:14

Copilot started reviewing on behalf of leoafarias January 28, 2026 14:15 View session

Copilot AI reviewed Jan 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: address critical code review issues for 1.0 release #32

fix: address critical code review issues for 1.0 release #32

Uh oh!

leoafarias commented Jan 28, 2026

Uh oh!

docs-page bot commented Jan 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		final rawSegments = trimmed.split('/');
		if (rawSegments.any((s) => s == '..' \|\| s == '%2e%2e' \|\| s == '%2E%2E')) {

		final segments = pathValue.split(RegExp(r'[/\\]'));
		if (segments.contains('..')) {

	_browser = await _browserInitFuture!;
	_browser = await _browserInitFuture!;
	_browserInitFuture = null;

		if (rawSegments.any((s) => s == '..' \|\| s == '%2e%2e' \|\| s == '%2E%2E')) {
		throw FormatException('Path traversal detected');

-      if (rawSegments.any((s) => s == '..' || s == '%2e%2e' || s == '%2E%2E')) {
-        throw FormatException('Path traversal detected');
+      for (final segment in rawSegments) {
+        // Percent-decode each segment and look for decoded ".." components,
+        // including cases like "..%2F" that become "../" after decoding.
+        final decoded = Uri.decodeComponent(segment);
+        final decodedParts = decoded.split(RegExp(r'[\/\\]'));
+        if (decodedParts.any((part) => part == '..')) {
+          throw FormatException('Path traversal detected');
+        }

fix: address critical code review issues for 1.0 release #32

Are you sure you want to change the base?

fix: address critical code review issues for 1.0 release #32

Uh oh!

Conversation

leoafarias commented Jan 28, 2026

Uh oh!

docs-page bot commented Jan 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants