nginx/csp/frontend: ensure POSTs to Sentry are not blocked - #2066
Conversation
alxndrsn
changed the title
alxndrsn
changed the title
matthew-white
left a comment
matthew-white
left a comment
There was a problem hiding this comment.
I didn't take a close look at everything in test/, but I saw that there were some good new tests. The changes to files/ make sense to me. 👍 I can take a closer look at test/ later, but I don't feel like that needs to block merge. I think this PR can be merged at any point for inclusion in the upcoming patch release.
There was a problem hiding this comment.
Just making a note that I didn't review this file.
There was a problem hiding this comment.
Why are these changes needed for this PR out of curiosity?
There was a problem hiding this comment.
- generating SSL certs is very slow
- the existing
nginx-ssl-selfsignis explicitly testing the generation of SSL certs - the new tests start new containers repeatedly, but existence of SSL certs is only incidental to those tests
So either the tests can be really slow, or use pre-generated certs.
| > /etc/nginx/conf.d/redirector.conf | ||
|
|
||
| CERT_DOMAIN=$( [ "$SSL_TYPE" = "customssl" ] && echo "local" || echo "$DOMAIN") \ | ||
| SENTRY_DSN_FRONTEND_ROOT="$(awk 'sub(/:\/\/[a-z0-9]*@/, "://") && sub(/\/[0-9]*$/, "")' <<<"$SENTRY_DSN_FRONTEND")" \ |
There was a problem hiding this comment.
@alxndrsn what about DSN with a trailing slash? that could easily bite back the system admin
There was a problem hiding this comment.
👍 Please suggest a new test case.
There was a problem hiding this comment.
If an admin accidentally includes a trailing slash in SENTRY_DSN_FRONTEND, for example: https://.....ingest.sentry.io/1234567/
There was a problem hiding this comment.
Hmm yes I can see that surprising an admin. But I don't think it's a valid DSN per https://docs.sentry.io/concepts/key-terms/dsn-explainer/#the-parts-of-the-data-source-name-dsn, so I'd rather just treat it as invalid.
There was a problem hiding this comment.
Would it be helpful somewhere to validate the supplied DSN, and warn if it looks invalid?
There was a problem hiding this comment.
OK! If that's already not working in Sentry, agree to a reject and treat it as invalid
3 participants
Closes #2056
What has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
Could make the CSP more lax (e.g. the whole of sentry.org). This would be a smaller header, but would allow unexpected things from those domains.
How does this change impact users? Describe intentional behavior changes from code updates. What are the regression risks?
No effect expected.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.