HJ-127 - Fix API router sanitizer being too aggressive with NextJS Catch-all Segments

ethyca / fides

The Privacy Engineering & Compliance Framework

https://ethyca.com/docs

Apache License 2.0

360 stars 72 forks source link

HJ-127 - Fix API router sanitizer being too aggressive with NextJS Catch-all Segments #5438

Closed andres-torres-marroquin closed 3 weeks ago

andres-torres-marroquin commented 3 weeks ago

Closes #HJ-127

Description Of Changes

Fix API router sanitizer being too aggressive with NextJS Catch-all Segments

Code Changes

[ ] Updated sanitise_url_path on src/fides/api/main.py so it is more permissive with NextJS urls.

Steps to Confirm

[ ] Run the test suite

Pre-Merge Checklist

[ ] All CI Pipelines Succeeded
Documentation:
- [ ] documentation complete, PR opened in fidesdocs
- [ ] documentation issue created in fidesdocs
- [ ] if there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
[ ] Issue Requirements are Met
[ ] Relevant Follow-Up Issues Created
[ ] Update CHANGELOG.md
[ ] For API changes, the Postman collection has been updated
If there are any database migrations:
- [ ] Ensure that your downrev is up to date with the latest revision on main
- [ ] Ensure that your downgrade() migration is correct and works
- [ ] If a downgrade migration is not possible for this change, please call this out in the PR description!

vercel[bot] commented 3 weeks ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

cypress[bot] commented 3 weeks ago

fides Run #10701

Run Properties: Passed #10701 • 6f332af830 ℹ️: Merge 8637ff8fb79c9871045f8a330775799aec9a1bf6 into 811a691d32bad392ff8287a690e4...

Project	`fides`
Branch Review	`refs/pull/5438/merge`
Run status	`Passed #10701`
Run duration	`00m 39s`
Commit	`6f332af830 ℹ️: Merge 8637ff8fb79c9871045f8a330775799aec9a1bf6 into 811a691d32bad392ff8287a690e4...`
Committer	`Andres Torres`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.47%. Comparing base (811a691) to head (8637ff8). Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #5438 +/- ## ======================================= Coverage 85.47% 85.47% ======================================= Files 384 384 Lines 24116 24118 +2 Branches 2624 2624 ======================================= + Hits 20612 20614 +2 Misses 2950 2950 Partials 554 554 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

daveqnet commented 3 weeks ago

If this is not helpful, tell me, but an alternative approach we could take here would be to side-step all the checking for .. strings and fully resolve the path first using os.path.abspath or possibly os.path.realpath (instead of using os.path.normpath to get a relative path), and only proceed if the resolved (canonical) path matches an allow-list.

This at least applies to files that can be served, not API or UI routes, as we only need path traversal protection when serving real files.

cypress[bot] commented 3 weeks ago

fides Run #10702

Run Properties: Passed #10702 • 8bc0425f43: HJ-127 - Fix API router sanitizer being too aggressive with NextJS Catch-all Seg...

Project	`fides`
Branch Review	`main`
Run status	`Passed #10702`
Run duration	`00m 37s`
Commit	`8bc0425f43: HJ-127 - Fix API router sanitizer being too aggressive with NextJS Catch-all Seg...`
Committer	`Andres Torres`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎