HOLD: Enable Content Security Policy for AWBW#1491
Conversation
|
Some tests are broken; going to look into them and fix. For now, switching to draft. |
|
@lenikadali i think these are flaky but am going to rebase and see if we get a green light |
7061e68 to
c69ec03
Compare
I rebased and it seems now the build is failing because there's a security vulnerability surfaced by |
Have updated to fix the security vulnerability and the failures are persisting. Will investigate 🧐 |
4781727 to
f852455
Compare
|
@maebeale these tests seem to be failing because of something related to "Turbo Stream", going by the logs: web-1 | Started PATCH "/stories/1" for 172.18.0.1 at 2026-05-22 10:23:13 +0000
web-1 | Processing by StoriesController#update as TURBO_STREAM
web-1 | Parameters: {"authenticity_token" => "[FILTERED]", "story" => {"title" => "Healing Through Art: A Survivor's Journey", "published" => "1", "featured" => "1", "publicly_visible" => "0", "publicly_featured" => "0", "windows_type_id" => "3", "workshop_id" => "21", "external_workshop_title" => "", "organization_id" => "8", "rhino_body" => "<p>Natus sit eum. Necessitatibus voluptas ipsa. Assumenda tenetur temporibus. Rerum quo aut. Deserunt et facilis. Fugiat sit dolore. Tenetur est est. Nisi consequuntur qui. Voluptas id provident. Sed at sit.</p>", "sector_ids" => ["", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", ""], "category_ids" => ["", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", ""], "author_credit_preference" => "", "youtube_url" => "https://youtube.com/watch?v=abcd1234xyz", "website_url" => "", "created_by_id" => "1", "spotlighted_facilitator_id" => "", "story_idea_id" => "", "updated_by_id" => "1"}, "commit" => "Save changes", "id" => "1"}Manually editing the story title as the user Umberto successfully shows the message "Story was successfully updated." on the page, so I can confirm that the feature still works as intended, without any errors in the logs. I am not as familiar with Turbo, so it would take me a bit longer to figure out a solution for the failing tests. |
jmilljr24
left a comment
There was a problem hiding this comment.
@lenikadali I'm happy with this! Thanks!!
Thank you for manually testing the story failure. I'm personally ok with you disabling that test and opening a new issue for it if you want to get this PR merged. I can work on the test/turbo at a later date.
Enables Content Security Policy for AWBW based on what we're already using in the codebase (the code is mostly vanilla Rails with minimal to no JavaScript) so much of the Rails defaults have removed.
f852455 to
93d7fc3
Compare
|
Currently the test in Have commented it out with a |
Commented out the "Log out and reset" and "Edit Story" tests
because they fail due to something about the tests' interaction
with Turbo Stream.
Manually, the test case is successful as the user is redirected
to the new_user_password path ("/users/password/new")
and the Story page does have the message "Story was successfully updated."
93d7fc3 to
fa43459
Compare
Disabled out the "does not allow login and shows locked message" test because it fails due to something about the test's interaction with Turbo Stream.
Closes #261
What is the goal of this PR and why is this important?
Enables Content Security Policy for AWBW based on what we're already using in the codebase.
The code is mostly vanilla Rails with minimal to no JavaScript so much of the Rails defaults have been removed.
We're adding this so that AWBW is not vulnerable to Cross-site scripting (XSS) attacks.
How did you approach the change?
I reviewed the issue, then read up on how Rails handles CSP configuration and compared the different options available to what the AWBW code is using. Important notes:
<%= csp_meta_tag %>is used when considering using'unsafe-inline'as per Rails guide here. Since we are not considering using'unsafe-inline'and it is not already used in the codebase, it has been omitted from the initial configuration.Anything else to add?
I could have left in the Vite/JavaScript lines but removing them keeps the file cleaner and easy to read for someone looking at it. Happy to restore the lines if there's a preference for preserving them 😄
Edit: Note on the PR itself
It seems enabling the Content Security Policy interferes with how end-to-end tests run. The currently commented out tests succeed when you follow the test steps in a development environment but fail in the GitHub workflow; for some reason Capybara no longer is able to find the banner/flash message with the appropriate message nor is it able to detect that the redirection has happened.
I think this may be related to the usage of Turbo Stream in the different paths under test (going by the logs) but I cannot say for sure. I tried some fixes I saw in the wild here and here, but the first two commented out tests continued to fail.
For now putting this on hold because the number of specs affected seems likely to grow and disabling them all doesn't seem like the right solution.