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

Use DOMParser to safely parse html without script execution #2226

Merged
merged 1 commit into from
Aug 9, 2018
Merged

Use DOMParser to safely parse html without script execution #2226

merged 1 commit into from
Aug 9, 2018

Conversation

RyanV
Copy link
Contributor

@RyanV RyanV commented Aug 7, 2018

Fix for #2224

DOMParser will safely parse html without executing scripts or inline javascript handlers. It will also automatically put script tags in the head of the document which we should be able to safely ignore.
This implementation does change the resulting delta of the paste by not the script content as text.

Issue #2224 identifies another issue where inline event handlers can execute javascript from non script tags. I was unable to create a failing test with an img.onerror handler defined even though the failed img src request can be seen in the test report. I can continue to work on this if necessary. Here is the test:

it('does not execute javascript', function() {
    window.unsafeFunction = jasmine.createSpy('unsafeFunction');
    const html = "<img src='a' onerror='window.unsafeFunction()'/>";
    this.clipboard.convert({ html });
    expect(window.unsafeFunction).not.toHaveBeenCalled();
    delete window.unsafeFunction;
});

@jhchen
Copy link
Member

jhchen commented Aug 7, 2018

Thanks @RyanV! Can you add the test here into test/unit/modules/clipboard.js as well?

@RyanV
Copy link
Contributor Author

RyanV commented Aug 7, 2018

@jhchen as mentioned, there is an issue with karma firing the on error callback, but only in the test. I can explore the issue more

@jhchen
Copy link
Member

jhchen commented Aug 7, 2018

Can you try onload instead of onerror and using src="/assets/favicon.png"?

@RyanV
Copy link
Contributor Author

RyanV commented Aug 8, 2018

yup, onload worked fine. updated branch with test.

@RyanV
Copy link
Contributor Author

RyanV commented Aug 8, 2018

Any ideas why Travis is failing? looks like it's can't find the Karma executable sh: 1: karma: not found

@jhchen jhchen merged commit 8802da7 into slab:develop Aug 9, 2018
@jhchen
Copy link
Member

jhchen commented Aug 9, 2018

The karma issue seems like a bad cache -- I removed it and it got past but then complained about credentials. Will have to look more into the latter issue. Thanks for the bug-fix and tests!

@RyanV
Copy link
Contributor Author

RyanV commented Aug 9, 2018

👍

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