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

Update UiRequest.py #2248

Closed
wants to merge 15 commits into from
Closed

Update UiRequest.py #2248

wants to merge 15 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 26, 2019

Fonts are NOT applications! Added PGP related MIME types.

Fonts are NOT applications! Added PGP related MIME types.
@purplesyringa
Copy link
Contributor

Why are the .png (and similar) changes required? mimetypes module sets correct mimetypes for (most) images already.

@ghost
Copy link
Author

ghost commented Oct 26, 2019

Feel free to add or remove MIME types. I think it's better to have common extensions stated than rely on the guessing of the mimetype module. This is my preference, that is why I included the .png, .webm and others.

@purplesyringa
Copy link
Contributor

It's not guessing. The mimetypes module detects mimetype based on the extension, and .png is hardcoded. You can take a look at its source code if you're not sure.

@ghost
Copy link
Author

ghost commented Oct 26, 2019

else: content_type = mimetypes.guess_type(file_name)[0] is guessing by checking the extension and then looking up in the module the supposedly correct MIME type. I think that ZeroNet should not rely on the module since some types are incorrect or not included at all like the PGP MIME types which I stated.

@purplesyringa
Copy link
Contributor

since some types are incorrect

Which ones?

I see no reason in adding unneeded code when it works correctly. To be honest, I think that it's mimetypes module that should be patched, not ZeroNet code, but that's unlikely.

@purplesyringa
Copy link
Contributor

I'd leave .js, .webp, fonts and gpg stuff only. I'm also not sure whether .js is required (application/javascript is a correct mimetype as well but old browsers might not support it). Everything else is correct.

@HelloZeroNet
Copy link
Owner

Type name:
application

According to https://www.iana.org/assignments/media-types/application/font-woff

Remove some MIME types because mimetype module correctly provides them. text/plain, text/html and text/css is not removed. Fonts MIME types are fixed because they are not applications. I have added PGP MIME types for keys, encrypted and for signature. 
I declared also utf8 encoding for text/css, application/json and for application/javascript. 
JavaScript should be application/javascript and not text/javascript according to [RFC4329] (https://tools.ietf.org/html/rfc4329#page-9).
@purplesyringa
Copy link
Contributor

@purplesyringa
Copy link
Contributor

@HelloZeroNet @filips123 @krixano Do you see any problems in using application/javascript instead of text/javascript? I'd leave text/javascript but we could remove it (and thus simplify code) if application/javascript works as well.

@purplesyringa
Copy link
Contributor

.txt, .html and .css are handled by mimetypes module as well. BTW, I'd recommend not to use .html extension to detect mimetype because ZeroNet uses text/html mimetype to detect when to add sandbox. Mentioning .html might make it look like if mimetypes module could be removed, but there are other (strange) extensions like .htm and .xht, so removing might have serious consequences for ZeroNet security.

@filips123
Copy link
Contributor

filips123 commented Oct 26, 2019

@imachug Regarding JS mime type, I already searched about this around the web and it seems that nobody actually know which one is correct.

Also, if you are already adding new mime types, you should also add .webmanifest ==> application/manifest+json mime type as this is official mime type for Web App Manifest for PWA. It was already added to mimetypes module (by me), but the change is only present in Python 3.8+.

@HelloZeroNet
Copy link
Owner

$ curl --head https://github.githubassets.com/static/fonts/inter/Inter-Medium.woff | grep content-type
Content-Type: application/font-woff

But if we want to change it, then

        elif ext in ("woff", "woff2", "ttf"):
            content_type = "font/%s" % ext

would be more clean/compact solution

@ghost
Copy link
Author

ghost commented Oct 26, 2019

Yeah @imachug that's correct! https://www.iana.org/assignments/media-types/media-types.xhtml#font and [RFC8081] states font/ instead of application/ MIME for fonts.

@ghost
Copy link
Author

ghost commented Oct 26, 2019

@filips123 JavaScript should be application/javascript and not text/javascript according to RFC4329.

@ghost
Copy link

ghost commented Oct 26, 2019

This pr also belongs in the mimetypes repo - don't blame ZeroNet devs for things that aren't of their doing. In the meantime, a workaround is good, because this fonts issue has been a problem for a while I think.

@HelloZeroNet
Copy link
Owner

fonts issue has been a problem for a while I think.

As far I know there were no issue reported that is connected to invalid mime types.

All issues I have seen was because if someone enables the UiPassword plugin, then the cookie won't be passed to the css font requests and the client will display the login form instead of the resource.

@ghost
Copy link

ghost commented Oct 26, 2019

Idk, I'm pretty sure there was a comment in one issue that mentioned this problem with the mime type. Maybe I can find it, idk.

@ghost
Copy link

ghost commented Oct 26, 2019

@HelloZeroNet It's right here: #1940
Tangdou closed it before it was even fixed. I made a comment on it recently.

@ghost ghost mentioned this pull request Oct 26, 2019
@ghost
Copy link

ghost commented Oct 26, 2019

Hold on a sec... Previously, I wasn't getting correct results when running this in python when that issue (#1940 ) originally occured:

print(import('mimetypes').guess_type('MaterialIcons-Regular.woff'))

But now I am getting "font/woff" in python. I suppose the mimetypes module was fixed for fonts but the zeronet workaround wasn't removed or something? idk...

Aso, @HelloZeroNet , some people were still getting font problems in Tor browser very recently... don't know if this pr fixes it though.

Removed some MIME types because mimetypes module correctly provides them. text/plain, text/html and text/css is not removed. 
Font MIME types are fixed because they are not applications. All font MIME types from https://www.iana.org/assignments/media-types/media-types.xhtml#font is stated in a clean/compact fashion as @HelloZeroNet suggested #2248 (comment). 
I have added PGP MIME types for keys, encrypted and for signature. 
I declared also utf8 encoding for text/css, application/json and for application/javascript. 
JavaScript should be application/javascript and not text/javascript according to [RFC4329] (https://tools.ietf.org/html/rfc4329#page-9).
I don't add application/manifest+json at this time but you are free to do @filips123
@HelloZeroNet
Copy link
Owner

@HelloZeroNet It's right here: #1940
Tangdou closed it before it was even fixed. I made a comment on it recently.

As he stated in last comment: "When enable the ui_password plugin , the problem will reproduce ."

So it was because of the sandbox.

@filips123
Copy link
Contributor

But now I am getting "font/woff" in python. I suppose the mimetypes module was fixed for fonts but the zeronet workaround wasn't removed or something?

Which Python version do you have? Because mimetypes is in standard library, it is updated with Python. So maybe this was changed in recent Python versions.

@purplesyringa
Copy link
Contributor

@filips123 @krixano mimetypes library has a small built-in list of common extensions, but other mimetypes are loaded from registry (in Windows) and configuration files (in Linux).

@ghost
Copy link
Author

ghost commented Oct 26, 2019

@filips123

Why you didn't add Web App Manifest. If I would add it, this would be another PR and if you already have PR that updated mime types this would be unnecessary complicating.

See: w3c/manifest#689 (comment)

@filips123
Copy link
Contributor

@imachug There are around 100 built-in extensions but yes other are loaded from system.

@purplesyringa
Copy link
Contributor

@hacktivist It looks like the comment you're referring too agrees with @filips123, doesn't it?

@filips123
Copy link
Contributor

filips123 commented Oct 26, 2019

@hacktivist Can you explain that comment a bit more? Because I see it as point for using .webmanifest extension and correct content type.

On my gateway I use for text/html the Link header also because I serving files (font, logo etc) using HTTP/2.0 with PUSH.
src/Ui/UiRequest.py Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Oct 26, 2019

Loading fonts with application/octet-sream content-type should work.

Eg. Cloudflare/Firefox serve it this way:

$ curl --head https://www.mozilla.org/media/fonts/Metropolis-Bold.6a80125e795a.woff2
Content-Type: binary/octet-stream

So I still think the problem was because of missing cookie header and not because of invalid content type, but including more content types in UiRequest.py should not hurt.

@HelloZeroNet I don't think you get what I was saying at all. You were trying to say that that other issue wasn't about this at all, but you literally propose a "fix" for this exact problem in that other issue (and you never actually stated that it was fixed) - so it was about this.

So I still think the problem was because of missing cookie header and not because of invalid content type

Because apparently you can't have two problems at once right? I was having that content-type font problem also, and I wasn't using ui_password (and I probably have photo-evidence of this somewhere on ZeroMe)... so that goes to show.
I mean, logically, the problem was very clearly both ui_password and a wrong content-type on fonts considering both YOU and I tested it and it wasn't giving back the correct content-type, but let's just forget about that why don't we!

Anyways @HelloZeroNet , icon fonts don't currently work in Tor browser - fix your crap!

New Issue: #2249

…"no-referrer" header.

Adding Referrer Policy to mitigate Reverse Tabnabbing (https://www.owasp.org/index.php/Reverse_Tabnabbing)
See recent commit escaping sandbox using target _blank: e82155a
@ghost
Copy link
Author

ghost commented Oct 27, 2019

The MIME type font/* should be used on fonts because, I do serving ZeroNet content with a reverse proxy and some resource is cached on the client, more specifically the 1,5 MB woff2.

@ghost ghost changed the title Update MIME Update UiRequest.py Oct 27, 2019
Hacktivist added 5 commits October 27, 2019 01:18
Like this.
Update MIME type from text/javascript to application/javascript. 
Note, that MIME type application/* is not caching. 
See: 10d44cf
Let me know if this don't work.
font-src '127.0.0.1:43110'
@ghost
Copy link
Author

ghost commented Oct 27, 2019

@imachug Now the fonts working? :)

src/Ui/UiRequest.py Outdated Show resolved Hide resolved
headers["Cache-Control"] = "public, max-age=600" # Cache 10 min
else:
headers["Cache-Control"] = "no-cache, no-store, private, must-revalidate, max-age=0" # No caching at all
if status in (200, 206) and cache_txt_css_html: # Cache MIME type text/* for 1 week
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK we don't do cache invalidation so this might be a problem. Static site update might take a long while...

Copy link
Author

Choose a reason for hiding this comment

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

Cache is public as you see. In the other hand after siteSign there should be sitePublish which will notify all peers about the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

How does this help? I mean, the browser would still use cached html, wouldn't it?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but not for 10 minutes. It would be crazy to assume that a static sites changes in every 10 minutes.
Does the browser cache content.json? No, is not because we are not caching any MIME type application/*, specially not JavaScript.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's crazy to assume 10 minutes but it's crazy to assume a whole week too. I'd recommend 2 hours or so.

Copy link

Choose a reason for hiding this comment

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

ZeroNet should check the content.json and if the hash is different than refresh the site.

You clearly have no idea what you're talking about. LOL

Copy link

@ghost ghost Oct 28, 2019

Choose a reason for hiding this comment

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

sites but you fail to come up with an example where HTML is changes every 10 minutes.

You know how I can tell you're a very fishy person... because you keep spewing that everyone's saying "10 minutes" when nobody has said that. Everyone here has only said changes happen shorter than a week.

I'll be making my own PR.

Btw, I did have a static zite, lol. And it changed frequently. Again with the fishiness... you clearly spew crap when you don't actually know what the hell you're talking about, lol

Copy link

@ghost ghost Oct 28, 2019

Choose a reason for hiding this comment

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

Right... Because apparently everyone who opens an issue also can't program? Because apparently you think that there's no other reason for people to not fix the issue themselves. Because apparently people don't have work or school or anything else going on in their life. Again a very fishy person.

Btw, there's a different between fixing something and half-assing it (and deliberately preventing things from working because you're an asshole).

Btw, you should probably stop with the Strawman (and Tu Quoque)

Copy link

Choose a reason for hiding this comment

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

Right... says the person who also said crap about ivanq who is the second highest for most commits and has fixed more vulnerabilities than you've even found in ZeroNet, LOL

Copy link

@ghost ghost Oct 28, 2019

Choose a reason for hiding this comment

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

You make your own PR but copy nothing from mine!

Does this mean you're claiming full copyright for your commit?

@HelloZeroNet I'd be weary of the potential legal consequences of this in the future. This is precisely why open-source repos have a file that requires all committers to give up rights to their commits.

@ghost
Copy link
Author

ghost commented Oct 27, 2019

😞 I sent the wrong link in 99bea95 that should be: #2248 (comment)

@ghost ghost mentioned this pull request Oct 28, 2019
@HelloZeroNet
Copy link
Owner

I have pushed modifications in Rev4250 that has updated list of bundled content types based on this PR and also solves the font loading problems.
Adding long cache values is not a great idea as the content can change any time.

@ghost ghost mentioned this pull request Oct 28, 2019
@ghost
Copy link

ghost commented Nov 5, 2019

Also posted on antifa/ZeroNet#1

Here's a 3 way diff. Center is the original before hacktivist/antifa's PR. Left is nofish's implementation. Right is hacktivist/antifa's implementation from the PR (#2248)

Screenshot from 2019-11-05 00-28-20
Screenshot from 2019-11-05 00-27-28
Screenshot from 2019-11-05 00-27-47
Screenshot from 2019-11-05 00-28-03

Here's a video of me scrolling through the whole file:
https://www.youtube.com/watch?v=YaxcDekczYo

Here's another diff I did - a two way diff comparing ZeroNet current code with antifa/hacktivist's PR:
https://www.diffchecker.com/NO4OAFuM
(Note: because this is two way, it doesn't show which parts were in there originally before the PR or nofish's implementation).

@ghost
Copy link

ghost commented Nov 6, 2019

I'm posting here to update people on what's going on with the user @hacktivist / @antifa. After antifa was blocked, he created a new account called @LiberateZeroNet. He has deleted his account again and created a new account called @fukcyouintheass. This account has already been deleted, and 6 hours ago he has created another new account called AnonymousChronicler. I believe that he has an additional account that he has created an hour ago called bela-miklos. Both of these accounts have one repository that happens to be a copy of ZeroNet source code (but not shown as a fork, and under a different name).

Every single time I find out about an additional username this person created, I report it to GitHub Support where I have an issue open with them about the user hacktivist all the way back from when he still used that name - I created this report with GitHub Support on October 30th.

Screenshot from 2019-11-05 19-02-08
Screenshot from 2019-11-05 19-02-25
Screenshot from 2019-11-05 19-02-40
Screenshot from 2019-11-05 19-02-55
Screenshot from 2019-11-05 19-03-13

@ghost
Copy link

ghost commented Nov 6, 2019

Another update - Looks like another account by this person has been created: PICSANRUGLAK
Joined an hour ago.

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

Successfully merging this pull request may close these issues.

3 participants