Git Merge Request #143: Attachments listing and lightbox added to discussion tool (merged)
Merging...
Merged
Something went wrong. Please, merge manually
Checking if merge is possible...
Something went wrong. Please, merge manually
Rohan Verma wants to merge 5 commits from /u/rhnvrm/allura/ to master, 2016-08-01
Preview:
Please review and suggest changes.
| Commit | Date | |
|---|---|---|
|
[560107]
(rhnvrm/design/view-attachment)
by
Rohan Verma
Update test_forum:testposts test with updated html |
2016-07-30 13:58:49 | Tree |
| 2016-07-26 08:38:46 | Tree | |
| 2016-07-13 12:51:22 | Tree | |
| 2016-07-12 22:51:22 | Tree | |
|
[4c71ed]
by
Rohan Verma
Initial design changes to list attachments. Added lightbox_me to view images |
2016-07-12 01:43:22 | Tree |
Looks a lot better :D
It would be great to have this applied to other attachment areas, like wiki pages and tickets. That's fine to be done later separately though.
<a href="http://localhost:8080/p/test/tickets/1/#dd1a"...is hard-coded in theredeleteAttachmentreferences amyformform that isn't defined anywhere, probably can remove that function?onLoadfunction that is empty and can be cleaned upword-wrap: break-word;would help so you can always see the full filename.Updated!
Will do the other areas in a different mr.
What do you think of opening PDFs in an iframe? Although I don't know if this will be compatible with all browsers.
I think PDFs can have security risks since they can have scripts embedded in them (but I'm not an expert). So we'd have to host them on a domain separate from the main domain. Which obviously would require extra setup and configuration, like having Allura running and responding to an additional domain somehow. Or possibly store attachments on disk ([#2560]) and then run a simple webserver for them, instead of Allura serving them.
Related
Tickets:
#2560I'm also not sure about the security aspect. Could you point me to some keywords so I can search more about this.
Also wanted a clarification regarding [#2560] and [#6390], from what I understand you are suggesting that we should upload to a server and store the link to the file in the db instead of the file, right?
Regarding the compatibility, I had found a MIT Licensed library that handles that https://pdfobject.com/ and also mozilla's https://mozilla.github.io/pdf.js/web/viewer.html (https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions). Would these be satisfying our requirements? (sample code: https://github.com/mozilla/pdf.js/tree/master/examples/helloworld)
Related
Tickets:
#2560Tickets: #6390
I don't know much about the security aspect either :) The risk would be that you could embed javascript in a PDF (e.g. a script that reads document.cookies and sends it off somewhere for an attacker to hijack your session). If someone uploaded that PDF to an allura site, any visitor that loaded it could get their session hijacked. We had a similar vulnerabililty with SVG+JS a while ago [#8011] I googled around and can't find good information on what is possible, or even how to make a PDF w/ js in it so we could test (it is possible to put JS inside I know).
For 2560/6390 yes, but I think that's a lot of extra work and probably not worth pursuing at this point, in my opinion.
pdf.js is a good idea. It might not allow JS to run from within a PDF. But hard to say, since we don't know how PDF+JS works.
Related
Tickets:
#8011forgediscussion.tests.functional.test_forum:TestForumAsync.test_postsis failing. Otherwise looks good.I didn't know about the
downloadattribute, that is nice. Makes [#6132] mostly taken care of (except for allowing text attachments to be displayed without download)Related
Tickets: #6132
Updated the test. Yes I guess [#6132] is taken care of. I will look into the PDF problem it a bit further this weekend.
Related
Tickets: #6132
Added another commit to fix JS from running too much and making it difficult to close the lightboxes: [cd3b40]