Misconfigured SAXParser in AmazonS3 breaks fetching of advertised loose refs

ericsteele commented 2 weeks ago

Version

6.0.0+

Operating System

Linux/Unix, MacOS

Bug description

Hi folks! (esp. @msohn, @tomaswolf)

I have a bug to report, and also the solution/fix, which is only a few lines of code!

A few years back I helped author a change in the AmazonS3 class to add support for AWS API signature version 4.

Bug: 579907
Change-Id: If289dbc6d0f57323cfeaac2624c4eb5028f78d13

That change landed in 5.13.1.202206130422-r and I have been using that until recently when I tried upgrading to the latest version 6.8.0.202311291450-r. After upgrading I found that fetch and pull commands were no longer working due to this error:

org.eclipse.jgit.api.errors.RefNotAdvertisedException: Remote origin did not advertise Ref for branch refs/heads/test_branch. This Ref may not exist in the remote or may be hidden by permission settings.
    at org.eclipse.jgit.api.PullCommand.call(PullCommand.java:293)
    ...

I was able to replicate this issue as far back as version 6.0.0. I debugged the code and it appears that this change is the cause:

https://github.com/eclipse-jgit/jgit/commit/1aa19c67e691d62f3d9560fec0cfe7f5443d6497#diff-d82944100478207e2338215ed095ce61a63cc9ae0ba10f5a363b9810fb7a803f

When the AmazonS3Transport::readLooseRefs() method executes, it calls AmazonS3:list() method, which calls the ListParser::list() method. In JGit version 5.13, the ListParser::list() method uses an XmlReader created by the XMLReaderFactory. But in JGit version 6.0.0+, the ListParser::list() method uses an XmlReader created by SAXParserFactory.

If you step through the XML parsing logic as it executes and look at the inputs provided to the ListParser::startElement() and ListParser::endElement() methods, you will see why it no longer works. When XMLReaderFactory is used, all of the input parameters are set to valid/expected values. When SAXParserFactory is used, the only input parameter that gets set is the qName parameter. All other parameters are set to null.

I did some googling, and found this StackOverflow thread in which another user saw the same issue when using SAXParserFactory:

https://stackoverflow.com/q/24891323

The solution for that user was to call SaxParserFactory.setNamespaceAware(true) before calling SaxParserFactory.newSAXParser(). I built JGit with this change and tested it out on my local machine and it worked!

final SAXParserFactory saxParserFactory = SAXParserFactory.newInstance();
saxParserFactory.setNamespaceAware(true);
xr = saxParserFactory.newSAXParser().getXMLReader();

@msohn Would it be possible for you to submit a fix for this? I can do so if needed, but it has been 2 years since I last contributed and am having trouble logging into things and recreating my setup from back then. It is a very small change, and is blocking something that I am working on, so I'm hoping you can help squash this bug for me. Thank you! 🙏

Actual behavior

When trying to fetch or pull changes from a repo hosted in AWS S3, I am seeing:

org.eclipse.jgit.api.errors.RefNotAdvertisedException: Remote origin did not advertise Ref for branch refs/heads/test_branch. This Ref may not exist in the remote or may be hidden by permission settings.
    at org.eclipse.jgit.api.PullCommand.call(PullCommand.java:293)
    ...

This affects jgit versions 6.0.0+

Expected behavior

Expected behavior is that fetch and pull operations from AWS S3 should work, as they did in version 5.13.

Relevant log output

No response

Other information

No response

msohn commented 2 weeks ago

Please review and test https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/+/1199781

See the contributor guide how to login to GerritHub.

ericsteele commented 2 weeks ago

Looks good to me. Tested on my local and it works. Thanks!

tomaswolf commented 2 weeks ago

We also had https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/+/197494/3/org.eclipse.jgit/src/org/eclipse/jgit/transport/AmazonS3.java which somehow fell through the cracks.

Either of these is fine.

msohn commented 2 weeks ago

Could I get a positive vote on the change ? I need to prepare 7.0 RC1 today and would like to include this fix.

msohn commented 2 weeks ago

https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/+/1199781 was merged to stable-6.10 branch and will be merged up to master

ericsteele commented 2 weeks ago

Thank you everyone! Really appreciate the fast turnaround on this.

eclipse-jgit / jgit