Skip to content

[4.x] Only replace hostname in tenant_route helper and the domain redirect macro - #1490

Merged
stancl merged 3 commits into
masterfrom
correct-tenant-route
Sep 30, 2026
Merged

stancl merged 3 commits into
masterfrom
correct-tenant-route

Conversation

@lukinovec

@lukinovec lukinovec commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

tenant_route() replaces all occurrences of the hostname in the URL generated by route(). When used like tenant_route('foo.localhost', 'foo', ['email' => 'foo@localhost']), it generates http://foo.localhost/abcdef?email=foo%40foo.localhost. So the query parameter's localhost gets replaced with foo.localhost which shouldn't happen -- only the actual hostname should be replaced. Same goes for the domain macro registered in CrossDomainRedirect.

This PR makes both tenant_route() and the domain() macro replace only the hostname. So tenant_route('foo.localhost', 'foo', ['email' => 'foo@localhost']) now generates http://foo.localhost/abcdef?email=foo%40localhost, and redirect()->route('home', ['email' => 'foo@localhost'])->domain('abcd') redirects to http://abcd/foobar?email=foo%40localhost.

Summary by CodeRabbit

  • Bug Fixes
    • Redirects now replace only the hostname in the URL’s domain, preserving other matching text.
    • Redirect URLs correctly include email parameters as encoded query values.

@lukinovec lukinovec added the bug Something isn't working label Sep 28, 2026
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ef54be9d-710e-4cd1-b2dc-36fd72a0d534

📥 Commits

Reviewing files that changed from the base of the PR and between 34438f9 and 2c7ecff.

📒 Files selected for processing (3)
  • src/Features/CrossDomainRedirect.php
  • src/helpers.php
  • tests/Features/RedirectTest.php

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The redirect and tenant route helper now replace only the first protocol-and-hostname match. Updated tests check that email parameters remain encoded in generated URLs.

Changes

URL hostname replacement

Layer / File(s) Summary
Limit hostname replacement and verify query parameters
src/Features/CrossDomainRedirect.php, src/helpers.php, tests/Features/RedirectTest.php
Both URL helpers limit replacement to the first protocol-and-hostname match. The tests pass foo@localhost as an email parameter and expect the encoded value foo%40localhost in the generated URLs.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 2c7ec

The URL helpers now limit hostname substitution to the URL authority, preserving encoded email query values. No merge-blocking risk is established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 2c7ec

The change confines hostname replacement to the start of the URL authority and preserves matching text in query values. No new redirect entrypoint or boundary bypass was established, though behavior for less-common URL forms and production callers is not fully covered.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected behavior is destination URL construction for callers of the existing macro or helper; production macro callers are not fully resolved.

Trust Boundaries and Controls

  • inferred — The replacement assumes a protocol immediately precedes the parsed hostname. A target with authority userinfo or no scheme would not satisfy that match; no inspected caller establishes those forms as supported inputs to the macro.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: limiting hostname replacement in both the tenant_route helper and the domain() redirect macro.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

A rabbit hops along the route,
And keeps the email value stout.
The host swaps once, then leaves query text,
Encoded safely for what comes next.
With carrots packed, it bounds away.

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

@codecov

codecov Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.89%. Comparing base (34438f9) to head (2c7ecff).

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1490   +/-   ##
=========================================
  Coverage     86.89%   86.89%           
  Complexity     1252     1252           
=========================================
  Files           186      186           
  Lines          3654     3654           
=========================================
  Hits           3175     3175           
  Misses          479      479           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lukinovec
lukinovec marked this pull request as ready for review September 29, 2026 09:55
@lukinovec lukinovec changed the title [4.x] Make the tenant_route helper replace only the hostname [4.x] Make the tenant_route helper and the domain redirect macro replace only the hostname Sep 29, 2026
@stancl stancl changed the title [4.x] Make the tenant_route helper and the domain redirect macro replace only the hostname [4.x] Only replace hostname in tenant_route helper and the domain redirect macro Sep 30, 2026
@stancl
stancl merged commit 7681326 into master Sep 30, 2026
14 checks passed
@stancl
stancl deleted the correct-tenant-route branch September 30, 2026 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants