Fix OmniAuth redirect_uri to resolve dynamically from request host (Closes #246) #247

Merged

ldraney merged 2 commits from fix/246-omniauth-redirect-uri into main 2026-06-17 12:30:09 +00:00

Conversation 2 Commits 2 Files changed 1 +14 -3

ldraney commented 2026-06-17 12:11:01 +00:00

Owner

Copy link

Summary

Replace the hardcoded APP_URL-based redirect_uri in the OmniAuth initializer with a dynamic setup lambda that computes the callback URL from the incoming request's scheme and host at request time. This fixes Keycloak login when accessing the app via landscaping-assistant.app -- previously the redirect_uri always pointed to the Tailscale hostname fallback.

Changes

• config/initializers/omniauth.rb -- Removed static redirect_uri from client_options and added an OmniAuth setup lambda that sets redirect_uri dynamically using Rack::Request#scheme and #host_with_port from the current request

Test Plan

• All 9 session request specs pass (OmniAuth setup phase fires correctly per debug logs)
• After deploy, verify login via landscaping-assistant.app produces a redirect_uri pointing to landscaping-assistant.app/auth/keycloak/callback
• Verify login via landscaping-assistant.tail5b443a.ts.net still works with the Tailscale hostname in the callback

Review Checklist

• Single file change, minimal diff
• No new dependencies
• Existing tests pass
• Both public domain and Tailscale hostname will produce correct redirect_uri

Related

Closes #246

Related Notes

None

## Summary Replace the hardcoded `APP_URL`-based `redirect_uri` in the OmniAuth initializer with a dynamic setup lambda that computes the callback URL from the incoming request's scheme and host at request time. This fixes Keycloak login when accessing the app via `landscaping-assistant.app` -- previously the redirect_uri always pointed to the Tailscale hostname fallback. ## Changes - `config/initializers/omniauth.rb` -- Removed static `redirect_uri` from `client_options` and added an OmniAuth `setup` lambda that sets `redirect_uri` dynamically using `Rack::Request#scheme` and `#host_with_port` from the current request ## Test Plan - All 9 session request specs pass (OmniAuth setup phase fires correctly per debug logs) - After deploy, verify login via `landscaping-assistant.app` produces a `redirect_uri` pointing to `landscaping-assistant.app/auth/keycloak/callback` - Verify login via `landscaping-assistant.tail5b443a.ts.net` still works with the Tailscale hostname in the callback ## Review Checklist - [x] Single file change, minimal diff - [x] No new dependencies - [x] Existing tests pass - [x] Both public domain and Tailscale hostname will produce correct redirect_uri ## Related Closes #246 ## Related Notes None

ldraney added 1 commit 2026-06-17 12:11:01 +00:00

Fix OmniAuth redirect_uri to resolve dynamically from request host ... 048fe86da0

Replace the hardcoded APP_URL-based redirect_uri with an OmniAuth setup
lambda that computes the callback URL from the incoming request's scheme
and host. This ensures login works correctly whether the app is accessed
via landscaping-assistant.app or the Tailscale hostname.

Closes #246

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-17 12:12:27 +00:00

Author

Owner

Copy link

PR #247 Review

DOMAIN REVIEW

Stack: Ruby on Rails, OmniAuth (OpenID Connect), Rack, Keycloak

This PR replaces a hardcoded redirect_uri (built from ENV['APP_URL'] with a Tailscale fallback) with a dynamic setup lambda that derives the callback URL from Rack::Request#scheme and #host_with_port at request time. This is the correct OmniAuth pattern for multi-host environments.

Host Header Injection (OWASP):
Constructing redirect_uri from the request's Host header is a well-known attack surface. An attacker could send a forged Host header to redirect the OAuth callback to an attacker-controlled domain. However, this PR is safe because of two independent mitigations already in place:

1. Rails config.hosts allowlist -- PRs #230, #232, and #239 confirm that HostAuthorization is configured. Rails rejects requests with unrecognized Host headers with a 403 before the OmniAuth middleware fires.
2. Keycloak redirect_uri validation -- Keycloak validates the redirect_uri against its configured allowed redirect URIs. Even if a forged Host somehow bypassed Rails, Keycloak would reject the authorization request.

These two layers make the dynamic approach safe for this deployment.

OmniAuth setup lambda: The implementation follows the documented OmniAuth setup-phase pattern correctly. Accessing env["omniauth.strategy"].options[:client_options][:redirect_uri] is the standard way to dynamically configure options per-request.

BLOCKERS

None.

Test coverage assessment: This is a config-only change (+13/-3) to an OmniAuth initializer. The 9 existing session request specs verify the auth flow end-to-end. Writing a unit test for the setup lambda in isolation would require mocking Rack env and OmniAuth internals with low marginal value. The real verification is the post-deploy check described in the Test Plan. No blocker here.

NITS

1. Comment precision: The new comment block (lines 10-13) says "without a hardcoded APP_URL fallback" -- the old code did not just fall back to APP_URL, it always used APP_URL (with a Tailscale default if APP_URL was unset). Minor wording nit, not blocking.

2. Trailing comma consistency: The diff removes the comma after secret: and the old redirect_uri line. The new structure places callback_path and setup as sibling keys to client_options. Formatting looks correct; just noting the structural change is clean.

SOP COMPLIANCE

• PR body has Summary, Changes, Test Plan, Related
• No secrets committed (ENV.fetch used for credentials, no hardcoded values)
• Single file change -- no scope creep
• Commit message is descriptive (references issue #246)
• Branch naming follows convention (fix/246-omniauth-redirect-uri)

PROCESS OBSERVATIONS

• This is the third host-related fix in recent history (#230 added the domain to config.hosts, #232/#239 fixed HostAuthorization for k8s probes, now #247 fixes OmniAuth redirect_uri). The custom domain rollout has been incremental, with each layer surfacing the next issue. This is normal for multi-ingress setups but worth noting: after this merges, the entire public-domain auth chain should be end-to-end functional.
• Change failure risk is low -- the setup lambda is a well-documented OmniAuth pattern, and the fallback behavior (Tailscale hostname) continues to work because the lambda reads whatever host the request arrives on.

VERDICT: APPROVED

## PR #247 Review ### DOMAIN REVIEW **Stack**: Ruby on Rails, OmniAuth (OpenID Connect), Rack, Keycloak This PR replaces a hardcoded `redirect_uri` (built from `ENV['APP_URL']` with a Tailscale fallback) with a dynamic `setup` lambda that derives the callback URL from `Rack::Request#scheme` and `#host_with_port` at request time. This is the correct OmniAuth pattern for multi-host environments. **Host Header Injection (OWASP)**: Constructing `redirect_uri` from the request's Host header is a well-known attack surface. An attacker could send a forged Host header to redirect the OAuth callback to an attacker-controlled domain. However, this PR is safe because of two independent mitigations already in place: 1. **Rails `config.hosts` allowlist** -- PRs #230, #232, and #239 confirm that `HostAuthorization` is configured. Rails rejects requests with unrecognized Host headers with a 403 *before* the OmniAuth middleware fires. 2. **Keycloak redirect_uri validation** -- Keycloak validates the `redirect_uri` against its configured allowed redirect URIs. Even if a forged Host somehow bypassed Rails, Keycloak would reject the authorization request. These two layers make the dynamic approach safe for this deployment. **OmniAuth `setup` lambda**: The implementation follows the documented OmniAuth setup-phase pattern correctly. Accessing `env["omniauth.strategy"].options[:client_options][:redirect_uri]` is the standard way to dynamically configure options per-request. ### BLOCKERS None. **Test coverage assessment**: This is a config-only change (+13/-3) to an OmniAuth initializer. The 9 existing session request specs verify the auth flow end-to-end. Writing a unit test for the setup lambda in isolation would require mocking Rack env and OmniAuth internals with low marginal value. The real verification is the post-deploy check described in the Test Plan. No blocker here. ### NITS 1. **Comment precision**: The new comment block (lines 10-13) says "without a hardcoded APP_URL fallback" -- the old code did not just fall back to APP_URL, it *always* used APP_URL (with a Tailscale default if APP_URL was unset). Minor wording nit, not blocking. 2. **Trailing comma consistency**: The diff removes the comma after `secret:` and the old `redirect_uri` line. The new structure places `callback_path` and `setup` as sibling keys to `client_options`. Formatting looks correct; just noting the structural change is clean. ### SOP COMPLIANCE - [x] PR body has Summary, Changes, Test Plan, Related - [x] No secrets committed (ENV.fetch used for credentials, no hardcoded values) - [x] Single file change -- no scope creep - [x] Commit message is descriptive (references issue #246) - [x] Branch naming follows convention (`fix/246-omniauth-redirect-uri`) ### PROCESS OBSERVATIONS - This is the third host-related fix in recent history (#230 added the domain to config.hosts, #232/#239 fixed HostAuthorization for k8s probes, now #247 fixes OmniAuth redirect_uri). The custom domain rollout has been incremental, with each layer surfacing the next issue. This is normal for multi-ingress setups but worth noting: after this merges, the entire public-domain auth chain should be end-to-end functional. - Change failure risk is low -- the setup lambda is a well-documented OmniAuth pattern, and the fallback behavior (Tailscale hostname) continues to work because the lambda reads whatever host the request arrives on. ### VERDICT: APPROVED

ldraney commented 2026-06-17 12:18:33 +00:00

Author

Owner

Copy link

PR #247 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails, OmniAuth (OpenID Connect strategy), Keycloak IdP.

What this PR does: Replaces a static redirect_uri (built from ENV['APP_URL'] with a hardcoded Tailscale fallback) with an OmniAuth setup lambda that resolves the callback URL dynamically from Rack::Request#scheme and #host_with_port. This is the canonical OmniAuth pattern for multi-host deployments and correctly solves the problem described in #246.

Security analysis -- open redirect via Host header?
The setup lambda trusts the incoming request's Host header to build the redirect_uri. In isolation, this would be an open-redirect risk: an attacker could forge a Host: evil.com header and the callback would redirect to evil.com. However, Rails config.hosts (added in PR #230, hardened in PR #239) rejects requests with unrecognized Host headers before they reach OmniAuth middleware. This makes the approach safe -- HostAuthorization is the gate, and the setup lambda only runs for allowed hosts. No BLOCKER here, but this dependency is worth documenting (see nit below).

OmniAuth pattern correctness: The setup lambda receives the Rack env, constructs a Rack::Request, and sets strategy.options[:client_options][:redirect_uri]. This is the documented OmniAuth setup phase pattern. The lambda fires before the authorization request is built, so the redirect_uri is correct for both the authorize redirect and the token exchange.

APP_URL env var: The PR removes the only consumer of APP_URL in this file. If APP_URL is no longer used elsewhere, it can be removed from deployment manifests / env configuration to avoid confusion. Not a blocker.

BLOCKERS

None.

The setup lambda is not directly unit-testable because OmniAuth.config.test_mode = true bypasses the setup phase entirely. This is an OmniAuth framework limitation, not a coverage gap in this PR. The Test Plan correctly identifies that post-deploy verification against both landscaping-assistant.app and the Tailscale hostname is the real validation path. The 9 existing session request specs continue to pass and cover callback handling, session creation, role extraction, logout, and failure handling.

NITS

1. Document the config.hosts dependency: The security of this approach depends on HostAuthorization rejecting spoofed Host headers upstream. Consider adding a one-line comment in the setup lambda noting this dependency, e.g.:

   # Safe to trust request.host_with_port because Rails config.hosts
   # rejects unrecognised Host headers before OmniAuth fires.
   

2. APP_URL cleanup: If APP_URL is no longer referenced anywhere else, consider removing it from deployment env config (k8s manifests, .env.example, etc.) to avoid confusion about whether it is still needed.

3. Trailing comma consistency: The callback_path line now ends without a trailing comma before setup:, which is fine for the last-before-block position. No action needed, just noting the style is consistent.

SOP COMPLIANCE

• PR body has Summary, Changes, Test Plan, Related sections
• Single file change, tightly scoped to issue #246
• No secrets, .env files, or credentials committed
• No unnecessary file changes (zero scope creep)
• Commit message is descriptive and references the issue
• Existing tests pass per PR description (9 session request specs)

PROCESS OBSERVATIONS

• Change failure risk: Low. Single-file, 13-line change to an auth initializer. The logic is straightforward string interpolation from request metadata. Rollback is trivial (revert to static redirect_uri).
• Deployment frequency: No impact. This is a targeted bugfix.
• Documentation: The inline comment added at lines 10-13 of the diff is good. The Test Plan correctly identifies that real validation requires post-deploy testing against both hostnames.

VERDICT: APPROVED

## PR #247 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails, OmniAuth (OpenID Connect strategy), Keycloak IdP. **What this PR does**: Replaces a static `redirect_uri` (built from `ENV['APP_URL']` with a hardcoded Tailscale fallback) with an OmniAuth `setup` lambda that resolves the callback URL dynamically from `Rack::Request#scheme` and `#host_with_port`. This is the canonical OmniAuth pattern for multi-host deployments and correctly solves the problem described in #246. **Security analysis -- open redirect via Host header?** The setup lambda trusts the incoming request's `Host` header to build the `redirect_uri`. In isolation, this would be an open-redirect risk: an attacker could forge a `Host: evil.com` header and the callback would redirect to `evil.com`. However, Rails `config.hosts` (added in PR #230, hardened in PR #239) rejects requests with unrecognized `Host` headers before they reach OmniAuth middleware. This makes the approach safe -- `HostAuthorization` is the gate, and the setup lambda only runs for allowed hosts. No BLOCKER here, but this dependency is worth documenting (see nit below). **OmniAuth pattern correctness**: The `setup` lambda receives the Rack env, constructs a `Rack::Request`, and sets `strategy.options[:client_options][:redirect_uri]`. This is the documented OmniAuth setup phase pattern. The lambda fires before the authorization request is built, so the `redirect_uri` is correct for both the authorize redirect and the token exchange. **`APP_URL` env var**: The PR removes the only consumer of `APP_URL` in this file. If `APP_URL` is no longer used elsewhere, it can be removed from deployment manifests / env configuration to avoid confusion. Not a blocker. ### BLOCKERS None. The setup lambda is not directly unit-testable because `OmniAuth.config.test_mode = true` bypasses the setup phase entirely. This is an OmniAuth framework limitation, not a coverage gap in this PR. The Test Plan correctly identifies that post-deploy verification against both `landscaping-assistant.app` and the Tailscale hostname is the real validation path. The 9 existing session request specs continue to pass and cover callback handling, session creation, role extraction, logout, and failure handling. ### NITS 1. **Document the `config.hosts` dependency**: The security of this approach depends on `HostAuthorization` rejecting spoofed `Host` headers upstream. Consider adding a one-line comment in the setup lambda noting this dependency, e.g.: ```ruby # Safe to trust request.host_with_port because Rails config.hosts # rejects unrecognised Host headers before OmniAuth fires. ``` 2. **`APP_URL` cleanup**: If `APP_URL` is no longer referenced anywhere else, consider removing it from deployment env config (k8s manifests, `.env.example`, etc.) to avoid confusion about whether it is still needed. 3. **Trailing comma consistency**: The `callback_path` line now ends without a trailing comma before `setup:`, which is fine for the last-before-block position. No action needed, just noting the style is consistent. ### SOP COMPLIANCE - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Single file change, tightly scoped to issue #246 - [x] No secrets, `.env` files, or credentials committed - [x] No unnecessary file changes (zero scope creep) - [x] Commit message is descriptive and references the issue - [x] Existing tests pass per PR description (9 session request specs) ### PROCESS OBSERVATIONS - **Change failure risk**: Low. Single-file, 13-line change to an auth initializer. The logic is straightforward string interpolation from request metadata. Rollback is trivial (revert to static `redirect_uri`). - **Deployment frequency**: No impact. This is a targeted bugfix. - **Documentation**: The inline comment added at lines 10-13 of the diff is good. The Test Plan correctly identifies that real validation requires post-deploy testing against both hostnames. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-17 12:27:47 +00:00

Add config.hosts safety comment on setup lambda ... 8e497e90fe

QA nit: document that Rails config.hosts rejects spoofed Host headers
before the OmniAuth setup phase fires, preventing open-redirect risk.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney merged commit 4a37ed7bef into main 2026-06-17 12:30:09 +00:00

ldraney deleted branch fix/246-omniauth-redirect-uri 2026-06-17 12:30:09 +00:00

ldraney referenced this pull request from a commit 2026-06-17 12:30:10 +00:00

Fix OmniAuth redirect_uri to resolve dynamically from request host (Closes #246) (#247)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/landscaping-assistant!247

No description provided.