Bug fix/favicon referrerpolicy 2895

[devanyk2]

fixes 2895

test help: I have a test for the code modification but the code requires the environment to be in production mode, and I am having trouble getting that working in the test.

[johrstrom]

test help: I have a test for the code modification but the code requires the environment to be in production mode, and I am having trouble getting that working in the test.

It looks like you've uncovered a bit of technical debt. The real solution I think is to change the code to not toggle off of the environment, but instead to toggle off of whether or not @user_configuration.public_url.join('favicon.ico') is legitimate or not. Without looking at that object I can't really say if it'll be easy to make that determination or not.

[devanyk2]

It looks like you've uncovered a bit of technical debt. The real solution I think is to change the code to not toggle off of the environment, but instead to toggle off of whether or not @user_configuration.public_url.join('favicon.ico') is legitimate or not. Without looking at that object I can't really say if it'll be easy to make that determination or not.

Gotcha, I'll look into making that change.

[devanyk2]

I don't think there is a way to verify if the value of public_uri is legitimate or not as the default value points to the default public asset location, and any user-provided value should be assumed to be legitimate.

Given that the default URI for the favicon is valid, does the favicon location need to be conditional?

 def favicon
      favicon_link_tag('favicon.ico', href: @user_configuration.public_url.join('favicon.ico'), skip_pipeline: true,  referrerpolicy: 'origin')
  end

[johrstrom]

Given that the default URI for the favicon is valid, does the favicon location need to be conditional?

Doesn't sound like it.

[johrstrom]

Ummm the default /public/favicon.ico doesn't seem to work in the test environment.

[devanyk2]

Apologies, I did not realize that the bin/rake test command did not run the system tests.
I've reverted the changes to the favicon function and instead modified the test cases to mock the Rails.env.production? function.

[devanyk2]

I'm not sure what caused these test failures, and I can't replicate them. Could they be transient?

Failure:
BatchConnectWidgetsTest#test_path_selector_handles_no_provided_file_pattern [test/system/batch_connect_widgets_test.rb:187]:
Expected false to be truthy.

Error:
ProjectManagerTest#test_submitting_a_script_with_auto_attributes_that_succeeds:
Errno::ENOTEMPTY: Directory not empty @ dir_s_rmdir - /tmp/d[202](https://github.com/OSC/ondemand/actions/runs/13420049992/job/37498656313?pr=4166#step:12:203)30327-15151-c5ftpl/projects
    test/system/project_manager_test.rb:379:in `block in <class:ProjectManagerTest>'

[johrstrom]

I'm not sure what caused these test failures, and I can't replicate them. Could they be transient?

Yea it's #4105

[devanyk2]

Awesome, thank you for all your help and time!

Is there anything else I need to do on my end?

[johrstrom]

Is there anything else I need to do on my end?

Nope! Thanks for the bug fix.