Skip to content

[api-minor] Implement securityHandler in the scripting API (bug 1731578) #14189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 27, 2021
Merged

[api-minor] Implement securityHandler in the scripting API (bug 1731578) #14189

merged 1 commit into from
Oct 27, 2021

Conversation

janekotovich
Copy link
Contributor

@janekotovich janekotovich commented Oct 25, 2021 •
edited
Loading

Because

Missing security handler throws an error on pdf.js

The bug has been reported in bugzilla:
https://bugzilla.mozilla.org/show_bug.cgi?id=1731578

This pull request

Add Security Handler to pdf.js
Add puppeteer tests

Issue that this pull request solves

Closes: #14146

@Snuffleupagus Snuffleupagus mentioned this pull request Oct 25, 2021
Closed
@Snuffleupagus Snuffleupagus changed the title Security handler [api-minor] Implement securityHandler in the scripting API Oct 25, 2021
Snuffleupagus
Snuffleupagus requested changes Oct 25, 2021
View reviewed changes
src/core/catalog.js Outdated
get securityHandler() {
const encrypt = this.xref.trailer.get("Encrypt");
if (!isDict(encrypt)) {
return null;
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Oct 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference (since I've suggested changes that makes this getter unnecessary):
Please take care to shadow all code-paths, rather than just one, when using that functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, no problem!

@marco-c marco-c changed the title [api-minor] Implement securityHandler in the scripting API [api-minor] Implement securityHandler in the scripting API (bug 1731578) Oct 25, 2021
@janekotovich janekotovich force-pushed the security_handler branch 2 times, most recently from e893b8c to 50512c8 Compare October 25, 2021 12:02
@janekotovich
Copy link
Contributor Author

janekotovich commented Oct 25, 2021 •
edited
Loading

@Snuffleupagus Hope I'm on the right way. Basically if we including filter in info, we do not need to specify securityHandler on genericScripting ( as it is in ...info), same as in getMetadata() as it is a part of info: results[0]. However still cannot figure out why after building and everything it gives me error.

btw - I see that the tests are falling, will research what suitable define entry would be. Followed the failing log and can see that I compare Standard with null so they fall, because my filter comes as a null now probably

Snuffleupagus
Snuffleupagus reviewed Oct 25, 2021
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there's a merge-commit accidentally included here, please remove than one and squash the commits.

However still cannot figure out why after building and everything it gives me error.

Where is the error thrown, in the terminal or in the browser?
Also, can you please provide the exact error message here?

@janekotovich janekotovich force-pushed the security_handler branch 3 times, most recently from 3856e36 to 7691958 Compare October 25, 2021 13:43
@Snuffleupagus
Copy link
Collaborator

There's a number of conflicts here, which should go away if you rebase the patch. Assuming that you've followed https://github.com/mozilla/pdf.js/wiki/Contributing, then rebasing your local branch should be as simple as running git rebase upstream/master (obviously fixing any conflicts that arise during the process).

@janekotovich
Copy link
Contributor Author

@Snuffleupagus yeah right now working on it, sorry for extra submits.

@janekotovich janekotovich force-pushed the security_handler branch 2 times, most recently from 9a38042 to 74c7eaa Compare October 25, 2021 14:17
@janekotovich
Copy link
Contributor Author

janekotovich commented Oct 25, 2021 •
edited
Loading

@Snuffleupagus basically what happens is localhost:8888 still throws me an error on load - thats the translation
These documents only work in Spanish versions of Acrobat equal to or greater than 5.0
so I assume securityHandler is null / undefined and I don't understand why
Screen Shot 2021-10-26 at 12 41 28 am

@calixteman
Copy link
Contributor

@Snuffleupagus basically what happens is localhost:8888 still throws me an error on load - thats the translation These documents only work in Spanish versions of Acrobat equal to or greater than 5.0 so I assume securityHandler is null / undefined and I don't understand why Screen Shot 2021-10-26 at 12 41 28 am

Yeah but it isn't the same message in the popup (you can open the pdf in firefox to compare).

@janekotovich
Copy link
Contributor Author

Hm, okay. I though it still mistake then ( even though I've noticed msgs are different. Will resubmit patch with null default val

Yeah but it isn't the same message in the popup (you can open the pdf in firefox to compare).

@janekotovich janekotovich force-pushed the security_handler branch 2 times, most recently from 04b58cb to 0487282 Compare October 26, 2021 13:09
@janekotovich janekotovich marked this pull request as ready for review October 26, 2021 13:09
calixteman
calixteman reviewed Oct 26, 2021
View reviewed changes
test/integration/scripting_spec.js Outdated
afterAll(async () => {
await closePages(pages);
});
it("must print secHandler valiue in a text field", async () => {
Copy link
Contributor

@calixteman calixteman Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: securityHandler instead of secHandler and there's a typo with value

@calixteman
Copy link
Contributor

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

Command cmd_unittest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/2c04377959c9844/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/4e7f7d6a468af31/output.txt

Total script time: 7.85 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/b277e1b0eebe027/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/d1216af429db5a3/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/b277e1b0eebe027/output.txt

Total script time: 4.69 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor

/botio-windows integrationtest

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/4639e203f0e395a/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/4639e203f0e395a/output.txt

Total script time: 6.53 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor

/botio-windows integrationtest

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/c7c0c5aa3895fc6/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/c7c0c5aa3895fc6/output.txt

Total script time: 8.78 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor

/botio-windows integrationtest

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/4f4adfaa91099d8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/4f4adfaa91099d8/output.txt

Total script time: 8.99 mins

  • Integration Tests: FAILED

calixteman added a commit to calixteman/pdf.js that referenced this pull request Oct 27, 2021
@calixteman
Avoid to display download panel during integration tests
a24322f 
  - it could be the cause of the failures in mozilla#14189;
  - and the patch in firefox to enable the pref landed very recently:
  https://hg.mozilla.org/mozilla-central/rev/3de56e38f3c87f33a1e7849701edb3c62bc472a5
@calixteman calixteman mentioned this pull request Oct 27, 2021
Merged
@calixteman
Copy link
Contributor

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 1

Live output at: http://54.193.163.58:8877/c19472a59cde797/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/3231efee05e10d2/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/3231efee05e10d2/output.txt

Total script time: 3.86 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/c19472a59cde797/output.txt

Total script time: 6.54 mins

  • Integration Tests: Passed

calixteman
calixteman approved these changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for doing that.

@calixteman calixteman merged commit 74bc6d2 into mozilla:master Oct 27, 2021
@janekotovich janekotovich deleted the security_handler branch October 27, 2021 11:17
@janekotovich janekotovich mentioned this pull request Oct 29, 2021
Closed
bh213 pushed a commit to bh213/pdf.js that referenced this pull request Jun 3, 2022
@calixteman @bh213
Avoid to display download panel during integration tests
94b335e 
  - it could be the cause of the failures in mozilla#14189;
  - and the patch in firefox to enable the pref landed very recently:
  https://hg.mozilla.org/mozilla-central/rev/3de56e38f3c87f33a1e7849701edb3c62bc472a5
rousek pushed a commit to signosoft/pdf.js that referenced this pull request Aug 10, 2022
@calixteman @rousek
Avoid to display download panel during integration tests
7ded9b8 
  - it could be the cause of the failures in mozilla#14189;
  - and the patch in firefox to enable the pref landed very recently:
  https://hg.mozilla.org/mozilla-central/rev/3de56e38f3c87f33a1e7849701edb3c62bc472a5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus requested changes

@calixteman calixteman calixteman approved these changes

Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JS - Implement securityHandler in the scripting API
5 participants
@janekotovich @Snuffleupagus @calixteman @pdfjsbot @timvandermeij