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

Lightbox Ajax and CORS, Security #1561

Closed
ChrisHougard opened this issue Nov 24, 2014 · 5 comments
Closed

Lightbox Ajax and CORS, Security #1561

ChrisHougard opened this issue Nov 24, 2014 · 5 comments

Comments

@ChrisHougard
Copy link
Contributor

While trying to fix #1557 I was thinking about the ajax embed. I think the lightbox should either be removed for conversations, the ajax option changed to an iframe, or only allowed for images.

First, CORS is going to block any ajax requests to other domains unless the remote server is configured to allow the request. Since the conversations block is designed to be used by external people I would imagine this will be a very common problem.

Second, we're opening up a major security vulnerability. Any javascript you put on a link that's opened with the ajax embed is going to execute and it will have access to the active Concrete5 session.

@aembler
Copy link
Member

aembler commented Nov 24, 2014

Honestly I think having redactor as a conversation editor is probably more trouble than it's worth. Markdown and BBCode are probably better options. Having to use htmlLawed and then making sure that the various options get properly turned on and off...it's just a lot of work for what's basically on ever going to be the ability to bold some text. You can't even reliably post an image in the text because all image posting is ever going to be is a SRC link (since most people using conversations won't have full access to the file manager.)

On Nov 24, 2014, at 12:34 AM, Christopher Hougard notifications@github.com wrote:

While trying to fix #1557 I was thinking about the ajax embed. I think the lightbox should either be removed for conversations, the ajax option changed to an iframe, or only allowed for images.

First, CORS is going to block any ajax requests to other domains unless the remote server is configured to allow the request.

Secondly, we're opening up a major security vulnerability. Any javascript you put on a link that's opened with the ajax embed is going to execute and it will have access to the active Concrete5 session.


Reply to this email directly or view it on GitHub.

@ChrisHougard
Copy link
Contributor Author

Is having the lightbox use ajax in general a good idea for anything user provided? If I try to add a link to a page I'm going to run in to the same problem. I'm thinking anything user-provided, unless it is an image, should load in an iframe based lightbox. That the content runs isolated from the current page.

If I try to load a normal c5 page using the lightbox I could see that causing problems as well because any inline javascript is going to execute. Say if someone has some javascript globally in the footer that adds an event handler. Every time the lightbox loads that event handler is going to be registered again.

@KorvinSzanto
Copy link
Member

@EC-Chris I'm not against this at all, I believe me and @aembler talked about this but were unable to get it in before I started working on other stuff. Feel free to get this one done and submit a PR!

@aembler I agree. markdown is all we need, we should find a commonmark compiler or write one ourselves so that we can extend syntax.

@aembler
Copy link
Member

aembler commented Nov 24, 2014

@EC-Chris Yeah the lightbox with content option was meant to be used with the blank page template (in elemental at least). It's sort of up to the user to make sure that the content looks good. I think having an iframe as another option there would make sense too – for the vast majority of times when people just want to link to some other page that looks good independently of the page they're looking at.

@aembler
Copy link
Member

aembler commented Nov 24, 2014

#1562 #1564

@aembler aembler closed this as completed Nov 24, 2014
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

No branches or pull requests

3 participants