T09 · Insecure Skill Coding Practices
Error
- Location
- skill.md:80
- Finding
- SQL Injection Through Unvalidated Workflow Variable in Auto-Numbering Query<![CDATA[ ## Vulnerability Details **File Location**: `skill.md:80` **Vulnerability Type**: SQL injection through direct template interpolation **Risk Level**: High ### Vulnerable Code ```text nb_add_node(wf_id, "sql", "Generate Number", '{"dataSource": "main", "sql": "UPDATE purchase_requests SET request_no = \'PR-\' || TO_CHAR(NOW(), \'YYYY\') || \'-\' || LPAD((SELECT COALESCE(MAX(CAST(SUBSTRING(request_no FROM \'[0-9]+$\') AS INT)),0)+1 FROM purchase_requests WHERE request_no LIKE \'PR-\' || TO_CHAR(NOW(), \'YYYY\') || \'-%\')::TEXT, 3, \'0\') WHERE id = {{$context.data.id}}"}') ``` ### Technical Analysis The SQL node directly inserts `{{$context.data.id}}` into executable SQL: ```sql WHERE id = {{$context.data.id}} ``` The template does not use a bound parameter, SQL escaping, quoting, or explicit numeric type validation. If an attacker can influence the trigger context or cause a crafted identifier to reach this variable, the resulting text can alter the intended SQL statement. Because the interpolation occurs in SQL syntax rather than in a parameter value, database parsing takes place after substitution. An injected expression or statement may therefore change the `WHERE` clause or execute additional SQL, subject to the template engine, database driver, and multi-statement configuration. The auto-numbering query also uses a `MAX(...)+1` sequence-generation pattern. Concurrent workflow executions can calculate the same next number, creating duplicate values unless a unique constraint and retry strategy are present. This concurrency issue is secondary to the injection risk. ### Attack Path 1. An attacker creates or manipulates a record or trigger payload that controls the value exposed as `$context.data.id`. 2. A collection workflow starts and evaluates the raw SQL node. 3. NocoBase substitutes the attacker-controlled value directly into the `WHERE id = ...` clause. 4. The database parses the substituted content as part of the SQL statement. 5. ...[truncated 855 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Replace the raw SQL node with a NocoBase `update` node so the framework handles value binding: - Target collection: `purchase_requests` - Filter: record ID equal to the trigger record ID - Values: generated request number 2. If raw SQL is unavoidable, use the SQL node or database driver's supported bound-parameter mechanism. Do not concatenate workflow variables into SQL text. 3. Validate and normalize the identifier before database execution: - Require the expected type, such as a positive integer or canonical UUID. - Reject malformed, empty, or unexpected values. - Convert numeric identifiers to an integer before use. 4. Run the workflow under a least-privileged database account restricted to required tables and operations. 5. Add a unique constraint on `request_no`. 6. Replace `MAX(...)+1` with a database sequence, identity mechanism, or transactionally locked counter to prevent duplicate numbers during concurrent execution. 7. Add negative tests using malformed identifiers and verify that no generated SQL syntax can be influenced by trigger data. ]]>