Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CSP headers to unauthenticated controller #1474

Merged
merged 1 commit into from
Jul 18, 2022

Conversation

paulomarg
Copy link
Contributor

@paulomarg paulomarg commented Jul 12, 2022

What this PR does

As per #1389 (comment) in a different attempt at adding CSP headers, we should make sure to only include the frame-ancestors directive on requests that are actually for embedded apps.

This PR attempts to solve that problem using the suggested lambda approach from the Rails docs and creating a controller concern specifically for this purpose, which is also included in the existing EmbeddedApp concern, so that controllers marked as embedded app ones are always setting the appropriate value for that directive.

Reviewer's guide to testing

If you use a local clone of this gem in an app created with the CLI's ruby template, you should see this header in the document request for / when loading the app within the admin:

content-security-policy: frame-ancestors https://<shop>.myshopify.com https://admin.shopify.com;

Things to focus on

  1. Is setting the directive on this concern enough? Are there other places we should be doing that?

Checklist

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users

@paulomarg paulomarg requested a review from a team July 12, 2022 16:02
@paulomarg paulomarg force-pushed the add_csp_headers_for_embedded_apps branch 2 times, most recently from fb869cd to 87f0433 Compare July 12, 2022 16:05
@luigi-zarrelli-latori
Copy link

luigi-zarrelli-latori commented Jul 13, 2022

@paulomarg did you already test if this PR passes the app submission security robot? I tried a similar approach and it didn't pass it, sadly there's no further information or details provided in the rejection email.

Update: seems like it passes 👍 (no rejection by the robot yet for over 1 hour), thanks!

@bernardoamc
Copy link

@luigi-zarrelli-latori Do you mind sharing the name of the app used? We would also like to double check to guarantee that the checks are passing. And thank you for testing! 🙏

@luigi-zarrelli-latori
Copy link

luigi-zarrelli-latori commented Jul 14, 2022

@luigi-zarrelli-latori Do you mind sharing the name of the app used? We would also like to double check to guarantee that the checks are passing. And thank you for testing! 🙏

# frozen_string_literal: true

module EmbeddedAppFrameAncestors
  extend ActiveSupport::Concern

  included do
    content_security_policy do |policy|
      policy.frame_ancestors(-> { "https://#{current_shopify_domain} https://admin.shopify.com;" })
    end
  end
end

Version: shopify_app (19.1.0) SPA and only included in the home controller. So far no rejection 🤞

Oh sorry I think I misunderstood you @bernardoamc Tax Exempt Manager 2 is the name of the app.

@bernardoamc
Copy link

@luigi-zarrelli-latori Thank you, I can confirm that your app passed the check! 🎉

@paulomarg paulomarg force-pushed the add_csp_headers_for_embedded_apps branch from 87f0433 to 90840f9 Compare July 18, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants