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.

[praveenc7]

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 I don't quite understand, we are striping the matixParamter during urlCheck and not handling for any specific case. Can you provide more details or example on what you mean?

[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 I don't quite understand, we are striping the matixParamter during urlCheck and not handling for any specific case. Can you provide more details or example on what you mean?

You are stripping the matrixparams from the URL before, https://pinot.example.com/file.txt;param1=value1 will behave as https://pinot.example.com/file.txt . My questions is, why have https://pinot.example.com/file.txt;param1=value1 URL? IMO, Pinot does not support MatrixParam, it supports QueryParam .

[praveenc7]

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 I don't quite understand, we are striping the matixParamter during urlCheck and not handling for any specific case. Can you provide more details or example on what you mean?

You are stripping the matrixparams from the URL before, https://pinot.example.com/file.txt;param1=value1 will behave as https://pinot.example.com/file.txt . My questions is, why have https://pinot.example.com/file.txt;param1=value1 URL? IMO, Pinot does not support MatrixParam, it supports QueryParam .

@soumitra-st Thanks for the example. You’re right that Pinot doesn’t support MatrixParam but instead relies on QueryParam. However, users may unintentionally or maliciously include special characters after matrix-style parameters (;) in URLs, which could pose security risks if not handled properly.
cc : @siddharthteotia

[Jackie-Jiang]

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.

@soumitra-st Do we need to carry-over this behavior? I don't follow why we want to allow such access without going through auth

[soumitra-st]

@soumitra-st Thanks for the example. You’re right that Pinot doesn’t support MatrixParam but instead relies on QueryParam. However, users may unintentionally or maliciously include special characters after matrix-style parameters (;) in URLs, which could pose security risks if not handled properly. cc : @siddharthteotia

@praveenc7, I agree with the security issue. Did you discover this issue from a code/security review or pen testing?

I see one issue. Now https://pinot.example.com/file.txt;param1=value1/blah will be checked as if it is https://pinot.example.com/file.txt and will be executed without AUTH. Is it desirable?

[soumitra-st]

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.

@soumitra-st Do we need to carry-over this behavior? I don't follow why we want to allow such access without going through auth

@Jackie-Jiang , I checked the git history but could not find why we have both whitelisting (UNPROTECTED_PATHS) and open top-level resources. We can remove the open top-level resources, find the list of top-level files, and add them to the whitelist. Let me know if we should do that; I can create a task.

[Jackie-Jiang]

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.

@soumitra-st Do we need to carry-over this behavior? I don't follow why we want to allow such access without going through auth

@Jackie-Jiang , I checked the git history but could not find why we have both whitelisting (UNPROTECTED_PATHS) and open top-level resources. We can remove the open top-level resources, find the list of top-level files, and add them to the whitelist. Let me know if we should do that; I can create a task.

Agree. I'm not familiar with how these resources are utilized, maybe we can directly remove them.