[SCM-1028] Fix clear password logging vulnerability #237

mhoffrog · 2025-03-31T20:24:24Z

JIRA

SCM-1028 Vulnerability: Clear text password is logged by JGit provider and by gitexe remoteinfo on a ls-remote failure

Changes

GitUtil.java:
- add method maskPasswordInUrl(String urlWithCredentials)
  - implementation taken from AnonymousCommandLine.java
  - improve regex pattern to be more precise
  - replace wrapped with delimiters ':' and '@' to avoid replacing the password within probable other places of the URL to avoid password guessing by using e.g. redundant URL parameters
AnonymousCommandLine.java:
- move current password masking implementation to GitUtil
- use implementation from GitUtil
GitScmProviderRepository.java:
- add method getFetchUrlWithMaskedPassword()
- add method getPushUrlWithMaskedPassword()
- toString():
  - 👉 BREAKING change: provide URL content with masked password to reduce risk of usage within logs or exceptions with showing passwords by that
JGitUtils.java:
- method prepareSession(Git git, GitScmProviderRepository repository):
  - log using methods:
    - GitScmProviderRepository.getFetchUrlWithMaskedPassword()
    - GitScmProviderRepository.getPushUrlWithMaskedPassword()
GitRemoteInfoCommand.java:
- use GitScmProviderRepository.getFetchUrlWithMaskedPassword() for exception message
Update JUnit tests accordingly:
- GitScmProviderRepositoryTest.java
- GitCommandLineUtilsTest.java

Test result

All JUnit tests passed

mhoffrog · 2025-04-01T08:07:30Z

Fixed issue with @see in JavaDoc of GitUtil.java by recent force push.

...ons/src/main/java/org/apache/maven/scm/provider/git/repository/GitScmProviderRepository.java

...n-scm-provider-git-commons/src/main/java/org/apache/maven/scm/provider/git/util/GitUtil.java

mhoffrog · 2025-04-01T21:04:39Z

fixed spotless violations

- GitUtil.java: - add method maskPasswordInUrl(String urlWithCredentials) - implementation taken from AnonymousCommandLine.java - improve regex pattern to be more precise - replace wrapped with delimiters ':' and '@' to avoid replacing the password within probable other places of the URL to avoid password guessing by using e.g. redundant URL parameters - AnonymousCommandLine.java: - move current password masking implementation to GitUtil - use implementation from GitUtil - GitScmProviderRepository.java: - add method getFetchUrlWithMaskedPassword() - add method getPushUrlWithMaskedPassword() - toString(): - BREAKING change: provide URL content with masked password to reduce risk of usage within logs or exceptions with showing passwords by that - JGitUtils.java: - method prepareSession(Git git, GitScmProviderRepository repository): - log using methods: - GitScmProviderRepository.getFetchUrlWithMaskedPassword() - GitScmProviderRepository.getPushUrlWithMaskedPassword() - GitRemoteInfoCommand.java: - use GitScmProviderRepository.getFetchUrlWithMaskedPassword() for exception message - Update JUnit tests accordingly: - GitScmProviderRepositoryTest.java - GitCommandLineUtilsTest.java

mhoffrog · 2025-04-02T20:35:39Z

@michael-o Update according to resolved conversations above.

...n-scm-provider-git-commons/src/main/java/org/apache/maven/scm/provider/git/util/GitUtil.java

mhoffrog

@michael-o Many thanks for your review! Please find my answers accordingly and let me know if / what should be changed or improved.

...n-scm-provider-git-commons/src/main/java/org/apache/maven/scm/provider/git/util/GitUtil.java

mhoffrog · 2025-04-07T09:10:31Z

...exe/src/main/java/org/apache/maven/scm/provider/git/gitexe/command/AnonymousCommandLine.java

    public String toString() {
-        String output = super.toString();
-        final Matcher passwordMatcher = passwordPattern.matcher(output);
-        if (passwordMatcher.find()) {


@michael-o This is the place where the find() did have been taken from - did not want to touch this.

mhoffrog · 2025-04-26T22:15:36Z

@michael-o Please let me know if there anything missing or is to be improved from my end.

slawekjaranowski · 2025-05-12T09:19:26Z

@michael-o - Please take account that PR with status 'closed' will be not reported by release drafter.

So we will not have it in release notes.

slawekjaranowski · 2025-05-12T09:20:09Z

please look ar draft release notes - https://github.com/apache/maven-scm/releases

mhoffrog · 2025-05-12T09:28:13Z

@slawekjaranowski @michael-o

Is there a reason why asf-gitbox-commits is closing without merging the PR and creating a dedicated commit to master with the changes? Looks pretty irritating.
Same question for release notes - I'd like to get it appearing in the release notes - just because it is a vulnerability fix.

olly-cb · 2025-05-16T06:22:35Z

@slawekjaranowski @michael-o @hboutemy shouldn't we ask INFRA to mark the PR merged instead of closed.
And so everybody is happy? :)

maybe instead

This closes https://github.com/apache/maven-scm/pull/237

this should be

This merge https://github.com/apache/maven-scm/pull/237

not sure if supported by INFRA tooling though

olamy · 2025-05-16T06:51:46Z

but yeah nice release notes generated automatically is important too :P

mhoffrog · 2025-05-16T08:53:03Z

@slawekjaranowski @michael-o @hboutemy shouldn't we ask INFRA to mark the PR merged instead of closed. And so everybody is happy? :)
...
not sure if supported by INFRA tooling though

@olly-cb
That would be the perfect solution!

If this is not possible or too complicated I could file another PR with the same JIRA reference and description providing a simple change - additionally addressing one of the review comments e.g. fix that "asterisk" phrasing or the regex find() vs. matches().
@michael-o - please let me know what you prefer - I am open to support in any way.

hboutemy · 2025-05-17T10:12:54Z

I'm trying to understand the issue and where we should look for solution
no idea if the change is at ASF level or at GH level
but in the past, it seems that the way @michael-o was merging code was perfectly identified as PR merge by GH: see #151

any idea about what is different between the current PR and #151?

michael-o · 2025-05-17T10:14:45Z

I'm trying to understand the issue and where we should look for solution no idea if the change is at ASF level or at GH level but in the past, it seems that the way @michael-o was merging code was perfectly identified as PR merge by GH: see #151

any idea about what is different between the current PR and #151?

For the past 5+ years I have been rebasing, amending with a ref to the actual PR upstreaming to master. Worked like a charm.

hboutemy · 2025-05-17T10:20:33Z

writing down the factual differences:

in 2022 for [SCM-991] GitDiffConsumer cannot parse moved files #151, there were 2 messages with the expected successful GH PR conclusion:
in 2025 in this PR, only one message with the problematic GH PR conclusion:

hboutemy · 2025-05-17T10:31:13Z

any idea from anybody on what magic was adding the "asfgit merged commit" step?
= what seems currently broken

hboutemy · 2025-05-17T10:34:07Z

searching for pointers, I see Jena project seems to have clearly documented the PR merge process via GH or via GitBas = https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=75968211#CommitWorkflowforGithubASF-Process(viagitbox.apache.org)

michael-o · 2025-05-17T10:44:45Z

searching for pointers, I see Jena project seems to have clearly documented the PR merge process via GH or via GitBas = https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=75968211#CommitWorkflowforGithubASF-Process(viagitbox.apache.org)

This is basically what I have been doing: "This closes #NNN".

pzygielo · 2025-05-17T10:56:16Z

any idea from anybody on what magic was adding the "asfgit merged commit" step? = what seems currently broken

I think it might be the matter of pushing commit KNOWN to GitHub in context of PR (like in referenced [SCM-991] GitDiffConsumer cannot parse moved files #151 - the same commit present in PR was pushed to the target branch) or UNKNOWN (like here - 8b44e4f was brand new commit (re-committed content) pushed to target branch with closing keyword - so the PR was closed as commanded, but not recognized as being merged; the commit comment just closed the PR for some, perhaps unreladed, reason, but it was not merged.)

This is basically what I have been doing: "This closes #NNN".

And such comment "closes" issues/PRs as requested. If merged "correctly" - there would be no reason to add such comment.

With merge being fast-forwarded or squashed or done with merge commit, in GitHub or externally to GitHub - there are many possibilities to be considered.

michael-o · 2025-05-17T13:27:46Z

any idea from anybody on what magic was adding the "asfgit merged commit" step? = what seems currently broken
* I think it might be the matter of pushing commit KNOWN to GitHub in context of PR (like in referenced [[SCM-991] GitDiffConsumer cannot parse moved files #151](https://github.com/apache/maven-scm/pull/151) - the same commit present in PR was pushed to the target branch) or UNKNOWN (like here - [8b44e4f](https://github.com/apache/maven-scm/commit/8b44e4f6745e7bd677428be093921267317b8fe8) was brand new commit (re-committed content) pushed to target branch with closing keyword - so the PR was closed as commanded, but not recognized as being merged; the commit comment just closed the PR for some, perhaps unreladed, reason, but it was not _merged_.)
This is basically what I have been doing: "This closes #NNN".

And such comment "closes" issues/PRs as requested. If merged "correctly" - there would be no reason to add such comment.

With merge being fast-forwarded or squashed or done with merge commit, in GitHub or externally to GitHub - there are many possibilities to be considered.

Technically not necessary, but for reference helpful.

hboutemy · 2025-05-17T19:02:00Z

@michael-o it seems @pzygielo 's explanation is right, and it was the case 4 years ago: see #120
PR's commit https://github.com/apache/maven-scm/pull/120/commits is different form master commit 3a10d0f

so it does not seem GitBox/GH behaviour changed, BUT we now do care about PR's not seen as merged by GH but only closed...

I don't know if it is a limitation known to other projects: for example, Jena does not seem to have seen it

and to work on a solution: I don't know by which magic GH PR merge works when it reworks the commit(s) = what GitBox should mimic I suppose

mhoffrog · 2025-05-22T15:42:35Z

@hboutemy I could reset my PRs branch to commit 8b44e4f of the current master and force push my branch. Maybe GitHub would recognize it on this PR and mark the PR as merged.

Please let me know if I should give it a try? (I guess it cannot get worse as it currently is.)

Edited 23-May-2025:
@michael-o Looking to #238 you did exactly that - force pushing the commit on master to the PR branch - and this did change the state of the PR from closed to merged as far as I can see.
I guess that is what is missing on this PR here.
Can you do the same here? - you have the appropriate rights on my branch since I did check to allow edits for maintainers on it.

michael-o · 2025-05-23T11:41:26Z

@hboutemy I could reset my PRs branch to commit 8b44e4f of the current master and force push my branch. Maybe GitHub would recognize it on this PR and mark the PR as merged.

Please let me know if I should give it a try? (I guess it cannot get worse as it currently is.)

Edited 23-May-2025: @michael-o Looking to #238 you did exactly that - force pushing the commit on master to the PR branch - and this did change the state of the PR from closed to merged as far as I can see. I guess that is what is missing on this PR here. Can you do the same here? - you have the appropriate rights on my branch since I did check to allow edits for maintainers on it.

Try yourself and we will see.

mhoffrog · 2025-05-23T13:35:06Z

@michael-o

Try yourself and we will see.

Done - but it does not work this way.
There is an instruction at How to reopen a pull-request after a force-push but I guess that will not work for me, since it requires rights to re-open a PR on this repo.
Could you or one of the repo maintainers give it a try?

michael-o · 2025-05-23T13:44:47Z

@michael-o

Try yourself and we will see.

Done - but it does not work this way. There is an instruction at How to reopen a pull-request after a force-push but I guess that will not work for me, since it requires rights to re-open a PR on this repo. Could you or one of the repo maintainers give it a try?

I can't either. It has been terminally closed.

mhoffrog · 2025-05-25T13:35:40Z

Did file PR #244 hoping that GH would close it as merged after force pushing it to commit 8b44e4f - but that did not work either.

@michael-o It looks that the remaining option to get it listed by the release notes is to manually edit release notes for the next release.

hboutemy · 2025-05-25T15:13:29Z

yes, we'll have to update release notes by hand for the current PR: that's life

it's for future cases like that that we need to find a good way to avoid that manual overhead

mhoffrog · 2025-05-25T17:06:49Z

Lessons learned - rules for PR merging:

If one creates a commit hash deviating from the final commit of a PR before merging, then force push this commit hash to the PR branch first.
Make sure in any case to merge the PR branches final commit hash to the target branch to let GitHub properly recognize the PR as merged to the target branch.

jira-importer · 2025-06-12T01:13:16Z

Resolve #1254

mhoffrog force-pushed the SCM-1028 branch from 4ca5df3 to 5d23f87 Compare April 1, 2025 08:05

mhoffrog force-pushed the SCM-1028 branch from 5d23f87 to 6267b6a Compare April 1, 2025 08:15

michael-o requested changes Apr 1, 2025

View reviewed changes

...ons/src/main/java/org/apache/maven/scm/provider/git/repository/GitScmProviderRepository.java Outdated Show resolved Hide resolved

...n-scm-provider-git-commons/src/main/java/org/apache/maven/scm/provider/git/util/GitUtil.java Outdated Show resolved Hide resolved

mhoffrog force-pushed the SCM-1028 branch from 6267b6a to 2c9cf69 Compare April 1, 2025 21:03

mhoffrog force-pushed the SCM-1028 branch from 2c9cf69 to f60072c Compare April 2, 2025 20:29

mhoffrog requested a review from michael-o April 2, 2025 20:35

michael-o requested changes Apr 6, 2025

View reviewed changes

...n-scm-provider-git-commons/src/main/java/org/apache/maven/scm/provider/git/util/GitUtil.java Show resolved Hide resolved

...n-scm-provider-git-commons/src/main/java/org/apache/maven/scm/provider/git/util/GitUtil.java Show resolved Hide resolved

mhoffrog commented Apr 7, 2025

View reviewed changes

mhoffrog requested a review from michael-o April 7, 2025 09:26

michael-o approved these changes May 12, 2025

View reviewed changes

asf-gitbox-commits closed this in 8b44e4f May 12, 2025

mhoffrog mentioned this pull request May 25, 2025

[SCM-1028] Fix clear password logging vulnerability #244

Closed

jira-importer mentioned this pull request May 25, 2025

[SCM-1028] Vulnerability: Clear text password is logged by JGit provider and by gitexe remoteinfo on a ls-remote failure #1254

Closed

mhoffrog mentioned this pull request Jan 7, 2026

[SCM-1028] is missing in release notes of release 2.2.1 #1353

Open

[SCM-1028] Fix clear password logging vulnerability #237

[SCM-1028] Fix clear password logging vulnerability #237

Uh oh!

Conversation

mhoffrog commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA

Changes

Test result

Uh oh!

mhoffrog commented Apr 1, 2025

Uh oh!

Uh oh!

Uh oh!

mhoffrog commented Apr 1, 2025

Uh oh!

mhoffrog commented Apr 2, 2025

Uh oh!

Uh oh!

Uh oh!

mhoffrog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhoffrog Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

mhoffrog commented Apr 26, 2025

Uh oh!

slawekjaranowski commented May 12, 2025

Uh oh!

slawekjaranowski commented May 12, 2025

Uh oh!

mhoffrog commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olly-cb commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olamy commented May 16, 2025

Uh oh!

mhoffrog commented May 16, 2025

Uh oh!

hboutemy commented May 17, 2025

Uh oh!

michael-o commented May 17, 2025

Uh oh!

hboutemy commented May 17, 2025

Uh oh!

hboutemy commented May 17, 2025

Uh oh!

hboutemy commented May 17, 2025

Uh oh!

michael-o commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pzygielo commented May 17, 2025

Uh oh!

michael-o commented May 17, 2025

Uh oh!

hboutemy commented May 17, 2025

Uh oh!

mhoffrog commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-o commented May 23, 2025

Uh oh!

mhoffrog commented May 23, 2025

Uh oh!

michael-o commented May 23, 2025

Uh oh!

mhoffrog commented May 25, 2025

Uh oh!

hboutemy commented May 25, 2025

Uh oh!

mhoffrog commented May 25, 2025

Uh oh!

jira-importer commented Jun 12, 2025

Uh oh!

mhoffrog commented Mar 31, 2025 •

edited

Loading

mhoffrog commented May 12, 2025 •

edited

Loading

olly-cb commented May 16, 2025 •

edited

Loading

michael-o commented May 17, 2025 •

edited

Loading

mhoffrog commented May 22, 2025 •

edited

Loading