Problem/Motivation

The static rule direct_db_connection (modules/audit_security/rules/static_security_rules.yml) reports every \Drupal::database( in .php, .module, .inc and .theme files as a WARNING, with the message "Direct use of \Drupal::database() was found." and the recommendation "Prefer the Entity API (Storage/Query), which respects entity access hooks and cacheability."

Problems:

- The pattern also matches the text inside string literals. On a real site all 4 DIRECT_DB_CONNECTION warnings were help texts and recommendations of the audit analyzers themselves ('recommendation' => 'Use \\Drupal::database()->query() ...'), not calls.
- The message does not say why a direct database call matters. The static wrapper itself is not a vulnerability; what has to be reviewed is that queries on entity tables bypass entity access (unpublished or access-restricted content), in classes and in procedural code (.module hooks, .theme preprocess functions) alike. A notice is the right level for such a review hint.
- The recommendation does not apply to tables that are not entities (custom tables, aggregates) and does not mention the alternatives when the query is needed (accessCheck(TRUE), entity access query tags) nor dependency injection in classes.

Steps to reproduce

1. Add $help = 'Use \Drupal::database() in procedural code'; to a class in a custom module.
2. Run drush audit:run security --format=json.
3. DIRECT_DB_CONNECTION is reported as a warning on that line.

Proposed resolution

- Add an optional rule key ignore_strings to the static rule format. When it is set, scanStaticFile() matches the rule against a copy of the PHP code whose string literals are blanked with the PHP tokenizer (T_CONSTANT_ENCAPSED_STRING contents, T_ENCAPSED_AND_WHITESPACE parts of double-quoted strings and heredocs), keeping quotes, newlines and offsets, like the comment blanking from #3630434. A simpler "no quote before the call on the line" regex was rejected because it misses common code such as $data['rows'] = \Drupal::database()->select(...).
- Enable ignore_strings for direct_db_connection and keep scanning .php, .module, .inc and .theme files, including static functions and closures: the entity access concern applies everywhere, and intentional cases can be acknowledged with an audit-ignore marker.
- Lower the severity to notice, with the message "Direct database access through \Drupal::database(). Queries on entity tables bypass entity access (unpublished or restricted content)." and a recommendation covering the Entity API with accessCheck(TRUE), the entity access query tag (node_access) and dependency injection in classes.

Inline audit-ignore markers and rule_overrides for DIRECT_DB_CONNECTION keep working (same code).

Remaining tasks

- Implement ignore_strings (rule loading, blankPhpStrings() helper, scanStaticFile()) and document it in the rules file header.
- Update the rule.
- Unit test loading the shipped rule: calls in a method, in a static batch callback, after a static arrow function, after an array key, in a .module hook and in a .theme preprocess function => reported as notices; mentions inside single- and double-quoted strings => not reported.

User interface changes

DIRECT_DB_CONNECTION is a notice with a new message and recommendation, and is no longer reported for text inside strings.

API changes

New optional key ignore_strings in rules/static_security_rules.yml.

Data model changes

None.

Comments

trebormc created an issue. See original summary.

  • trebormc committed 688c1780 on 1.x
    Issue #3630463 by trebormc: Ignore strings and report \Drupal::database...
trebormc’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.