Skip to content

refactor: rework scheme validation in oauth controller and frontend#1026

Merged
steveiliop56 merged 3 commits into
mainfrom
refactor/domain-validator-schemes
Jul 18, 2026
Merged

refactor: rework scheme validation in oauth controller and frontend#1026
steveiliop56 merged 3 commits into
mainfrom
refactor/domain-validator-schemes

Conversation

@steveiliop56

@steveiliop56 steveiliop56 commented Jul 18, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Bug Fixes
    • Improved redirect URL safety by tightening how redirect validation errors are treated, and adjusted redirect URI trust decisions for scheme and port comparisons.
    • Refined domain/URL validation to better detect scheme, hostname, and port mismatches, using direct port comparison when port checking is enabled.
    • Improved access-control debug logging by emitting validation-failure details for non-hostname mismatches while suppressing hostname-mismatch noise.
  • Documentation
    • Updated guidance for the reusable pkg directory, including best-effort compatibility expectations and version-pinning recommendations.

@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 18, 2026
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f9fdd980-bc93-4206-a4bb-195c00bba684

📥 Commits

Reviewing files that changed from the base of the PR and between af38aa5 and 866703c.

📒 Files selected for processing (1)
  • pkg/validators/domain_validator.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/validators/domain_validator.go

📝 Walkthrough

Walkthrough

Domain validation now distinguishes scheme, port, and hostname mismatches while changing URL and hostname normalization. OAuth redirect checks, ACL logging, frontend trust checks, tests, and package documentation were updated.

Changes

Domain validation and redirect safety

Layer / File(s) Summary
Validator parsing and mismatch contracts
pkg/validators/domain_validator.go, pkg/validators/domain_validator_test.go, pkg/README.md
URL parsing, scheme handling, hostname normalization, direct port comparison, exported mismatch errors, tests, and package documentation were updated.
Redirect and ACL validation handling
internal/controller/oauth_controller.go, internal/controller/oauth_controller_test.go, internal/service/access_controls_service.go, frontend/src/lib/hooks/redirect-uri.ts
Redirect safety now continues only for hostname mismatches, ACL logging reports other validation failures, and frontend trust checks compare ports directly.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

Suggested reviewers: rycochet

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main refactor: scheme validation changes in the OAuth controller and frontend.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ 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 refactor/domain-validator-schemes

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.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@pkg/validators/domain_validator_test.go`:
- Around line 226-229: Update the “Valid domains with non matching ports should
fail” test case to provide an errorFunc that asserts ErrPortMismatch, so the
expected port mismatch is explicitly validated instead of requiring no error.
Keep the existing DomainValidatorOptions, expected, and actual values unchanged.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ec9fb3cd-d642-4d77-9b06-86048335d36e

📥 Commits

Reviewing files that changed from the base of the PR and between 50c25e4 and daf16b7.

📒 Files selected for processing (7)
  • frontend/src/lib/hooks/redirect-uri.ts
  • internal/controller/oauth_controller.go
  • internal/controller/oauth_controller_test.go
  • internal/service/access_controls_service.go
  • pkg/README.md
  • pkg/validators/domain_validator.go
  • pkg/validators/domain_validator_test.go
💤 Files with no reviewable changes (1)
  • internal/controller/oauth_controller_test.go

Comment thread pkg/validators/domain_validator_test.go
@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.48649% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/validators/domain_validator.go 88.57% 2 Missing and 2 partials ⚠️
internal/service/access_controls_service.go 0.00% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Rycochet
Rycochet previously approved these changes Jul 18, 2026
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jul 18, 2026
@steveiliop56
steveiliop56 merged commit f43d690 into main Jul 18, 2026
3 checks passed
@steveiliop56
steveiliop56 deleted the refactor/domain-validator-schemes branch July 18, 2026 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants