fix: add resolvability check before redirecting to prevent insecure redirects after publishing #436
Conversation
Reviewer's Guide by SourceryThis PR implements a security enhancement for the publish view redirect functionality. It adds a URL resolvability check before performing redirects after publishing content to prevent potential insecure redirects. If a requested redirect URL is not resolvable within the application, it falls back to the version list URL. Sequence diagram for URL resolvability check before redirectsequenceDiagram
actor User
participant Application
participant URLResolver
User->>Application: Publish content
Application->>URLResolver: Check if requested URL is resolvable
URLResolver-->>Application: URL is resolvable
Application->>User: Redirect to requested URL
alt URL not resolvable
URLResolver-->>Application: URL not resolvable
Application->>User: Redirect to version list URL
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @theShinigami - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tests/test_admin.py
Outdated
| def test_publish_resolvable_redirect_url(self): | ||
| from djangocms_versioning import conf | ||
|
|
||
| conf.ON_PUBLISH_REDIRECT = "published" |
There was a problem hiding this comment.
issue (testing): Missing test cleanup for ON_PUBLISH_REDIRECT setting
The test modifies a global setting but doesn't restore it after the test. This could affect other tests. Consider using setUp/tearDown or storing the original value and restoring it at the end of the test, similar to how test_publish_view_redirects_according_to_settings does it.
tests/test_admin.py
Outdated
| # when the requested url is not resolvable, should default to version list url | ||
| not_resolvable_url = url + "?next=http://example.com" | ||
|
|
||
| with self.login_user_context(user): | ||
| response = self.client.post(not_resolvable_url) | ||
|
|
||
| self.assertEqual(response.url, version_list_url(poll_version.content)) |
There was a problem hiding this comment.
issue (testing): Missing security-related test cases for redirect validation
Given that this is a security-related change to prevent insecure redirects, consider adding test cases for potentially malicious URLs (e.g., URLs with different schemes, URLs to external domains, URLs with special characters). This would help ensure the security aspect is properly tested.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #436 +/- ##
==========================================
+ Coverage 90.88% 91.23% +0.35%
==========================================
Files 72 72
Lines 2546 2671 +125
Branches 361 308 -53
==========================================
+ Hits 2314 2437 +123
+ Misses 168 163 -5
- Partials 64 71 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hey @theShinigami ! Thanks for the PR! That's a good step in the right direction.
I have two comments:
- In the case of
conf.ON_PUBLISH_REDIRECTnot in("preview", "published")therequested_redirectis not validated - Also the published object's absolute url in line 1067 needs to be validated: Some custom object could return an invalid url.
Description
After publishing, if there is a requested URL, it first checks if the requested URL is resolvable. If it is not, it defaults to the version list URL.
Related resources
Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Implement a resolvability check for requested URLs in the publish view to prevent insecure redirects after publishing. Update tests to cover scenarios with resolvable and non-resolvable URLs.
Bug Fixes:
Tests: