Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

enhancement: add additional command hints for PowerShell & CMD #33548

Merged
merged 6 commits into from
Feb 10, 2025

Conversation

jason19970210
Copy link
Contributor

  • resolving wrong signature calculations for SSH key verification

Fixed #22693

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 10, 2025
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 10, 2025
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Feb 10, 2025
@jason19970210
Copy link
Contributor Author

jason19970210 commented Feb 10, 2025

As the command hint by SSH key verification shows echo -n "token" | ... which including trailing new line symbol by PowerShell and makes the wrong signature for the verification process.

That means only Linux and macOS can work properly with the origin command hint.
This PR shows what the command hints that Windows user could be easily calculated the correct signature value as Linux does.

ref: PowerShell/PowerShell#5974 (comment)

And I also make the command hints collapse to minimize the section.

Output

Screenshot 2025-02-11 at 12 54 48 AM Screenshot 2025-02-11 at 12 55 09 AM

remove additional space before '=' for lint checking
@wxiaoguang
Copy link
Contributor

I think we could remove the "Linux & macOS" hint because it works for all POSIX standard shells.

And I think we can simplify the template by removing the printf, will make some changes here, just a moment.

@wxiaoguang
Copy link
Contributor

And one more question: are these "quotes" necessary?

image

@lunny
Copy link
Member

lunny commented Feb 10, 2025

Should we detect user's OS from the web browser and display the right command?

@wxiaoguang
Copy link
Contributor

Should we detect user's OS from the web browser and display the right command?

I prefer to keep it simple, do not introduce too much JS code

@jason19970210
Copy link
Contributor Author

And one more question: are these "quotes" necessary?

image

As the output I just tried on my PowerShell, the quotes are necessary.

@wxiaoguang
Copy link
Contributor

As the output I just tried on my PowerShell, the quotes are necessary.

But the real output is a string which only contains letters&numbers, there is no < or >. Then is the `" still necessary?

@lunny lunny added the type/docs This PR mainly updates/creates documentation label Feb 10, 2025
@lunny lunny added this to the 1.24.0 milestone Feb 10, 2025
@jason19970210
Copy link
Contributor Author

jason19970210 commented Feb 10, 2025

As the output I just tried on my PowerShell, the quotes are necessary.

But the real output is a string which only contains letters&numbers, there is no < or >. Then is the `" still necessary?

So here is my output

## Correct
PS > cmd /c "<NUL set /p=`"b6320474e4ee14cbedf20856d5371904f23bc8beea20c5358d1cee7379cf3d66`"| ssh-keygen -Y sign -n gitea -f id_ed25519"
Signing data on standard input
-----BEGIN SSH SIGNATURE-----
U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgmjx3mDaLWkLlHptkELlfNhgA8J
FcHFn3m+ODd68ufggAAAAFZ2l0ZWEAAAAAAAAABnNoYTUxMgAAAFMAAAALc3NoLWVkMjU1
MTkAAABA0AR+1fJyD9G+at8fSEdQJsx+f7Od0FmvFcr3P53V9bTDKwIJAMs0SR3Q86CpHT
ELsuRdTlR3S62Bb9J9JDbyAg==
-----END SSH SIGNATURE-----

## Wrong
PS > cmd /c "<NUL set /p=b6320474e4ee14cbedf20856d5371904f23bc8beea20c5358d1cee7379cf3d66| ssh-keygen -Y sign -n gitea -f id_ed25519"
Signing data on standard input
-----BEGIN SSH SIGNATURE-----
U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgmjx3mDaLWkLlHptkELlfNhgA8J
FcHFn3m+ODd68ufggAAAAFZ2l0ZWEAAAAAAAAABnNoYTUxMgAAAFMAAAALc3NoLWVkMjU1
MTkAAABAVrx4k+c7wELor/jRErKKABPaOjiTJoCJ0iQgZeyRhxaLRF0LxdwB6EeEe2G2lh
BPBwnr6DiLeqt89Tg7nPgoDg==
-----END SSH SIGNATURE-----

And I also tried with /p=`xxxx`, the output isn't the correct one

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Interesting .... Windows command line has too many quirks ....

Now I think now the generated commands should be the same as the initial ones by your first commit.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 10, 2025
@jason19970210
Copy link
Contributor Author

Interesting .... Windows command line has too many quirks ....

Now I think now the generated commands should be the same as the initial ones by your first commit.

kind of magic, both of PowerShell & CMD

Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

Trust LGTM

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 10, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 10, 2025
@wxiaoguang wxiaoguang merged commit e3adb68 into go-gitea:main Feb 10, 2025
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 10, 2025
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 11, 2025
* giteaofficial/main:
  Enhance routers for the Actions runner operations (go-gitea#33549)
  [skip ci] Updated translations via Crowdin
  Run yamllint with strict mode, fix issue (go-gitea#33551)
  Enhance routers for the Actions variable operations (go-gitea#33547)
  enhancement: add additional command hints for PowerShell & CMD (go-gitea#33548)
  Feature: Support workflow event dispatch via API (go-gitea#33545)
  Optimize the dashboard (go-gitea#32990)
  Rework suggestion backend (go-gitea#33538)
  Revert "Feature: Support workflow event dispatch via API (go-gitea#32059)" (go-gitea#33541)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/templates This PR modifies the template files size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/docs This PR mainly updates/creates documentation
Projects
None yet
4 participants