fix: allow root cookie domain host redirects by scottmckendry · Pull Request #409 · tinyauthapp/tinyauth

scottmckendry · 2025-10-12T21:02:59Z

Fixes #408

Previously IsRedirectSafe rejected redirects to the exact cookie domain when AppURL had multiple subdomain levels, because it stripped the first label twice.

Summary by CodeRabbit

Bug Fixes
- Improved redirect validation: destinations that exactly match the expected host or derive from its cookie domain are now correctly allowed, fixing root-domain redirect handling and making subdomain decisions more consistent.
Tests
- Expanded test coverage for multi-level subdomains and malformed URLs to verify corrected redirect behavior and edge cases.

coderabbitai · 2025-10-12T21:03:07Z

Walkthrough

Refactors IsRedirectSafe to short-circuit and return true when the redirect host equals the provided domain; otherwise it falls back to GetCookieDomain(redirectURL) and returns true only if that cookie domain equals the domain. Tests updated and a new multi-level domain test added.

Changes

Cohort / File(s)	Summary
Redirect safety logic `internal/utils/app_utils.go`	Updated `IsRedirectSafe` control flow: if parsed `redirectURL` host equals `domain`, return true immediately; otherwise call `GetCookieDomain(redirectURL)` and return true only if that equals `domain`. No exported API changes.
Tests (updated & added) `internal/utils/app_utils_test.go`	Adjusted `TestIsRedirectSafe` expectation for the "no subdomain" case (now true). Added `TestIsRedirectSafeMultiLevel` covering 3rd/4th/5th-level domains, differing subdomain, and malformed URL cases.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller
    participant IsRedirectSafe
    participant Parser
    participant CookieDomain

    Caller->>IsRedirectSafe: IsRedirectSafe(redirectURL, domain)
    IsRedirectSafe->>Parser: parse redirectURL -> host
    Parser-->>IsRedirectSafe: host
    alt host == domain
        IsRedirectSafe-->>Caller: return true
    else host != domain
        IsRedirectSafe->>CookieDomain: GetCookieDomain(redirectURL)
        CookieDomain-->>IsRedirectSafe: cookieDomain / error
        alt cookieDomain == domain
            IsRedirectSafe-->>Caller: return true
        else
            IsRedirectSafe-->>Caller: return false
        end
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I hop and check the host I see,
If it’s the same, I bound with glee.
Else I sniff the cookie trail,
Match it right and I prevail.
A little leap, secure and free. 🥕🐇

Pre-merge checks and finishing touches

❌ 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%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ 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 succinctly describes the primary change—allowing redirects to the root cookie domain—using clear, concise language that directly reflects the behavior fixed in the code.
Linked Issues Check	✅ Passed	This change adjusts IsRedirectSafe to allow exact-domain redirects and adds tests for multi-level domains, directly addressing the inconsistent redirect from 4th-level to 3rd-level described in issue #408 while preserving existing security boundaries against deeper subdomains.
Out of Scope Changes Check	✅ Passed	All modifications are confined to the IsRedirectSafe implementation and its tests in internal/utils/app_utils.go and app_utils_test.go, which directly relate to the bug fix for multi-level redirect domains, with no unrelated code altered.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 036ee13 and abc9fc2.

📒 Files selected for processing (2)

internal/utils/app_utils.go (1 hunks)
internal/utils/app_utils_test.go (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

internal/utils/app_utils.go
internal/utils/app_utils_test.go

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 and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9c1bf8 and 0f33427.

📒 Files selected for processing (1)

internal/utils/app_utils.go (2 hunks)

🔇 Additional comments (1)

internal/utils/app_utils.go (1)

5-5: LGTM! Necessary import for maps.Copy usage.

The maps import is required for the maps.Copy calls on lines 156 and 176. This import was likely missing before, which would have caused compilation issues.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

internal/utils/app_utils_test.go (1)
160-203: Consider adding test for deeper subdomain rejection.

While the existing tests cover the primary scenarios, the past review comment suggested adding a test case for deeper subdomain rejection (e.g., foo.service1.example.com → example.com → false). This would help prevent regression and ensure the cookie domain computation correctly rejects redirects where the computed cookie domain doesn't match.

Add this test case after line 197:
+	// Case with deeper subdomain (should be rejected)
+	redirectURL = "http://deep.sub.example.com/page"
+	result = utils.IsRedirectSafe(redirectURL, domain)
+	assert.Equal(t, false, result)
+

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f33427 and 55ba4e9.

📒 Files selected for processing (2)

internal/utils/app_utils.go (1 hunks)
internal/utils/app_utils_test.go (1 hunks)

🔇 Additional comments (2)

internal/utils/app_utils_test.go (1)

164-167: LGTM! Test expectation correctly updated.

The change from false to true aligns with the new behavior where redirecting to the exact cookie domain host (e.g., example.com → example.com) is now permitted. This correctly reflects the fix for issue #408.

internal/utils/app_utils.go (1)

103-113: Excellent fix for the multi-level domain redirect issue!

The early host equality check (lines 103-106) correctly addresses issue #408 by allowing redirects to the exact cookie domain host. The fallback to cookie domain computation (lines 108-113) maintains safety for subdomain redirects.

Logic verification:

✅ Root cookie domain redirect (cluster.domain.com → cluster.domain.com): Line 104 returns true

✅ Same-level subdomain (service1.cluster.domain.com → cluster.domain.com): Line 113 comparison passes

✅ Malicious domain: GetCookieDomain error causes rejection

scottmckendry · 2025-10-12T21:38:58Z

@steveiliop56 since this is a change in behaviour for what appears to be some kind a security improvement introduced in v4, I've tried not to alter things too much.

However, currently there is still an issue where if you have higher level domains, these will be rejected. E.g. foo.bar.baz.example.com. We could implicitly trust all domain levels off of the host, but I think I need to understand the security implications better before doing so.

Previously IsRedirectSafe rejected redirects to the exact cookie domain when AppURL had multiple subdomain levels, because it stripped the first label twice.

codecov · 2025-10-13T08:54:45Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 24.22%. Comparing base (a9c1bf8) to head (abc9fc2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #409      +/-   ##
==========================================
+ Coverage   24.11%   24.22%   +0.10%     
==========================================
  Files          35       35              
  Lines        2778     2778              
==========================================
+ Hits          670      673       +3     
+ Misses       2072     2070       -2     
+ Partials       36       35       -1

☔ View full report in Codecov by Sentry.
📢 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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

steveiliop56 · 2025-10-13T08:55:51Z

Thank you!

coderabbitai Bot reviewed Oct 12, 2025

View reviewed changes

Comment thread internal/utils/app_utils.go

scottmckendry force-pushed the root-redirects-fix branch 2 times, most recently from 213d0eb to 55ba4e9 Compare October 12, 2025 21:17

coderabbitai Bot reviewed Oct 12, 2025

View reviewed changes

scottmckendry force-pushed the root-redirects-fix branch from 55ba4e9 to 036ee13 Compare October 12, 2025 21:30

fix(redirect): allow root cookie domain host redirects

abc9fc2

Previously IsRedirectSafe rejected redirects to the exact cookie domain when AppURL had multiple subdomain levels, because it stripped the first label twice.

scottmckendry force-pushed the root-redirects-fix branch from 036ee13 to abc9fc2 Compare October 12, 2025 21:42

steveiliop56 approved these changes Oct 13, 2025

View reviewed changes

steveiliop56 merged commit f628d1f into tinyauthapp:main Oct 13, 2025
4 checks passed

Navigation Menu

Repository navigation

fix: allow root cookie domain host redirects - #409

fix: allow root cookie domain host redirects#409
steveiliop56 merged 1 commit into
tinyauthapp:mainfrom
scottmckendry:root-redirects-fix

scottmckendry commented Oct 12, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 12, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

scottmckendry commented Oct 12, 2025

Uh oh!

codecov Bot commented Oct 13, 2025 •

edited

Loading

Uh oh!

Uh oh!

steveiliop56 commented Oct 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

scottmckendry commented Oct 12, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

scottmckendry commented Oct 12, 2025

Uh oh!

codecov Bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

steveiliop56 commented Oct 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

scottmckendry commented Oct 12, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 12, 2025 •

edited

Loading

codecov Bot commented Oct 13, 2025 •

edited

Loading