Skip to content

Add config option for report URI in CSP#761

Merged
SISheogorath merged 1 commit intohackmdio:masterfrom
SISheogorath:feature/reportURI
Mar 14, 2018
Merged

Add config option for report URI in CSP#761
SISheogorath merged 1 commit intohackmdio:masterfrom
SISheogorath:feature/reportURI

Conversation

@SISheogorath
Copy link
Copy Markdown
Contributor

This option is needed as it's currently not possible to add an report
URI by the directives array. This option also allows to get CSP reports
not only on docker based setup but also on our heroku instances.

Improves CSP support #594

@SISheogorath SISheogorath added enhancement Wants to improvide an existing feature security labels Mar 10, 2018
@SISheogorath SISheogorath added this to the 1.1.0-CE Release milestone Mar 10, 2018
@SISheogorath SISheogorath requested a review from ccoenen March 10, 2018 19:40
Copy link
Copy Markdown
Contributor

@literalplus literalplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me from a code perspective (without testing)

#594 (comment)

Comment thread lib/csp.js Outdated

function addReportURI (directives) {
if (config.csp.reportURI) {
directives.reportURI = config.csp.reportURI
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? The docs have reportUri and fields are case-sensitive, right?

I think it would make sense to use reportUri for the config option as well, for consistency with their format – that way we can't have confusions with their documentation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. But yes, it worked ^^

This option is needed as it's currently not possible to add an report
URI by the directives array. This option also allows to get CSP reports
not only on docker based setup but also on our heroku instances.

Signed-off-by: Sheogorath <sheogorath@shivering-isles.com>
@SISheogorath SISheogorath merged commit 9cbe03d into hackmdio:master Mar 14, 2018
@SISheogorath SISheogorath deleted the feature/reportURI branch March 14, 2018 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Wants to improvide an existing feature security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants