Add a whitelist of origins #58
Conversation
lib/client.js
Outdated
| /.*dashboard\.zopim\.org.*/gi | ||
| ]; | ||
|
|
||
| var i; |
There was a problem hiding this comment.
Couldn't just declare the iterator var in the for statement? Seems unnecessary to pull it out. If you were going to do that, I'd expect you might save the length of the Array in the constant into another var, like len = WHITELISTED_ORIGINS.length.
| this._context = options.context || null; | ||
| this.ready = false; | ||
|
|
||
| var WHITELISTED_ORIGINS = [ |
There was a problem hiding this comment.
I'm guessing it was a conscious choice to make the Regexp literals simple so as to be unambiguous? (Could have put all of them into a single Regexp with optional groups, then just avoided the loop below)
| if (WHITELISTED_ORIGINS[i].test(this._origin)) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we extract this into a function that returns true if the origin passed matches any regex, then if the function returns true throw the exception below?
There was a problem hiding this comment.
Could we add a test please?
There was a problem hiding this comment.
I'll move it to a separate function. I think I also need to add *.apps.zdusercontent.com to the list.
|
@svizzari @danielbreves Please help review again. 🙏 |
spec/client_spec.js
Outdated
| } | ||
|
|
||
| describe('isDomainValid', function() { | ||
| it('Should instantiate client when domain is valid 1', function() { |
There was a problem hiding this comment.
Could we refer to the domain in the description, rather than a count, just so it's easier understand when they fail?
spec/client_spec.js
Outdated
| window.addEventListener.callArgWith(1, evt); | ||
| } | ||
|
|
||
| describe('isDomainValid', function() { |
There was a problem hiding this comment.
isOriginValid to match the helper method (intent)? Or, possibly just change to:
describe("with a valid origin", () => {})
describe("with an invalid origin", () => {})?
Mehul-Porwal
force-pushed
the
mehul/origin_security_issue
branch
from
b3783ed to
cee0754
Compare
Mehul-Porwal
force-pushed
the
mehul/origin_security_issue
branch
from
cee0754 to
4ac65ea
Compare
|
@svizzari @danielbreves Do you know why the tests are failing? |
|
Seems like it's failing on master too, since @ocke's merge: https://travis-ci.org/zendesk/zendesk_app_framework_sdk/builds/359212069 |
|
@token-cjg Can I request you to QA this PR, it is already deployed to Staging and Master. The first step is to verify that all apps should continue to function normally in all stages including Master, Staging and Production. Second step should be to test the use case mentioned in https://support.zendesk.com/agent/tickets/3371584. |
|
@zendesk/wombat, might be similar to https://github.com/zendesk/zendesk_app_market/pull/2737 and https://github.com/zendesk/zendesk_app_market/pull/2743 in impact re: host-mapped accounts. @Mehul-Porwal, I think you've reverted this now and merged the revert into master, yes? |
|
This PR was deployed to Production Release. Reference: v2.0.13 |
|
This PR was deployed to Pod18. Reference: v2.0.13 |
|
This PR was deployed to Pod17. Reference: v2.0.13 |
|
This PR was deployed to Update Major Version. Reference: v2.0.13 |
|
This PR was deployed to Pod19-Pod24 (Release). Reference: v2.0.16 |
Any website can include zendesk_app_framework, embed one of our iframed app, and be able to sniff the events and data. This PR is adding a whitelist of domains to prevent this.
Reported Issue: https://support.zendesk.com/agent/tickets/3371584
@zendesk/vegemite
Limitations: Since this is a front-end only fix, anyone can easily bypass this, we should think about adding nginx rules on the app assets to not be allowed to be iframed on any other domains.