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

Make it embeddable #154

Merged
merged 11 commits into from
Aug 20, 2015
Merged

Make it embeddable #154

merged 11 commits into from
Aug 20, 2015

Conversation

Gaurav0
Copy link
Contributor

@Gaurav0 Gaurav0 commented Aug 14, 2015

Fixes issue #13

Note: This builds on pull request #141, please review that first.

embed() {
let src = window.location.href.split("?")[0];
src += "?numColumns=0";
let embedCode = `<iframe width="800" height="600" src="${src}"></iframe>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

the resulting embed values are not correctly sanitized, and could pose a XSS risk.

potential vector:

  1. evil twiddler sends some cleverly crafted href, able to sanitize the src + frame tag in the interpolated string.

I believe this can be safely escaped, or direct DOM api's could be used (which make some issues just impossible):

var iframe = document.createElement("iframe")
iframe.src = src;
iframe.width = 800;
iframe.height = 600;
let embedCode = iframe.outerHTML;

The src still needs to be sanitized, but in the later case the vector is only stuff like javascript: Which is likely tricky to get to via this approach, but we should likely be sure due diligence was done here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's really cool. But we are already using href in Share Twiddle. I'm going to open a PR ASAP and update Ember Twiddle for security reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually maybe I'm misunderstanding. According to this, using window.location.href is relatively safe, and generally more safe than alternatives: http://stackoverflow.com/questions/24078332/is-it-secure-to-use-window-location-href-directly-without-validation

@stefanpenner
Copy link
Contributor

i left comments re: the approach taking by columns here

@Gaurav0 Gaurav0 mentioned this pull request Aug 18, 2015
@Gaurav0
Copy link
Contributor Author

Gaurav0 commented Aug 20, 2015

@stefanpenner @rwjblue @joostdevries Please review.

@stefanpenner
Copy link
Contributor

sgtm

Gaurav0 added a commit that referenced this pull request Aug 20, 2015
@Gaurav0 Gaurav0 merged commit a0368bf into ember-cli:master Aug 20, 2015
@Gaurav0 Gaurav0 deleted the embed branch March 21, 2016 13:49
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.

3 participants