Skip to content
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.

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

Ensure router only targets scripts for execution #12177

Merged
merged 9 commits into from
Oct 11, 2024
Merged

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Oct 10, 2024

Changes

  • Uses document.getElementsByTagName instead of document.scripts.
  • HTML has a strange feature where elements with a name attribute get placed on the document object, so if you had for example <img name="scripts"> this would override document.scripts.
  • This is a potential security vulnerability. Also we want to actually execute the scripts, so it's also a bug.

Testing

  • Added a test via a DOM that interferes with scripts running. Existing script tests fail without the fix.

Docs

N/A, bug fix

Copy link

changeset-bot bot commented Oct 10, 2024

🦋 Changeset detected

Latest commit: 6dedef4

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Oct 10, 2024
@matthewp matthewp marked this pull request as ready for review October 11, 2024 01:35
Copy link
Member

Choose a reason for hiding this comment

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

Is this change to fix the odd firefox e2e fail? I sent a PR to fix it #12188, but skipped for firefox instead since the tests isn't actually working and testing correctly. (I also updated the test). Maybe we can revert this part and merge that PR instead.

Copy link
Contributor Author

@matthewp matthewp Oct 11, 2024

Choose a reason for hiding this comment

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

archive.org is down due to a DDOS, was down all day yesterday, I spend a significant amount of time thinking the tests were really broken! https://archive.org/details/movies

I added the smallest mp4 I could create, I promise.

Copy link
Member

Choose a reason for hiding this comment

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

I know you did, it's slightly smaller than mine, but dare I say mine has more pizzazz 😄

Anyways I don't mind if you want to merge with your fix. I'd still like to get mine in too after to fix the tests though as it wasn't really testing the video playback right (it wasn't ever started in the test so currentTime is always 0).

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Fix looks great. Have a question about the e2e test change

@matthewp matthewp merged commit a4ffbfa into main Oct 11, 2024
13 checks passed
@matthewp matthewp deleted the fix-clobbering branch October 11, 2024 13:37
@astrobot-houston astrobot-houston mentioned this pull request Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants