Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: implement paypal webhook refunds #309

Merged
merged 19 commits into from
Dec 13, 2024
Merged

feat: implement paypal webhook refunds #309

merged 19 commits into from
Dec 13, 2024

Conversation

aht007
Copy link
Member

@aht007 aht007 commented Dec 13, 2024

Merge checklist:
Check off if complete or not applicable:

  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, migration issues, deadlines, tickets

Post-merge:

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 13, 2024
View reviewed changes
commerce_coordinator/apps/paypal/views.py Outdated
except Exception as e: # pylint: disable=broad-exception-caught
if not self._is_valid_url(url):
raise ValueError("Invalid or untrusted URL provided") from e
r = requests.get(url) # pylint: disable=missing-timeout

Check failure

Code scanning / CodeQL

Full server-side request forgery

The full URL of this request depends on a [user-provided value](1).

Copilot Autofix AI 5 days ago

To fix the problem, we need to ensure that the URL used in the requests.get call is fully validated and cannot be manipulated by an attacker. One way to achieve this is to maintain a list of authorized URLs on the server and choose from that list based on the input provided. Alternatively, we can enhance the validation to ensure that the URL is safe.

In this case, we will enhance the validation by ensuring that the URL path is also checked to prevent any potential SSRF attacks. We will modify the _is_valid_url method to include this additional validation.

Suggested changeset 1
commerce_coordinator/apps/paypal/views.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/commerce_coordinator/apps/paypal/views.py b/commerce_coordinator/apps/paypal/views.py
--- a/commerce_coordinator/apps/paypal/views.py
+++ b/commerce_coordinator/apps/paypal/views.py
@@ -61,2 +61,4 @@
                 return False
+            if not parsed_url.path.startswith('/'):
+                return False
             return True
EOF
@@ -61,2 +61,4 @@
return False
if not parsed_url.path.startswith('/'):
return False
return True
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@github-actions GitHub Actions
Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  commerce_coordinator
  urls.py
  commerce_coordinator/apps/commercetools
  clients.py 405-408, 570-571
  pipeline.py
  signals.py 62
  tasks.py 88-99
  utils.py
  commerce_coordinator/apps/commercetools/catalog_info
  constants.py
  edx_utils.py
  commerce_coordinator/apps/commercetools/tests
  test_utils.py
  commerce_coordinator/apps/paypal
  models.py
  pipeline.py
  signals.py
  urls.py
  views.py 42-50, 56-64, 70-73, 78-134
  commerce_coordinator/apps/paypal/tests
  __init__.py
  test_pipeline.py
  commerce_coordinator/apps/stripe
  pipeline.py
Project Total  

This report was generated by python-coverage-comment-action

@aht007 aht007 changed the base branch from main to manwar/SONIC-784 December 13, 2024 14:08
@shafqatfarhan shafqatfarhan changed the base branch from manwar/SONIC-784 to main December 13, 2024 14:52
@aht007 aht007 changed the base branch from main to manwar/SONIC-784 December 13, 2024 14:55
@shafqatfarhan shafqatfarhan force-pushed the aht007/SONIC-808-paypal-webhook-refunds branch from ec251d6 to d03d47a Compare December 13, 2024 15:03
shafqatfarhan
shafqatfarhan reviewed Dec 13, 2024
View reviewed changes
commerce_coordinator/apps/paypal/views.py Show resolved Hide resolved
commerce_coordinator/apps/paypal/views.py Outdated Show resolved Hide resolved
commerce_coordinator/apps/paypal/views.py Show resolved Hide resolved
commerce_coordinator/apps/paypal/views.py Outdated Show resolved Hide resolved
commerce_coordinator/apps/paypal/views.py Outdated Show resolved Hide resolved
commerce_coordinator/apps/commercetools/clients.py Show resolved Hide resolved
commerce_coordinator/apps/commercetools/clients.py Outdated Show resolved Hide resolved
commerce_coordinator/apps/commercetools/clients.py Outdated Show resolved Hide resolved
commerce_coordinator/apps/commercetools/clients.py Outdated Show resolved Hide resolved
commerce_coordinator/apps/commercetools/clients.py Outdated Show resolved Hide resolved
@aht007
Copy link
Member Author

aht007 commented Dec 13, 2024

Failing tests here are related to Paypal receipts which will be resolved in its own branch.

@aht007 aht007 merged commit a4ea805 into manwar/SONIC-784 Dec 13, 2024
4 of 6 checks passed
@aht007 aht007 deleted the aht007/SONIC-808-paypal-webhook-refunds branch December 13, 2024 18:36
@shafqatfarhan shafqatfarhan restored the aht007/SONIC-808-paypal-webhook-refunds branch December 16, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shafqatfarhan shafqatfarhan shafqatfarhan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aht007 @shafqatfarhan @mubbsharanwar