Skip to content

Fix NPE in StaticTokenAuthProvider and UrlAuthProvider when required config property is absent#18456

Merged
yashmayya merged 2 commits into
apache:masterfrom
Akanksha-kedia:fix-auth-provider-null-checks
May 12, 2026
Merged

Fix NPE in StaticTokenAuthProvider and UrlAuthProvider when required config property is absent#18456
yashmayya merged 2 commits into
apache:masterfrom
Akanksha-kedia:fix-auth-provider-null-checks

Conversation

@Akanksha-kedia
Copy link
Copy Markdown
Contributor

Summary

Both StaticTokenAuthProvider(AuthConfig) and UrlAuthProvider(AuthConfig) call .toString() directly on authConfig.getProperties().get(...) without a null check. When the required property (token or url) is absent from the config the result is an opaque NullPointerException with no indication of which property is missing.

This PR replaces the implicit null deref with an explicit check that throws IllegalArgumentException naming the missing property, giving operators an actionable error message.

// Before
String userToken = authConfig.getProperties().get(TOKEN).toString();  // NPE if TOKEN absent

// After
Object tokenValue = authConfig.getProperties().get(TOKEN);
if (tokenValue == null) {
    throw new IllegalArgumentException("Missing required auth config property: " + TOKEN);
}
String userToken = tokenValue.toString();

The same pattern is applied to UrlAuthProvider.

Test plan

  • Existing auth provider unit tests pass
  • Constructing either provider with a config missing the required property throws IllegalArgumentException with a clear message instead of NullPointerException

…config property is missing

Both constructors that accept an AuthConfig called .toString() directly on the result of
authConfig.getProperties().get(...) without a null check. If the required property (token
or url) is absent from the config, this throws a NullPointerException with no useful context.

Replace the implicit null deref with an explicit null check that throws an
IllegalArgumentException naming the missing property, giving operators a clear error message.
@Akanksha-kedia
Copy link
Copy Markdown
Contributor Author

@xiangfu0 please review

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 11, 2026

Codecov Report

❌ Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.70%. Comparing base (1881884) to head (425509f).
⚠️ Report is 85 commits behind head on master.

Files with missing lines Patch % Lines
.../org/apache/pinot/common/auth/UrlAuthProvider.java 0.00% 6 Missing ⚠️
...che/pinot/common/auth/StaticTokenAuthProvider.java 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18456      +/-   ##
============================================
+ Coverage     63.40%   63.70%   +0.29%     
- Complexity     1679     1684       +5     
============================================
  Files          3253     3262       +9     
  Lines        198767   199832    +1065     
  Branches      30791    31033     +242     
============================================
+ Hits         126034   127297    +1263     
+ Misses        62659    62392     -267     
- Partials      10074    10143      +69     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (?)
java-21 63.70% <0.00%> (+0.29%) ⬆️
temurin 63.70% <0.00%> (+0.29%) ⬆️
unittests 63.69% <0.00%> (+0.29%) ⬆️
unittests1 55.80% <0.00%> (+0.43%) ⬆️
unittests2 34.96% <0.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread pinot-common/src/main/java/org/apache/pinot/common/auth/UrlAuthProvider.java Outdated
@Akanksha-kedia
Copy link
Copy Markdown
Contributor Author

@yashmayya please merge

@yashmayya yashmayya merged commit 7430730 into apache:master May 12, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants