feat: add TINYAUTH_AUTH_SINGLECOOKIEDOMAIN option (and fix release workflow)#710
feat: add TINYAUTH_AUTH_SINGLECOOKIEDOMAIN option (and fix release workflow)#710jacekkow wants to merge 1 commit intosteveiliop56:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe changes introduce a standalone authentication mode by adding a configuration option and implementing a strategy pattern for cookie domain resolution. When enabled, the system uses simplified hostname extraction instead of default domain validation, logging that proxied app automatic authentication will not work. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. 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
from
6465057 to
f66ddf9
Compare
Allows to use Tinyauth on top-level domain only, but forbids automatic cross-app authentication using Traefik or Nginx.
jacekkow
force-pushed
the
single-cookie-domain
branch
from
f66ddf9 to
1848a7a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #710 +/- ##
==========================================
- Coverage 16.85% 16.81% -0.04%
==========================================
Files 50 50
Lines 3820 3829 +9
==========================================
Hits 644 644
- Misses 3112 3121 +9
Partials 64 64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 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
standaloneconfiguration option for authentication settings, enabling alternative cookie domain handling behavior.