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

MCL-19983 & WEB-1429 Fixed legacy skin loading & server authentication. #33

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

craftycodie
Copy link

@craftycodie craftycodie commented Nov 2, 2021

Summary of Changes

MCL-19983 Skin Fix

Per MCL-19983, this pull requests fixes skins in legacy versions of Minecraft by registering a custom HTTP protocol, to essentially override requests to the URL where skins used to be back then.
The protocol uses authlib to fetch skins just like modern versions of Minecraft do.

I believe this is the only way to fix this skin issue, as skins used to be hosted at s3.amazonaws.com, it is unlikely it can be resolved server-side.

WEB-1429 Server Login Fix

As described in WEB-1429, it is currently impossible to join secured (online-mode=true) legacy servers. The game used to authenticate via minecraft.net, but this has since been moved. I have solved this using a custom HTTP handler which intercepts the server login request before it is performed, and uses Authlib to perform this request instead.

I believe this is the only suitable solution for this bug, as legacy minecraft versions are written to use HTTP rather than HTTPS, and include the access token in the URL, so handling this on the server side is insecure, and it should be done on the client side instead.

Additional Information

Since legacy Minecraft does not support the slim skin model, slim skins will be appropriately stretched out for the classic model. The screenshot below is an example of a slim skin.

image

I have also updated log4j, due to the infamous vulnerability, you can see #34 for more on that.

Deployment

Were this pull request accepted, to deploy the fix would require updating launcher manifest JSON files for legacy versions. The json files would need launchwrapper bumped to the latest version, and would need some additional dependencies for authlib, the same ones modern minecraft has.
The new dependencies are:

  • org.apache.logging.log4j:log4j-api:2.16.0
  • org.apache.logging.log4j:log4j-core:2.16.0
  • org.apache.logging.log4j:log4j-slf4j18-impl:2.16.0
  • com.mojang:authlib:2.3.31
  • com.google.code.gson:gson:2.8.0
  • org.apache.commons:commons-lang3:3.5
  • com.google.guava:guava:21.0
  • commons-io:commons-io:2.5
  • commons-codec:commons-codec:1.10

Testing Guidance

Skins

  • Launch a legacy version using the new launchwrapper with any custom skin.
  • Load into any world and after a moment your custom skin should appear.

Server Login

  • Start a legacy server with an online-mode fix applied (you can find one here, there are many others.)
  • Join the server.

@ExplodingBottle
Copy link

This pull request seems well coded !
But I'm pretty sure it won't be included because Mojang no longer checks pull requests...

@RealEMK
Copy link

RealEMK commented Nov 2, 2021

I really hope Mojang gets to working on this. I've been asking for a fix for 6-7 years now.

@Patbox
Copy link

Patbox commented Nov 2, 2021

While it would be nice addition, sadly LegacyLauncher is somewhat abandoned (as it's also questionable in case of licence/ownership). I don't think it will be ever merged (but proof me wrong Mojang)

@PhoenixVX
Copy link

👍

@craftycodie
Copy link
Author

While it would be nice addition, sadly LegacyLauncher is somewhat abandoned (as it's also questionable in case of licence/ownership). I don't think it will be ever merged (but proof me wrong Mojang)

Worth a shot!

@yellowcheeseman
Copy link

I hope this gets fixed. Would like to have skins on old minecraft versions.

@peterix
Copy link

peterix commented Nov 4, 2021

@craftycodie
This looks like it could be reused even without LaunchWrapper.

Would you be OK with me using this in the MultiMC launcher startup jar?

Code is here:
https://github.com/MultiMC/Launcher/tree/develop/libraries/launcher

And licensed under Apache 2.0.

@craftycodie
Copy link
Author

@peterix it can indeed, I originally wrote this fix for my own launcher for legacy versions, it might be worth you having a look at that as it's protocol contains more fixes such as online-mode.

https://github.com/craftycodie/MineOnline/blob/main/src/main/java/gg/codie/mineonline/protocol/MineOnlineURLStreamHandler.java

Feel free!

@TorutheRedFox
Copy link

TorutheRedFox commented Apr 2, 2022

this would also require setting minecraftArguments in the JSON to ${auth_player_name} ${auth_session} --gameDir ${game_directory} --assetsDir ${game_assets} --tweakClass net.minecraft.launchwrapper.AlphaVanillaTweaker for certain versions (most notably Alpha and earlier Beta versions)

@craftycodie
Copy link
Author

this would also require setting minecraftArguments in the JSON to "${auth_player_name} ${auth_session} --gameDir ${game_directory} --assetsDir ${game_assets} --tweakClass net.minecraft.launchwrapper.AlphaVanillaTweaker" for certain versions (most notably Alpha and earlier Beta versions)

I think this is only required for Alpha 1.2.6.

@RealEMK
Copy link

RealEMK commented Apr 27, 2022

@craftycodie This looks like it could be reused even without LaunchWrapper.

Would you be OK with me using this in the MultiMC launcher startup jar?

Code is here: https://github.com/MultiMC/Launcher/tree/develop/libraries/launcher

And licensed under Apache 2.0.

Did you end up adding that to MultiMC?
Currently use PolyMC but would love for an easy fix.

@craftycodie craftycodie changed the title MCL-19983 Fixed legacy skin loading. MCL-19983 & WEB-1429 Fixed legacy skin loading & server authentication. Dec 21, 2022
@EasternBloxxer
Copy link

@Mojang/Members

Copy link

@EasternBloxxer EasternBloxxer left a comment

Choose a reason for hiding this comment

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

🍳

@MrMasrozYTLIVE
Copy link

@Mojang accept it

Copy link

@thegu5 thegu5 left a comment

Choose a reason for hiding this comment

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

!!!

Copy link

@FoxBlocks FoxBlocks left a comment

Choose a reason for hiding this comment

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

Please let it happen, Mojang.

@halotroop2288
Copy link

Top 10 reasons to hate Mojang:
4. The length of time this PR has been open and the amount of reviews it has gotten without a response from Mojang

This is the most basic form of maintenance, and the fix has been sitting, ready to go for years.

@Doodle173
Copy link

Any updates on this issue?

@powerboat9
Copy link

At this point people should just use a mod like this. Mojang is ultimately self interested and unwilling to cooperate

@MrMasrozYTLIVE
Copy link

MrMasrozYTLIVE commented Nov 30, 2023

SkinFix was implemented in PrismLauncher 8.0 with pull request PrismLauncher/PrismLauncher#443

@FoxBlocks
Copy link

SkinFix was implemented in PrismLauncher 8.0 with pull request PrismLauncher/PrismLauncher#443

Because the community does what Ninten-- er, Mojang don't.

@niceEli
Copy link

niceEli commented Jul 28, 2024

Seems like good PR

@Dwalley3DT
Copy link

Does this fix include /whitelist add username user not found uuid endpoint error or is this only for older versions authentication & skins as stated? I know it is not called out, to be fair I did not check but hoping it does (apologies). Stellar job on these fixes by the way, thank you! It actually took a while to find ANY real info, the majority of results had a series of old issue logs on jira stating it's resolved then closed but I couldn't find an actual fix anywhere near the place. Until, perhaps, by accident, I stumbled in on an umpteenth jira post on which someone made a post that brought me here, thank you for that post too.

Copy link

@maximuslotro maximuslotro left a comment

Choose a reason for hiding this comment

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

Looks good, Fixes my issues with skins when compiling and running locally!

Copy link

@amyspark-ng amyspark-ng left a comment

Choose a reason for hiding this comment

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

cool

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.