prawn
prawn copied to clipboard
prawnpdf
Really fix encryption issue
The previous fix incorrectly escaped "\r" as "\\r". However, it needs to be escaped as "\r" (so backslash followed by letter r).
This pull request needs https://github.com/prawnpdf/pdf-core/pull/55 to work since there the needed constant is defined.
@pointlessone @gettalong So this seems like a good example of the additional complexity introduced by a separate pdf-core gem. We're going to have to do some gymnastics here to get CI to pass.
Rather than doing a pdf-core gem release and then upping the requirement for prawn (which would be one option), I suggest we doing something similar to what I did here - https://github.com/prawnpdf/prawn-table/blob/38b5bdb5dd95237646675c968091706f57a7a641/lib/prawn/table.rb#L32
Specifically we'd introduce a line like:
STRING_ESCAPE_MAP = { '(' => '\(', ')' => '\)', '\\' => '\\\\', "\r" => '\r' }.freeze unless defined?(STRING_ESCAPE_MAP)
into lib/prawn/security.rb.
This is undoubtedly a bit of a hack, but it will get this PR unblocked and can be removed once we release a pdf-core version.
Thoughts?
@petergoldstein @pointlessone Wouldn't it be more "correct" to use the master branch of each repo in the CI pipeline instead of the released versions?
@gettalong I don't think so. It's EXTREMELY uncommon to use a main branch from another branch in CI (with one notable exception, discussed below). The reason is that it disconnects the experience of the gem's users from what's being tested in CI. As the basic purpose of CI in a library is to give users confidence in the state of the gem, that's a big problem.
In this case for example, once this is merged to master, there'd be nothing to stop someone from using prawn from master and an older version of pdf-core in tandem, which would be broken. And CI wouldn't tell you that.
The exception is when one is tracking changes in a separate library (set of libraries) with wider adoption, and ensuring that ongoing changes to the repo and the other library don't break. In Ruby the most commonly case where such an approach is used is with Rails (or some subset, like ActiveRecord, etc.). In that case the repo will generally define a set of supported released versions of the external library with which it is compatible. Each compatible version (typically at the minor version level) will have its own CI entry, and the main branch will be tracked as another entry.
@petergoldstein I understand what you are saying but I think the exception you mentioned comes into play here since pdf-core and prawn are interdependent, as can be seen by this pull-request.
So even if we work around it and someone would use prawn from master seeing it as green, the whole system prawn+pdf-core wouldn't correctly work since the correct pdf-core wasn't yet released. This is the reverse from the case you have described.
In essence a user has to know that only released gems are safe to use. This should always be the case and if they uses some not-released code, they are own their own. Just my 2c.
@gettalong Given the release cadence for prawn I don't think that's a realistic expectation. Just read through the issues - many people are using non-released versions (off their forks, etc.), because there hasn't been a regular cadence.
Using the reverse situation (pdf-core from master, prawn from a released gem) it's not going to be any more broken than it would be using currently released gems. So I'm not sure what the issue is here.
In my view the exception I mention isn't relevant here. pdf-core is not a widely used gem - it's used exclusively by prawn. What we're looking at is an incomplete, aborted extraction. That's a very different situation.
Given the current situation in the prawn organization (number of maintainers, number of contributors, state of open issues, etc.) I'd recommend that we just fold in the pdf-core code into the prawn gem and call it a day. No one is likely to finish the pdf-core extraction or make that a supportable interface. While merging pdf-core back into prawn is probably about ~0.5 day of work. I'm happy to put up a PR with that if there's concurrence on the approach.
Oh, and one additional benefit of that merge is that it'll make it easier to land updated encryption, PDF/A-lb, and likely other items in the future.
I'd rather not merge the two gems. Not in a minor release anyway. And bumping the major just for the merge doesn't feel like an enough of a change from the user perspective. So if you have half a day maybe spend a quarter investigating what's left to decouple pdf-core from Prawn. It might turn out that there's not that much work left.
As for testing. Given that pdf-core and prawn are coupled right now I don't see an issue in testing against pdf-core's master.
Customary we release both prawn and pdf-core at the same time. We can release pdf-core a day earlier and test and merge this PR right before the release of prawn.