feat: add option to run tinyauth on a top-level domain#710
feat: add option to run tinyauth on a top-level domain#710jacekkow wants to merge 4 commits intotinyauthapp:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRuntime cookie-domain selection now respects a new Changes
Sequence Diagram(s)sequenceDiagram
participant Bootstrap as BootstrapApp
participant Config as Auth Config
participant Utils as utils
Bootstrap->>Config: read Auth.SubdomainsEnabled
alt SubdomainsEnabled == true
Bootstrap->>Utils: GetCookieDomain(app.context.appUrl)
else SubdomainsEnabled == false
Bootstrap->>Utils: GetStandaloneCookieDomain(app.context.appUrl)
end
Utils-->>Bootstrap: cookieDomain / error
Bootstrap-->>Bootstrap: set app.context.cookieDomain or return error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/utils/app_utils.go (1)
46-53: Consider validating empty hostname edge case.Unlike
GetCookieDomain, this function doesn't validate for IP addresses or empty hostnames. While the relaxed validation may be intentional for single-domain mode,url.Parsecan return an empty hostname for malformed URLs (e.g.,"not-a-url"), which would silently return an empty string instead of an error.💡 Optional: Add minimal validation
func GetCookieSingleDomain(u string) (string, error) { parsed, err := url.Parse(u) if err != nil { return "", err } + hostname := parsed.Hostname() + if hostname == "" { + return "", errors.New("invalid URL: empty hostname") + } + - return parsed.Hostname(), nil + return hostname, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/utils/app_utils.go` around lines 46 - 53, GetCookieSingleDomain currently returns parsed.Hostname() even when url.Parse yields an empty hostname (e.g., for malformed inputs); update GetCookieSingleDomain to validate the parsed.Hostname() result after calling url.Parse and return a descriptive error if the hostname is empty (or otherwise invalid), so callers receive an error instead of an empty string; reference the GetCookieSingleDomain function, the url.Parse call and parsed.Hostname() usage when implementing this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/utils/app_utils.go`:
- Around line 46-53: GetCookieSingleDomain currently returns parsed.Hostname()
even when url.Parse yields an empty hostname (e.g., for malformed inputs);
update GetCookieSingleDomain to validate the parsed.Hostname() result after
calling url.Parse and return a descriptive error if the hostname is empty (or
otherwise invalid), so callers receive an error instead of an empty string;
reference the GetCookieSingleDomain function, the url.Parse call and
parsed.Hostname() usage when implementing this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c33fc343-de8e-419c-a237-26836edd6bc4
📒 Files selected for processing (4)
.github/workflows/release.ymlinternal/bootstrap/app_bootstrap.gointernal/config/config.gointernal/utils/app_utils.go
💤 Files with no reviewable changes (1)
- .github/workflows/release.yml
|
#724 related @jacekkow I believe the correct approach would be to automatically detect if we are running on a non-subdomain app URL. On start-up check what |
|
@steveiliop56 - that is not my use case. I want to use this app as a standalone component (auth server). I do not want it to set cookies for any other domain (esp. wildcard ones). Secured applications use OIDC and are at completely different URLs. |
|
@jacekkow that's fine, it will not set cookies if you don't use the authentication feature. The idea is to just automatically handle root domains instead of needing another config option. Your pull request basically does this. Are you interested in implementing said feature or should I do it? |
OIDC login does set cookies.
My instance of Tinyauth is not hosted on root domain! So unless you've missed something my use case is different and requires a different solution than an algorithm from your first comment (#710 (comment)). |
My brain was not thinking when I wrote this. ..for the rest.. Yes you are correct again, I thought your instance was on the root. Anyway in that case I guess we could have such option. But maybe under a different name? |
|
@steveiliop56 Sure - naming can be changed. What I also considered is having an option to just set arbitrary "cookie domain" ( What would you prefer? Renaming "my option" to |
|
I think the standalone idea is nice, let's change it to that. As for a cookie domain option, I think it's better to avoid adding it because it can and probably will cause confusion. Also maybe we should add a small info log saying that authentication will not work for proxied apps with this option enabled (again to avoid future issues). Small note: This pull request is "queued" for v5.1.0 which will come later. I am focusing on fixing some issues in a patch release and then we can start adding new features. |
|
@steveiliop56 Sure, no problem, I can work with a fork for now. BTW - would you like to have commit 6465057 submitted as a separate PR for the fix release? |
|
Was planning to fix this with #715, I noticed it there too. But yeah feel free to create a pull request - please check if there is any nightly leftover anywhere else -. |
|
Haven't found any more of those. PR: #725 |
jacekkow
force-pushed
the
single-cookie-domain
branch
2 times, most recently
from
f66ddf9 to
1848a7a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #710 +/- ##
==========================================
- Coverage 20.05% 20.00% -0.06%
==========================================
Files 50 50
Lines 3995 4005 +10
==========================================
Hits 801 801
- Misses 3121 3131 +10
Partials 73 73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I don't like the naming of this option - it doesn't explain exactly what it does and has connotations that don't match, instead how about using the same pattern as analytics with a default-true value that explains it directly and call it
(The option itself is awesome though!) |
|
@steveiliop56 - should I do one more rename to |
Setting it to false allows to use Tinyauth on top-level domain only, but forbids automatic cross-app authentication using Traefik/Nginx.
jacekkow
force-pushed
the
single-cookie-domain
branch
from
1848a7a to
d90e3d6
Compare
dosubot
Bot
added
the
size:S
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/bootstrap/app_bootstrap.go (1)
109-121:⚠️ Potential issue | 🟠 MajorWhen subdomains are disabled, cookies must be host-only to prevent automatic authentication on subdomains.
Line 115 calls
GetStandaloneCookieDomain, which returns the hostname forhttps://example.comasexample.com. This is then formatted with a leading dot in all cookie setters (fmt.Sprintf(".%s", config.CookieDomain)), producingDomain=.example.com. Per RFC 6265, this allows browsers to send the cookie to all subdomains, defeating the intent of disabling subdomain authentication. SetcookieDomainto an empty string in standalone mode, which will result in an invalid/empty domain attribute that browsers treat as host-only cookies.Proposed fix
- cookieDomainResolver := utils.GetCookieDomain - if !app.config.Auth.SubdomainsEnabled { - tlog.App.Info().Msg("Subdomains disabled, automatic authentication for proxied apps will not work") - cookieDomainResolver = utils.GetStandaloneCookieDomain - } - - cookieDomain, err := cookieDomainResolver(app.context.appUrl) - - if err != nil { - return err - } - - app.context.cookieDomain = cookieDomain + if app.config.Auth.SubdomainsEnabled { + cookieDomain, err := utils.GetCookieDomain(app.context.appUrl) + if err != nil { + return err + } + app.context.cookieDomain = cookieDomain + } else { + tlog.App.Info().Msg("Subdomains disabled, cookies will be host-only and automatic authentication for proxied apps will not work") + app.context.cookieDomain = "" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/app_bootstrap.go` around lines 109 - 121, The code currently calls GetStandaloneCookieDomain via cookieDomainResolver when app.config.Auth.SubdomainsEnabled is false, which produces a host name that later gets formatted as ".%s" and enables subdomain cookies; change the logic so that when SubdomainsEnabled is false you do not call GetStandaloneCookieDomain but instead set cookieDomain to the empty string (""), then assign app.context.cookieDomain = "" so cookie setters will omit a domain attribute and the browser will treat cookies as host-only; update the conditional around cookieDomainResolver/app.context.cookieDomain to reflect this behavior and keep GetCookieDomain used only when SubdomainsEnabled is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/bootstrap/app_bootstrap.go`:
- Around line 109-121: The code currently calls GetStandaloneCookieDomain via
cookieDomainResolver when app.config.Auth.SubdomainsEnabled is false, which
produces a host name that later gets formatted as ".%s" and enables subdomain
cookies; change the logic so that when SubdomainsEnabled is false you do not
call GetStandaloneCookieDomain but instead set cookieDomain to the empty string
(""), then assign app.context.cookieDomain = "" so cookie setters will omit a
domain attribute and the browser will treat cookies as host-only; update the
conditional around cookieDomainResolver/app.context.cookieDomain to reflect this
behavior and keep GetCookieDomain used only when SubdomainsEnabled is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f640cb00-ea72-4e72-ab1b-df5cfff1b5a7
📒 Files selected for processing (3)
internal/bootstrap/app_bootstrap.gointernal/config/config.gointernal/utils/app_utils.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/utils/app_utils.go
- internal/config/config.go
|
I have changed the naming. Coderabbit is actually correct that |
|
@jacekkow if you can also fix that, it would be amazing. You should be able to do it by either passing the cookie domain already formatted so as the controllers don't have to append anything or by adding a separate option in the controller configuration. I would honestly prefer the first one. |
dosubot
Bot
added
size:M
|
@steveiliop56 - thanks for taking that on - I'd have time only during next weekend. BTW. Should maybe |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/oauth_controller.go`:
- Line 30: The OAuthControllerConfig gains a SubdomainsEnabled field but the
controller creation (the OAuthControllerConfig struct literal in the router
bootstrap) never sets it, so getCookieDomain() will never apply subdomain
prefixes; update the struct literal that constructs OAuthControllerConfig to
include SubdomainsEnabled: cfg.SubdomainsEnabled (or the appropriate app config
boolean) so the controller receives the flag and getCookieDomain() can behave
correctly.
🪄 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: e59be7f4-cc6e-4bf5-bef5-d6e95ab95353
📒 Files selected for processing (6)
internal/bootstrap/app_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/config/config.gointernal/controller/oauth_controller.gointernal/service/auth_service.gointernal/utils/app_utils.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/utils/app_utils.go
- internal/config/config.go
|
@steveiliop56 - thanks for taking that on. BTW. Should |
|
@jacekkow don't worry I will take it over from here, meaning we merge because nothing is left.
Most probably yes. But let's leave it as-is for now since I would like to rework the cookie and context logic later. |
steveiliop56
changed the title
| ) | ||
|
|
||
| // Get cookie domain parses a hostname and returns the upper domain (e.g. sub1.sub2.domain.com -> sub2.domain.com) | ||
| func GetCookieDomain(u string) (string, error) { |
There was a problem hiding this comment.
Am I tripping or do we now have three implementations of getCookieDomain? This will cause a bug at some point in the future, I'm sure of it.
There was a problem hiding this comment.
Also, this one has test coverage, the other two do not.
| return domain, nil | ||
| } | ||
|
|
||
| func GetStandaloneCookieDomain(u string) (string, error) { |
There was a problem hiding this comment.
This probably needs tests to cover the scenarios it may be used in.
4 participants
It allows to set-up Tinyauth on top-level domain or to have it as a standalone OIDC server, without transparent authentication for apps in subdomains.
Additionally fix issue in release workflow where metadata (
VERSION,COMMIT_HASH,BUILD_TIMESTAMP) is fetched fromnightlytag rather than from the relevantv...tag.Summary by CodeRabbit