Arksine / moonraker

Web API Server for Klipper
https://moonraker.readthedocs.io
GNU General Public License v3.0
1.05k stars 406 forks source link

Hard reset does not respect `primary_branch` from moonraker.conf. #801

Closed miklschmidt closed 8 months ago

miklschmidt commented 8 months ago

What happened

Running a hard reset on an extension managed by the update manager re-clones the repository with the default branch, not the one configured as primary_branch.

Client

Other

Browser

Other or N/A

How to reproduce

1) configure an extension with a primary_branch different from the default branch. 2) modify a local file in the extension repo 3) refresh update manager 4) repo is now dirty 5) select "hard reset" 6) repo is cloned with the default branch and moonraker reports that the reinstallation was succesfull 7) extension is now broken as it's running the wrong branch

Additional information

The repositories i've tested are configured in an included file, not sure if that makes any difference for the bug.

3dprintpt commented 8 months ago

Had this, can replicate and test if needed. Mikkel already gave everything I need to restore ;)

Arksine commented 8 months ago

It was originally intended for the primary branch and default branch to be one and the same, which is why Moonraker simply performs a straight clone. That said, there probably isn't any harm in adding the branch option to git clone. It provides a roundabout way of switching branches via the API which I don't love, but I can likely prevent that too.

miklschmidt commented 8 months ago

It was originally intended for the primary branch and default branch to be one and the same,

Then i'm not sure i understand why primary_branch is a config option at all? git remote show origin | grep "HEAD branch:" there's your default branch?

It's very helpful having a primary_branch when maintaining several versions of a project. Crowsnest has recently done this (between v3 and v4), and i do it every time i release a new major version of RatOS. I also have beta and development builds that run their own branches, it leads to all sorts of problems though if an older version is still maintained but the default branch is switched to the most recent stable version and people do a hard reset.

It provides a roundabout way of switching branches via the API which I don't love

Yeah, i see what you mean 🤔 wouldn't you have to modify the repository files for that to work, or am i missing something?

Arksine commented 8 months ago

Then i'm not sure i understand why primary_branch is a config option at all? git remote show origin | grep "HEAD branch:" there's your default branch?

Because I wasn't aware I could find the default branch that way 😄. When the Update Manager was in its infancy there was no primary_branch option, it just checked against the master branch. We soon realized that there were plenty of repos defaulting to main, so the option was added to provide a quick fix. The option is really just intended to be a sanity check.

That said, I agree that being able to specify any branch as the primary is beneficial, so I'm willing to support that.

Yeah, i see what you mean 🤔 wouldn't you have to modify the repository files for that to work, or am i missing something?

You will be able to change the primary branch in moonraker.conf and perform a hard recovery. I can restrict the recovery branch to the last detected match from the repo, but I'm not sure its necessary. The origin is strict, it must the be origin detected on the disk. I don't think we are introducing a vulnerability by allowing a reclone to a different branch, although it would better for the user to just log in and switch the branch using the cli. TBH, I believe its already possible to switch branches using the soft recovery mechanism.

Arksine commented 8 months ago

FWIW, commit 3f7cae09bb12dc332ba779e5aaae84f69c58de7d should resolve this. I'm not doing any checks on the branch, it will switch to whatever branch is supplied in the config. If necessary I can add them at a later date.

miklschmidt commented 8 months ago

Because I wasn't aware I could find the default branch that way 😄

Fair enough! :D

I can restrict the recovery branch to the last detected match from the repo, but I'm not sure its necessary. The origin is strict, it must the be origin detected on the disk. I don't think we are introducing a vulnerability by allowing a reclone to a different branch, although it would better for the user to just log in and switch the branch using the cli. TBH, I believe its already possible to switch branches using the soft recovery mechanism.

That's actually more a pro than a con from my perspective, i understand that doing it via CLI feels more intentional though. I agree as long as the origin is strict, it's OK from a security persepective :+1:

FWIW, commit https://github.com/Arksine/moonraker/commit/3f7cae09bb12dc332ba779e5aaae84f69c58de7d should resolve this. I'm not doing any checks on the branch, it will switch to whatever branch is supplied in the config. If necessary I can add them at a later date.

You're the best! I'll let you know if there's a need for branch checking, thank you! :)