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

fix: fix content-type detection by doing a fall-back based on the ext… #1482

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

satazor
Copy link
Contributor

@satazor satazor commented Aug 1, 2018

The JS IPFS gateway was only responding with the correct content-type for some mime-types, see https://github.com/sindresorhus/file-type#supported-file-types.
This commit now fall-backs to detecting based on the extension as well.

Note that SVGs aren't supported by the file-type module.

@satazor
Copy link
Contributor Author

satazor commented Aug 1, 2018

//cc @vasco-santos @diasdavid

This is related with ipfs-inactive/old-js-ipfs-website#122 because the SVGs aren't being rendered correctly when using the JS IPFS Gateway or when enabling the service-worker due to missing content-type header.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

SGTM

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

LGTM - waiting on CI

@@ -12,6 +12,21 @@ const Stream = require('readable-stream')
const { resolver } = require('ipfs-http-response')
const PathUtils = require('../utils/path')

function detectContenType (ref, chunk) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor typo here detectContenType - missing "t" on "conten"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix once the workshop I’m attending on dweb ends. :p


// try to guess the filetype based on the first bytes
// note that `file-type` doesn't support svgs, thereore we assume it's a svg if ref looks like it
if (!ref.endsWith('.svg')) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this check necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, file-type doesn’t support svgs and might actually return invalid mine types. They state that in their readme.

Copy link
Member

Choose a reason for hiding this comment

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

Just saw there’s already a comment there sorry!

@satazor
Copy link
Contributor Author

satazor commented Aug 1, 2018

The typo should be fixed!

The JS IPFS gateway was only responding with the correct content-type for some mime-types, see https://github.com/sindresorhus/file-type#supported-file-types.
This commit now fall-backs to detecting based on the extension as well.

Note that SVGs aren't supported by the `file-type` module.
@alanshaw alanshaw merged commit d528b3f into ipfs:master Aug 2, 2018
@olizilla
Copy link
Member

olizilla commented Aug 2, 2018

Of note: ipfs/kubo#4025
Perhaps not relevant for js-ipfs, but worth knowing about that it's currently important that the mime types from the public gateway are not correct for js file; while we don't have sub-origin restrictions, anyone could publish malicious service worker to IPFS...

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

Successfully merging this pull request may close these issues.

4 participants