feat: enhance Raycast integration and redirect URI validation - #165
Open
Zingzy wants to merge 3 commits into
Open
feat: enhance Raycast integration and redirect URI validation#165Zingzy wants to merge 3 commits into
Zingzy wants to merge 3 commits into
Conversation
- Updated Raycast extension description to clarify link management functionality. - Added redirect URI allowlist validation to ensure secure handling of redirect URIs in device authentication. - Improved the `validate_redirect_uri` method to support exact and prefix matching for redirect URIs, enhancing security for OAuth clients.
Contributor
Reviewer's GuideImplements stricter and more flexible redirect URI validation for device auth (including prefix wildcards) and configures the Raycast app to use it, along with updating its metadata. Sequence diagram for device auth callback redirect with enhanced redirect URI validationsequenceDiagram
actor RaycastUser
participant RaycastExtension
participant DeviceAuthEndpoint
participant DeviceAuthService
participant AppEntry
RaycastUser->>RaycastExtension: Complete device auth in Raycast
RaycastExtension->>DeviceAuthEndpoint: GET /auth/device/callback?code&state&redirect_uri
DeviceAuthEndpoint->>DeviceAuthService: resolve_app(app_id)
DeviceAuthService-->>DeviceAuthEndpoint: AppEntry
DeviceAuthEndpoint->>DeviceAuthService: validate_redirect_uri(redirect_uri, app)
DeviceAuthService->>AppEntry: read redirect_uris allowlist
alt redirect_uri empty
DeviceAuthService-->>DeviceAuthEndpoint: true
else redirect_uri matches exact allowlist entry
DeviceAuthService-->>DeviceAuthEndpoint: true
else redirect_uri matches prefix of entry ending with *
DeviceAuthService-->>DeviceAuthEndpoint: true
else no match
DeviceAuthService-->>DeviceAuthEndpoint: false
end
alt redirect_uri allowed
DeviceAuthEndpoint->>DeviceAuthEndpoint: build redirect_uri with code and state
DeviceAuthEndpoint-->>RaycastExtension: 302 Redirect to redirect_uri
else redirect_uri not allowed
DeviceAuthEndpoint->>DeviceAuthEndpoint: build default /auth/device/callback URL
DeviceAuthEndpoint-->>RaycastExtension: 302 Redirect to default callback
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds wildcard prefix matching for OAuth redirect URI validation, updates device auth callback to use the service validator, and updates the Raycast app registry entry with redirect URIs, links, permissions, and status. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Route as DeviceAuthRoute
participant Svc as DeviceAuthSvc
participant AppReg as AppRegistry
Client->>Route: request device callback (with redirect_uri)
Route->>AppReg: load app entry
Route->>Svc: validate_redirect_uri(redirect_uri, app)
Svc-->>Route: valid / invalid
alt valid
Route->>Client: 302 redirect to chosen callback
else invalid
Route->>Client: 400 or error page
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The redirect URI allowlist logic is duplicated between
_redirect_uri_allowedinroutes/auth/device.pyandvalidate_redirect_uriinDeviceAuthService; consider extracting a shared helper or reusing the service method in the route to avoid future divergence. - When doing prefix-based redirect URI checks with
startswith, consider normalizing and validating the URI components (e.g., scheme/host, usingurlparse) so a crafted URL that only shares a string prefix cannot bypass intended domain or protocol constraints.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The redirect URI allowlist logic is duplicated between `_redirect_uri_allowed` in `routes/auth/device.py` and `validate_redirect_uri` in `DeviceAuthService`; consider extracting a shared helper or reusing the service method in the route to avoid future divergence.
- When doing prefix-based redirect URI checks with `startswith`, consider normalizing and validating the URI components (e.g., scheme/host, using `urlparse`) so a crafted URL that only shares a string prefix cannot bypass intended domain or protocol constraints.
## Individual Comments
### Comment 1
<location path="services/auth/device.py" line_range="70-85" />
<code_context>
+ """
+ if not redirect_uri:
+ return False
+ for allowed in app.redirect_uris:
+ if allowed.endswith("*"):
+ if redirect_uri.startswith(allowed[:-1]):
+ return True
+ elif redirect_uri == allowed:
+ return True
+ return False
</code_context>
<issue_to_address>
**🚨 suggestion (security):** The prefix wildcard matching may be more permissive than intended for some redirect URI patterns.
Using `startswith` on the prefix before `*` means `https://raycast.com/redirect*` will also match `https://raycast.com/redirect-malicious`, not just `.../redirect?…` or `.../redirect/...`. If only that exact path with arbitrary query params is intended, consider tightening the match (e.g., checking the next character is `?` or `/`) or clearly requiring integrators to include a trailing `/` or `?` in wildcard entries to avoid over-broad matches.
```suggestion
"""Return True if redirect_uri is empty, exact-matches an allowlist
entry, or path-matches an allowlist entry ending with ``*``.
The ``*`` suffix is used for OAuth clients (e.g. Raycast) that append
a varying query string or sub-path to a fixed redirect URL.
For wildcard entries, the prefix before ``*`` must either match the
redirect URI exactly, or be followed by ``?`` or ``/``. This avoids
over-broad matches such as allowing ``.../redirect-malicious`` for an
entry of ``.../redirect*``.
"""
if not redirect_uri:
return True
for allowed in app.redirect_uris:
if allowed.endswith("*"):
prefix = allowed[:-1]
if (
redirect_uri == prefix
or redirect_uri.startswith(prefix + "?")
or redirect_uri.startswith(prefix + "/")
):
return True
elif redirect_uri == allowed:
return True
return False
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR enhances the device-auth (extension/desktop) OAuth-like flow by tightening redirect URI validation and updating the Raycast app registry entry to support Raycast’s redirect behavior.
Changes:
- Extend redirect URI validation to allow exact matches and explicit
*-suffix prefix matches. - Update device-auth redirect building to use the new allowlist matching rules.
- Update the Raycast app registry entry (description + redirect URI allowlist + links/permissions).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| services/auth/device.py | Implements *-suffix prefix matching in validate_redirect_uri. |
| routes/auth/device.py | Adds route-layer allowlist matcher for building callback redirects. |
| config/apps.yaml | Updates Raycast app metadata and adds a wildcard redirect URI entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/apps.yaml`:
- Around line 85-86: Update the redirect_uris entry so it only allows the fixed
/redirect path with an optional query string instead of the broad prefix; in
config/apps.yaml replace the value under redirect_uris (currently
"https://raycast.com/redirect*") with a pattern that anchors the wildcard at the
query boundary such as "https://raycast.com/redirect" and
"https://raycast.com/redirect?*" (or a single pattern
"https://raycast.com/redirect?*") to prevent matching sibling paths; ensure you
modify the redirect_uris array entry for the Raycast app accordingly.
In `@routes/auth/device.py`:
- Around line 69-94: There are two duplicated redirect-URI allowlist
implementations: the route-level _redirect_uri_allowed and
DeviceAuthService.validate_redirect_uri; remove the route-local helper and
delegate to the service to avoid divergence. Update _build_callback_redirect to
call DeviceAuthService.validate_redirect_uri (inject or import the service
instance) and handle empty redirect behavior in the route layer (either by
passing an allow_empty flag to the service or by checking bool(redirect_uri)
before calling the service), then delete _redirect_uri_allowed so only the
service implements the matching logic.
In `@services/auth/device.py`:
- Around line 69-85: The validate_redirect_uri method currently accepts
degenerate wildcards and unanchored prefixes; update validate_redirect_uri to
reject a bare "*" (i.e., require prefix := allowed[:-1] to be non-empty),
require the prefix to start with "http://" or "https://", and when matching a
wildcard ensure redirect_uri.startswith(prefix) AND that the character
immediately following the prefix in redirect_uri (if any) is one of '/', '?',
'#' or end-of-string (to anchor on a path/query/fragment boundary and prevent
matches like "/redirector"); also harmonize this logic with the near-duplicate
_redirect_uri_allowed in routes/auth/device.py so both handle empty redirect_uri
the same way, and add unit tests for validate_redirect_uri covering exact
matches, valid wildcard matches, bare "*" rejection, and unanchored-prefix
rejection instead of relying only on the existing integration mock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27bac235-3640-480b-ba14-cf169d23084d
📒 Files selected for processing (3)
config/apps.yamlroutes/auth/device.pyservices/auth/device.py
- Changed the status of the Raycast app in the configuration file to reflect its current availability, enhancing clarity for users and developers.
- Updated the redirect URI format in the configuration for the Raycast app to support query parameters. - Refactored the redirect URI validation logic to utilize a service method for improved security and maintainability. - Added comprehensive unit tests for the new validation logic, including support for wildcard matching and various edge cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/services/test_redirect_uri.py (1)
16-17: Optional: instantiate via the public constructor (or a fixture) instead of__new__.
DeviceAuthService.__new__(DeviceAuthService)works only becausevalidate_redirect_urihappens to be a pure function of its arguments today. The moment anyone touchesself.<dep>inside it, every test here turns into anAttributeErrorthat's unrelated to what's being tested. A small fixture that constructs the service with stub dependencies (or — if the method really is stateless — making it a@staticmethod) would be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/services/test_redirect_uri.py` around lines 16 - 17, The test uses DeviceAuthService.__new__(DeviceAuthService) in setup_method which bypasses initialization and risks AttributeError if validate_redirect_uri later touches instance dependencies; replace this by constructing the service via its public constructor (DeviceAuthService(...)) or a pytest fixture that supplies stub/mock dependencies so the instance is fully initialized before calling validate_redirect_uri, or alternatively make validate_redirect_uri a `@staticmethod` on DeviceAuthService if it truly has no reliance on instance state; update setup_method/tests to use the chosen approach instead of __new__.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/services/test_redirect_uri.py`:
- Around line 72-77: The test in test_bare_star_matches_everything locks in a
dangerous behavior: a bare "*" in an app allowlist lets any redirect pass;
instead modify DeviceAuthService.validate_redirect_uri to explicitly reject bare
"*" or any allowlist entry that collapses to an empty prefix after stripping a
trailing '*' (i.e., treat entries equal to "*" or where a[:-1].strip() == "" as
invalid), return False (or raise a validation error) for those entries, and
update the test to assert that such entries are rejected; alternatively, enforce
that any wildcard entry must include a scheme+host (ensure the substring before
the trailing '*' contains "://" and a "/") when checking in
validate_redirect_uri so misconfigured bare wildcards fail at service layer.
---
Nitpick comments:
In `@tests/unit/services/test_redirect_uri.py`:
- Around line 16-17: The test uses DeviceAuthService.__new__(DeviceAuthService)
in setup_method which bypasses initialization and risks AttributeError if
validate_redirect_uri later touches instance dependencies; replace this by
constructing the service via its public constructor (DeviceAuthService(...)) or
a pytest fixture that supplies stub/mock dependencies so the instance is fully
initialized before calling validate_redirect_uri, or alternatively make
validate_redirect_uri a `@staticmethod` on DeviceAuthService if it truly has no
reliance on instance state; update setup_method/tests to use the chosen approach
instead of __new__.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 155d09a8-27d9-44d2-899c-3417158123a7
📒 Files selected for processing (4)
config/apps.yamlroutes/auth/device.pytests/integration/test_device_auth.pytests/unit/services/test_redirect_uri.py
🚧 Files skipped from review as they are similar to previous changes (1)
- routes/auth/device.py
Comment on lines
+72
to
+77
| def test_bare_star_matches_everything(self): | ||
| """A bare '*' entry matches any URI — intentional if configured.""" | ||
| app = _app(["*"]) | ||
| assert ( | ||
| self.svc.validate_redirect_uri("https://anything.com/whatever", app) is True | ||
| ) |
There was a problem hiding this comment.
Bare * allowlist is a footgun — consider rejecting it explicitly.
Documenting this behavior in a test locks in a configuration mode where a single typo (- "*") in apps.yaml silently disables redirect URI validation for an app, allowing an attacker to receive a freshly minted device auth code on any host. There's no legitimate need for a bare * entry today (every real app has a fixed callback or a host-anchored prefix), so it's safer to treat it as a misconfiguration.
Suggest either:
- Rejecting bare
*(and any allowlist entry whose prefix collapses to""after stripping the trailing*) insideDeviceAuthService.validate_redirect_uriand/or at app-registry load time, and replacing this test with one that asserts the rejection; or - At minimum, also requiring a scheme+host before the
*(e.g. enforce thata[:-1]contains://and a/).
Tracking this at the service layer is preferable to relying on YAML hygiene.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/services/test_redirect_uri.py` around lines 72 - 77, The test in
test_bare_star_matches_everything locks in a dangerous behavior: a bare "*" in
an app allowlist lets any redirect pass; instead modify
DeviceAuthService.validate_redirect_uri to explicitly reject bare "*" or any
allowlist entry that collapses to an empty prefix after stripping a trailing '*'
(i.e., treat entries equal to "*" or where a[:-1].strip() == "" as invalid),
return False (or raise a validation error) for those entries, and update the
test to assert that such entries are rejected; alternatively, enforce that any
wildcard entry must include a scheme+host (ensure the substring before the
trailing '*' contains "://" and a "/") when checking in validate_redirect_uri so
misconfigured bare wildcards fail at service layer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
validate_redirect_urimethod to support exact and prefix matching for redirect URIs, enhancing security for OAuth clients.Summary by Sourcery
Add wildcard-aware redirect URI validation for device auth and update Raycast app configuration.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Tests