Skip to content
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

Use raw.githubusercontent.com instead of raw.github.com for binaries #99

Merged
merged 1 commit into from
Feb 20, 2019
Merged

Use raw.githubusercontent.com instead of raw.github.com for binaries #99

merged 1 commit into from
Feb 20, 2019

Conversation

atma
Copy link
Contributor

@atma atma commented Feb 19, 2019 •
edited
Loading

According to Github to avoid XSS all user-generated content must be accesed from alternative domain: raw.githubusercontent.com.

Current:

curl -I -XGET https://raw.github.com/imagemin/pngquant-bin/v5.0.1/vendor/macos/pngquant
HTTP/1.1 503 Backend unavailable, connection timeout

Updated:

curl -I -XGET https://raw.githubusercontent.com/imagemin/pngquant-bin/v5.0.1/vendor/macos/pngquant
HTTP/1.1 200 OK

UPD: as @cadejscroggins indicated below raw.github.com seems to be working now, but would be nice to be sure that issue will not appear again some day.

@atma
Use raw.githubusercontent.com instead of raw.github.com for binaries
6c53a56 
According to [Github](https://developer.github.com/changes/2014-04-25-user-content-security/) to avoid XSS all user-generated content must be accesed from alternative domain: `raw.githubusercontent.com`.
@aarondfrancis aarondfrancis mentioned this pull request Feb 19, 2019
Closed
@pieterbeulque pieterbeulque mentioned this pull request Feb 19, 2019
Closed
@ChrissiQ ChrissiQ mentioned this pull request Feb 19, 2019
Open
@mfb-andy-zz
Copy link

can we please merge this. all of our builds are failing until this is pushed.

@frissekom
Copy link

We are also stuck for now. please merge this request

@miket2marcom miket2marcom mentioned this pull request Feb 19, 2019
Open
@joshbeam
Copy link

Please merge 👍

@sueannioanis
Copy link

👍

@avieseegobinsc
Copy link

🆒

colinodell
colinodell approved these changes Feb 19, 2019
View reviewed changes
Copy link

@colinodell colinodell left a comment

Choose a reason for hiding this comment

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

I can confirm this fixes the issue.

@gabrielerzinger
Copy link

please merge this

@josephmaxim
Copy link

Please merge.

@samwardatanchor
Copy link

:merge_it:

@austinknight
Copy link

Is it merged yet?

No 😑

@nik
Copy link

nik commented Feb 19, 2019

someone for the love of god merge this and unblock me

@fmolina24
Copy link

🙏 merge.

soonwong
soonwong approved these changes Feb 19, 2019
View reviewed changes
Copy link

@soonwong soonwong left a comment

Choose a reason for hiding this comment

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

MERGE ME MERGE ME MERGE ME

Kyle-Muir
Kyle-Muir approved these changes Feb 19, 2019
View reviewed changes
Copy link

@Kyle-Muir Kyle-Muir left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dylanhotfire
Copy link

Can I get a MERGE ma bro

seanwang-okta
seanwang-okta approved these changes Feb 19, 2019
View reviewed changes
Copy link

@seanwang-okta seanwang-okta left a comment

Choose a reason for hiding this comment

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

🚢

fmolina24
fmolina24 approved these changes Feb 19, 2019
View reviewed changes
Copy link

@fmolina24 fmolina24 left a comment

Choose a reason for hiding this comment

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

🚀

@markmssd
Copy link

🆘

michaelkozakovsc
michaelkozakovsc suggested changes Feb 19, 2019
View reviewed changes
Copy link

@michaelkozakovsc michaelkozakovsc left a comment

Choose a reason for hiding this comment

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

image

@colinodell
Copy link

colinodell commented Feb 19, 2019 •
edited
Loading

If you don't have any new information to add please 👍 an existing message instead of adding duplicates or spamming gifs. The fewer messages the maintainers have to read through the faster they'll get a fix released 😉

michaelkozakovsc
michaelkozakovsc approved these changes Feb 19, 2019
View reviewed changes
Copy link

@michaelkozakovsc michaelkozakovsc left a comment

Choose a reason for hiding this comment

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

image

raymondwu1
raymondwu1 approved these changes Feb 19, 2019
View reviewed changes
Copy link

@raymondwu1 raymondwu1 left a comment

Choose a reason for hiding this comment

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

This change looks amazing please merge.

@Kcharle Kcharle mentioned this pull request Feb 19, 2019
Closed
@xvvvyz
Copy link

xvvvyz commented Feb 19, 2019

raw.github.com seems to be working now.

shaun-sweet
shaun-sweet suggested changes Feb 19, 2019
View reviewed changes
Copy link

@shaun-sweet shaun-sweet left a comment

Choose a reason for hiding this comment

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

should this be in a encodeURI() call?

shaun-sweet
shaun-sweet suggested changes Feb 19, 2019
View reviewed changes
lib/index.js
@@ -3,7 +3,7 @@ const path = require('path');
const BinWrapper = require('bin-wrapper');
const pkg = require('../package.json');

const url = `https://raw.github.com/imagemin/pngquant-bin/v${pkg.version}/vendor/`;
const url = `https://raw.githubusercontent.com/imagemin/pngquant-bin/v${pkg.version}/vendor/`;
Copy link

Choose a reason for hiding this comment

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

should this be in a encodeURI() call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shaun-sweet encodeURI() has no sense here, it does not encode specials characters like :, . or / that can be found in this URL, please do not confuse with encodeURIComponent(). Please the the corresponding article on MDN.

Copy link

Choose a reason for hiding this comment

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

Good call!!

@1000ch 1000ch merged commit 0cd3a9e into imagemin:master Feb 20, 2019
@1000ch
Copy link
Member

1000ch commented Feb 20, 2019

Published v5.0.2.

@atma atma deleted the fix/bin-domain branch February 20, 2019 12:39
@destinytaoer
Copy link

In my case, it failed in githubusercontent, but it successed in github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaun-sweet shaun-sweet shaun-sweet requested changes

@gabrielerzinger gabrielerzinger gabrielerzinger approved these changes

@liamgold liamgold liamgold approved these changes

@desmondpnw3dev desmondpnw3dev desmondpnw3dev approved these changes

@zorigitano zorigitano zorigitano approved these changes

@Obi-Dann Obi-Dann Obi-Dann approved these changes

@kevinbrosamle kevinbrosamle kevinbrosamle approved these changes

@raymondwu1 raymondwu1 raymondwu1 approved these changes

@fmolina24 fmolina24 fmolina24 approved these changes

@burndeal burndeal burndeal approved these changes

@aazhari aazhari aazhari approved these changes

@dylanhotfire dylanhotfire dylanhotfire approved these changes

@seanwang-okta seanwang-okta seanwang-okta approved these changes

@jtcurr jtcurr jtcurr approved these changes

@michaelkozakovsc michaelkozakovsc michaelkozakovsc approved these changes

@immortalObject immortalObject immortalObject approved these changes

@putman10 putman10 putman10 approved these changes

@p-smiley p-smiley p-smiley approved these changes

@mfb-andy-zz mfb-andy-zz mfb-andy-zz approved these changes

@spnc-omz spnc-omz spnc-omz approved these changes

@colinodell colinodell colinodell approved these changes

@y8r y8r y8r approved these changes

@nik nik nik approved these changes

@ChrissiQ ChrissiQ ChrissiQ approved these changes

@1000ch 1000ch 1000ch approved these changes

@Kyle-Muir Kyle-Muir Kyle-Muir approved these changes

@HansBrems HansBrems HansBrems approved these changes

@frissekom frissekom frissekom approved these changes

@dhruvaldarji dhruvaldarji dhruvaldarji approved these changes

@ckknight ckknight ckknight approved these changes

@develohpanda develohpanda develohpanda approved these changes

@eighthjouster eighthjouster eighthjouster approved these changes

@Rocko10 Rocko10 Rocko10 approved these changes

@soonwong soonwong soonwong approved these changes

@markmssd markmssd markmssd approved these changes

@abrel abrel abrel approved these changes

@FalseNight FalseNight FalseNight approved these changes

@cbown75 cbown75 cbown75 approved these changes

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

Successfully merging this pull request may close these issues.

None yet