Skip to content

replace hardcoded script tags with core api #360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
oxyc wants to merge 2 commits into
base: master
Choose a base branch
from
Open

replace hardcoded script tags with core api #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29ee06e
replace hardcoded script tags with core api
oxyc Jan 20, 2026
94ef5cf
fix invalid variable reference
oxyc Jan 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 35 additions & 33 deletions src/view/frontend/scripts/cb_frame/cookiebot-js.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,39 @@
* @var array $data_regions
* @var string|bool $tcf
*/
?>
<script type="text/javascript"
id="Cookiebot"
src="https://consent.cookiebot.com/uc.js"
data-implementation="wp"
data-cbid="<?php echo esc_attr( $cbid ); ?>"
<?php if ( $tcf ) : ?>
data-framework="<?php echo esc_attr( $tcf ); ?>"
<?php endif; ?>
<?php
if ( $data_regions ) {
echo 'data-georegions="';
foreach ( $data_regions as $cbid => $regions ) {
echo '{\'region\':\'' . esc_attr( $regions ) . '\',\'cbid\':\'' . esc_attr( $cbid ) . '\'}';
end( $data_regions );
if ( $cbid !== key( $data_regions ) ) {
echo ',';
}
}
echo '"';

$script_attributes = array(
'type' => 'text/javascript',
'id' => 'Cookiebot',
'src' => 'https://consent.cookiebot.com/uc.js',
'data-implementation' => 'wp',
'data-cbid' => $cbid,
Copy link

@codeant-ai codeant-ai bot Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Unsanitized assignment to data-cbid: the value from $cbid is assigned directly to attributes array without escaping, which can lead to unsafe output; escape the value with esc_attr() when building attributes. [security]

Severity Level: Critical 🚨 
- ❌ Cookiebot frontend attribute may enable XSS.
- ⚠️ Plugin admin-entered cbid values are risk vectors.
Suggested change
'data-cbid' => $cbid,
'data-cbid' => esc_attr( $cbid ),
Steps of Reproduction ✅ 
1. In a test WordPress environment or simple PHP harness, set $cbid to a value containing
special characters, e.g. $cbid = '" onmouseover="alert(1)'. Save this value as if it were
coming from plugin configuration.

2. Include the template file: src/view/frontend/scripts/cb_frame/cookiebot-js.php. The
attribute array is built at lines 11-17 and includes 'data-cbid' => $cbid (line 16).

3. The template calls wp_print_script_tag() at line 45, which will emit the <script> with
attributes composed from $script_attributes.

4. Inspect the page source / rendered output: if $cbid is not escaped before being passed
into the attributes array, attribute content may be injected verbatim, demonstrating
unescaped attribute output and potential XSS.

Note: This reproduction references the exact assignment (line 16) and the emission point
wp_print_script_tag() (line 45) in the same file.
Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** src/view/frontend/scripts/cb_frame/cookiebot-js.php
**Line:** 16:16
**Comment:**
	*Security: Unsanitized assignment to `data-cbid`: the value from `$cbid` is assigned directly to attributes array without escaping, which can lead to unsafe output; escape the value with `esc_attr()` when building attributes.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

);

if ( $tcf ) {
$script_attributes['data-framework'] = $tcf;
}

if ( $data_regions ) {
$georegions = array();
foreach ( $data_regions as $cbid => $regions ) {
$georegions[] = '{\'region\':\'' . esc_attr( $regions ) . '\',\'cbid\':\'' . esc_attr( $cbid ) . '\'}';
Comment on lines +25 to +26
Copy link

@pantoaibot pantoaibot bot Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[REFACTORING] The foreach uses foreach ( $data_regions as $cbid => $regions ) which re-uses the outer $cbid variable name (the Cookiebot ID). Rename the loop key (e.g. $region_cbid) to avoid variable shadowing and confusion: foreach ( $data_regions as $region_cbid => $regions ) { ... }. This improves readability and avoids accidental bugs if the variable usage order changes.

if ( $data_regions ) {
	$georegions = array();
	foreach ( $data_regions as $region_cbid => $regions ) {
		$georegions[] = '{\'region\':\'' . esc_attr( $regions ) . '\',\'cbid\':\'' . esc_attr( $region_cbid ) . '\'}';
	}
	$script_attributes['data-georegions'] = implode( ',', $georegions );
}

Copy link
Author

@oxyc oxyc Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While true this case was present before this patch.

Copy link

@qodo-code-review qodo-code-review bot Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

While true this case was present before this patch.

Answer:

To reduce “this existed before the patch” style refactoring suggestions, configure /improve to focus only on issues introduced by the PR diff and avoid pure readability/naming nits unless they are required for correctness.

Add this to your .pr_agent.toml (or wiki config):

[pr_code_suggestions]
extra_instructions = """\
Only suggest changes that are directly caused by, introduced by, or modified in this PR.
Do not suggest refactors/cleanups for pre-existing code issues unless they create a real bug/security issue in the PR diff.
Prefer correctness/safety issues over readability or naming suggestions.
"""

# Optional: further reduce style-level suggestions
focus_only_on_problems = true
# Optional: filter out low-importance suggestions (tune as needed)
suggestions_score_threshold = 7

If you still want occasional refactoring suggestions, keep the threshold lower (e.g., 5–6) and remove/soften the “prefer correctness” line.

Relevant Sources:

}
?>
<?php if ( (bool) get_option( 'cookiebot-gtm' ) !== false && ! empty( get_option( 'cookiebot-data-layer' ) ) ) : ?>
data-layer-name="<?php echo esc_attr( get_option( 'cookiebot-data-layer' ) ); ?>"
<?php endif; ?>
<?php if ( ! empty( $lang ) ) : ?>
data-culture="<?php echo esc_attr( strtoupper( $lang ) ); ?>"
<?php endif; ?>
<?php if ( $cookie_blocking_mode === 'auto' ) : ?>
data-blockingmode="auto"
<?php else : ?>
<?php echo esc_attr( $tag_attr ); ?>
<?php endif; ?>
></script>
$script_attributes['data-georegions'] = implode( ',', $georegions );
}

if ( (bool) get_option( 'cookiebot-gtm' ) !== false && ! empty( get_option( 'cookiebot-data-layer' ) ) ) {
$script_attributes['data-layer-name'] = get_option( 'cookiebot-data-layer' );
Comment on lines +31 to +32
Copy link

@codeant-ai codeant-ai bot Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Unescaped option value used for data-layer-name: get_option( 'cookiebot-data-layer' ) is assigned directly to attributes array without escaping; fetch the option once, check it, and escape it with esc_attr() before use. [security]

Severity Level: Critical 🚨 
- ❌ Admin-set option may inject unsafe frontend attribute.
- ⚠️ Data layer integration could be exploited.
Suggested change
if ( (bool) get_option( 'cookiebot-gtm' ) !== false && ! empty( get_option( 'cookiebot-data-layer' ) ) ) {
$script_attributes['data-layer-name'] = get_option( 'cookiebot-data-layer' );
$layer = get_option( 'cookiebot-data-layer' );
if ( (bool) get_option( 'cookiebot-gtm' ) !== false && ! empty( $layer ) ) {
$script_attributes['data-layer-name'] = esc_attr( $layer );
Steps of Reproduction ✅ 
1. As a site admin, set the option 'cookiebot-data-layer' to a payload value via the WP
options UI or direct DB write (e.g. data like '" onclick="alert(1)').

2. Load a frontend page that renders the plugin template
src/view/frontend/scripts/cb_frame/cookiebot-js.php.

3. The conditional at line 31 checks options and (when true) assigns
get_option('cookiebot-data-layer') directly into $script_attributes at line 32.

4. The template emits the script attributes with wp_print_script_tag() at line 45; because
the option value was not escaped before assignment, the output can contain unescaped
content, demonstrating possible XSS via an admin-configured option.

Note: Steps reference concrete code locations: option fetch and assignment (lines 31-32)
and emission at line 45.
Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** src/view/frontend/scripts/cb_frame/cookiebot-js.php
**Line:** 31:32
**Comment:**
	*Security: Unescaped option value used for `data-layer-name`: `get_option( 'cookiebot-data-layer' )` is assigned directly to attributes array without escaping; fetch the option once, check it, and escape it with `esc_attr()` before use.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

Copy link
Author

@oxyc oxyc Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See wp_sanitize_script_attributes() which runs esc_attr() on all passed atribtues

Copy link

@qodo-code-review qodo-code-review bot Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

See wp_sanitize_script_attributes() which runs esc_attr() on all passed atribtues

Answer:

If wp_print_script_tag() (via wp_sanitize_script_attributes()) already escapes all attribute values with esc_attr(), then the “unescaped attribute → XSS” claim is likely a false positive (you’d have double-escaping at worst, not a security fix).

To reduce these kinds of WP-specific false positives in /improve suggestions, add an instruction to verify whether output is already sanitized/escaped by the WordPress API used at render time before flagging a security issue or proposing esc_*() changes.

[pr_code_suggestions]
extra_instructions = """\
- For WordPress/PHP code: before suggesting esc_attr()/esc_html()/esc_url() (or tagging as security/XSS), verify whether the output API already sanitizes/escapes (e.g., wp_print_script_tag() -> wp_sanitize_script_attributes()).
- If escaping is already applied by the sink function, do not recommend redundant escaping; instead, briefly mention why it's already safe.
- Prefer referencing the escaping/sanitization function in the rationale when concluding it's safe.
"""

#### Relevant Sources:

> - https://docs.qodo.ai/qodo-documentation/qodo-merge/tools/improve#extra-instructions-and-best-practices
> - https://docs.qodo.ai/qodo-documentation/qodo-merge/faq/index#-note-"q:-i-received-an-incorrect-or-irrelevant-suggestion-why"
> - https://docs.qodo.ai/qodo-documentation/qodo-merge/usage-guide/additional_configurations#extra-instructions

}

if ( ! empty( $lang ) ) {
$script_attributes['data-culture'] = strtoupper( $lang );
}

if ( $cookie_blocking_mode === 'auto' ) {
$script_attributes['data-blockingmode'] = 'auto';
} elseif ( ! empty( $tag_attr ) ) {
Comment on lines +11 to +41
Copy link

@pantoaibot pantoaibot bot Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[VALIDATION] User-supplied or option-derived values are assigned to attributes without explicit escaping (e.g. 'data-cbid', 'data-framework', 'data-layer-name', 'data-culture'). Ensure values are sanitized/escaped before insertion (use esc_attr() or cast to string) or validate them. For example: $script_attributes['data-cbid'] = esc_attr( (string) $cbid ); and $script_attributes['data-layer-name'] = esc_attr( (string) get_option('cookiebot-data-layer') ); This prevents injection or malformed attributes when rendering via wp_print_script_tag.

$script_attributes = array(
	'type'                  => 'text/javascript',
	'id'                    => 'Cookiebot',
	'src'                   => 'https://consent.cookiebot.com/uc.js',
	'data-implementation'   => 'wp',
	'data-cbid'             => esc_attr( (string) $cbid ),
);

if ( $tcf ) {
	$script_attributes['data-framework'] = esc_attr( (string) $tcf );
}

if ( $data_regions ) {
	$georegions = array();
	foreach ( $data_regions as $region_cbid => $regions ) {
		$georegions[] = '{\'region\':\'' . esc_attr( $regions ) . '\',\'cbid\':\'' . esc_attr( $region_cbid ) . '\'}';
	}
	$script_attributes['data-georegions'] = implode( ',', $georegions );
}

$data_layer = get_option( 'cookiebot-data-layer' );

if ( (bool) get_option( 'cookiebot-gtm' ) !== false && ! empty( $data_layer ) ) {
	$script_attributes['data-layer-name'] = esc_attr( (string) $data_layer );
}

if ( ! empty( $lang ) ) {
	$script_attributes['data-culture'] = esc_attr( strtoupper( $lang ) );
}

$script_attributes[ $tag_attr ] = true;
}
Comment on lines +41 to +43
Copy link

@pantoaibot pantoaibot bot Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL_BUG] This line sets $script_attributes[ $tag_attr ] = true; but $tag_attr in the original code was a prebuilt attribute string (e.g. 'type="text/plain" data-cookieconsent="..."'). Using it as an array key will produce an invalid attribute and break output. Restore the original behavior by either: (a) parsing $tag_attr into key/value pairs and merging into $script_attributes, or (b) when $cookie_blocking_mode !== 'auto' output the raw $tag_attr string instead of using wp_print_script_tag. Example actionable fix: parse attributes with preg_match_all('/([\w:-]+)(?:\s*=\s*"([^"]*)")?/', $tag_attr, $m) and set $script_attributes[$m[1][$i]] = $m[2][$i] for each match before calling wp_print_script_tag.

if ( $cookie_blocking_mode === 'auto' ) {
	$script_attributes['data-blockingmode'] = 'auto';
} elseif ( ! empty( $tag_attr ) ) {
	// Preserve legacy behavior for non-auto mode by parsing composite attributes.
	if ( preg_match_all( '/([\w:-]+)(?:\s*=\s*"([^"]*)")?/', $tag_attr, $matches, PREG_SET_ORDER ) ) {
		foreach ( $matches as $match ) {
			$attr_name  = $match[1];
			$attr_value = isset( $match[2] ) ? $match[2] : true;
			$script_attributes[ $attr_name ] = $attr_value;
		}
	}
}

wp_print_script_tag( $script_attributes );

Copy link
Author

@oxyc oxyc Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. The tag_attr comes from the database option. which can be empty, defer or async.

wp_print_script_tag() handles booleans correctly: https://github.com/WordPress/wordpress-develop/blob/6.9/src/wp-includes/script-loader.php#L2884-L2888

Copy link

@qodo-code-review qodo-code-review bot Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

This is wrong. The tag_attr comes from the database option. which can be empty, defer or async. wp_print_script_tag() handles booleans correctly: https://github.com/WordPress/wordpress-develop/blob/6.9/src/wp-includes/script-loader.php#L2884-L2888

Answer:

This looks like a classic “wrong assumption about variable provenance” issue: the suggestion assumed $tag_attr is a composite attribute string, but in your codebase it can be "", "defer" or "async", and wp_print_script_tag() handles boolean attributes correctly.

This can be reduced via configuration by tightening the /improve prompt so it:

  • verifies where variables come from (diff + surrounding context) before claiming a “critical bug”
  • avoids proposing broad parsing logic unless the diff clearly shows $tag_attr is a composite string
  • respects WP’s boolean-attribute behavior (async, defer)

Add a repo-level instruction like:

[pr_code_suggestions]
extra_instructions = """\
When reviewing PHP/WordPress code:
- Do NOT assume a variable contains a composite HTML-attribute string unless it is explicitly constructed/validated in the changed code.
- Before flagging a CRITICAL_BUG, confirm the variable’s source (e.g., options/DB values) and allowed values from nearby code/context.
- For wp_print_script_tag()/script attributes, treat 'async' and 'defer' as valid boolean attributes; don't recommend parsing them into key/value pairs unless required by the diff.
- Prefer asking for clarification over asserting breakage when the variable type/value-set is ambiguous.
"""

If you want this to apply only to /improve (and not /review), keep it under [pr_code_suggestions] as above. If you can share which tool produced the original comment (likely /improve) and which platform you’re using (GitHub/GitLab/etc.), we can tailor the wording to reduce this specific false-positive pattern even further.

Relevant Sources:


wp_print_script_tag( $script_attributes );
12 changes: 10 additions & 2 deletions src/view/frontend/scripts/common/google-consent-mode-js.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,15 @@
* @var string $script_type
*/

$script_attributes = array(
'type' => $script_type,
);
if ($consent_attribute) {
$script_attributes['data-cookieconsent'] = $consent_attribute;
}
Comment on lines +12 to +14
Copy link

@pantoaibot pantoaibot bot Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[VALIDATION] When assigning $consent_attribute into the script attributes use escaping/validation: $script_attributes['data-cookieconsent'] = esc_attr( (string) $consent_attribute ); This prevents malformed attribute values from being output in the inline script tag.

$script_attributes = array(
    'type' => $script_type,
);

if ( $consent_attribute ) {
    $script_attributes['data-cookieconsent'] = esc_attr( (string) $consent_attribute );
}

ob_start();
?>
<script>
	window.<?php echo esc_js( $data_layer ); ?> = window.<?php echo esc_js( $data_layer ); ?> || [];
	// ... rest of script body ...
</script>
<?php
wp_print_inline_script_tag( ob_get_clean(), $script_attributes );


ob_start();
?>
<script type="<?php echo esc_attr( $script_type ); ?>"<?php echo ! $consent_attribute ? '' : ' data-cookieconsent="' . esc_attr( $consent_attribute ) . '"'; ?>>
window.<?php echo esc_js( $data_layer ); ?> = window.<?php echo esc_js( $data_layer ); ?> || [];

function gtag() {
Expand All @@ -31,4 +38,5 @@ functionality_storage: "denied",
'gtag("set", "url_passthrough", true);' . PHP_EOL;
}
?>
</script>

<?php wp_print_inline_script_tag( ob_get_clean(), $script_attributes ); ?>
14 changes: 12 additions & 2 deletions src/view/frontend/scripts/common/google-tag-manager-js.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,16 @@
* @var string $script_type
* @var bool $iab
*/

$script_attributes = array(
'type' => $script_type,
);
if ($consent_attribute) {
$script_attributes['data-cookieconsent'] = $consent_attribute;
}

ob_start();
?>
<script type="<?php echo esc_attr( $script_type ); ?>"<?php echo ! $consent_attribute ? '' : ' data-cookieconsent="' . esc_attr( $consent_attribute ) . '"'; ?>>
<?php if ( $iab ) : ?>
window ["gtag_enable_tcf_support"] = true;
<?php endif; ?>
Expand All @@ -25,4 +33,6 @@
'<?php echo esc_js( $data_layer ); ?>',
'<?php echo esc_js( $gtm_id ); ?>'
);
</script>

<?php
wp_print_inline_script_tag( ob_get_clean(), $script_attributes ); ?>