Fix addNamedDestination writing the wrong number of parameters - #1783
Fix addNamedDestination writing the wrong number of parameters#1783MahathirMohammadShuvo wants to merge 1 commit into
Conversation
addNamedDestination passed its arguments straight through, so a call could write a destination carrying more or fewer parameters than its type takes (ISO 32000-1, Table 151). Surplus parameters are the clearer problem, because a reader does not tolerate them: pdfjs-dist rejects [page /Fit 99] and [page /XYZ left top zoom extra] outright, so the link goes nowhere, while accepting both of them trimmed. Leaving the top off an XYZ destination, or passing it as undefined, was the noisier case. The top is flipped against the page height, and page.height minus undefined is NaN, which threw `unsupported number: NaN` from PDFObject once the destination was serialised — an error naming neither the method nor the argument. Only that one position was affected: an undefined left or zoom already serialised as null, so the method was inconsistent about which missing parameter it would tolerate. Arguments are now trimmed to the type's parameter list, and a short list is filled out with null for the five types whose parameters Table 151 allows to be null: XYZ, FitH, FitV, FitBH and FitBV. FitR has no such allowance, so a short FitR is passed through as it was given, as is a destination type that is not recognised. This resizes the argument list; it does not validate the parameter values.
|
IMHO such defensive approaches should not land. All examples are using the api in a wrong way pdfkit is a low level library and sanitization of input should be done a layer above |
|
Fair enough — that's a reasonable line for a low-level library, and you're right that all three examples are misuse. The one piece I'd still flag isn't sanitisation: with a surplus parameter pdfkit emits a destination that pdf.js rejects outright, so the link silently goes nowhere rather than erroring. If you'd rather the library stay permissive there, would a one-line note in |
Yes. Please do. Another thing to do is proper type checking. Hopefully in next release we can incorporate the types |
|
Closing this in favour of #1785, which is the docs note instead. |
2 participants
What kind of change does this PR introduce?
Bug fix. No linked issue.
addNamedDestinationpasses its arguments straight through, so a call can write adestination carrying more or fewer parameters than its type takes (ISO 32000-1,
Table 151). Two things go wrong.
It can throw. An
XYZtop is flipped against the page height, andpage.height - undefinedisNaN:That is raised from
PDFObject.numberatdoc.end()and names neither the methodnor the argument. Only the top position was affected — an undefined
leftorzoomalready serialised as
null— so the method was inconsistent about which missingparameter it tolerated. Passing the top explicitly as
undefinedthrew the same wayand no longer does, which is the one behaviour change an existing caller could
notice.
Surplus parameters silently produce a dead link.
pdfjs-distrejects theseoutright, so
getDestination()resolves tonulland the link goes nowhere:addNamedDestination('L', 'Fit', 99)[page /Fit 99]— dead[page /Fit]addNamedDestination('L', 'XYZ', 1, 2, 3, 4)[page /XYZ 1 790 3 4]— dead[page /XYZ 1 790 3]Short parameter lists are the milder half, since a reader does accept
[page /XYZ left top]. But it is still a parameter short of what Table 151 defines,and it is the natural way to write a contents entry meant to jump to a point without
disturbing the reader's zoom:
addNamedDestination('LINK', 'XYZ', x, y).What the change does. Arguments are trimmed to the type's parameter list, and a
short list is filled out with
nullfor the five types whose parameters Table 151allows to be null:
XYZ,FitH,FitV,FitBHandFitBV.FitRhas no suchallowance, so a short
FitRis passed through exactly as it is today, as is adestination type that is not recognised. This resizes the argument list; it does not
validate the parameter values, so
('L', 'FitR', 1, null, 3, 4)still goes out as itdoes now.
Across all eight types at arities 0–5, every exact-arity output is byte-identical to
master— no well-formed call changes.doc.text(..., { destination })and the imagepath both pass every parameter explicitly and are unaffected.
One thing I left alone, and a question.
XYZ's top is flipped against the page height butFitH's is not, sodocs/destinations.md:11describes('LINK', 'FitH', 100)as "vertical top is100" while that value reaches the reader in bottom-left origin — unlike the
XYZexample below it on line 14.
tests/unit/trailer.spec.jspins the current behaviour,so I have not touched it and it is out of scope here. Is that deliberate? If you
would like it changed I am happy to do it separately.
Checklist: