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

Add a allowOrigin option to bypass invalid origins #159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luis-almeida
Copy link

✌️

/cc @zendesk/vegemite

Description

In Guide (Help Center) we have successfully built a POC for an enduser app using zaf, zaf_sdk and zcli. We are now doing a final push to release this app in an EAP.

As we talked about in one of our last meetings, Help Center supports host-mapped domains and this is currently leading to Invalid domain [origin] error screens. In this PR we're following your recommendation to introduce a way to bypass this check.

We understand this allow list was added in #58 to address a security vulnerability so I'm thinking that, even thought left undocumented, this solution might reopen that vulnerability.

We'd really appreciate your feedback here 🙏🏼
Alternatively, help us think of another way to have this available only in Help Center.

Tasks

  • Include comments/inline docs where appropriate
  • Add unit tests
  • Update changelog here

References

DevQA Steps

NOTE: DevQA steps are to be actioned only once code has been reviewed and approved.

Risks

  • [HIGH | medium | low] Does it work across browsers (including IE!)?
  • [HIGH | medium | low] Does it work in the different products (Support, Chat, Connect)?
  • [HIGH | medium | low] Are there any performance implications?
  • [HIGH | medium | low] Any security risks?
  • [HIGH | medium | low] What features does this touch?

Rollback Plan

  1. Quickly roll back to the prior release so that customers can refresh to resolve their issue.
  2. Revert this PR to restore the master branch to a deployable green state.
  3. Notify the author.

@token-cjg
Copy link
Contributor

I suspect that we'll want to adopt a different approach, as this will create a security issue if we follow it as-is.

@GorkaCardona
Copy link

Thanks @token-cjg for the fast response! @taoza @dahall-Z3N

What would be your teams recommendation for a different approach?

@GorkaCardona
Copy link

I should have tagged @zach-anthony above instead of David and Tao, sorry for the confusion.

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