feat(auth): allow login via reverse-proxy email header#1685
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:
📝 WalkthroughWalkthroughAdds optional reverse-proxy header SSO (trusted-header JIT user creation, session handling, proxy allowlist) and a new API surface for balance history (index/show) with controller, views, OpenAPI schemas, routes, pagination, filters, NDJSON export of balances, and many tests. Also includes small model/controller validation tweaks and related docs/initializers. ChangesRemote Header-Based SSO
API V1 Balances (Controller, Views, Routes, OpenAPI, Specs)
Data Export: Balance NDJSON
Validation, Small Behavior & Tests
Sequence DiagramsequenceDiagram
actor Client
participant Proxy as Reverse Proxy
participant Rails as Rails App
participant Auth as Authentication Concern
participant User as User Model
participant Audit as SsoAuditLog
Client->>Proxy: HTTP request
Proxy->>Rails: request + Remote-Email header
Rails->>Auth: authenticate_user!()
Auth->>Auth: read config.remote_user_header_email & trusted_proxies
Auth->>Auth: verify proxy IP trusted
Auth->>Auth: extract & validate header email
Auth->>User: find_or_create_remote_header_user(email)
User-->>Auth: return user (found or created)
Auth->>Auth: create or reuse session (invalidate cookie if mismatch)
Auth->>Audit: log creation/login with REMOTE_HEADER_SSO_PROVIDER
Auth-->>Rails: set Current.session
Rails-->>Client: respond (authenticated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
superagent-security
Bot
added
contributor:flagged
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ea63b400a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| user.email = user_email | ||
| user.skip_password_validation = true | ||
| user.family = Family.new |
There was a problem hiding this comment.
This JIT path creates users with password_digest unset, but it does not attach any marker that makes them sso_only?; User#sso_only? currently requires an OIDC identity, so these accounts are treated as resettable local accounts in PasswordResetsController. In practice, a header-provisioned user can request a password reset email and set a local password, which breaks the intended "proxy-only" authentication model for this feature.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Independently confirmed against head 40286d8. User#sso_only? requires oidc_identities.exists?, which is false for JIT remote-header users (they have no OIDC identity attached). Both the email-send gate (unless user.sso_only? in create) and the write gate (if @user.sso_only? in update) evaluate to the wrong branch for these users — password reset emails are sent and the password update is allowed through. The PR description's claim that these users "can't fall back to local login or password reset" is not implemented. Needs a fix before merge — see the review comment above for specifics.
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/concerns/authentication.rb`:
- Around line 81-88: The method remote_user_proxy_trusted? is using
request.remote_ip (end-user IP) and treating trusted.blank? as "allow all";
change it to use the immediate peer address request.env['REMOTE_ADDR'] for
comparison, and change the allow-all logic to only return true when
Rails.application.config.remote_user_trusted_proxies is nil (not when it's an
empty array), e.g. check for trusted.nil? instead of trusted.blank?; keep the
IPAddr parsing and trusted.any? { |range| range.include?(IPAddr.new(peer_ip)) }
flow and preserve the IPAddr::Error rescue to return false.
In `@docs/hosting/docker.md`:
- Around line 326-338: The fenced code blocks showing environment variables
REMOTE_USER_HEADER_EMAIL and REMOTE_USER_TRUSTED_PROXIES lack a language
identifier and trigger MD040; update the two blocks that contain
REMOTE_USER_HEADER_EMAIL="Remote-Email" and
REMOTE_USER_TRUSTED_PROXIES="10.0.0.5,172.18.0.0/16" to use a language tag
(e.g., txt) immediately after the opening backticks so the blocks read ```txt
... ``` to satisfy markdownlint.
In `@test/integration/remote_user_header_authentication_test.rb`:
- Around line 7-9: The tests rely on global REMOTE_USER_TRUSTED_PROXIES
state—update the setup block that already stubs
Rails.application.config.remote_user_header_email (and references HEADER_NAME)
to also stub Rails.application.config.remote_user_trusted_proxies to a
deterministic value (e.g. nil or an empty array) so tests assume “no allowlist
configured”; locate the setup method in
remote_user_header_authentication_test.rb and add a stub for
remote_user_trusted_proxies to ensure the test environment is consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f3015c5-45d7-4ae4-819a-cae2b65e6216
📥 Commits
Reviewing files that changed from the base of the PR and between ec4559b and 0ea63b400ac42b8152622b31a8a6112b1d92b5cc.
📒 Files selected for processing (4)
app/controllers/concerns/authentication.rbconfig/application.rbdocs/hosting/docker.mdtest/integration/remote_user_header_authentication_test.rb
|
Code review: the implementation is carefully constructed — opt-in via env var, email regex guard, JIT mirrors OIDC, session invalidation when header disagrees, Two concerns before merging: Default-open when no IP allowlist is set Consider defaulting to fail-closed: require at least one trusted CIDR, or emit a startup warning (e.g.,
Generated by Claude Code |
jjmata
removed
the
contributor:flagged
|
Thanks for the contribution, @aroberts! Can you address/respond to the open PR comments? 🙏 |
|
Can you review/suggest ways to minimize the threat surface if we take this, @GoByeBye? Put it behind a feature flag? 🤔 |
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
superagent-security
Bot
added
the
contributor:flagged
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/concerns/authentication.rb`:
- Around line 81-89: The remote_user_proxy_trusted? method currently treats a
nil REMOTE_USER_TRUSTED_PROXIES as trusted, which is unsafe; add a startup check
instead of changing runtime behavior: in an initializer (e.g.,
config/initializers/remote_user_header.rb) add a
Rails.application.config.after_initialize hook that logs a one-time WARN if
Rails.application.config.remote_user_header_email.present? &&
Rails.application.config.remote_user_trusted_proxies.nil?, warning that the
header will be trusted from any source IP and advising to set
REMOTE_USER_TRUSTED_PROXIES or restrict exposure; alternatively, if you prefer
fail-closed, change remote_user_proxy_trusted? to treat nil the same as empty
(return false) and update tests/docs that assume nil default.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9ca3c63-e2ab-44f1-8a11-c32c9865ff81
📥 Commits
Reviewing files that changed from the base of the PR and between 03b32f1c51a7d584eda9a5ef10f8e5bf97b6e096 and 40286d8.
📒 Files selected for processing (4)
app/controllers/concerns/authentication.rbconfig/application.rbdocs/hosting/docker.mdtest/integration/remote_user_header_authentication_test.rb
|
(deleted some "agentic eager beaver" comments from @sure-admin, sorry about that ... trying to fine-tune things!) |
|
@coderabbitai re-review |
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
jjmata
removed
the
contributor:flagged
sure-design
left a comment
sure-design
left a comment
There was a problem hiding this comment.
Independent re-review against the current head (40286d8). Most prior comments are well-addressed — the REMOTE_ADDR fix, the nil?-vs-blank? fix, the markdown lint, and the test setup stub are all confirmed correct. One blocking P1 remains, plus two smaller items.
❌ Blocking — P1: JIT remote-header users can bypass proxy via password reset
The PR description says "JIT users have no password_digest so they can't fall back to local login or password reset." This is not enforced by the code.
User#sso_only? (unchanged by this PR) is:
def sso_only?
password_digest.nil? && oidc_identities.exists?
endJIT remote-header users have password_digest = nil and no OIDC identities, so sso_only? returns false for them.
PasswordResetsController#create sends the reset email unless user.sso_only? — that guard is false for these users → the email is sent.
PasswordResetsController#update blocks the write if @user.sso_only? — also false → the password update succeeds → the user now has a password_digest and can log in locally without the proxy entirely.
The fix: make sso_only? cover any user with password_digest.nil?, regardless of OIDC identity. The simplest approach is to broaden the condition:
def sso_only?
password_digest.nil?
endOr add a separate predicate (remote_header_provisioned?) and update both password-reset checks. Either way, a test should cover this path.
⚠️ Non-blocking — No startup warning when allowlist is unconfigured
When REMOTE_USER_HEADER_EMAIL is set but REMOTE_USER_TRUSTED_PROXIES is not, the header is trusted from any source IP. The docs mention this, but an operator who misses that line gets silent default-open behavior. A one-shot Rails.logger.warn in an after_initialize block would catch real deployment mistakes at zero cost:
# config/initializers/remote_user_header.rb
Rails.application.config.after_initialize do
if Rails.application.config.remote_user_header_email.present? &&
Rails.application.config.remote_user_trusted_proxies.nil?
Rails.logger.warn(
"[remote_user_header] REMOTE_USER_HEADER_EMAIL is set but " \
"REMOTE_USER_TRUSTED_PROXIES is unset. The header will be trusted " \
"from ANY source IP. Set REMOTE_USER_TRUSTED_PROXIES or ensure " \
"Sure is not reachable except via your authenticating reverse proxy."
)
end
endℹ️ Minor — No self_hosted? guard
The OIDC path and the first-login path both check app_mode.self_hosted?. This feature has no such guard, so a managed deployment that accidentally sets REMOTE_USER_HEADER_EMAIL enables header auth (including JIT super_admin creation). Lowest priority, but worth making consistent.
Generated by Claude Code
superagent-security
Bot
added
the
contributor:verified
|
addressed feedback; rebased back onto to main. I kept the |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/controllers/valuations_controller_test.rb (1)
54-79: 💤 Low valueLGTM — tests are minimal, correct, and follow project conventions.
Both tests use Minitest + fixtures, post to the right named route helpers, and assert both the HTTP status and the presence of the i18n error string in the body. Coverage is proportional to the change.
One small heads-up:
assert_match I18n.t("valuations.errors.amount_required"), response.bodypasses vacuously if the i18n key is absent (both controller and test would produce the same "translation missing" sentinel). Since the key is confirmed to be present inen.yml, this is low risk, but asserting the literal English string instead of the i18n lookup would make the test self-contained.🔍 Optional: assert literal string to avoid vacuous pass on missing key
- assert_match I18n.t("valuations.errors.amount_required"), response.body + assert_match "Amount is required", response.bodyApply the same change to the
confirm_updatetest. This makes the assertion independent of the i18n infrastructure and catches key-missing regressions explicitly. Only do this if the team prefers literal assertions in integration tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/valuations_controller_test.rb` around lines 54 - 79, Replace the i18n lookup in the two tests so the assertion checks the literal English error message instead of I18n.t(...) to avoid a vacuous pass when the translation key is missing: in the "confirm_create with blank amount returns unprocessable entity" and "confirm_update with blank amount returns unprocessable entity" tests (the ones posting to confirm_create_valuations_url and confirm_update_valuation_url), change the assert_match that currently uses I18n.t("valuations.errors.amount_required") to assert against the literal string shown in en.yml (e.g. "Amount is required" or the exact English message used) so the response body assertion fails if the translation key is removed or changed.test/controllers/password_resets_controller_test.rb (1)
94-124: 💤 Low valueExtract the repeated inline user creation into a private helper.
Both new tests repeat the same
User.create!/Family.create!block verbatim. A private helper keeps the intent visible and avoids drift if the JIT creation contract changes.♻️ Suggested refactor
+ private + + def create_jit_user(email: "headerjit@test.example") + User.create!( + email: email, + family: Family.create!, + role: :admin, + skip_password_validation: true + ) + end + test "create does not send email for password-less user without OIDC identity" do - jit_user = User.create!( - email: "headerjit@test.example", - family: Family.create!, - role: :admin, - skip_password_validation: true - ) + jit_user = create_jit_user ... end test "update blocks password setting for password-less user without OIDC identity" do - jit_user = User.create!( - email: "headerjit@test.example", - family: Family.create!, - role: :admin, - skip_password_validation: true - ) + jit_user = create_jit_user ... end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/password_resets_controller_test.rb` around lines 94 - 124, Extract the repeated User.create! / Family.create! block used in the tests "create does not send email for password-less user without OIDC identity" and "update blocks password setting for password-less user without OIDC identity" into a private helper method (e.g., build_jit_user or create_jit_user) inside the test class; have both tests call that helper to return the jit_user so they reuse the same attributes (email, associated Family.create!, role: :admin, skip_password_validation: true) and update any references (token generation or post/patch calls) to use the helper result.test/controllers/api/v1/balances_controller_test.rb (1)
116-124: ⚡ Quick win
account_idfilter test only asserts inclusion, not exclusionThe test confirms
@balanceappears in the filtered results but doesn't assert that balances belonging to other accounts are absent. A second account with its own balance created insetup(or inline here) and anassert_not_includeswould make the filter's isolation guarantee explicit.✅ Suggested assertion addition
test "filters balances by account_id" do + other_account_balance = `@family.accounts.create`!( + name: "Other Account", + accountable: Depository.new, + balance: 500, + currency: "USD" + ).balances.create!(date: Date.parse("2024-01-15"), balance: 500, currency: "USD") + get api_v1_balances_url, params: { account_id: `@account.id` }, headers: api_headers(`@api_key`) assert_response :success response_data = JSON.parse(response.body) balance_ids = response_data["balances"].map { |balance| balance["id"] } assert_includes balance_ids, `@balance.id` + assert_not_includes balance_ids, other_account_balance.id end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/api/v1/balances_controller_test.rb` around lines 116 - 124, The test "filters balances by account_id" currently only asserts that `@balance` is present; add a second account and its balance (either in the test or in setup) and after parsing response_data assert that the other balance's id is not present using assert_not_includes on response_data["balances"].map { |balance| balance["id"] }, referencing the existing api_v1_balances_url request and api_headers(`@api_key`) to ensure the filter excludes balances from other accounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/family/data_exporter.rb`:
- Around line 157-188: The Balance export loop in generate_ndjson currently uses
Balance.joins(...).chronological.each which materializes the entire result set;
change it to a batched iteration to avoid high memory use by either (A)
replacing .each with in_batches(of: 1000).order('date').each_record (or using
in_batches.with_record_fetching preserving the chronological scope) so ordering
by date is preserved while loading batches, or (B) if ordering by date is not
required, drop .chronological and use find_each(batch_size: 1000) on Balance to
stream records; update the Balance iteration in generate_ndjson accordingly
(referencing Balance, chronological, generate_ndjson, find_each, in_batches).
In `@test/controllers/password_resets_controller_test.rb`:
- Around line 109-124: The test "update blocks password setting for
password-less user without OIDC identity" currently only asserts redirect; add
an assertion that flash[:alert] contains the expected error message to mirror
the sso_only test coverage. After calling patch password_reset_path(token:
token) and asserting assert_redirected_to new_session_path, assert that
flash[:alert] (or flash[:error] if your app uses that key) equals or matches the
same message used in the controller for this branch so the error message path is
verified; reference the test name, jit_user, token, and the patch to
password_reset_path to locate where to add the assertion.
---
Nitpick comments:
In `@test/controllers/api/v1/balances_controller_test.rb`:
- Around line 116-124: The test "filters balances by account_id" currently only
asserts that `@balance` is present; add a second account and its balance (either
in the test or in setup) and after parsing response_data assert that the other
balance's id is not present using assert_not_includes on
response_data["balances"].map { |balance| balance["id"] }, referencing the
existing api_v1_balances_url request and api_headers(`@api_key`) to ensure the
filter excludes balances from other accounts.
In `@test/controllers/password_resets_controller_test.rb`:
- Around line 94-124: Extract the repeated User.create! / Family.create! block
used in the tests "create does not send email for password-less user without
OIDC identity" and "update blocks password setting for password-less user
without OIDC identity" into a private helper method (e.g., build_jit_user or
create_jit_user) inside the test class; have both tests call that helper to
return the jit_user so they reuse the same attributes (email, associated
Family.create!, role: :admin, skip_password_validation: true) and update any
references (token generation or post/patch calls) to use the helper result.
In `@test/controllers/valuations_controller_test.rb`:
- Around line 54-79: Replace the i18n lookup in the two tests so the assertion
checks the literal English error message instead of I18n.t(...) to avoid a
vacuous pass when the translation key is missing: in the "confirm_create with
blank amount returns unprocessable entity" and "confirm_update with blank amount
returns unprocessable entity" tests (the ones posting to
confirm_create_valuations_url and confirm_update_valuation_url), change the
assert_match that currently uses I18n.t("valuations.errors.amount_required") to
assert against the literal string shown in en.yml (e.g. "Amount is required" or
the exact English message used) so the response body assertion fails if the
translation key is removed or changed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 475b4aa7-5ee5-40a5-905c-99cfd05e9271
📥 Commits
Reviewing files that changed from the base of the PR and between 40286d8 and 96088575e6bb80cef4e03b9694c84a5d02dac27b.
📒 Files selected for processing (23)
app/controllers/api/v1/balances_controller.rbapp/controllers/api/v1/base_controller.rbapp/controllers/concerns/authentication.rbapp/controllers/valuations_controller.rbapp/models/concerns/monetizable.rbapp/models/family/data_exporter.rbapp/models/user.rbapp/views/api/v1/balances/_balance.json.jbuilderapp/views/api/v1/balances/index.json.jbuilderapp/views/api/v1/balances/show.json.jbuilderconfig/application.rbconfig/initializers/remote_user_header.rbconfig/locales/views/valuations/en.ymlconfig/routes.rbdocs/api/openapi.yamldocs/hosting/docker.mdspec/requests/api/v1/balances_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/balances_controller_test.rbtest/controllers/password_resets_controller_test.rbtest/controllers/valuations_controller_test.rbtest/integration/remote_user_header_authentication_test.rbtest/models/family/data_exporter_test.rb
✅ Files skipped from review due to trivial changes (2)
- app/views/api/v1/balances/show.json.jbuilder
- config/locales/views/valuations/en.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- config/application.rb
- app/controllers/concerns/authentication.rb
- test/integration/remote_user_header_authentication_test.rb
| Balance.joins(:account) | ||
| .where(accounts: { family_id: @family.id }) | ||
| .chronological | ||
| .each do |balance| | ||
| lines << { | ||
| type: "Balance", | ||
| data: { | ||
| id: balance.id, | ||
| account_id: balance.account_id, | ||
| date: balance.date, | ||
| balance: balance.balance, | ||
| currency: balance.currency, | ||
| cash_balance: balance.cash_balance, | ||
| start_cash_balance: balance.start_cash_balance, | ||
| start_non_cash_balance: balance.start_non_cash_balance, | ||
| cash_inflows: balance.cash_inflows, | ||
| cash_outflows: balance.cash_outflows, | ||
| non_cash_inflows: balance.non_cash_inflows, | ||
| non_cash_outflows: balance.non_cash_outflows, | ||
| net_market_flows: balance.net_market_flows, | ||
| cash_adjustments: balance.cash_adjustments, | ||
| non_cash_adjustments: balance.non_cash_adjustments, | ||
| flows_factor: balance.flows_factor, | ||
| start_balance: balance.start_balance, | ||
| end_cash_balance: balance.end_cash_balance, | ||
| end_non_cash_balance: balance.end_non_cash_balance, | ||
| end_balance: balance.end_balance, | ||
| created_at: balance.created_at, | ||
| updated_at: balance.updated_at | ||
| } | ||
| }.to_json | ||
| end |
There was a problem hiding this comment.
Use batched loading for Balance records; .each loads the full result set into memory.
Every other section in generate_ndjson calls find_each to process records in 1,000-row DB batches, capping instantiated AR objects in memory. This block uses plain .each, which materializes the entire Balance result set at once. Balance tables grow at one row per account per day; a family with 15 accounts and 3 years of history already has ~16,000 rows — and the export contains the full family.
The chronological scope's ORDER BY date is incompatible with find_each's id-cursor batching, so a naive swap loses ordering. Two workable alternatives:
in_batcheswith preserved ordering (Rails 7+ supports this):
⚡ Proposed fix using `in_batches`
- Balance.joins(:account)
- .where(accounts: { family_id: `@family.id` })
- .chronological
- .each do |balance|
+ Balance.joins(:account)
+ .where(accounts: { family_id: `@family.id` })
+ .chronological
+ .in_batches(of: 1000) do |batch|
+ batch.each do |balance|
lines << {
type: "Balance",
data: {
id: balance.id,
# ...
}
}.to_json
+ end
end- Drop the
chronologicalscope and usefind_each(id order), which is acceptable for a backup export where exact date ordering is not a semantic requirement. The test at line 406 would need to be adjusted accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/family/data_exporter.rb` around lines 157 - 188, The Balance
export loop in generate_ndjson currently uses
Balance.joins(...).chronological.each which materializes the entire result set;
change it to a batched iteration to avoid high memory use by either (A)
replacing .each with in_batches(of: 1000).order('date').each_record (or using
in_batches.with_record_fetching preserving the chronological scope) so ordering
by date is preserved while loading batches, or (B) if ordering by date is not
required, drop .chronological and use find_each(batch_size: 1000) on Balance to
stream records; update the Balance iteration in generate_ndjson accordingly
(referencing Balance, chronological, generate_ndjson, find_each, in_batches).
|
#1685 (review) My only real concern is that when I'd reckon (a sysadmin tailing application logs only when something visibly breaks, a Docker setup where the warning scrolls past during boot under a hundred other Sidekiq/Rails lines, a managed-by-Watchtower restart at 3am with nobody watching, a Loki/Promtail pipeline that filters [Gitea](https://docs.gitea.com/administration/config-cheat-sheet) ship With the current setup, a self-hoster who skips the optional var ships default-open and a logged warning. With a loopback default, that same self-hoster ships default-closed, the proxy (almost never on loopback) gets ignored, and they hit a tractable "login isn't going through" on day one instead of a silent compromise on day ninety. Same change as before: config.remote_user_trusted_proxies = (ENV["REMOTE_USER_TRUSTED_PROXIES"].presence || "127.0.0.0/8,::1/128")
.split(",")
.map(&:strip)
.reject(&:empty?)
.filter_map { |s| IPAddr.new(s) rescue nil }(then drop Outside of that LGTM |
|
sure, I can do that |
|
Two things worth addressing before this lands: Merge conflicts — the branch is currently dirty against PR scope — the diff bundles several unrelated changes alongside the SSO feature:
The SSO auth feature is substantial enough on its own (22 files, security-sensitive). The unrelated additions make the diff harder to reason about safely and would complicate any targeted rollback. Would recommend splitting out at minimum the Balances API and the valuation fixes as separate PRs. The SSO implementation itself looks solid — fail-closed IP default, email regex guard, JIT mirrors OIDC path, audit logging, session invalidation on identity mismatch. The Generated by Claude Code |
Soft `save` swallowed validation errors and let `create_session_for` proceed with an unpersisted User, producing FK errors on session create or orphaned rows.
Two concurrent requests with the same header email both miss the `find_by`, both attempt insert; the deterministic-encrypted email index makes one win. Previously the loser bubbled `RecordNotUnique` to a 500. Rescue and re-fetch the row the winner created.
…nt user If a browser carries a session cookie for user A but the upstream proxy sets the remote-user header to user B (e.g. shared kiosk, proxy-side identity rotation), the cookie short-circuit auth'd as A and ignored B. When a header is present, treat it as authoritative: invalidate any cookie session that doesn't match and fall through to the header path.
The first JIT user on a fresh instance should become super_admin to match the OIDC JIT path; subsequent users get :admin. Previously every remote-header user was hardcoded to :admin, leaving the instance without a super_admin if the first sign-in came through this path.
Setting a random password meant a remote-header user could later acquire local-login access via password reset, defeating the "proxy is the only way in" guarantee. Match the OIDC JIT pattern: skip password validation, leave password_digest nil.
OIDC JIT writes to sso_audit_logs; remote-header auth was silent. Log a jit_account_created entry on first sign-in and a login entry on each subsequent header-driven session, both tagged with the remote_user_header provider so they're filterable from the OIDC traffic.
A misconfigured proxy can send arbitrary garbage in the configured header. Pre-validate against the standard email regexp (and also guard against an unset config header name) so we fail closed silently instead of throwing on User#save validations and 500ing the request. Also normalize via strip+downcase before lookup so the cookie/header mismatch comparison stays consistent.
A header-only auth path is only as trustworthy as the network in front of it. Add an opt-in source-IP allowlist that the request must pass before the header is honored: - REMOTE_USER_TRUSTED_PROXIES: comma-separated IPs/CIDRs; request must originate from one of them. Defaults to off so existing single-tenant homelab setups behave unchanged. Doc updated.
Eight tests covering the security and correctness invariants of the remote-header path. Each defends a property whose regression wouldn't be caught by reading the diff: - feature is opt-in: header ignored when REMOTE_USER_HEADER_EMAIL unset - JIT user has password_digest = nil and a created family - JIT delegates role to User.role_for_new_family_creator - SsoAuditLog: jit_account_created once, login on every header session - cookie session for user A is destroyed when header asserts user B - IP allowlist denies non-allowlisted source - IP allowlist permits allowlisted CIDR (incl. 127.0.0.0/8 happy path) - malformed/empty header value fails closed without raising
The allowlist gate now compares against REMOTE_ADDR (the immediate peer) instead of request.remote_ip; the latter resolves through XFF and could let an attacker spoof past the gate if they reach Sure outside the proxy. An empty or unparseable trusted-proxies list now fails closed (deny-all) instead of silently disabling the gate; only an unset env var disables. Doc updated to spell out the three states. Added a language tag to the env-var fenced blocks for markdownlint, and stubbed remote_user_trusted_proxies in the integration test setup so cases that don't configure it have a deterministic baseline.
Header-JIT users have password_digest = nil but no OIDC identity, so the original sso_only? definition (password_digest.nil? && oidc_ identities.exists?) returned false for them. PasswordResetsController gates on sso_only?, so these users could request a password reset, set a password, and from then on bypass the reverse proxy entirely. Broaden the predicate to password_digest.nil?. The two existing production callers (PasswordResetsController#create and #update) want "this user can't use a local password" semantics; neither of them relies on the OIDC clause specifically. Existing user-model tests still pass under the broader definition. Adds two controller tests for the regression: a password-less user with no OIDC identity can't trigger a reset email, and even with a forged token can't write a new password_digest.
…st unconfigured Two minor follow-ups from review: Header auth is now gated on Rails.application.config.app_mode.self_hosted? in trusted_remote_user_email, matching the OIDC and first-login paths. A managed deployment that accidentally sets REMOTE_USER_HEADER_EMAIL no longer enables JIT super_admin creation. Doc updated. Adds config/initializers/remote_user_header.rb that emits a one-time Rails.logger.warn at startup when REMOTE_USER_HEADER_EMAIL is set but REMOTE_USER_TRUSTED_PROXIES is unset. Catches the operator-error case where someone enables header auth but forgets the source-IP allowlist and silently default-opens. Adds an integration test asserting the feature is inert in managed mode even with all config set.
CodeRabbit re-review caught that the new password-reset update test verified the redirect but not the flash[:alert] body, so a wrong-or- empty message would have slipped through. Add substring assertions on the alert text rather than pinning the full literal sentence, so wording tweaks don't break the test. Also extract the duplicated User.create!/Family.create! block into a private create_jit_user helper, per the same review's nit.
Per review feedback: a Rails.logger.warn for an unconfigured allowlist is structurally weak. It's silenced by warn-level log filters, scrolls past during boot under Sidekiq/Rails noise, and only gets noticed when the perimeter has already leaked. Real-world ops setups (log-tail-on- incident, unattended Watchtower restarts) ship default-open + a logged warning that's functionally equivalent to silently default-open. Borrow the pattern Gitea uses for its REVERSE_PROXY_TRUSTED_PROXIES: default to loopback (127.0.0.0/8 + ::1/128). A self-hoster who skips the env var ships fail-closed; their proxy on a Docker bridge or separate host gets ignored, login breaks at first test, they get an actionable signal on day one instead of a silent compromise on day ninety. Also drops the now-redundant after_initialize warning in config/initializers/remote_user_header.rb (the warning's only purpose was warning operators about default-open; with default-closed there's nothing to warn about) and the dead trusted.nil? short-circuit in remote_user_proxy_trusted? (the config is now always populated). Doc and tests updated. Test coverage expands to (a) the new default behavior (loopback peer is honored), and (b) explicit override (a non-loopback peer paired with a non-loopback CIDR override is honored, where it would be denied under the default).
|
rebased onto the latest |
Adds a second perimeter on top of the IP allowlist for setups where peer-IP isn't sufficient — a multi-tenant Docker bridge where many containers share an IP range, or any topology where the immediate peer isn't fully trusted. When REMOTE_USER_SHARED_SECRET is set, the proxy must echo the same value in REMOTE_USER_SHARED_SECRET_HEADER (default X-Remote-User-Secret) on every request. Compared in constant time. Mismatches and missing headers fall through to the normal login redirect — same behavior as a non-allowlisted source IP. Defaults to unset, so existing deployments behave unchanged. The IP allowlist remains the load-bearing gate; the shared secret is opt-in defense-in-depth. Tests cover the three branches: secret configured + missing header, secret configured + mismatch, secret configured + match.
|
P1 security issue: JIT remote-header users can bypass the reverse proxy via password reset. Fix: broaden This should be addressed before merge regardless of the separate Generated by Claude Code |
|
This was fixed in commit # app/models/user.rb (current)
def sso_only?
password_digest.nil?
endCoverage at Looks like an automation re-post of the original review. |
|
Separately, the pipelock scan seems to be 2 false positives as well. Please let me know if there's anything you'd like changed regarding the pipelock report. |
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Want to double-check on the no password recovery before we merge.
4 participants
Summary
Adds an opt-in path that lets self-hosters put Sure behind a reverse proxy handling auth (Authelia, oauth2-proxy, Traefik forward-auth) and trust a header from it as the user's identity. Unknown emails JIT-create a User and Family, like
OidcAccountsController#create_userdoes for first OIDC sign-ins. Off by default. SetREMOTE_USER_HEADER_EMAILto enable:Docs:
docs/hosting/docker.md.Hardening
Threat model assumes Sure isn't reachable past the proxy; the header itself is unauthenticated. JIT users have no
password_digestso they can't fall back to local login or password reset. First user becomes super_admin viaUser.role_for_new_family_creator(matches OIDC). A cookie session for user A is dropped when a header asserts user B. Concurrent JIT inserts for the same email rescueRecordNotUniquefrom the unique index. JIT creation and every header-driven login writeSsoAuditLogentries taggedprovider: "remote_user_header". Header values are validated against the email regex before lookup, so a misconfigured proxy gets ignored, not 500'd.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests