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.

Sign up for GitHub

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.sh] Parsing token inside each upstream #16289

Closed
wants to merge 30 commits into from
Closed

[update.sh] Parsing token inside each upstream #16289

Changes from 24 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c9f6f14
[update.sh] Parse local token in git remote.origin.url config
sazriel26 Dec 4, 2023
fc9b64a
Merge branch 'token-support-in-update'
sazriel26 Dec 4, 2023
9102492
Release Candidate 1
sazriel26 Dec 6, 2023
195a57b
Merge branch 'Homebrew:master' into master
sazriel26 Dec 6, 2023
d1c2d5f
Release Candidate 2
sazriel26 Dec 6, 2023
f4c25a1
Merge branch 'Homebrew:master' into token-support-in-update
sazriel26 Dec 6, 2023
234d054
Merge remote-tracking branch 'sazriel26/token-support-in-update' into…
sazriel26 Dec 6, 2023
06e79a2
Merge branch 'sazriel26-tmp' into HEAD
sazriel26 Dec 6, 2023
e7c1575
Update Library/Homebrew/cmd/update.sh
sazriel26 Dec 7, 2023
29ba761
Merge remote-tracking branch 'upstream/master'
sazriel26 Jan 7, 2024
079e8a0
New implementation through BASH regexes match
sazriel26 Jan 7, 2024
16fb088
Fix missing ;
sazriel26 Jan 7, 2024
2f3af88
Fix brew style on update.sh
sazriel26 Jan 7, 2024
ed1c0fd
Fix brew style on update.sh (2)
sazriel26 Jan 7, 2024
bced796
[Library/Homebrew/cmd/update.sh] Release Candidate for pull request
sazriel26 Jan 10, 2024
8d363a1
Merge changes for pull request
sazriel26 Jan 10, 2024
3642082
Merge remote-tracking branch 'upstream/master'
sazriel26 Jan 10, 2024
9c887df
[Library/Homebrew/cmd/update.sh] Add support for token provided w/o u…
sazriel26 Jan 11, 2024
5b3ce14
Merge remote-tracking branch 'upstream/master'
sazriel26 Jan 11, 2024
4a7018b
Rewrite of branching tests in parsing URL
sazriel26 Jan 11, 2024
f2e9b54
Brew style --fix (applied)
sazriel26 Jan 11, 2024
b21bfb1
Update Library/Homebrew/cmd/update.sh
sazriel26 Jan 11, 2024
226133c
Ready for review?
sazriel26 Jan 11, 2024
1d7191b
Update Library/Homebrew/cmd/update.sh
sazriel26 Jan 11, 2024
7b5cc6a
Merge branch 'Homebrew:master' into master
sazriel26 Jan 22, 2024
618a94f
Merge branch 'Homebrew:master' into devel
sazriel26 Jan 22, 2024
5908569
[update.sh] New test with protection of Homebrew directory
sazriel26 Jan 22, 2024
816f785
Merge remote-tracking branch 'sazriel26/devel' into HEAD
sazriel26 Jan 22, 2024
10dfd3c
Merge branch 'Homebrew:master' into master
sazriel26 Feb 7, 2024
4b8a187
Merge branch 'Homebrew:master' into master
sazriel26 Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions Library/Homebrew/cmd/update.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,15 +511,6 @@ EOS
CURL_DISABLE_CURLRC_ARGS=()
fi

# HOMEBREW_GITHUB_API_TOKEN is optionally defined in the user environment.
# shellcheck disable=SC2153
if [[ -n "${HOMEBREW_GITHUB_API_TOKEN}" ]]
then
CURL_GITHUB_API_ARGS=("--header" "Authorization: token ${HOMEBREW_GITHUB_API_TOKEN}")
else
CURL_GITHUB_API_ARGS=()
fi

# only allow one instance of brew update
lock update

Expand Down Expand Up @@ -637,13 +628,35 @@ EOS
# the refspec ensures that the default upstream branch gets updated
(
UPSTREAM_REPOSITORY_URL="$(git config remote.origin.url)"
unset UPSTREAM_REPOSITORY
unset UPSTREAM_REPOSITORY_TOKEN

# HOMEBREW_UPDATE_FORCE and HOMEBREW_UPDATE_AUTO aren't modified here so ignore subshell warning.
# shellcheck disable=SC2030
if [[ "${UPSTREAM_REPOSITORY_URL}" == "https://github.com/"* ]]
then
UPSTREAM_REPOSITORY="${UPSTREAM_REPOSITORY_URL#https://github.com/}"
UPSTREAM_REPOSITORY="${UPSTREAM_REPOSITORY%.git}"
elif [[ "${UPSTREAM_REPOSITORY_URL}" =~ https://(([^:@]+)(:([^@]+))?@)?github.com/(.*)$ ]]
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to leave a comment explaining the format we're trying to match here exactly.

Copy link
Member

@Bo98 Bo98 Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
elif [[ "${UPSTREAM_REPOSITORY_URL}" =~ https://(([^:@]+)(:([^@]+))?@)?github.com/(.*)$ ]]
elif [[ "${UPSTREAM_REPOSITORY_URL}" =~ https://(([^:@/?#]+)(:([^@/?#]+))?@)github\.com/(.*)$ ]]
  • Group 1 ? seemed unnecessary as that just reduces it down to the earlier if condition.
  • / and similar URL component separators should not be matched in the username password as if they appear then the github.com part moves from matching the host component to matching the path component of the URL, which we don't want. Probably could be stricter but this is good enough
  • Escape . so we're only actually matching GitHub

Copy link
Member

Choose a reason for hiding this comment

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

  • Group 1 ? seemed unnecessary as that just reduces it down to the earlier if condition.

Is Group 1 itself even necessary? ${BASH_REMATCH[1]} isn't used so perhaps remove group and shift numbers down.

Copy link
Member

@Bo98 Bo98 Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed it seems we can remove that grouping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you all for your feedbacks and suggestions!

Indeed, based on https://en.wikipedia.org/wiki/URL#Syntax, what about to rewrite the parsing in the elif to suit to:
https://[userinfo]@github.com/[path]
(${BASH_REMATCH[1]} will be userinfo part and ${BASH_REMATH[2]} will be the path)

and then to look inside how it is formatted (user[:password]) and apply onto some protections like avoiding any characters that are not expected (would add $ in the list to avoid also shell expansion at evaluation / runtime...)

From my experiences, the GIT API Key looks to match ^[[:alnum:]_]+$, maybe could be another interesting point to simplify the parsing above?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
elif [[ "${UPSTREAM_REPOSITORY_URL}" =~ https://(([^:@]+)(:([^@]+))?@)?github.com/(.*)$ ]]
elif [[ "${DIR}" != "${HOMEBREW_REPOSITORY}" ]] && [[ "${UPSTREAM_REPOSITORY_URL}" =~ https://(([^:@]+)(:([^@]+))?@)?github.com/(.*)$ ]]

I think this is what we want to do here based on discussion with @MikeMcQuaid.

then
UPSTREAM_REPOSITORY="${BASH_REMATCH[5]%.git}"
UPSTREAM_REPOSITORY_TOKEN="${BASH_REMATCH[4]:-${BASH_REMATCH[2]}}"
Copy link
Member

Choose a reason for hiding this comment

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

Comments explaining which parts of the regex we're looking for here would be good too.

fi

if [[ -n "${UPSTREAM_REPOSITORY}" ]]
then
# UPSTREAM_REPOSITORY_TOKEN is parsed (if exists) from UPSTREAM_REPOSITORY_URL
# HOMEBREW_GITHUB_API_TOKEN is optionally defined in the user environment.
# shellcheck disable=SC2153
if [[ -n "${UPSTREAM_REPOSITORY_TOKEN}" ]]
then
CURL_GITHUB_API_ARGS=("--header" "Authorization: token ${UPSTREAM_REPOSITORY_TOKEN}")
elif [[ -n "${HOMEBREW_GITHUB_API_TOKEN}" ]]
then
CURL_GITHUB_API_ARGS=("--header" "Authorization: token ${HOMEBREW_GITHUB_API_TOKEN}")
else
CURL_GITHUB_API_ARGS=()
fi

if [[ "${DIR}" == "${HOMEBREW_REPOSITORY}" && -n "${HOMEBREW_UPDATE_TO_TAG}" ]]
then
Expand Down
Loading