Skip to content

fix: require redirect URI for clients with multiple redirects - #13

Merged
MartinKolarik merged 1 commit into
masterfrom
gh-9
Jul 31, 2026
Merged

MartinKolarik merged 1 commit into
masterfrom
gh-9

Conversation

@MartinKolarik

Copy link
Copy Markdown
Member

Closes #9.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The App One seed now includes two callback URLs. The authorization endpoint reads client_id and redirect_uri from the request body or query string. If redirect_uri is missing, it uses the sole registered URL or returns invalid_request when multiple URLs exist. Integration tests cover both cases.

Possibly related PRs

  • jsdelivr/globalping-auth#11: Both changes modify OAuth authorization handling in src/oauth/server.ts, but implement different validation logic.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes requiring a redirect URI when clients have multiple configured redirects.
Description check ✅ Passed The description links issue #9, which directly matches the redirect URI handling changes.
Linked Issues check ✅ Passed The changes enforce an error for missing redirect_uri with multiple client redirects and preserve single-redirect fallback behavior [#9].
Out of Scope Changes check ✅ Passed All changes support redirect URI handling and its integration tests; no unrelated code changes are identified.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch gh-9

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
test/tests/integration/oauth.test.ts (1)

263-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Verify the resolved redirect URI, not only the dashboard redirect.

This test checks only the intermediate dashboard redirect. It does not confirm that the authorization code carries client2's sole registered redirect URL. Extend the test to fetch the approval details for the returned publicCodeId and assert code.redirectUri equals the client's registered URL.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/tests/integration/oauth.test.ts` around lines 263 - 270, Extend the test
around defaultAuthorizationRequest to fetch the approval details using the
returned publicCodeId, then assert that code.redirectUri equals client2’s sole
registered redirect URL. Keep the existing status and dashboard location
assertions unchanged.

Source: MCP tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/tests/integration/oauth.test.ts`:
- Around line 263-270: Extend the test around defaultAuthorizationRequest to
fetch the approval details using the returned publicCodeId, then assert that
code.redirectUri equals client2’s sole registered redirect URL. Keep the
existing status and dashboard location assertions unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 02e45298-1200-4035-bc68-77332e1e41ac

📥 Commits

Reviewing files that changed from the base of the PR and between 210d5be and 1c8b466.

📒 Files selected for processing (3)
  • seeds/test/index.ts
  • src/oauth/server.ts
  • test/tests/integration/oauth.test.ts

@MartinKolarik
MartinKolarik merged commit 37e4f7c into master Jul 31, 2026
3 checks passed
@MartinKolarik
MartinKolarik deleted the gh-9 branch July 31, 2026 14:08
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.

Require redirect_url for clients with multiple redirects configured

1 participant