#508 add support for embedded file #509
Conversation
openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfBoxFastLinkManager.java
Outdated
Show resolved
Hide resolved
| PDEmbeddedFile embeddedFile = new PDEmbeddedFile(_od.getWriter(), new ByteArrayInputStream(file)); | ||
| embeddedFile.setSubtype(elem.getAttribute("data-content-type") != null ? elem.getAttribute("data-content-type") : "application/octet-stream"); | ||
| fs.setEmbeddedFile(embeddedFile); | ||
| String fileName = Paths.get(uri).getFileName().toString(); |
There was a problem hiding this comment.
This won't work for custom protocol handlers reliably. I think it may be better to use another attribute, possibly download.
There was a problem hiding this comment.
You are right, it may fail. Maybe data-file-name as an attribute is more telling?
There was a problem hiding this comment.
I agree download isn't the best attribute name, but it is the standard html5 attribute for this purpose.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#Attributes
There was a problem hiding this comment.
I didn't know about the download attribute :D . So I could remove "data-embed-file" and use the download attribute with a name.
|
In regard to security, we have the URL resolver which can return null for bad resource attempts. Do we think this is enough? In hindsight the uri resolver should have taken resource type as well as uri, I guess. |
|
about the security implication, I think the current url resolver may not be fine grained enough for this specific case. I'm not a fan of adding yet another allow list, but as we are embedding resources in the pdf, I think avoiding by default the possibility to let a user embed some arbitrary file from the file system (like the classic |
|
I think we need to have a standard method for all resources (or at least newly added ones), something like:
Then we could just add to the enum for each resource, rather than adding methods. The problem is backwards compatibility. Perhaps we could have defaults for each resource type (everything for xml, nothing for file embed, etc) and calling this method would override those defaults. |
|
Agree about the using a ResourceType. I wonder if protocol is enough? Maybe adding the possibility to pass a custom For the method name, maybe And as you said, we define a sane set of default :). This way we can centralize the resource access and maybe remove the ad hoc constructor that I've added in https://github.com/danfickle/openhtmltopdf/blob/open-dev-v1/openhtmltopdf-svg-support/src/main/java/com/openhtmltopdf/svgsupport/BatikSVGDrawer.java#L58 |
Agree with all of this. The only issue is whether the predicate should be run before or after the uri resolver. |
I would guess after? If you don't mind, I'll open a new PR with only the access control part so we can focus better on that specific feature :) |
|
@syjer, Just letting you know, I'd like to do a release, probably tomorrow. If you are busy, no worries, we can leave this PR until the next release. Are you otherwise OK with me doing a release? |
yep, I prefer to let it rest for the next release.
ok on my side :) |
Based heavily on code by @syjer whom I'm indebted to. Thanks. Still todo: + Prevent duplicate file embeds. Possibly create a map of uris to PDComplexFileSpecification and reuse if encountered again. I have to peruse the PDF spec to see if this is allowed. + Logging on fail.
|
superseded by #640 :) |
hi, this PR implement the support of embedding files in the pdf.
You can see in #508 (comment) how it work. Basically, adding the attribute
data-embed-file="true"to a link will make the linked resource embedded.As written in #508 (comment) , the support from the pdf viewer is, uh, variable.
Especially the icon is quite ugly, and at the moment I'm not able to hide it.Edit: I was able to customize it. Seems to globally work, except for the viewer from IE/Edge. But at least adobe reader seems to work.Note: there are security implication if a third party user has control over the html. Maybe we need to add an explicit list of embeddable uri resource in the builder?
I still need to add some kind of test.
WDYT?