Skip to content

Fix URL sanitization bypass through formaction attributes#21446

Closed
metsw24-max wants to merge 1 commit into
emberjs:mainfrom
metsw24-max:formaction-url-sanitization-bypass
Closed

Fix URL sanitization bypass through formaction attributes#21446
metsw24-max wants to merge 1 commit into
emberjs:mainfrom
metsw24-max:formaction-url-sanitization-bypass

Conversation

@metsw24-max

Copy link
Copy Markdown

This patch fixes a URL sanitization bypass involving the HTML formaction attribute.

The sanitizer currently protects URL-bearing attributes such as action from dangerous schemes including javascript: and vbscript:. However, formaction was not included in the protected attribute set.

Because formaction overrides a form's action during submission, attacker-controlled values could bypass existing URL sanitization and execute script when a submit button is activated.

Root Cause

The sanitizer denylist covered:

  • action on <form>
  • Other protected URL-bearing attributes

but omitted formaction, even though it serves the same navigation purpose for submit controls.

As a result, templates such as:

<form>
  <button formaction={{this.userInput}}>Submit</button>
</form>

could emit dangerous URLs unchanged when userInput contained a javascript: or vbscript: URL.

Changes

  • Add formaction to the protected URL attribute list.
  • Add BUTTON and INPUT to the protected element set since they are the only elements that honor formaction.
  • Document the override semantics with inline comments.
  • Add integration tests covering dangerous formaction values.

Security Impact

Before this change, a malicious value supplied to formaction could bypass the existing URL sanitization logic and execute script when a submit control was activated.

After this change, dangerous schemes are sanitized using the same mechanism already applied to action, ensuring consistent protection across form submission entry points.

Verification

Verified against the transpiled sanitizer implementation.

Before

<button formaction="javascript:alert(1)">

Dangerous URL emitted unchanged.

After

<button formaction="unsafe:javascript:alert(1)">

Dangerous URL is sanitized and blocked.

Regression testing confirmed no behavioral changes for existing protected attributes and elements, including:

  • <form action>
  • <a href>
  • <embed src> with supported data: URLs
  • Attributes intentionally outside sanitizer scope

The added integration tests validate the vulnerable path and prevent future regressions.

@NullVoxPopuli

Copy link
Copy Markdown
Contributor

practically, when would formaction be dynamic?

@metsw24-max

Copy link
Copy Markdown
Author

It is rarer than dynamic href/action, agreed.

The argument isn't frequency, it's parity with a control you already ship. Glimmer already sanitizes dynamic <form action={{…}}> values. formaction on a submit <button> or <input> is the same submission sink — per the HTML spec it overrides the form's action at submit time — so <button formaction={{x}}> resolves to the exact same javascript: execution that action={{x}} does. Right now an attacker who can influence that value just moves it from action to formaction and the existing protection no longer applies. That's an inconsistency in the threat model, not a new class of risk.

A realistic dynamic case: a form whose submit target comes from server/config/CMS data rather than a literal — e.g. a configurable or multi-tenant form:

<form>
  <button formaction={{this.submitUrl}}>Publish</button>
</form>

where submitUrl originates from data a user can influence. Same flow that makes dynamic action worth guarding.

So I would frame this as closing a gap in the existing action sanitization rather than a brand-new exploit
happy to scope the PR description that way if that reads better.

@NullVoxPopuli

Copy link
Copy Markdown
Contributor

This work has been pulled in to #21442

thank you!

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.

2 participants