Test sprockets remove#6122
Conversation
|
|
||
| // otherincome checkbox fuctionality | ||
| $(document).on('turbolinks:load', function () { | ||
| function disableSave(form) { |
| form.find('.interaction-click-control-save').attr('disabled', 'disabled'); | ||
| } | ||
|
|
||
| function enableSave(form) { |
| } | ||
| }); | ||
|
|
||
| function validateForm(form) { |
| }); | ||
| }; | ||
|
|
||
| $(function() { initializeDataTables(); }); |
| }; | ||
|
|
||
| $(function() { initializeDataTables(); }); | ||
| $(document).on('turbolinks:load', function() { initializeDataTables(); }); |
| uri = URI.parse(return_to_path) rescue nil # rubocop:disable Lint/EmptyRescueClause | ||
| Rails.logger.info("In ApplicationController redirect_back_safely refferer -> #{return_to_path}, uri -> #{uri}, uri.host -> #{uri&.host}, request.host -> #{request&.host}") | ||
| if uri&.host == request.host | ||
| redirect_to return_to_path, flash: flash |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, to fix an untrusted redirect, we must not redirect to arbitrary user-provided URLs. Instead, we should either (a) validate that the URL is on the same host and uses an expected scheme, or (b) allow only relative paths and optionally validate them against a whitelist or pattern. Since there are multiple callers all going through redirect_back_safely, the best fix is to strengthen this helper so that any untrusted input passed to it is validated consistently.
Concretely, we will modify ApplicationController#redirect_back_safely (lines 118–125) to:
- Reject blank or nil
return_to_pathvalues and immediately redirect to the fallback. - Parse
return_to_pathwithURI.parsebut, importantly:- Treat relative paths (no scheme and no host, starting with
/) as safe and redirect to them. - For absolute URLs, allow them only if:
- The scheme is
httporhttps, and - The host matches
request.host.
- The scheme is
- Treat relative paths (no scheme and no host, starting with
- For anything else (invalid URI, different host, non-http(s) scheme, or suspicious path), use the fallback.
This change keeps the public interface of redirect_back_safely unchanged, so we do not need to touch the callers in Insured::ConsumerRolesController or Insured::GroupSelectionController; the new, stricter checks will automatically apply to all 11 CodeQL alert variants. We can implement this with plain Ruby and Rails, using the existing URI class that is already being used in the method. No new methods or imports are strictly required beyond the updated logic.
| @@ -116,10 +116,20 @@ | ||
| end | ||
|
|
||
| def redirect_back_safely(return_to_path, flash: {}, fallback: root_path) | ||
| # Guard against nil/blank values early. | ||
| if return_to_path.blank? | ||
| return redirect_to fallback, flash: flash | ||
| end | ||
|
|
||
| uri = URI.parse(return_to_path) rescue nil # rubocop:disable Lint/EmptyRescueClause | ||
| Rails.logger.info("In ApplicationController redirect_back_safely refferer -> #{return_to_path}, uri -> #{uri}, uri.host -> #{uri&.host}, request.host -> #{request&.host}") | ||
| if uri&.host == request.host | ||
| redirect_to return_to_path, flash: flash | ||
|
|
||
| # Allow relative paths that start with "/" and do not specify a scheme/host. | ||
| if uri && uri.scheme.nil? && uri.host.nil? && uri.path.present? && uri.path.start_with?("/") | ||
| redirect_to uri.to_s, flash: flash | ||
| # Allow absolute URLs only if they use http/https and point to this host. | ||
| elsif uri && %w[http https].include?(uri.scheme) && uri.host == request.host | ||
| redirect_to uri.to_s, flash: flash | ||
| else | ||
| redirect_to fallback, flash: flash | ||
| end |
| end | ||
| documents = @employer_profile.employer_attestation.employer_attestation_documents | ||
| raise "Sorry! You are not authorized to download this document." unless authorized_to_download? | ||
| uri = documents.find(params[:employer_attestation_id]).identifier |
Check warning
Code scanning / CodeQL
Sensitive data read from GET request Medium
| data: data, | ||
| url: url | ||
| }).done(function(data) { | ||
| window.location.href = data.url + "&profile_id=" + $("#profile_id").val(); |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
Copilot Autofix
AI 4 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| url: url | ||
| }).done(function(){ | ||
| var url = $("#add_dental_url").val() | ||
| window.location.href = url + "&profile_id=" + $("#profile_id").val() |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general terms: avoid directly trusting text pulled from the DOM when using it in a navigation context. Before assigning to window.location.href, restrict the URL to expected safe values (e.g., same-origin, HTTP(S), and optionally specific path patterns). This avoids situations where DOM text bypasses earlier encoding or where an attacker injects a dangerous URL (such as javascript:alert(1) or a cross-domain phishing URL).
Best specific fix here: validate and normalize the URL from #add_dental_url before using it. A robust, non-invasive approach is:
- Read the raw URL string from
#add_dental_url. - Attempt to construct a
URLobject from it:- If it’s relative, resolve it against
window.location.origin.
- If it’s relative, resolve it against
- Enforce that:
- The protocol is
http:orhttps:. - The hostname matches
window.location.hostname(to prevent open redirects), or at least is within allowed hosts.
- The protocol is
- If the URL fails validation, either:
- Abort navigation, or
- Fallback to a known safe URL.
- Only then build
window.location.hrefwith theprofile_idparameter, ideally usingencodeURIComponenton the profile id.
We only touch the shown snippet in AddDentalToPlanDesignProposal. Concretely, we will:
-
Replace:
var url = $("#add_dental_url").val() window.location.href = url + "&profile_id=" + $("#profile_id").val()
-
With code that:
- Reads
rawUrlandprofileId. - Safely constructs a
URLobject, resolving relative URLs. - Checks protocol and hostname.
- If valid, appends/overwrites the
profile_idquery parameter using the URL API (searchParams.set), ensuring correct escaping. - Assigns the resulting
url.toString()towindow.location.hrefonly if valid.
- Reads
This approach preserves existing behavior (navigate to whatever #add_dental_url points to, plus the profile_id parameter) but prevents malformed or hostile URLs from being used.
No new imports are needed; URL and window.location are standard Web APIs.
| @@ -658,8 +658,24 @@ | ||
| data: data, | ||
| url: url | ||
| }).done(function(){ | ||
| var url = $("#add_dental_url").val() | ||
| window.location.href = url + "&profile_id=" + $("#profile_id").val() | ||
| var rawUrl = $("#add_dental_url").val(); | ||
| var profileId = $("#profile_id").val(); | ||
| if (rawUrl) { | ||
| try { | ||
| // Resolve relative URLs against the current origin | ||
| var targetUrl = new URL(rawUrl, window.location.origin); | ||
| // Enforce same-origin and safe protocols | ||
| if ((targetUrl.protocol === "http:" || targetUrl.protocol === "https:") && | ||
| targetUrl.hostname === window.location.hostname) { | ||
| if (profileId) { | ||
| targetUrl.searchParams.set("profile_id", profileId); | ||
| } | ||
| window.location.href = targetUrl.toString(); | ||
| } | ||
| } catch (e) { | ||
| // Invalid URL; do not navigate | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| } |
| error: function(data) { | ||
| console.log("error fetching proposal url: " + data.url); | ||
| var assembled_url = data.url + "&profile_id=" + $("#profile_id").val(); | ||
| window.location.replace(assembled_url) |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
General approach: Ensure that any data coming from the DOM ($("#profile_id").val()) is (a) constrained to an expected format and (b) properly URL-encoded before being concatenated into a URL used for navigation. This removes the possibility of breaking the query string structure or injecting unexpected characters.
Best concrete fix here without changing existing behavior:
- Extract the
profile_idvalue into a local variable. - Optionally constrain it to an expected pattern (e.g., numeric or alphanumeric) using a simple validation; if it fails, either do not append it or handle the error.
- Apply
encodeURIComponentbefore appending it todata.url. - Use the encoded value both in the success handler (line 636) and in the error handler (line 641) to keep behavior consistent.
We only touch app/javascript/components/sponsored_benefits/plan_design_proposals.js within the shown snippet. No new imports are needed; encodeURIComponent is a standard global. The changes are:
- Inside
success: function(data) { ... }, introduce avar profileId = $("#profile_id").val();and then useencodeURIComponent(profileId)when concatenating todata.url. - Inside
error: function(data) { ... }, similarly extract and encodeprofileId, then buildassembled_urlwith the encoded value and pass that towindow.location.replace.
This preserves the existing flow and logic while ensuring that meta-characters in profile_id are safely encoded before being embedded in the URL.
| @@ -633,11 +633,15 @@ | ||
| profile_id: $("#profile_id").val() | ||
| }, | ||
| success: function(data) { | ||
| window.location.href = data.url + "&profile_id=" + $("#profile_id").val(); | ||
| var profileId = $("#profile_id").val(); | ||
| var safeProfileId = encodeURIComponent(profileId); | ||
| window.location.href = data.url + "&profile_id=" + safeProfileId; | ||
| }, | ||
| error: function(data) { | ||
| var profileId = $("#profile_id").val(); | ||
| var safeProfileId = encodeURIComponent(profileId); | ||
| console.log("error fetching proposal url: " + data.url); | ||
| var assembled_url = data.url + "&profile_id=" + $("#profile_id").val(); | ||
| var assembled_url = data.url + "&profile_id=" + safeProfileId; | ||
| window.location.replace(assembled_url) | ||
| } | ||
| }); |
| profile_id: $("#profile_id").val() | ||
| }, | ||
| success: function(data) { | ||
| window.location.href = data.url + "&profile_id=" + $("#profile_id").val(); |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, when taking text from the DOM and using it in contexts where meta-characters are meaningful (HTML, JavaScript, URLs, selectors), you should either (a) validate it against an expected pattern and reject anything unexpected, and/or (b) encode/escape it appropriately for the context (e.g., encodeURIComponent for query parameters, HTML escaping for HTML contexts).
For this specific issue, the problematic use is building a URL query string "...&profile_id=" + $("#profile_id").val() and assigning it to window.location.href. The safest, least intrusive fix that preserves existing functionality is to URL-encode the profile_id value using encodeURIComponent before concatenating it. This ensures any special characters in the DOM value are percent-encoded and cannot break out of the intended parameter value or introduce HTML/JavaScript into other contexts, while still yielding the same semantics for typical alphanumeric IDs.
Concretely, in app/javascript/components/sponsored_benefits/plan_design_proposals.js, inside the saveProposalAndPublish function, update line 636 to:
window.location.href = data.url + "&profile_id=" + encodeURIComponent($("#profile_id").val());and leave the rest of the function unchanged. No new imports or helper methods are required because encodeURIComponent is a built-in global in JavaScript.
| @@ -633,11 +633,11 @@ | ||
| profile_id: $("#profile_id").val() | ||
| }, | ||
| success: function(data) { | ||
| window.location.href = data.url + "&profile_id=" + $("#profile_id").val(); | ||
| window.location.href = data.url + "&profile_id=" + encodeURIComponent($("#profile_id").val()); | ||
| }, | ||
| error: function(data) { | ||
| console.log("error fetching proposal url: " + data.url); | ||
| var assembled_url = data.url + "&profile_id=" + $("#profile_id").val(); | ||
| var assembled_url = data.url + "&profile_id=" + encodeURIComponent($("#profile_id").val()); | ||
| window.location.replace(assembled_url) | ||
| } | ||
| }); |
| }, | ||
| dataType: 'json', | ||
| success: function(data) { | ||
| window.location.href = data.url + "&profile_id=" + $("#profile_id").val(); |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
Copilot Autofix
AI 4 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| <% end %> | ||
| <%# jQuery-migrate is disabled in development - uncomment if needed for debugging %> | ||
| <% if false && EnrollRegistry.feature_enabled?(:jquery_migrate) %> | ||
| <script src="https://code.jquery.com/jquery-migrate-3.4.1.min.js" crossorigin="anonymous"></script> |
Check warning
Code scanning / CodeQL
Inclusion of functionality from an untrusted source Medium
|
|
||
| <%# jQuery-migrate is disabled in development - uncomment if needed for debugging %> | ||
| <% if false && EnrollRegistry.feature_enabled?(:jquery_migrate) %> | ||
| <script src="https://code.jquery.com/jquery-migrate-3.4.1.min.js" crossorigin="anonymous"></script> |
Check warning
Code scanning / CodeQL
Inclusion of functionality from an untrusted source Medium
|
|
||
| <%# jQuery-migrate is disabled in development - uncomment if needed for debugging %> | ||
| <% if false && EnrollRegistry.feature_enabled?(:jquery_migrate) %> | ||
| <script src="https://code.jquery.com/jquery-migrate-3.4.1.min.js" crossorigin="anonymous"></script> |
Check warning
Code scanning / CodeQL
Inclusion of functionality from an untrusted source Medium
| <link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.2.0/css/all.css" integrity="sha384-hWVjflwFxL6sNzntih27bfxkr27PmbbK/iSvJ+a4+0owXq79v+lsFkW54bOGbiDQ" crossorigin="anonymous"> | ||
| <%# jQuery-migrate is disabled in development - uncomment if needed for debugging %> | ||
| <% if false && ::EnrollRegistry.feature_enabled?(:jquery_migrate) %> | ||
| <script src="https://code.jquery.com/jquery-migrate-3.4.1.min.js" crossorigin="anonymous"></script> |
Check warning
Code scanning / CodeQL
Inclusion of functionality from an untrusted source Medium
| <% end %> | ||
| <%# jQuery-migrate is disabled in development - uncomment if needed for debugging %> | ||
| <% if false && EnrollRegistry.feature_enabled?(:jquery_migrate) %> | ||
| <script src="https://code.jquery.com/jquery-migrate-3.4.1.min.js" crossorigin="anonymous"></script> |
Check warning
Code scanning / CodeQL
Inclusion of functionality from an untrusted source Medium
PR Checklist
Please check if your PR fulfills the following requirements:
lethelpers andbeforeblocks..html_safe.PR Type
What kind of change does this PR introduce?:
What is the ticket # detailing the issue?
Ticket:
A brief description of the changes:
Current behavior:
New behavior:
Feature Flag
For all new feature development, a feature flag is required to control the exposure of the feature to our end users. A feature flag needs a corresponding environment variable to initialize the state of the flag. Please share the name of the environment variable below that would enable/disable the feature and indicate which client(s) it applies to.
Variable name:
Additional Context
Include any additional context that may be relevant to the peer review process.