ldraney/landscaping-assistant
1
0
Fork 0
Code Issues 32 Pull requests 1 Projects Releases Packages Wiki Activity Actions 4

Enable assume_ssl for reverse proxy HTTPS callbacks #156

Merged
ldraney merged 1 commit from 154-enable-assume-ssl-v2 into main 2026-06-07 04:27:00 +00:00
Conversation 2 Commits 1 Files changed 1 +1 -1
ldraney commented 2026-06-07 04:23:55 +00:00
Owner
Copy link

Summary

  • Enable config.assume_ssl so Rails trusts X-Forwarded-Proto from the Tailscale funnel reverse proxy
  • Without this, OmniAuth builds http:// callback URLs which Keycloak rejects as invalid_redirect_uri
  • Supersedes #155 (rebased on main after #153 merge)

Changes

  • config/environments/production.rb: Uncommented config.assume_ssl = true

Test Plan

  • Click "Sign in with Keycloak" shows Keycloak login form (not grey error page)
  • Successful login redirects back to app as authenticated user
  • Super admin sees admin tabs after login

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive
  • Feature flag needed? No — bug fix for production auth

Related Notes

  • ldraney/landscaping-assistant #154 — Fix Keycloak login: enable assume_ssl for reverse proxy
  • landscaping-assistant project

Closes #154

## Summary - Enable `config.assume_ssl` so Rails trusts X-Forwarded-Proto from the Tailscale funnel reverse proxy - Without this, OmniAuth builds `http://` callback URLs which Keycloak rejects as `invalid_redirect_uri` - Supersedes #155 (rebased on main after #153 merge) ## Changes - `config/environments/production.rb`: Uncommented `config.assume_ssl = true` ## Test Plan - [ ] Click "Sign in with Keycloak" shows Keycloak login form (not grey error page) - [ ] Successful login redirects back to app as authenticated user - [ ] Super admin sees admin tabs after login ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No — bug fix for production auth ## Related Notes - `ldraney/landscaping-assistant #154` — Fix Keycloak login: enable assume_ssl for reverse proxy - `landscaping-assistant` project Closes #154
Enable assume_ssl for reverse proxy HTTPS callback URLs
Some checks failed
ci/woodpecker/push/woodpecker Pipeline was successful
Details
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
CI / scan_ruby (pull_request) Has been cancelled
Details
CI / scan_js (pull_request) Has been cancelled
Details
CI / lint (pull_request) Has been cancelled
Details
f4afb86b9c 
The app runs behind a Tailscale funnel that terminates TLS. Without
assume_ssl, Rails ignores X-Forwarded-Proto and OmniAuth builds
http:// callback URLs. Keycloak rejects these as invalid_redirect_uri.

Enabling assume_ssl makes Rails trust the forwarded proto header so
callback URLs use https:// as Keycloak expects.

Closes #154

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-07 04:25:06 +00:00
Author
Owner
Copy link

PR #156 Review

DOMAIN REVIEW

Stack: Ruby on Rails 7.1+ production configuration

config.assume_ssl = true is the correct Rails setting for apps behind an SSL-terminating reverse proxy (Tailscale funnel in this case). It tells ActionDispatch to trust the X-Forwarded-Proto header when generating URLs, which fixes OmniAuth building http:// callback URLs that Keycloak rejects as invalid_redirect_uri.

Key observations:

  • force_ssl remains commented out (line 31), which is correct -- the reverse proxy handles SSL termination, so Rails should not enforce its own HTTPS redirects.
  • The change is a single-line uncomment of a Rails-generated default comment that already describes the exact use case ("Assume all access to the app is happening through a SSL-terminating reverse proxy").
  • No additional configuration is needed -- assume_ssl handles protocol detection via X-Forwarded-Proto out of the box.

BLOCKERS

None.

This is a config-only change (uncommenting a Rails default), not new application logic. No test coverage is required for toggling a framework configuration flag. No user input, secrets, or auth logic is introduced.

NITS

  1. Superseded PR #155 still open -- PR body mentions this PR supersedes #155. Consider closing #155 after this merges to keep the PR list clean.

SOP COMPLIANCE

  • Branch named after issue (154-enable-assume-ssl-v2)
  • PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related Notes)
  • Closes #154 present
  • No secrets committed
  • No unnecessary file changes (1 file, 1 line)
  • Commit messages are descriptive
  • No plan slug expected (bug fix, not a planned feature)

PROCESS OBSERVATIONS

Minimal-blast-radius fix for a production auth regression. The change is well-scoped and low-risk. The manual test plan (click Sign in, verify Keycloak login form, verify authenticated redirect) is appropriate for a config change that cannot be unit tested. Superseding #155 with a rebased branch rather than force-pushing is good practice.

VERDICT: APPROVED

## PR #156 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails 7.1+ production configuration `config.assume_ssl = true` is the correct Rails setting for apps behind an SSL-terminating reverse proxy (Tailscale funnel in this case). It tells ActionDispatch to trust the `X-Forwarded-Proto` header when generating URLs, which fixes OmniAuth building `http://` callback URLs that Keycloak rejects as `invalid_redirect_uri`. Key observations: - `force_ssl` remains commented out (line 31), which is correct -- the reverse proxy handles SSL termination, so Rails should not enforce its own HTTPS redirects. - The change is a single-line uncomment of a Rails-generated default comment that already describes the exact use case ("Assume all access to the app is happening through a SSL-terminating reverse proxy"). - No additional configuration is needed -- `assume_ssl` handles protocol detection via `X-Forwarded-Proto` out of the box. ### BLOCKERS None. This is a config-only change (uncommenting a Rails default), not new application logic. No test coverage is required for toggling a framework configuration flag. No user input, secrets, or auth logic is introduced. ### NITS 1. **Superseded PR #155 still open** -- PR body mentions this PR supersedes #155. Consider closing #155 after this merges to keep the PR list clean. ### SOP COMPLIANCE - [x] Branch named after issue (`154-enable-assume-ssl-v2`) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related Notes) - [x] `Closes #154` present - [x] No secrets committed - [x] No unnecessary file changes (1 file, 1 line) - [x] Commit messages are descriptive - [x] No plan slug expected (bug fix, not a planned feature) ### PROCESS OBSERVATIONS Minimal-blast-radius fix for a production auth regression. The change is well-scoped and low-risk. The manual test plan (click Sign in, verify Keycloak login form, verify authenticated redirect) is appropriate for a config change that cannot be unit tested. Superseding #155 with a rebased branch rather than force-pushing is good practice. ### VERDICT: APPROVED
ldraney commented 2026-06-07 04:26:12 +00:00
Author
Owner
Copy link

PR #156 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails (production environment configuration)

Change: Uncomments config.assume_ssl = true in config/environments/production.rb (1 addition, 1 deletion, 1 file).

Correctness: This is the correct fix. When Rails runs behind an SSL-terminating reverse proxy (Tailscale funnel in this case), assume_ssl tells Rails to trust X-Forwarded-Proto headers. Without it, request.protocol returns http://, causing OmniAuth to build http:// callback URLs that Keycloak rightfully rejects as invalid_redirect_uri. This is a well-documented Rails 7.1+ pattern for reverse proxy deployments.

Relationship with force_ssl: Note that config.force_ssl (line 31) remains commented out. This is acceptable -- assume_ssl handles protocol detection without forcing HTTPS redirects, which could conflict with the reverse proxy's own TLS termination. The two settings serve different purposes and assume_ssl alone is sufficient here.

Security: No concerns. This change improves auth correctness without introducing any attack surface. The setting only affects how Rails interprets forwarded headers, which is appropriate for a reverse-proxy deployment.

Test coverage: This is a one-line config uncomment that fixes a production auth flow. There is no new functionality to unit test -- the behavior is validated by the manual test plan (Keycloak login round-trip). The existing OmniAuth callback specs in the test suite cover the auth flow logic itself. No BLOCKER.

BLOCKERS

None.

NITS

  1. PR #155 (the superseded PR) appears to still be open. Consider closing it after this one merges to avoid stale PRs accumulating.
  2. Line 70 in production.rb still has the Rails scaffold default config.action_mailer.default_url_options = { host: "example.com" } -- not related to this PR, but worth a future cleanup ticket if mailer is ever used.

SOP COMPLIANCE

  • Branch named after issue: 154-enable-assume-ssl-v2 follows {issue-number}-{kebab-case-purpose} convention
  • PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present
  • Related references parent issue: ldraney/landscaping-assistant #154 and Closes #154 present
  • No secrets committed: diff contains no credentials or env values
  • No unnecessary file changes: exactly 1 file, 1 line -- surgically scoped
  • Commit messages: PR title is descriptive and accurate

PROCESS OBSERVATIONS

  • This is the second attempt (v2) after #155, rebased on main after #153 merged. Clean process -- new branch rather than force-pushing.
  • Minimal blast radius: single config line change with clear causal chain (reverse proxy -> http:// callback -> Keycloak rejection). Low change failure risk.
  • The manual test plan is appropriate for this type of infrastructure config fix. Automated integration tests for the full Keycloak redirect flow would require a running IdP, which is out of scope here.

VERDICT: APPROVED

## PR #156 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails (production environment configuration) **Change**: Uncomments `config.assume_ssl = true` in `config/environments/production.rb` (1 addition, 1 deletion, 1 file). **Correctness**: This is the correct fix. When Rails runs behind an SSL-terminating reverse proxy (Tailscale funnel in this case), `assume_ssl` tells Rails to trust `X-Forwarded-Proto` headers. Without it, `request.protocol` returns `http://`, causing OmniAuth to build `http://` callback URLs that Keycloak rightfully rejects as `invalid_redirect_uri`. This is a well-documented Rails 7.1+ pattern for reverse proxy deployments. **Relationship with `force_ssl`**: Note that `config.force_ssl` (line 31) remains commented out. This is acceptable -- `assume_ssl` handles protocol detection without forcing HTTPS redirects, which could conflict with the reverse proxy's own TLS termination. The two settings serve different purposes and `assume_ssl` alone is sufficient here. **Security**: No concerns. This change improves auth correctness without introducing any attack surface. The setting only affects how Rails interprets forwarded headers, which is appropriate for a reverse-proxy deployment. **Test coverage**: This is a one-line config uncomment that fixes a production auth flow. There is no new functionality to unit test -- the behavior is validated by the manual test plan (Keycloak login round-trip). The existing OmniAuth callback specs in the test suite cover the auth flow logic itself. No BLOCKER. ### BLOCKERS None. ### NITS 1. PR #155 (the superseded PR) appears to still be open. Consider closing it after this one merges to avoid stale PRs accumulating. 2. Line 70 in `production.rb` still has the Rails scaffold default `config.action_mailer.default_url_options = { host: "example.com" }` -- not related to this PR, but worth a future cleanup ticket if mailer is ever used. ### SOP COMPLIANCE - [x] Branch named after issue: `154-enable-assume-ssl-v2` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present - [x] Related references parent issue: `ldraney/landscaping-assistant #154` and `Closes #154` present - [x] No secrets committed: diff contains no credentials or env values - [x] No unnecessary file changes: exactly 1 file, 1 line -- surgically scoped - [x] Commit messages: PR title is descriptive and accurate ### PROCESS OBSERVATIONS - This is the second attempt (v2) after #155, rebased on main after #153 merged. Clean process -- new branch rather than force-pushing. - Minimal blast radius: single config line change with clear causal chain (reverse proxy -> http:// callback -> Keycloak rejection). Low change failure risk. - The manual test plan is appropriate for this type of infrastructure config fix. Automated integration tests for the full Keycloak redirect flow would require a running IdP, which is out of scope here. ### VERDICT: APPROVED
ldraney deleted branch 154-enable-assume-ssl-v2 2026-06-07 04:27:00 +00:00
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
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!156
No description provided.