fix: surface catalog ID in PromptBuilder#919
Conversation
dbaf1bd to
133685f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the inclusion of the active catalog ID into the system prompt within the _BasicPromptBuilder class, supported by new unit tests and updated golden files. The review feedback identifies a security concern regarding the sanitization of the catalogId, specifically noting that failing to escape backslashes and carriage returns could lead to prompt injection. Additionally, the reviewer pointed out a potential logic inconsistency where the model might expect a catalog ID even when it is null, suggesting a more robust sanitization approach and a check for instruction alignment.
| String? activeCatalogId = catalog.catalogId | ||
| ?.replaceAll('"', '\\"') | ||
| .replaceAll('\n', '\\n'); |
There was a problem hiding this comment.
The catalogId is embedded into the system prompt within double quotes. The current sanitization logic attempts to escape double quotes and newlines but fails to escape backslashes (\) and carriage returns (\r). A trailing backslash in the catalogId (e.g., foo\) will escape the closing double quote in the prompt, potentially allowing the rest of the prompt to be interpreted as part of the string or allowing an attacker to break out of the quoted section. Carriage return characters (\r) are also not escaped, which could be used for line-break injection in some LLM contexts.
Additionally, there is a logic inconsistency: the system prompt instructions (defined in SurfaceOperations) explicitly tell the model to "use the catalog ID provided in system instructions". However, if catalog.catalogId is null, this line is omitted from the prompt, leaving the model without the required ID for createSurface operations. This will likely lead to model hallucinations or errors. Consider providing a fallback ID or ensuring the instructions are only present when an ID is actually provided.
| String? activeCatalogId = catalog.catalogId | |
| ?.replaceAll('"', '\\"') | |
| .replaceAll('\n', '\\n'); | |
| String? activeCatalogId = catalog.catalogId | |
| ?.replaceAll('\\', '\\\\') | |
| .replaceAll('"', '\"') | |
| .replaceAll('\n', '\\n') | |
| .replaceAll('\r', '\\r'); |
Description
This PR Fixes #900 by surfacing the catalog ID in the system prompt generated by
PromptBuilderwhen it is provided in theCatalog.Changes
The active catalog ID is: "${catalog.catalogId}".ifcatalog.catalogIdis not null.Impact & Risks
Testing
prompt_builder_test.dartand verified that all 10 tests pass.