Skip to content

SOLR-18102: Fix Admin UI serving issue with basic auth#4110

Merged
gerlowskija merged 3 commits intoapache:mainfrom
gerlowskija:fix-admin-ui-serving
Feb 6, 2026
Merged

SOLR-18102: Fix Admin UI serving issue with basic auth#4110
gerlowskija merged 3 commits intoapache:mainfrom
gerlowskija:fix-admin-ui-serving

Conversation

@gerlowskija
Copy link
Contributor

https://issues.apache.org/jira/browse/SOLR-18102

Description

A recent CVE fix added in some path normalization to HttpSolrCall, but neglected to update some special-casing we have when serving the static Admin UI files. This resulted in users visiting the Admin UI with auth enabled to get 401s, without a chance to authenticate themselves in the browser.

Solution

This commit updates the special-casing in HttpSolrCall to be more flexible and handle the post-normalization Admin UI paths.

(This PR fixes an unreleased issue, so I'm omitting a changelog entry.)

Tests

Unfortunately we don't have any Selenium or other tests for our Admin UI, so I've mostly had to test this manually for now.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added a changelog entry for my change

Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

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

Looks like what I posted, so I am plus one!!

@epugh
Copy link
Contributor

epugh commented Feb 6, 2026

I believe o saw it on main branch as well....

@gerlowskija
Copy link
Contributor Author

Tests and check/precommit pass locally. The "changelog" task fails here, but I'm skipping changelog intentionally in this PR since it's fixing an unreleased bug (see comment above).

@gerlowskija
Copy link
Contributor Author

Thanks Eric - I'll give other folks a bit of time to review and then merge before EOD.

@janhoy
Copy link
Contributor

janhoy commented Feb 6, 2026

+1

If you like you can cherry-pick in the BATS test from #4107 which should be green with this patch

@epugh
Copy link
Contributor

epugh commented Feb 6, 2026

I added the no-changelog label ;-)

janhoy and others added 2 commits February 6, 2026 13:47
Reverted whitespace-only edits (trailing space removal) while
preserving the new test case for admin UI basic auth exception.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@rahulgoswami
Copy link
Member

rahulgoswami commented Feb 6, 2026

Thanks @gerlowskija and @epugh for the patch. Interestingly the admin UI login page renders fine in standalone mode with basic auth enabled on RC3 (without any patch I mean). Which is what threw me off initially on the reproducibility. Looking at the patch I still can't figure out why? Any idea?

@gerlowskija
Copy link
Contributor Author

Hmm...HttpSolrCall (where this path gets normalized) has a few codepaths that are dependent on cores.isZooKeeperAware() and are involved in path-setting. So I suspect the standalone code-paths just happen to avoid it? Or maybe there are URLs that would trigger the bug for standalone that we just didn't notice.

Now, the deeper question of why HttpSolrCall should care whether we're in SolrCloud or not, I don't have a great answer for. I know Gus is setting out to disentangle a lot of this code, so maybe he has some context that's relevant here?

@gerlowskija
Copy link
Contributor Author

gerlowskija commented Feb 6, 2026

Thanks for the BATS test @janhoy !

I agree with @dsmiley 's sentiment about the growing runtime of our BATS suite. That said - I don't have time to convert it to a JUnit test right now, and I'd really like to get this change merged in time for the weekend test runs to flush out any issues. So I'm going to merge as-is for now.

I apologize for "laziness" of sorts there, but I think it's worth a quicker merge to get some additional testing on this before Anshum (presumably) cuts another RC on Monday. @dsmiley - if you have time to switch this to JUnit, please go ahead!

@gerlowskija gerlowskija merged commit cd4410d into apache:main Feb 6, 2026
7 checks passed
@gerlowskija gerlowskija deleted the fix-admin-ui-serving branch February 6, 2026 21:01
gerlowskija added a commit that referenced this pull request Feb 9, 2026
Co-authored-by: Jan Høydahl <janhoy@apache.org>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
gerlowskija added a commit that referenced this pull request Feb 9, 2026
Co-authored-by: Jan Høydahl <janhoy@apache.org>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants