1768671 - PDF.js doesn't honour filenames without pdf file extension when saving the document locally (e.g. from fido's web billing portal)

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect, P3)

Description

[:Gijs (he/him)]

See earlier comments in bug 1767321.

Apparently this is still not fixed in Nightly 102.

I'll try and find someone who uses FIDO, where I can debug what's going on when I'm (all being well) in the Toronto office next week.

Comment 1

[Vivien M. (he/him)]

Someone who uses Rogers is probably fine too. I haven’t tested on my Rogers cable account, but Rogers owns Fido and I suspect the billing portal’s code is the same for both brands.

Comment 2

[Ksenia Berezina [:ksenia] on leave until 09/15]

Attachment: samplepdf.pdf

Comment 3

[Ksenia Berezina [:ksenia] on leave until 09/15]

Attachment: 1768671.html

Comment 4

[Ksenia Berezina [:ksenia] on leave until 09/15]

Comment on attachment 9275898 [details]
1768671.html

This is a test case from fido.ca. Looks similar to the code from this comment.

Comment 5

[:Gijs (he/him)]

Per discussion with Ksenia, it seems that the difference between this and jscher's working testcase in bug 1756980 is the fact that the broken testcase doesn't specify a file extension (ie there's no ".pdf"), and the working one does.

Comment 6

[:Gijs (he/him)]

This is an explicit exception in PDF.js: https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#350-355

ISTM we could just suffix .pdf if there is no file extension present at all. WDYT, Marco?

Comment 7

[Marco Castelluccio [:marco]]

Sounds good to me.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:calixte, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 9

[Mathew Hodson]

(In reply to :Gijs (he/him) from comment #6)

This is an explicit exception in PDF.js: https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#350-355

I tried looking into this, and from what I could tell, data.filename is already "document.pdf" in PdfStreamConverter.jsm

The filename is passed to Firefox from PDF.js in firefoxcom.js

Following it back further, the filename is set at https://github.com/mozilla/pdf.js/blob/5d88233fbb11da28e155fa136d1abac0f7393f00/web/app.js#L768-L772

get _docFilename() {
  // Use `this.url` instead of `this.baseUrl` to perform filename detection
  // based on the reference fragment as ultimate fallback if needed.
  return this._contentDispositionFilename || getPdfFilenameFromUrl(this.url);
},

That's as far as I got. Maybe contentDispositionFilename isn't getting passed to PDF.js when there is a download attribute on an <a> element.

Comment 10

[Tom S. (please needinfo tschuster)]

The problem is actually in onOpenWithData. This function only sets _contentDispositionFilename iff the name ends with pdf. https://github.com/mozilla/pdf.js/blob/5d88233fbb11da28e155fa136d1abac0f7393f00/web/app.js#L718-L720

Comment 11

[Calixte Denizet (:calixte)]

The check:
https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#352
has been added a long time ago:
https://github.com/mozilla/pdf.js/pull/3437

and as pointed out by :evilpie, this one has been added in:
https://github.com/mozilla/pdf.js/pull/13014

:Snuffleupagus, wdyt about the Gijs' suggestion in https://bugzilla.mozilla.org/show_bug.cgi?id=1768671#c6 ?

Comment 12

[Jonas Jenwald [:Snuffleupagus]]

(In reply to Calixte Denizet (:calixte) from comment #11)

and as pointed out by :evilpie, this one has been added in:
https://github.com/mozilla/pdf.js/pull/13014

It seems that we have to relax the check in https://github.com/mozilla/pdf.js/blob/6ee538e0baee2434828efe97d9936f7ae436f5c6/web/app.js#L722 and replace it with something like this instead:

if (
  contentDispositionFilename &&
  typeof contentDispositionFilename === "string"
) {

:Snuffleupagus, wdyt about the Gijs' suggestion in https://bugzilla.mozilla.org/show_bug.cgi?id=1768671#c6 ?

That should work, as long as we take care to still exclude the isAttachment-case. Something like this could probably work:

if (!filename || typeof filename !== "string") {
  filename = "document";
}
if (!/\.pdf$/i.test(filename) && !data.isAttachment) {
  filename += ".pdf";
}

Comment 13

[Jonas Jenwald [:Snuffleupagus]]

It just occurred to me that a potentially simpler solution overall, assuming it's not a terrible idea for some reason I'm overlooking, could be to append ".pdf" (if it's missing) already here:
https://searchfox.org/mozilla-central/rev/b1a5802e0f73bfd6d2096e5fefc2b47831a50b2d/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#1133

Comment 14

[:Gijs (he/him)]

Attachment: Bug 1768671 - force append '.pdf' for pdf blob downloads displayed inline if it's missing, r?marco

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2cd3fe38c532
force append '.pdf' for pdf blob downloads displayed inline if it's missing, r=marco

Comment 16

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/2cd3fe38c532

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.

Comment 18

[:Gijs (he/him)]

Too late for uplift...

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Did we want to get this on to ESR102 before users start migrating over from ESR91 in a couple weeks?

Comment 20

[:Gijs (he/him)]

Comment on attachment 9285579 [details]
Bug 1768671 - force append '.pdf' for pdf blob downloads displayed inline if it's missing, r?marco

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Ergonomics fix for PDF downloads
• User impact if declined: We ignore the name suggested by the server for some PDFs
• Fix Landed on Version: 104
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): 4 lines of JS code to handle a condition of a file not having a .pdf suffix, plus an automated test that covers this case so we won't regress it again.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9285579 [details]
Bug 1768671 - force append '.pdf' for pdf blob downloads displayed inline if it's missing, r?marco

Approved for 102.3esr.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/47f3eb0e15e0

Comment 23

[Rares Doghi, Desktop QA]

Verified as fixed in out latest 102.3.0esr and 104.0.2 on all platforms.