Skip to content

validate exit url protocol in amp-ad-exit#40518

Open
rootvector2 wants to merge 1 commit into
ampproject:mainfrom
rootvector2:amp-ad-exit-protocol-validation
Open

validate exit url protocol in amp-ad-exit#40518
rootvector2 wants to merge 1 commit into
ampproject:mainfrom
rootvector2:amp-ad-exit-protocol-validation

Conversation

@rootvector2

Copy link
Copy Markdown

the exit() handler navigates to a creative-controlled finalUrl through win.open/HostServices.openUrl without a scheme check, so a creative configuring finalUrl (or a substituted var) as javascript:/data: reaches navigation while every other nav path goes through Navigation.navigateTo and rejects it via isProtocolValid; this validates the substituted finalUrl before any exit, found while comparing amp-ad-exit's direct win.open against navigation.js.

@CLAassistant

CLAassistant commented Jun 5, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@erwinmombay erwinmombay self-assigned this Jun 8, 2026
@powerivq

Copy link
Copy Markdown
Contributor

Looks good. Just need to sign CLA and rebase against latest main branch before we can merge this.

@powerivq

Copy link
Copy Markdown
Contributor

@rootvector2 lmk if you need help doing these.

@rootvector2 rootvector2 force-pushed the amp-ad-exit-protocol-validation branch from 7ad2778 to 7185b1c Compare June 27, 2026 13:47
@rootvector2

Copy link
Copy Markdown
Author

rebased on latest main, and the CLA check is already green. heads up though: CI was red before the rebase, not because of it. substituteVariables -> expandUrlSync already runs ensureProtocolMatches_ -> userAssert(isProtocolValid), so a literal javascript:/data: finalUrl was throwing inside substitution before my guard ran. moved the isProtocolValid check ahead of substitution so a bad protocol gets rejected cleanly, and added the repo's no-script-url exception for the test's literal. all squashed into 7185b1c.

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