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.

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

ref(toolbar): check allowed origins in iframe view #77756

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

aliu39
Copy link
Member

@aliu39 aliu39 commented Sep 18, 2024

Updates iframe view to lookup the project from the URL param, after auth. Handles missing project and allowed origins for the request (set up by a project setting).

Decided with @ryan953 we'll only allow wildcards in the project settings, for the leftmost subdomain only.

Closes #77214

@aliu39 aliu39 requested a review from ryan953 September 18, 2024 23:32
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 18, 2024
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 4 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/toolbar/utils/url.py 91.30% 1 Missing and 1 partial ⚠️
src/sentry/toolbar/views/iframe_view.py 93.54% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #77756       +/-   ##
===========================================
+ Coverage   56.23%   78.10%   +21.87%     
===========================================
  Files        6974     6989       +15     
  Lines      309287   310110      +823     
  Branches    50629    50742      +113     
===========================================
+ Hits       173932   242222    +68290     
+ Misses     130937    61443    -69494     
- Partials     4418     6445     +2027     

@aliu39
Copy link
Member Author

aliu39 commented Sep 19, 2024

  • Ready, but needs test coverage:

  • unit tests for url_matches

  • e2e tests making requests to iframe_view

    • project doesn't exist
    • missing REFERER
    • missing/unsupported scheme
    • calls mock url_matches

@aliu39 aliu39 changed the title ref(toolbar): check allowed origins and update response templates ref(toolbar): check allowed origins in iframe view Sep 20, 2024
@ryan953 ryan953 requested review from a team and cmanallen September 20, 2024 16:51
Copy link
Member

@cmanallen cmanallen left a comment

Choose a reason for hiding this comment

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

Finish the todos. Add test coverage.

src/sentry/toolbar/utils/url.py Outdated Show resolved Hide resolved
src/sentry/toolbar/views/iframe_view.py Outdated Show resolved Hide resolved
@aliu39
Copy link
Member Author

aliu39 commented Sep 21, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DevToolbar] Guard /iframe/ & /login-success/ pages against disallowed referer origins
2 participants