[Referral] Add support for redirecting to a custom URL #12352
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| end | ||
|
|
||
| redirect_to params[:return_to] || root_path | ||
| redirect_to params[:return_to] || root_path |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
The best practice is to never trust user input for redirect destinations without validation. To fix this, we should only allow redirection to:
- A whitelist of known good (relative) paths.
- Or, only allow relative URLs (i.e., not full URLs with protocols/hosts).
Here, the preferable solution is (2): check if params[:return_to] is present, is a relative path, and does not include a host or protocol. If it fails this check, redirect to root_path instead.
To implement this, parse the parameter with URI.parse, and only redirect if the result is (a) a path (relative path; i.e., the URI’s host and scheme are nil), and (b) not an absolute URL. Otherwise, fallback to root_path.
You may need to add require 'uri' at the top if it's not present.
All changes are within app/controllers/referral/links_controller.rb:
- Add a safe helper method, e.g.,
sanitize_return_to, that does this check. - Use this sanitized value in the redirect call at line 25.
| @@ -1,5 +1,7 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'uri' | ||
|
|
||
| module Referral | ||
| class LinksController < ApplicationController | ||
| before_action :set_link, only: :show | ||
| @@ -22,7 +24,7 @@ | ||
| else | ||
| skip_authorization | ||
|
|
||
| redirect_to params[:return_to] || root_path | ||
| redirect_to sanitize_return_to | ||
| end | ||
| end | ||
|
|
||
| @@ -32,5 +34,23 @@ | ||
| @link = Referral::Link.find_by(slug: params[:id]).presence || Referral::Link.find_by_hashid(params[:id]) | ||
| end | ||
|
|
||
| # Only allow redirection to a safe internal path. | ||
| def sanitize_return_to | ||
| return_to = params[:return_to] | ||
| return root_path unless return_to.present? | ||
|
|
||
| begin | ||
| uri = URI.parse(return_to) | ||
| # Allow only relative paths (no scheme or host) | ||
| if uri.host.nil? && uri.scheme.nil? && uri.path.present? && return_to.starts_with?('/') | ||
| return return_to | ||
| else | ||
| return root_path | ||
| end | ||
| rescue URI::InvalidURIError | ||
| root_path | ||
| end | ||
| end | ||
|
|
||
| end | ||
| end |
| Referral::Attribution.create!(user: current_user, program: @link.program, link: @link) | ||
| end | ||
|
|
||
| redirect_to @link.program.redirect_to.presence || root_path, allow_other_host: true |
There was a problem hiding this comment.
Since this is only settable by admins this should be fine - I would leave a quick comment here explaining that
Co-authored-by: Samuel Fernandez <[email protected]>
Summary of the problem
We want people to be able to share links that will redirect to pages such as hackclub.com/hcb or hcb.hackclub.com/mobile
closes #12329
Describe your changes