Skip to content

payment-widget: fix NaCl bundle + harden against XSS/callback/iframe#143

Closed
liu971227-sys wants to merge 1 commit intoScottcjn:masterfrom
liu971227-sys:bounty/payment-widget-security
Closed

payment-widget: fix NaCl bundle + harden against XSS/callback/iframe#143
liu971227-sys wants to merge 1 commit intoScottcjn:masterfrom
liu971227-sys:bounty/payment-widget-security

Conversation

@liu971227-sys
Copy link
Contributor

Fixes bounty #67 (payment widget hardening).

Key changes

  • Fix broken bundled NaCl code (replaced with upstream tweetnacl-js nacl-fast.min.js) so widget parses and runs again.
  • Remove DOM XSS sinks: avoid innerHTML for user-controlled content; use textContent / text nodes.
  • Validate recipient address format and normalize amount.
  • Default-deny iframe embedding unless explicitly allowed (data-allow-iframe=true); optional allowed-origins allowlist.
  • Restrict callback URL to same-origin by default; optional allow-any-origin flag.

PoCs (in-repo)

  • payment-widget/poc/xss-label.html
  • payment-widget/poc/xss-memo.html
  • payment-widget/poc/iframe-block.html

Notes

  • This PR targets base branch master because payment-widget lives on upstream/master (not main).

@David-code-tang
Copy link
Contributor

Security-focused review (PR #143)

  1. Encoding/BOM regression
  • payment-widget/rustchain-pay.js now starts with a UTF-8 BOM (\ufeff/** shown as +/** in the diff). This can cause tooling/linters/minifiers to behave oddly and also broke a UI string below.
  • fileLabel.textContent became "鉁?${file.name}" instead of the intended checkmark. This will show garbled text for users.
    Suggestion: remove the BOM and keep UI strings ASCII (e.g. "OK: " + file.name or "[loaded] " + file.name).
  1. XSS hardening looks good, but keep it consistent
  • Good change: label/memo/to/amount are now written via textContent/text nodes, not innerHTML.
  • escapeHtml() is added but (from this diff) not used. Consider removing it to avoid giving a false impression that HTML-escaping is the primary defense.
  1. Callback URL policy
  • Good change: default callback is same-origin, and URL parsing uses new URL().
  • Consider tightening defaults to https: only (except localhost) to avoid accidental insecure deployments. If keeping http: support, the README should explicitly warn it is for local/dev only.
  1. Origin allowlist
  • Current allowedOrigins compares raw strings to window.location.origin. Consider documenting that entries must be origins (scheme+host+port), not full URLs. A small normalization step (e.g. trimming trailing slashes) would reduce misconfig.

Overall: direction is solid for DOM injection + callback safety; please fix the BOM/garbled checkmark before merge.

Copy link
Contributor

@David-code-tang David-code-tang left a comment

Choose a reason for hiding this comment

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

Security direction looks good (textContent for user-controlled fields, iframe default-deny, callback URL restriction, PoCs included).

Two issues to fix before merge:

  1. UTF-8 BOM / encoding regressions
  • payment-widget/README.md and payment-widget/rustchain-pay.js appear to start with a BOM (shows as an invisible U+FEFF in diff). Please remove; it can cause tooling/display issues.
  • fileLabel.textContent changed from a checkmark to a garbled string (looks like "鉁?" prefix). That looks like encoding corruption. Suggest using ASCII (e.g. "OK " + file.name) or a unicode escape ("\u2713 " + file.name).
  1. Minor callback hardening nit
  • safeCallbackUrl() is good; consider explicitly rejecting URL usernames/passwords (u.username/u.password) or document the behavior, to avoid surprising callback handling.

Otherwise LGTM after the encoding fix.

Copy link
Owner

@Scottcjn Scottcjn left a comment

Choose a reason for hiding this comment

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

Closing — Duplicate of #152

This PR addresses the same payment widget XSS issue as #152 (David-code-tang), but #152 is cleaner, more focused, and was submitted first. Closing this as duplicate.

@liu971227-sys — You have several other open PRs (#155, #148, #140) that need work. Please focus on quality over quantity.

@Scottcjn Scottcjn closed this Feb 13, 2026
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.

4 participants