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.

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

Web extension resources return incorrect MIME type #470

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

Conversation

amvanbaren
Copy link
Contributor

Contributes to #468
Use Apache Tika to detect mimetype

@amvanbaren amvanbaren self-assigned this Jun 9, 2022
@jeanp413
Copy link
Contributor

jeanp413 commented Jun 9, 2022

@amvanbaren I just found that mime types are listed in the [Content_Types].xml file inside the vsix 🤦 So we just need to parse it and use the rules in there

<?xml version="1.0" encoding="utf-8"?>
<Types xmlns="http://schemas.openxmlformats.org/package/2006/content-types">
	<Default Extension=".json" ContentType="application/json"/>
	<Default Extension=".vsixmanifest" ContentType="text/xml"/>
	<Default Extension=".png" ContentType="image/png"/>
	<Default Extension=".md" ContentType="text/markdown"/>
	<Default Extension=".js" ContentType="application/javascript"/>
	<Default Extension=".map" ContentType="application/json"/>
	<Default Extension=".txt" ContentType="text/plain"/>
	<Default Extension=".woff2" ContentType="font/woff2"/>
	<Default Extension=".html" ContentType="text/html"/>
	<Default Extension=".css" ContentType="text/css"/>
</Types>

@jeanp413
Copy link
Contributor

jeanp413 commented Jun 9, 2022

Some more info https://docs.microsoft.com/en-us/visualstudio/extensibility/the-structure-of-the-content-types-dot-xml-file?view=vs-2022#attribute-name-attribute-1
vsix files should be application/zip and all other file types application/octet-stream

@amvanbaren
Copy link
Contributor Author

I just found that mime types are listed in the [Content_Types].xml file inside the vsix 🤦 So we just need to parse it and use the rules in there.

Ok, I'll use that file to assign the right content type to a file resource. If the resource isn't listed then application/octet-stream is used.

vsix files should be application/zip and all other file types application/octet-stream

When I download a vsix file, then the content-type is application/vsix
content-type-application-vsix

@jeanp413
Copy link
Contributor

When I download a vsix file, then the content-type is application/vsix

Then their docs are outdated 😅

@jeanp413
Copy link
Contributor

Need to test what happens when some files don't have file extension, e.g sometimes README files don't have the .md extension, maybe we should add some default rules by filename

@filiptronicek
Copy link
Member

filiptronicek commented Jul 1, 2022

From my testing, @jeanp413, if there is no extension present, it will not be in the [Content_Types].xml file. Do you think there could be cases in which serving everything with no extension with a text/plain would not work?

This is just concerning web extensions, correct? So there will be no binaries served, which are the only thing in my mind that don't usually have extensions.

cc @amvanbaren

@jeanp413
Copy link
Contributor

jeanp413 commented Jul 1, 2022

Thanks for testing this, @filiptronicek, from the MS docs anything else should be application/octet-stream.
Looking at vsce source code they allow license without extension but then they add the file extension if it's missing when packaged so we won't have any issue 🎉

Comment on lines 670 to 678
var cssChunkWebResourceFile = new FileResource();
cssChunkWebResourceFile.setExtension(extVersion);
cssChunkWebResourceFile.setName("extension/public/static/css/main.9cab4879.chunk.css");
cssChunkWebResourceFile.setType(FileResource.RESOURCE);
cssChunkWebResourceFile.setStorageType(STORAGE_DB);
cssChunkWebResourceFile.setContent(".root { margin: 0 auto; }".getBytes());
cssChunkWebResourceFile.setContentType("text/css");
Mockito.when(repositories.findFileByTypeAndName(extVersion, FileResource.RESOURCE, "extension/public/static/css/main.9cab4879.chunk.css"))
.thenReturn(cssChunkWebResourceFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add one more test for the default mime type application/octet-stream

@jeanp413
Copy link
Contributor

@amvanbaren I saw openvsx got finally deployed 🎉, can we merge this so it's shipped in next deployment 🙂
BTW, can you share until which commit got deployed?

@amvanbaren
Copy link
Contributor Author

can we merge this so it's shipped in next deployment 🙂

Yes, I'll rebase and complete this PR.

BTW, can you share until which commit got deployed?

Up until PR #517 is deployed. Production is 25 commits behind master.

@amvanbaren amvanbaren force-pushed the feature/issue-468 branch 3 times, most recently from 0a5dbba to d9e2b5e Compare September 27, 2022 08:55
@jeanp413
Copy link
Contributor

jeanp413 commented Nov 14, 2022

@amvanbaren let's merge 🙏

@amvanbaren
Copy link
Contributor Author

@amvanbaren let's merge 🙏

@jeanp413 Has this been reviewed and tested?

@jeanp413
Copy link
Contributor

I thought it was already approved 😅 , will take a look

@amvanbaren
Copy link
Contributor Author

This PR is missing a migration: #468 (comment)

@jeanp413
Copy link
Contributor

@filiptronicek if you have some free time could you test this 🙏 ?

Copy link
Member

@filiptronicek filiptronicek left a comment

Choose a reason for hiding this comment

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

What seems to have happened is that now everything is an octet stream, even though the files' extensions are listed in the content types file along with their MIME types.

image

image

@amvanbaren amvanbaren force-pushed the feature/issue-468 branch 3 times, most recently from fe0f247 to 180420d Compare December 20, 2022 15:15
@kineticsquid
Copy link
Contributor

Same priority as #468

Use [Content_Types].xml to set FileResource contentType
Add test case for default content type: application/octet-stream
Add migration to set FileResource.contentType
Set contentType for RESOURCE type
Remove dot ('.') if extension starts with a dot
Use utf-8 charset for text resources.
@amvanbaren
Copy link
Contributor Author

Ok, I've fixed the PR. @filiptronicek Do you have time to test it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

4 participants