Repository navigation
feat(integrations): Added Integrity policy - #6465
GemLunaMarine wants to merge 7 commits into
Conversation
loewenheim
left a comment
loewenheim
left a comment
There was a problem hiding this comment.
Thank you for the contribution! I have some nits and a bigger comment about attributes, but on the whole this looks very good. You're definitely on the right track.
| .route("/api/{project_id}/security/", security_report::route(config)) | ||
| .route("/api/{project_id}/csp-report/", security_report::route(config)) | ||
| .route("/api/{project_id}/nel/", nel::route(config)) | ||
| .route("/api/{project_id}/integration/integrity/", integrations::integrity::route(config)) |
There was a problem hiding this comment.
This belongs further down with the other integration routes.
| use relay_protocol::{Annotated, Empty, FromValue, IntoValue, Object, Value}; | ||
|
|
||
| /// Generated Integrity policy. | ||
| /// Can't name as "BodyRaw", like in nel.rs, due to name conflicts |
There was a problem hiding this comment.
i think this name is preferable anyway if we're going to pub use it.
| /// Models the content of a Integrity report. | ||
| #[derive(Debug, Default, Clone, PartialEq, FromValue, IntoValue, Empty)] | ||
| pub struct IntegrityReportRaw { | ||
| /// The age of the report since it got collected and before it got sent. |
There was a problem hiding this comment.
| /// The age of the report since it got collected and before it got sent. | |
| /// The time between collecting and sending the report. |
| add_string_attribute!("sentry.origin", "auto.http.browser_report.integrity"); | ||
| add_string_attribute!("browser.report.type", "integrity-violation"); | ||
|
|
||
| // Handle URL and extract server address if available | ||
| if let Some(url_str) = raw_report.url.value() { | ||
| let url_domain = extract_server_address(url_str); | ||
| add_string_attribute!("url.domain", &url_domain); | ||
| } | ||
| add_attribute!("url.full", raw_report.url); | ||
|
|
||
| // Integrity-specific attributes | ||
| add_attribute!("integrity.document_url", body.document_url); | ||
| add_attribute!("integrity.blocked_url", body.blocked_url); | ||
| add_attribute!("integrity.destination", body.destination); | ||
| add_attribute!("integrity.report_only", body.report_only); |
There was a problem hiding this comment.
Please use the attribute constants defined in use relay_conventions::attributes, e.g. SENTRY__ORIGIN instead of "sentry.origin". Those constants are generated from the sentry_conventions repo and make it easier for us to check that we aren't using something that isn't defined. We should change this in the nel module, too.
On a related note, you will need to define the new integrity attributes in sentry-conventions.
|
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
Also merge or rebase onto |
|
Remaining issues:
|
loewenheim
changed the title
There was a problem hiding this comment.
Are you intentionally changing this submodule?
There was a problem hiding this comment.
Oh, apologies. I think i accidentally added that. Lemme remove it.
This branch has not been deployed
3 participants
Browser-sent reports of type "integrity-violation" is not yet supported in Sentry Relay, and get rejected with "Invalid CSP". This Pull Request allows Relay to accept and send Integrity policy reports to Sentry backend. No Sentry backend changes are required, as it uses OurLog and is already formatted and correctly accepted into backend + frontend (except maybe like optimization or something).
Currently, testcases are not implemented (waiting to see if current direction is correct before creating tests). Current file lacking testcases is "/relay-event-normalization/src/normalize/integrity.rs".
Not sure if docs should have integration links (not just /integration/integrity/ but also for stuff like /integration/vercel/...). Should also probably update documentation to allow users to setup and use integrity reporting.
https://github.com/getsentry/sentry-docs/blob/master/docs/product/relay/operating-guidelines.mdx
Implementation to solve issue:
getsentry/sentry#124388
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.