Skip to content
This repository has been archived by the owner on May 16, 2024. It is now read-only.

[IMP] Add checksum verification for wkhtmltox #452

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lasley
Copy link
Contributor

@lasley lasley commented Jun 16, 2017

This adds checksum verification to the wkhtmltox download, as discussed in #450.

Depends on:


cd ${HOME}/maintainer-quality-tools/travis/ &&
wkhtmltox=$(wget -qO- -t 1 --timeout=240 https://github.com/wkhtmltopdf/wkhtmltopdf/releases/download/${WKHTMLTOPDF_VERSION}/wkhtmltox-${WKHTMLTOPDF_VERSION}_linux-generic-amd64.tar.xz) &&
WKHTMLTOX_CHECKSUM='049b2cdec9a8254f0ef8ac273afaf54f7e25459a273e27189591edc7d7cf29db'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From @moylop260 in #450 (comment)

I'm not sure of use this environment variable just for webkit.
I mean, we could use a wget_custom with a dictionary of checksum and url used from MQT.
But IMHO this is other feature (then other PR).

Could we use just the change of the URL like as main comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't being exported though, so it doesn't survive past the execution of this script - or am I wrong?

The alternative is just checking the sum against a string, which I do not feel is as clear.

I mean, we could use a wget_custom with a dictionary of checksum and url used from MQT.

IMO this is another feature in itself. We currently support one version of wkhtmltopdf according to what I see here.

@pedrobaeza
Copy link
Member

I have updated the dependency to #450, as I think it's the correct one (not #50)

@pedrobaeza
Copy link
Member

Waiting @moylop260's feedback

@moylop260
Copy link
Contributor

Is possible skip the checksum validation if the environment variable is missing?

@lasley
Copy link
Contributor Author

lasley commented Jun 18, 2017

@moylop260 - Which environment variable? The checksum validation does not take place if the env var for the wkhtmltox version is SKIP. Is that what you're referring to?

@moylop260
Copy link
Contributor

moylop260 commented Jun 19, 2017

Could re-use current environment of TESTS=1 from travis.yml to set the following variables?

  1. WKTHMLTOPDF_VERSION = version1
  2. WKTHMLTOPDF_VERSION = version2 (other valid version diferent)
  3. and SKIP
  4. w/o define

@moylop260 moylop260 closed this Jun 19, 2017
@moylop260 moylop260 reopened this Jun 19, 2017
@lasley
Copy link
Contributor Author

lasley commented Jun 20, 2017

Hey @moylop260 - sorry, but I'm not sure I understand. Are you proposing that we just default to the WKTHMLTOPDF_VERSION when TEST=1, but otherwise default to skip - if the variable isn't otherwise declared? So something like this?

TESTS WKTHMLTOPDF_VERSION defined WKTHMLTOPDF_VERSION actual
1 $CURRENT_VERSION
1 1.2.3 1.2.3
0 SKIP
0 1.2.3 1.2.3

@moylop260
Copy link
Contributor

sorry for confusion
I meant that we should increase the coverage for this script using all conditions.
So we should change the .travis.yml to test all cases

I'm not talking about a change of behaviour

@lasley
Copy link
Contributor Author

lasley commented Jul 7, 2017

@moylop260 - Hmmm so you're proposing that we test an invalid checksum? Do we have a way to check if something is installed in our environment using our Travis file?

@lasley
Copy link
Contributor Author

lasley commented Jan 3, 2018

@moylop260 - I am unclear on how to proceed on this. Do you have a suggestion on my above?

@moylop260
Copy link
Contributor

Is there a way to skip checksum validation if is not defined?

@lasley
Copy link
Contributor Author

lasley commented Jan 16, 2018

We could do that by just not checking the checksum if WKHTMLTOX_CHECKSUM, but I think that's opening up a security issue that is being closed here. What's the motivation behind no checksum while still installing wkhtmltox?

@moylop260
Copy link
Contributor

We will need change all .travis.yml files that don't have this checksum value filled yet.
Then if we merge it this PR all builds will be fail.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants