Strip Matrix parameter from BasePath check

[praveenc7]

Summary

Modify Parser to strip matrix parameter when checking for basePath

Testing

unit test added

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.78%. Comparing base (59551e4) to head (e493941).
Report is 1292 commits behind head on master.

Files with missing lines	Patch %	Lines
...ache/pinot/broker/broker/AuthenticationFilter.java	0.00%	2 Missing ⚠️
...ot/controller/api/access/AuthenticationFilter.java	0.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #14383      +/-   ##
============================================
+ Coverage     61.75%   63.78%   +2.03%     
- Complexity      207     1555    +1348     
============================================
  Files          2436     2660     +224     
  Lines        133233   145967   +12734     
  Branches      20636    22350    +1714     
============================================
+ Hits          82274    93101   +10827     
- Misses        44911    45985    +1074     
- Partials       6048     6881     +833     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (+99.99%)	⬆️
integration	100.00% <ø> (+99.99%)	⬆️
integration1	100.00% <ø> (+99.99%)	⬆️
integration2	0.00% <ø> (ø)
java-11	63.73% <50.00%> (+2.02%)	⬆️
java-21	63.67% <50.00%> (+2.04%)	⬆️
skip-bytebuffers-false	63.77% <50.00%> (+2.02%)	⬆️
skip-bytebuffers-true	63.62% <50.00%> (+35.89%)	⬆️
temurin	63.78% <50.00%> (+2.03%)	⬆️
unittests	63.77% <50.00%> (+2.03%)	⬆️
unittests1	55.49% <0.00%> (+8.60%)	⬆️
unittests2	34.15% <50.00%> (+6.41%)	⬆️

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.

[Jackie-Jiang]

IIUC, the intention is to avoid bypassing the isBaseFile() check?
What is the purpose of isBaseFile()? I'd suggest making it very limited (e.g. only allow ".", or is it even required?)

[praveenc7]

IIUC, the intention is to avoid bypassing the isBaseFile() check? What is the purpose of isBaseFile()? I'd suggest making it very limited (e.g. only allow ".", or is it even required?)

@Jackie-Jiang I’m not aware of the historical reasons for why this check is necessary, This type of check is generally used to filter paths, likely to allow access only to certain files, such as configuration files or top-level resources, while excluding directories and complex paths.

For example : https://example.com/file.txt , https://example.com/image.png

The check we have already ensures we don't allow for directory access and just top-level resources like
file.backup.txt or another.file.backup.txt.

Do we want to allow for something like just file.txt and not file.backup.txt as well?

[Jackie-Jiang]

@soumitra-st Can you help take a look? Which path do we need to allow bypassing the auth check?

I want to make this whitelist very restricted so that only the allowed path can bypass the auth check. This way we can avoid other possible attacks, instead of explicitly handling them one by one.

[soumitra-st]

Modify Parser to strip matrix parameter when checking for basePath

@praveenc7 ,Pinot does not support MatrixParam. Can you elaborate on the context of handling MatrixParam for top-level resources only?

[soumitra-st]

@soumitra-st Can you help take a look? Which path do we need to allow bypassing the auth check?

I want to make this whitelist very restricted so that only the allowed path can bypass the auth check. This way we can avoid other possible attacks, instead of explicitly handling them one by one.

The current whitelist paths are HERE:
new HashSet<>(Arrays.asList("", "help", "auth/info", "auth/verify", "health"));

In addition to the above whitelisted paths, all top files having at least one '.' are allowed. This is likely done to allow access to top-level resource files. If we need to remove implicit access to all the top-level resources, we must figure out the list and add it to the UNPROTECTED_PATHS.