Files
remark42/backend/app/cmd
Dmitry VerkhoturovandGitHub c9ba8520c7 fix(auth): close OAuth open-redirect by wiring AllowedRedirectHosts (#2049)
* fix(auth): close OAuth open-redirect by wiring AllowedRedirectHosts

Bump go-pkgz/auth/v2 to master (v2.1.2-0.20260421203319-686683f19cf7)
which carries the `from` redirect validator from go-pkgz/auth#275.

The library default with a nil AllowedRedirectHosts is permissive
(preserves legacy behavior for existing consumers on a dep bump), so
just bumping the dep leaves remark42 vulnerable — a crafted
/auth/<provider>/login?from=https://evil.example.com/... still issues
the 307 to the attacker host after the user completes legitimate
OAuth. Verified end-to-end against a local dev-auth instance before
and after this commit.

Wire Opts.AllowedRedirectHosts in getAuthenticator to the operator's
existing --allowed-hosts config, stripping the CSP "self" sentinel
which is not a real hostname. RemarkURL's own host is always implicit
per the library contract, so a default single-site deployment gains
the protection with no config change. Multi-host embeds work as soon
as their embedding hosts are added to AllowedHosts (they already need
to be there for CSP frame-ancestors).

Refreshed vendor tree to match the new module version.

* chore(lint): suppress G703 false positives on image Save

CI's newer gosec flags os.MkdirAll/os.WriteFile in FileSystem.Save with
G703 because id flows in from the caller. id is validated at the HTTP
layer (safePictureSegment in rest_public.go) and dst is derived via
f.location — not a real traversal. Targeted //nolint with reason.

* fix(auth): normalise AllowedRedirectHosts entries + add unit test

Address Copilot review on PR #2049. The previous closure passed raw
s.AllowedHosts entries straight to the auth library, but --allowed-hosts
holds CSP frame-ancestors source expressions: scheme-prefixed values
(https://blog.example.com), entries with ports, and wildcards
(*.cdn.example.com) are all valid there but the auth library compares
against u.Hostname() and would silently drop them — breaking legitimate
redirects on multi-host deployments.

Extract getAllowedRedirectHosts that:
* trims whitespace, drops empty / 'self' / "self" / wildcard entries
* prepends https:// if scheme missing then url.Parse to extract Hostname
* logs a warning on parse failure rather than poisoning the allowlist

Wire the closure in getAuthenticator to call the helper.

Test_getAllowedRedirectHosts covers all the edge cases Copilot flagged
(scheme stripping, port handling, self spellings, wildcards, empty,
mixed real-world).

* fix(auth): preserve explicit port in AllowedRedirectHosts + clarify fs_store nolint

Address Copilot follow-up on PR #2049:

* getAllowedRedirectHosts stripped explicit ports via u.Hostname(), which
  broadened the allowlist. The auth validator checks both Hostname() and
  Host, so an entry like admin.example.com:8443 can and should be kept
  host:port — allowing only that port, not any. Emit u.Host when
  u.Port() != "", u.Hostname() otherwise. Updated tests.

* fs_store Save nolint rationale said "id validated at HTTP layer", but
  Save is reached via image.Service.Save and SaveWithID (cache), neither
  of which is HTTP validation. id is actually a server-generated hash in
  both paths. Updated the comment.
2026-04-21 19:09:26 -05:00
..
2024-12-09 01:54:03 +00:00
2024-12-09 01:54:03 +00:00
2022-07-25 12:42:13 -05:00