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

Better XSS Check for frames.erb #1537

Closed
wants to merge 1 commit into from
Closed

Better XSS Check for frames.erb #1537

wants to merge 1 commit into from

Conversation

avivkeller
Copy link
Contributor

@avivkeller avivkeller commented Feb 29, 2024

Description

Yikes! I'm so embarrassed. My patch fails to resolve the XSS/Open-Redirect vulnerability. This second patch will fix it, and I've done much testing to confirm it. Please understand my mistake and implement this into the repository.

Proof-Of Concept

See the following pen for an example: https://codepen.io/Aviv-Keller/pen/mdgdmyW
#!:javascript:alert("XSS")

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@avivkeller
Copy link
Contributor Author

Even with this, I still believe the best way to resolve this permanently is to use URL:

let url = new URL(name, location.href)
url.origin === location.origin

@avivkeller
Copy link
Contributor Author

Dear @lsegal,
I apologize for the oversight regarding your library's XSS and Open-Redirect vulnerabilities. I acknowledge and accept full responsibility for this lapse in judgment. It was a regrettable mistake, and I deeply regret any inconvenience or concern it may have caused. My failure to patch these vulnerabilities thoroughly may have ended your trust in me. I understand the gravity of this situation and the impact it may have had on our professional relationship. Please know that I am genuinely remorseful and committed to rectifying the situation to the best of my abilities. In light of this, I have taken steps to address the vulnerabilities and have provided a suggested pull request (PR) for your consideration. Given the circumstances, I understand if you consider this suggestion with caution, and I respect your decision either way. Moving forward, I am dedicated to implementing more rigorous testing protocols to prevent such oversights in the future. Once again, I extend my deepest apologies for any inconvenience or disappointment caused. I am available to talk about this more whenever you get a chance and to collaborate on any necessary steps to ensure the security and integrity of your library. Thank you for your understanding and patience.

Comment on lines +10 to 13
if (name.match(/^((?:(?:[\w]*):)?[\/\\]*)/gm)[0]?.length > 0) {
name = '<%= url_for_main %>'
}
window.top.location.replace(name)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (name.match(/^((?:(?:[\w]*):)?[\/\\]*)/gm)[0]?.length > 0) {
name = '<%= url_for_main %>'
}
window.top.location.replace(name)
if (new URL(name, window.location.href).origin == window.location.origin) {
window.top.location.replace(name);
}

@avivkeller
Copy link
Contributor Author

I like your idea, I'm gonna squash and reopen a pull-request.

@avivkeller avivkeller closed this Feb 29, 2024
@avivkeller avivkeller deleted the patch-2 branch February 29, 2024 21:58
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