Prototype of a2ui_core library

[jacobsimionato]

No description provided.

[jacobsimionato]

I think we can do better than returning 'dynamic' here. Object at a minimum, or else some custom return type we define that is like a union type of the possible return types.

[gemini-code-assist]

Code Review

This pull request introduces the core A2UI protocol implementation for Dart, including reactive state management, JSON Pointer path handling, expression parsing, and a message processing system for UI surfaces and components. Key additions include CancellationSignal for operation cancellation, DataPath for JSON Pointer, ValueNotifier and ComputedNotifier for reactivity, an ExpressionParser for dynamic content, and a MessageProcessor to manage UI state. The GenericBinder is introduced to transform A2UI configurations into reactive properties, and a minimal catalog of UI components is provided. The review identified several critical issues: security vulnerabilities related to arbitrary expression injection in FormatStringFunction, potential stack overflows in ExpressionParser due to unchecked recursion depth, and resource exhaustion in DataModel.set from unbounded list expansion. Other significant concerns include incorrect method signatures for execute in FunctionImplementation and CapitalizeFunction, issues with DataPath's hashCode and operator== implementations, and a potential ConcurrentModificationError in CancellationSignal. Improvements are also suggested for DataPath.append's behavior, passing CancellationSignal in CatalogInvokerExtension, optimizing character checks in ExpressionParser, refactoring duplicated schema collection logic, removing a redundant notifyListeners call, and ensuring consistent error handling in SurfaceGroupModel.addSurface.

[gemini-code-assist]

The FormatStringFunction takes a value argument and parses it as an A2UI expression template using ExpressionParser. If this value is sourced from untrusted user input, an attacker can inject arbitrary expressions, including function calls and data model access, allowing them to execute any function registered in the Catalog or read sensitive data from the DataModel. To mitigate this, avoid using ExpressionParser on untrusted input or ensure the template is a trusted static string.

Additionally, the signature of the execute method does not match FunctionImplementation. The cancellationSignal parameter should be CancellationSignal? instead of dynamic. Remember to import ../common/cancellation.dart.

Suggested change

	dynamic execute(Map<String, dynamic> args, DataContext context, [dynamic cancellationSignal]) {
	final template = args['value'] as String;
	final parser = ExpressionParser();
	final parts = parser.parse(template);
	if (parts.isEmpty) return '';
	if (parts.length == 1 && parts[0] is String) return parts[0];
	return ComputedNotifier(() {
	final resolvedParts = parts.map((part) {
	if (part is String) return part;
	final listenable = context.resolveListenable(part);
	return listenable.value?.toString() ?? '';
	});
	return resolvedParts.join('');
	});
	dynamic execute(Map<String, dynamic> args, DataContext context, [CancellationSignal? cancellationSignal]) {

[gemini-code-assist]

The signature of the execute method does not match the one defined in the FunctionImplementation abstract class. The cancellationSignal parameter should be of type CancellationSignal?, not dynamic. You will also need to import ../common/cancellation.dart.

Suggested change

	dynamic execute(Map<String, dynamic> args, DataContext context, [dynamic cancellationSignal]) {
	dynamic execute(Map<String, dynamic> args, DataContext context, [CancellationSignal? cancellationSignal]) {

[gemini-code-assist]

The current implementations of operator== and hashCode have issues.

1. The hashCode implementation segments.join('/').hashCode can cause collisions. For example, DataPath(['a', 'b']) and DataPath(['a/b']) would have the same hash code but are not equal. This violates the hashCode contract and can cause problems when DataPath objects are used in hash-based collections like Map keys or Set elements.
2. The _listEquals method is a manual implementation of list comparison. Since you already have a dependency on the collection package, you can use ListEquality for a more robust and idiomatic solution for both equality and hash code generation.

Here is a suggested improvement using ListEquality which also allows removing the _listEquals method. You'll need to import package:collection/collection.dart.

Suggested change

	bool operator ==(Object other) =>
	identical(this, other) ||
	other is DataPath &&
	segments.length == other.segments.length &&
	_listEquals(segments, other.segments);
	@override
	int get hashCode => segments.join('/').hashCode;
	static bool _listEquals(List<String> a, List<String> b) {
	for (int i = 0; i < a.length; i++) {
	if (a[i] != b[i]) return false;
	}
	return true;
	}
	bool operator ==(Object other) =>
	identical(this, other) ||
	other is DataPath &&
	const ListEquality().equals(segments, other.segments);
	@override
	int get hashCode => const ListEquality().hash(segments);

[gemini-code-assist]

The DataModel.set method automatically expands lists to accommodate large indices provided in the path. If an attacker provides a path with an extremely large numeric segment (e.g., /data/1000000000/name), the code will attempt to fill the list with null values up to that index, leading to excessive memory allocation and an Out-of-Memory (OOM) crash.

Suggested change

	while (current.length <= index) {
	current.add(null);
	}
	if (index < 0 || index > 10000) {
	throw A2uiDataError("List index out of bounds: $index", path: path);
	}
	while (current.length <= index) {
	current.add(null);
	}

[gemini-code-assist]

The DataModel.set method automatically expands lists to accommodate large indices provided in the path. If an attacker provides a path with an extremely large numeric segment as the last segment (e.g., /data/1000000000), the code will attempt to fill the list with null values up to that index, leading to excessive memory allocation and an Out-of-Memory (OOM) crash.

          if (index < 0 || index > 10000) {
            throw A2uiDataError("List index out of bounds: $index", path: path);
          }
          while (current.length <= index) {
            current.add(null);
          }

[gemini-code-assist]

The ExpressionParser does not properly enforce recursion depth limits during expression parsing. While the parse method has a maxDepth check, it is bypassed because parseExpression and _parseExpressionInternal call each other recursively without checking the depth. An attacker can provide a deeply nested expression (e.g., ${${${...}}}) to cause a stack overflow and crash the application.

Suggested change

	dynamic parseExpression(String expr, [int depth = 0]) {
	final trimmed = expr.trim();
	dynamic parseExpression(String expr, [int depth = 0]) {
	if (depth > maxDepth) {
	throw A2uiExpressionError('Max recursion depth reached in parseExpression');
	}
	final trimmed = expr.trim();

[gemini-code-assist]

The logic for collecting schemas from allOf, anyOf, and oneOf is duplicated. This can be simplified into a loop to reduce code duplication and improve maintainability.

      for (final key in const ['allOf', 'anyOf', 'oneOf']) {
        if (s[key] is List) {
          for (final sub in s[key] as List) {
            if (sub is Map) collectSchemas(sub.cast<String, dynamic>());
          }
        }
      }

[gemini-code-assist]

The invoker extension method doesn't accept or pass the optional CancellationSignal to the function's execute method. This prevents the use of cancellation for function calls, which seems to be an intended feature given its presence in the FunctionImplementation.execute signature.

The invoker should be updated to accept and forward the CancellationSignal. This will likely require changes at the call sites in DataContext as well.

Suggested change

	dynamic invoker(String name, Map<String, dynamic> args, DataContext context) {
	final fn = functions[name];
	if (fn == null) {
	throw ArgumentError('Function not found: $name');
	}
	return fn.execute(args, context);
	}
	dynamic invoker(String name, Map<String, dynamic> args, DataContext context, [CancellationSignal? cancellationSignal]) {
	final fn = functions[name];
	if (fn == null) {
	throw ArgumentError('Function not found: $name');
	}
	return fn.execute(args, context, cancellationSignal);
	}

[gemini-code-assist]

The call to notifier.notifyListeners() is redundant. The ValueNotifier.value setter, which is called on the previous line, already calls notifyListeners() internally when the value changes. This redundant call could lead to listeners being invoked twice for a single data change, which might cause unexpected behavior or performance issues.

[gemini-code-assist]

Silently returning when trying to add a surface that already exists can hide potential logic errors. For consistency with other parts of the library (like MessageProcessor._processCreateSurface and SurfaceComponentsModel.addComponent), it would be better to throw an A2uiStateError in this case. You will need to import ../common/errors.dart.

Suggested change

	if (_surfaces.containsKey(surface.id)) {
	return;
	}
	if (_surfaces.containsKey(surface.id)) {
	throw A2uiStateError('Surface with id "${surface.id}" already exists.');
	}