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

CSP report only mode #3556

Closed
Tracked by #3499
Rich-Harris opened this issue Jan 26, 2022 · 7 comments
Closed
Tracked by #3499

CSP report only mode #3556

Rich-Harris opened this issue Jan 26, 2022 · 7 comments
Labels
feature / enhancement New feature or request
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the problem

It's generally considered a good idea to enable content-security-policy-report-only before enabling CSP, just in case it will break a bunch of stuff.

Describe the proposed solution

With this config, the header name would be content-security-policy-report-only instead of content-security-policy. Everything else would be unchanged.

// svelte.config.js
export default {
  kit: {
    csp: {
      reportOnly: true,
      directives: {...}
    }
  }
};

Alternatives considered

No response

Importance

nice to have

Additional Information

No response

This was referenced Jan 26, 2022
@Karlinator
Copy link
Contributor

Karlinator commented Jan 27, 2022

Full CSP and Report-Only can coexist completely independently, which can be particularly useful when tightening policies (you already have a CSP, but you want to remove some sources for instance).

Straightforward solution would be:

// svelte.config.js
export default {
  kit: {
    csp: {
      directives: {...},
      reportOnly: {...}
    }
  }
};

This would probably lead to a fair bit of duplication in people's configs, as you need to start by copying the whole directive you want to make changes to. I'm not sure we can do something about that without creating more confusion than we're solving if we start changing the basic structure of the headers.

@Rich-Harris Rich-Harris added the feature / enhancement New feature or request label Mar 3, 2022
@Rich-Harris Rich-Harris added this to the 1.0 milestone May 16, 2022
@elliott-with-the-longest-name-on-github
Copy link
Contributor

@Rich-Harris, I'm looking at the PR related to this by @mcmxcdev, and I've noticed a couple of things based on the MDN documentation:

This header is not supported inside a <meta> element.

and

The CSP report-uri directive should be used with this header, otherwise this header will be an expensive no-op machine.

Annoyingly, clicking ont he report-uri link takes you to a page warning all over the place that report-uri is deprecated, and you should specify both report-uri and report-to... but there are no examples on the Report Only page showing an example of how to use the report-to header. Ugh.

I think what this means for this feature is at least:

  • We do not generate <meta http-equiv="blah" /> tags for CSP Report Only in prerendering
  • We throw some error if a user supplies a config.kit.csp.reportOnly object not containing one of report-uri or report-to (there's really no reason to do this unless you just hate performance and enjoy shooting yourself in the foot)

What do you think?

@elliott-with-the-longest-name-on-github
Copy link
Contributor

@Rich-Harris

No rush on this one, but I'd like a resolution on a few questions before I put any more effort into it. I'm putting this comment on the issue rather than the PR, even though there's some technical discussion here, as issues tend to be more discoverable in the future if someone's having CSP issues and is looking for details.

First, Is the current implementation (as described above and implemented on the linked PR) what you're looking for?

Second, for testing: The CSP report-only mode is in a bit of a limbo right now, as the required directives report-url and report-to are deprecated and undergoing development respectively -- essentially, the Reporting API is destined to supersede the report-url directive. v0 of this API declared the report-to directive and the Report-To header. The Report-To header defines the reporting groups, and the report-to directive assigns the policy's (in this case, CSP) failure results to a reporting group. v1 of this (supported by Chromium and Edge, I believe), adds the Reporting-Endpoints header (see this article for a good summary), which moves group declaration from Report-To to Reporting-Endpoints. The report-to directive still just chooses an endpoint name to deliver reports to.

Essentially, if you want your CSP report-only to work in all environments:

  • Set report-to groupname; as a CSP directive
  • Set report-url https://www.mysite.com (deprecated, but for legacy support)
  • Set both a Report-To and a Reporting-Endpoints header declaring groupname (browsers supporting Reporting-Endpoints will ignore Report-To if receiving both)

Okay, with the background laid, here's the idea:

Common to both

  1. Set up an integration test app (probably just use basics?) with report-only on and both report-url and report-to set to send reports to respective endpoints (something like /csp/report-url and /csp/report-to)

report-url

  1. Create the reporting endpoint (/csp/report-url) -- it would need to persist the report (or at least confirmation that the report was received) somewhere. Where?
  2. In the test, make a request that would violate one of the CSP report-only directives
  3. Wait some number of seconds for the report to be delivered (is there a way we could actively await this rather than using a timer?)
  4. Check to see that the endpoint was called with the error we're expecting

report-to

  1. Add code to handle to add the Reporting-Endpoints header declaring a reporting group pointing at /csp/report-to. Since we're testing with Chrome, this is preferred over the Report-To header.
  2. In the test, make a request that would violate one of the CSP report-only directives
  3. Wait. With Reporting-Endpoints, it can take up to 60 seconds for reports to be delivered, as they're batched
  4. Check to see that the endpoint was called with the error we're expecting

So I suppose the biggest question is, how do we tackle the storage and retrieval of report data sent to endpoints? Do we just have the endpoint write a JSON file to some folder in the build directory? Do we implement that functionality in handle (where we could declare a global variable to hold the input, then return that variable when requested with another route)? And, can you think of a way we can proactively await the delivery of the report instead of setting a timer and "hoping" it's delivered?

Is this even something we want to test, or are there more targeted tests that would reduce complexity and provide similar benefits? I'd prefer being able to assert that the implementation works, but I also don't want to add tests that just verify spec behavior. It also may benefit us to have these tests set up, as when we inevitably run into some "my CSP doesn't work" edge-case, it'll be easier to debug and create a failing test.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

@Rich-Harris This obviously isn't super high on the priority list, but I wanted to make sure the issue (and associated PR) don't fall into the stale PR pit.

@Rich-Harris
Copy link
Member Author

Hey, sorry — totally dropped the ball on this. Tried to bring #5078 up to date with master but gh pr checkout 5078 failed on me (sometimes it does this, not sure why) so I can't work on the branch directly, and didn't want to create a new PR just for that. Luckily there's only a small merge conflict that's easily fixed.

Will review the implementation on the PR.

Man, I didn't realise the reporting API was such a mess! I love the fact that an API for reporting (among other things) deprecations is itself responsible for multiple deprecations. I think for our current purposes we can just ignore it and leave it as an exercise to the reader — if someone specifies 'report-to': ['foo'] then it's enough to test that the correct Content-Security-Policy/Content-Security-Policy-Report-Only headers are being generated; it's not our responsibility to test whether browsers are in fact correctly reporting violations.

I think the question is whether we want to verify that the reporting endpoints are being created, and/or whether we want to provide a way to add Report-To and Reporting-Endpoints headers automatically. I think there's probably some value in that, since they're finicky to setup, and otherwise there's an asymmetry where CSP headers are automatically added, but if they specify report-to then you have to manually add Report-To and Reporting-Endpoints inside handle.

Just spitballing:

export default {
  kit: {
    csp: {...},
    reporting: {
      'main-endpoint': {
        uri: '/reporting/main',
        maxage: 86400
      },
      default: {
        uri: '/reporting/default',
        maxage: 86400
      }
    }
  }
};

SvelteKit could then easily create the headers and report-to/report-uri directives and verify that the directives match the headers (i.e. the endpoints have been defined in reporting, and have a POST handler if they're implemented as SvelteKit endpoints).

However I don't think we need to do that as part of #5078 — I think it should be discussed and implemented separately.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

Agree about implementing built-in Report-To and Reporting-Endpoints as a separate discussion and PR, and also agree that it would be a good feature to add. It took me a couple of hours to figure out what the hell is going on with the Reporting API (the fact that multiple things are named Report-To, just with different capitalization was confusing as all get-out, and, as you said, we're nearing the point of needing a separate reporting API to report deprecations of the current Reporting API). Having everything defined in one place -- side-by-side in config -- would be a huge help. We could potentially even warn in dev if a reporting endpoint's URI doesn't have a corresponding endpoint to listen for reports.

I'll clean up the current PR, and when that's merged, I'll open a separate discussion on the Report-To and Reporting-Endpoints implementations.

@Rich-Harris
Copy link
Member Author

Opened #5634 for the reporting API headers stuff, so I'll close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / enhancement New feature or request
Projects
None yet
3 participants