Fix CSRF conflicts with Phoenix using automatic pipeline separation#51
Open
camilohollanda wants to merge 7 commits intotompave:masterfrom
Open
Fix CSRF conflicts with Phoenix using automatic pipeline separation#51camilohollanda wants to merge 7 commits intotompave:masterfrom
camilohollanda wants to merge 7 commits intotompave:masterfrom
Conversation
…cations This change introduces a disable_csrf option that allows host applications to disable FunWithFlags UI's built-in CSRF protection when they provide their own CSRF protection (like Phoenix applications do). Changes: - Add disable_csrf option to router configuration - Make protect_from_forgery plug conditional based on disable_csrf setting - Make CSRF token assignment conditional - Update module documentation with usage examples This resolves conflicts that occur when embedding FunWithFlags UI in Phoenix applications that already have CSRF protection enabled. Usage: forward "/feature-flags", FunWithFlags.UI.Router, disable_csrf: true
Clarify that namespace is used for internal redirects and provide a concrete example showing when and how to use it when the forward path differs from the desired UI path structure.
This commit replaces the disable_csrf configuration option with an automatic pipeline separation approach that resolves CSRF conflicts with Phoenix applications while maintaining security. Changes: - Remove disable_csrf option and related conditional logic - Implement automatic request routing by path: * /assets/* → Asset pipeline (no CSRF protection) * All other routes → Form pipeline (full CSRF protection) - Maintain CSRF tokens in HTML pages for JavaScript AJAX requests - Update documentation to explain the automatic approach Benefits: - No configuration required - works automatically - Fixes InvalidCrossOriginRequestError for JavaScript/CSS assets - Maintains security for all form submissions and API calls - Follows Phoenix pattern of separating assets from interactive routes This resolves conflicts when embedding in Phoenix applications while providing better security than fully disabling CSRF protection.
This commit adds extensive test coverage for the new pipeline separation feature that resolves CSRF conflicts with Phoenix applications. Test coverage includes: **Pipeline Separation Tests:** - Asset requests (/assets/*) go through asset pipeline without CSRF tokens - Different asset extensions (js, css, png, woff) all use asset pipeline - Form requests go through form pipeline with CSRF tokens assigned - All form routes receive CSRF tokens for JavaScript usage - POST requests work correctly with form pipeline and CSRF protection - Asset pipeline doesn't interfere with static file serving **CSRF Token Assignment Tests:** - HTML responses include CSRF tokens for JavaScript AJAX requests - Asset requests never receive CSRF tokens to avoid conflicts - Token assignment is consistent across different routes **Backward Compatibility Tests:** - All existing functionality continues to work unchanged - Namespace option works correctly with pipeline separation - Flag operations (create, read, update) work with new architecture The tests ensure the pipeline separation provides the expected security benefits (CSRF protection for forms, no conflicts for assets) while maintaining complete backward compatibility with existing usage. Tests require Redis for FunWithFlags functionality and will run in CI.
Author
|
@tompave any chance to get this merged? |
When `FunWithFlags.UI.Router` is forwarded under a host pipeline that includes `Plug.CSRFProtection` (for example, Phoenix's standard `:browser` pipeline), the host's CSRF check runs before control reaches this router. The browser's `<script>` and `<link>` requests for `/<mount>/assets/*` are flagged as cross-origin and the host raises `Plug.CSRFProtection.InvalidCrossOriginRequestError`, breaking the dashboard. Nothing inside the embedded router can prevent this, since the host's CSRF plug has already run by the time control gets here. This change adds `FunWithFlags.UI.Plug`, a host-side helper that users insert into their `:browser` pipeline before `:protect_from_forgery`. It marks safe (`GET`/`HEAD`) requests for `/assets/*` paths with `:plug_skip_csrf_protection` (the documented escape hatch from `Plug.CSRFProtection`) so the host's CSRF check lets them through. State-changing requests (form posts, deletes) flow through CSRF protection unchanged. An optional `:mount` option scopes the skip to a specific mount path when needed. Includes unit tests plus an integration test that runs the helper end-to-end through `Plug.Session` -> helper -> `Plug.CSRFProtection` -> JS response, confirming the helper neutralizes the real `InvalidCrossOriginRequestError` and that the error fires without it. The router itself is left unchanged from `master`. README adds a section showing how to mount under the standard `:browser` pipeline using the new helper.
9aabac6 to
86b82ec
Compare
Require the :mount option in FunWithFlags.UI.Plug.init/1 so the CSRF skip is always scoped to a known forwarded mount, instead of bypassing CSRF for any /assets/ path in the host application. Match against conn.path_info segments rather than substrings so that mount-prefix substrings (/feature-flags-extra/assets/...) and assets-prefix segments (/feature-flags/assets-extra/...) no longer match. Also tighten the asset check to require a file segment under /assets/, so /assets and /assets/ no longer skip CSRF.
5dafe0a to
d0b2b50
Compare
d0b2b50 to
176a742
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
FunWithFlags.UI.Plug, a host-side helper that lets users embedFunWithFlags.UI.Routerunder Phoenix's standard:browserpipeline (or any host pipeline that usesPlug.CSRFProtection) without hittingPlug.CSRFProtection.InvalidCrossOriginRequestErroron the dashboard's static assets.Fixes #52
Problem
When the UI is forwarded under a
:browser-style pipeline:Phoenix's
:protect_from_forgeryruns before control reachesFunWithFlags.UI.Router. The browser's<script>and<link>requests for/feature-flags/assets/*are flagged as cross-origin, andPlug.CSRFProtectionraises:The current README workaround (
:mounted_appspipeline) is fine but requires users to define a separate pipeline that omits:protect_from_forgery, which is non-obvious.Solution
FunWithFlags.UI.Plugis a host-side plug. Users insert it in their:browserpipeline before:protect_from_forgery, scoped to the path the router is forwarded under:For
GET/HEADrequests whose path is<mount>/assets/<file>, the helper setsconn.private[:plug_skip_csrf_protection] = true— the documented escape hatch fromPlug.CSRFProtection. Everything else (form submits, deletes, non-asset paths, paths outside the configured mount) flows through:protect_from_forgeryunchanged.The
:mountoption is required and matched againstconn.path_infosegments, so mount-prefix substrings (/feature-flags-extra/assets/...) andassets-prefix segments (/feature-flags/assets-extra/...) do not match. The bare<mount>/assetspath with no file is also not skipped.What's added
lib/fun_with_flags/ui/plug.ex—FunWithFlags.UI.Plug, the host-side helper.test/fun_with_flags/ui/plug_test.exs— unit tests coveringinit/1validation, segment-based path matching, mount/asset/method edge cases, plus an end-to-end integration test that runsPlug.Session→FunWithFlags.UI.Plug→Plug.CSRFProtectionon a JS response and asserts the error fires without the helper and is suppressed with it.README.md— documents the new helper as an option for mounting under the standard:browserpipeline. The existing:mounted_appsrecipe is unchanged.Backward compatibility
Fully backward-compatible.
FunWithFlags.UI.Routeris unchanged. Users following either the existing:mounted_appsrecipe or the standalonePlug.Routerrecipe in the README don't need to change anything. The new helper is purely opt-in for users who want to mount under a:browser-style pipeline that already protects against CSRF.Test plan
mix test— 81 tests, 0 failuresPlug.Session→ helper →Plug.CSRFProtection→ JS response) and verifies theInvalidCrossOriginRequestErroris raised without the helper and not raised with it