Skip to content

Harden Prism bootstrap order and retry highlighting in web engine#157

Merged
PrzemyslawKlys merged 1 commit intomainfrom
codex/web-prism-engine-hardening-20260221
Feb 22, 2026
Merged

Harden Prism bootstrap order and retry highlighting in web engine#157
PrzemyslawKlys merged 1 commit intomainfrom
codex/web-prism-engine-hardening-20260221

Conversation

@PrzemyslawKlys
Copy link
Member

Summary

  • add engine-level Prism bootstrap script that sets window.Prism.manual = true before loading Prism core/autoloader
  • harden Prism init script with guarded retries so highlighting recovers when plugin/lang path init timing is delayed
  • apply the same hardening for both site-page Prism injection and API docs generation
  • add regression tests for local hashed Prism assets and CDN Prism assets in website builds
  • extend PowerShell API docs tests to assert bootstrap ordering and retry script output

Why

We hit recurring Prism inconsistencies across generated websites (especially with local/hashed asset names). This change makes the engine deterministic so themes/sites do not need per-site JavaScript workarounds.

Validation

  • dotnet test .\\PowerForge.Tests\\PowerForge.Tests.csproj -c Release --filter "WebSiteBuilderPrismAssetInjectionTests|WebApiDocsGeneratorPowerShellTests"
    • Passed (12/12)
  • dotnet test .\\PowerForge.Tests\\PowerForge.Tests.csproj -c Release
    • 1 unrelated existing failure: WebSiteSocialCardsTests.Build_DoesNotAutoGenerateCards_ForDocsPages_UnlessOverridden

@chatgpt-codex-connector
Copy link

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@claude
Copy link

claude bot commented Feb 21, 2026

PR Review: Harden Prism bootstrap order and retry highlighting

Overall this is a well-motivated change that fixes a real timing issue. The approach — injecting window.Prism.manual=true before loading Prism core, then retrying highlighting — is correct and aligned with how Prism recommends manual mode be used.

A few findings worth discussing:


Bug: attempts incremented before early-exit guards (both files)

In both BuildPrismInitScript and BuildApiPrismInitScript, the counter is incremented unconditionally at the top of run(), but the function can return early before the setTimeout retry is scheduled:

var run=function(){
  attempts++;                             // consumed here
  var root=...;
  var p=window.Prism;
  if(!p||!hasCode(root)){return;}         // exits without scheduling retry
  ...
  if(!hasTokens(root)&&attempts<maxAttempts){setTimeout(run,delayMs);}
};

If window.Prism is undefined on the first call (e.g., the script tag failed to load or is async), all 8 attempts are exhausted immediately with no retry scheduled — because the retry setTimeout is only reachable after the early-exit guard. In the current setup scripts are synchronous so this guard should never trigger, but it's a latent correctness issue if loading strategy changes.

Suggestion: Move attempts++ inside the branch that actually schedules a retry, or restructure so early exits (e.g., Prism not loaded yet) schedule a retry rather than silently giving up.


Bug: Missing StringComparison.Ordinal in WebSiteBuilder.MarkdownAndTemplates.cs

BuildPrismInitScript in the site-builder file:

// WebSiteBuilder.MarkdownAndTemplates.cs line 163
var safePath = (languagesPath ?? string.Empty).Replace("'", "\\'");

The equivalent in WebApiDocsGenerator.Html.cs (line 736) correctly passes StringComparison.Ordinal:

var safePath = (languagesPath ?? string.Empty).Replace("'", "\\'", StringComparison.Ordinal);

On non-invariant cultures this inconsistency could theoretically produce different results. Small fix, easy to miss.


Design concern: duplicated init script logic across two files

BuildPrismManualScript / BuildApiPrismManualScript are byte-for-byte identical. BuildPrismInitScript / BuildApiPrismInitScript are nearly identical — the only meaningful difference is the root element selector (.api-content fallback vs. document directly).

Any future changes to the retry logic, maxAttempts, delayMs, or escape handling need to be applied in two places. This PR already demonstrates the risk: the StringComparison.Ordinal discrepancy above is a direct result of that duplication.

Consider extracting shared logic to a PrismScriptBuilder helper class in PowerForge.Web that both call into, with a parameter for the root selector.


Potential XSS: languagesPath escaping is incomplete

This is a pre-existing issue (not introduced by this PR), but the retry logic makes it more visible:

var safePath = (languagesPath ?? string.Empty).Replace("'", "\\'");

Only single quotes are escaped. A path containing </script> would prematurely close the script tag:

targetPath='</script><script>alert(1)//'

If languagesPath is ever sourced from user-controllable input (e.g., front matter or query parameters), this is an XSS vector. Worth tracking even if the current call sites use configuration-only values.


Test observation: ordering assertions rely on first-occurrence index

In WebSiteBuilderPrismAssetInjectionTests and the updated WebApiDocsGeneratorPowerShellTests, ordering is validated using IndexOf on the full HTML string. This is fine for the current template structure, but will produce false negatives if the same string appears earlier in the document for an unrelated reason (e.g., in a page title or meta tag). Worth noting for future maintenance.


Minor: autoloader.languages_path is set on every retry

if(p.plugins&&p.plugins.autoloader){p.plugins.autoloader.languages_path=targetPath;}

This assignment runs on every retry call. It is a benign no-op after the first successful set, but moving it outside the run loop (into the outer IIFE, executed once on init) would be marginally cleaner and avoids repeated property writes.


Tests look solid overall

  • Good coverage of both local-hashed and CDN asset paths.
  • Negative test updated correctly to assert the bootstrap script is absent when prism_mode=off.
  • Ordering assertions (bootstrap -> core -> autoloader) are the right way to validate this constraint.
  • CreateTempRoot / Cleanup pattern is consistent with other test classes in the suite.

Summary: The core change is correct and the tests verify the right things. The two actionable items before merging are:

  1. Add StringComparison.Ordinal to the Replace call in BuildPrismInitScript (site-builder version) to match the API-docs version.
  2. Consider whether the attempts++-before-early-exit pattern could silently swallow retries in edge cases.

The duplication and XSS issues are worth tracking but don't need to block this PR.

@PrzemyslawKlys PrzemyslawKlys merged commit d6e01af into main Feb 22, 2026
1 check passed
@PrzemyslawKlys PrzemyslawKlys deleted the codex/web-prism-engine-hardening-20260221 branch February 22, 2026 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant