Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Disable SAMEORIGIN on login page. #113

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

Conversation

adamhooper
Copy link
Contributor

This lets https://www.overviewproject.org display private DocumentCloud
documents to its users even when they aren't logged in to DocumentCloud.
The steps look like this:

  1. User opens /documents/X in an iframe (no X-Frame-Options)
  2. DocumentCloud sees it's a private document; it redirects to /login
  3. (after this patch), the browser loads /login in the iframe
  4. The user enters email/password
  5. Login succeeds; DocumentCloud redirects to /documents/X

This does open DocumentCloud up to clickjacking. Blame WebKit: it's the
only browser that doesn't allow an ALLOW-FROM X-Frame-Options setting,
which would limit the iframe to https://www.overviewproject.org.
https://bugs.webkit.org/show_bug.cgi?id=94836#c13

From Overview's point of view, there are no reliable workarounds.

This lets https://www.overviewproject.org display private DocumentCloud
documents to its users even when they aren't logged in to DocumentCloud.
The steps look like this:

1. User opens /documents/X in an iframe (no X-Frame-Options)
2. DocumentCloud sees it's a private document; it redirects to /login
3. (after this patch), the browser loads /login in the iframe
4. The user enters email/password
5. Login succeeds; DocumentCloud redirects to /documents/X

This does open DocumentCloud up to clickjacking. Blame WebKit: it's the
only browser that doesn't allow an ALLOW-FROM X-Frame-Options setting,
which would limit the iframe to https://www.overviewproject.org.
https://bugs.webkit.org/show_bug.cgi?id=94836#c13
@nathanstitt
Copy link
Member

Hi @adamhooper. The problem runs a bit deeper than just login I'm afraid. Right now we (actually Rails) sets the X-Frame-Options on all pages. So even if we were to open up the login, it still wouldn't allow displaying the documents.

Not a big problem, we just need to decide what actions should be allowed in an iframe and setup the relevant controller's actions accordingly.

It sounds like your use case would be limited to displaying documents. Correct?

editing in this sneakily: This presumes that we do want to allow iframe access at all. I'll let Ted speak to that.

@adamhooper
Copy link
Contributor Author

Hi @nathanstitt. Thanks for responding.

DocumentCloud already allows viewing documents in an iframe.

Overview's iframe only ever shows the document-view action and the login action. So this pull request should cover our needs.

@nathanstitt
Copy link
Member

Huh. I guess I mis-remembered. I wasn't aware that we'd opened up the view document action, but looks like we have. Thanks for setting me straight.

@knowtheory
Copy link
Member

@nathanstitt my eventual goal is to support both iframe embeds and script embeds, trending in the general case towards the former. Script embed tags are problematic in the general case for non-technical users. Obviously we will need to keep script tag embeds to ensure that all of the neat things that folks can do with documents are still possible.

@adamhooper
Copy link
Contributor Author

ping? Is there any reason not to merge this?

@adamhooper
Copy link
Contributor Author

ping?

@adamhooper
Copy link
Contributor Author

ping? It's this pull request's birthday. Just checking to see if there's any way I can help.

@reefdog
Copy link
Contributor

reefdog commented Mar 18, 2016

Loading the login page in the iframe is undoubtedly the most seamless experience, but I don't think it's worth the potential security concerns. Our current 403 pages include a login link:

403

I think I'd rather have that page add a target attr to the "log in" link when it's being iframed, so that it spawns a new tab rather than loading the log in page in-iframe. People will have to then refresh the original page after authenticating in the new tab. Again, not nearly as seamless, but safer, yeah? @knowtheory watcha think?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants