-
Notifications
You must be signed in to change notification settings - Fork 174
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
Configurable viewer origins #20
Conversation
@@ -42,6 +42,7 @@ See https://github.com/adobe-type-tools/cmap-resources | |||
</script> | |||
<% end %> | |||
<%= javascript_include_tag "pdfjs_viewer/application" %> | |||
<%= javascript_tag "window.HOSTED_VIEWER_ORIGINS = #{PdfjsViewer.hosted_viewer_origins.to_json.html_safe};" %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think configuring the viewer in the browser is a good idea. This check is a safeguard to prevent malicious requests. The browser environment is not something to be trusted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already configured in the browser isn't it?
On 4 Aug 2016, at 08:23, Yves Senn notifications@github.com wrote:
In app/views/pdfjs_viewer/viewer/_viewer.html.erb:
@@ -42,6 +42,7 @@ See https://github.com/adobe-type-tools/cmap-resources
</script>
<% end %>
<%= javascript_include_tag "pdfjs_viewer/application" %>
- <%= javascript_tag "window.HOSTED_VIEWER_ORIGINS = #{PdfjsViewer.hosted_viewer_origins.to_json.html_safe};" %>
I don't think configuring the viewer in the browser is a good idea. This check is a safeguard to prevent malicious requests. The browser environment is not something to be trusted.—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HOSTED_VIEWER_ORIGINS
is a variable created by viewer.js
, afaik my change doesn't make anything more accessible than it already is.
Thinking about it more, the file parameter is a security risk for content spoofing when HOSTED_VIEWER_ORIGINS is configured to let the viewer load any remote URL. Consider the case where pdfjs_viewer-rails is hosted on open-to-attack.com and HOSTED_VIEWER_ORIGINS is set to Maybe pdf.js needs another check against what URLs it'll load from, or pdfjs_viewer-rails shouldn't expose the file parameter on the viewer, loading PDFs in a different way perhaps. |
The Content-Security-Policy header could be used to protect sites from loading any remote URL. A configuration option could be made available to set that header on requests for any of pdfjs_viewer-rails views? |
@MattFenelon since the request from the viewer is always going to be a GET maybe it's not critical. I think it's something that should be left to the |
@MattFenelon I still don't quite understand why PDF.js chose to implement the check that way. Why would a hosted viewer be good to load all remote urls but one on the same host not. Somehow these things feel unrelated except that a hosted viewer might be very likely to load remote PDFs. |
@senny there's a FAQ entry about it, as far as I can tell it's purpose is to avoid opening a security hole on your site inadvertently, if you change the code to remove the check you've at least had to accept the risk of doing so. There's also some details in mozilla/pdf.js#6916. We could add a warning to the README about changing the |
What's the latest on this? |
viewer.js contains a cross-origin check against the array HOSTED_VIEWER_ORIGINS. If the viewer's origin is not contained in HOSTED_VIEWER_ORIGINS an error is thrown when attempting to load a PDF from a remote host.
a48f532
to
da91723
Compare
Closes #16.
I've changed the implementation to a solution that doesn't involve editing the
viewer.js
file.The HOSTED_VIEWER_ORIGINS can now be configured in an initialiser: