Skip to content

Conversation

@adrianosela
Copy link
Contributor

[CVE-2024-6839] Sort Paths by Regex Specificity

Before this change, the library matched CORS resource paths by sorting regex patterns based on string length. This flawed logic allowed less specific but longer regexes (e.g., /.*) to take precedence over more specific paths (e.g., /api/v1/secure), potentially applying overly permissive CORS rules to sensitive endpoints. This could lead to unauthorized cross-origin access, data leaks, and misuse of protected APIs.

The fix updates the resource matching logic in parse_resources to:

  • Prioritize exact string paths (most specific).
  • Among regex patterns, prefer deeper paths (with more /) and longer regexes, assuming they are more specific.
  • Ensure the most specific match is used first, reducing risk of applying a permissive policy to a sensitive route.

Addresses 1/3 issues raised in #384

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@5da9be4). Learn more about missing BASE report.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@          Coverage Diff           @@
##             main    #391   +/-   ##
======================================
  Coverage        ?   92.5%           
======================================
  Files           ?       5           
  Lines           ?     242           
  Branches        ?      47           
======================================
  Hits            ?     224           
  Misses          ?       7           
  Partials        ?      11           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Owner

@corydolphin corydolphin left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. I think this is an improvement, but doesn't address the heart of the problem in the CVE.

I think we need a more obviously explicit protocol around ordering of regexes which may require a deeper overhaul of the way resources are defined. If you are open to doing that, I would love to see a stricter, more explicit system!

@corydolphin corydolphin merged commit e970988 into corydolphin:main May 15, 2025
7 checks passed
@adrianosela
Copy link
Contributor Author

Sounds good @corydolphin. Give me a few days/weeks and I’ll cook something up.

@adrianosela adrianosela deleted the CVE-2024-6839-fix branch May 15, 2025 04:47
@adrianosela
Copy link
Contributor Author

Hey @corydolphin, I've got 2 questions for you:

  • I re-read the wording on the CVE and I believe this PR does fully address the vulnerability. I am wondering why you believe it doesn't address the heart of the problem in the CVE.
  • Did you have any rough ideas of what you'd like to see in terms of how resources should be defined? e.g. assigning a priority score, or otherwise just using the order of the list as a proxy for priority -- or anything else?

corydolphin pushed a commit that referenced this pull request Jun 11, 2025
* Rename "helper_tests.py" to "test_helpers.py".

This causes it to get picked up by pytest, which makes clear the failure
that I will fix in a followup commit.

* Sort paths longest to shortest.

Previously, path sort length was inverted, with short paths before long
ones, this led to a path like `/*` being handled *before* a path like
`/foo/bar/baz`, which is exactly what we didn't want. This was tested,
but the test was misnamed and so wasn't picked up by pytest.

---------

Co-authored-by: joshuamorton <[email protected]>
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