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 support for CSP Reporting & ndJSON #397

Closed
wants to merge 3 commits into from
Closed

Add support for CSP Reporting & ndJSON #397

wants to merge 3 commits into from

Conversation

JamoCA
Copy link
Contributor

@JamoCA JamoCA commented Sep 7, 2020

Resolves #383

CSP Reporting (application/csp-report) posts use standard JSON syntax, while ndJSON (application/x-ndjson) requires to be reformatted as an array of structs.

CSP Reporting (`application/csp-report`) posts use standard JSON syntax, while ndJSON (`application/x-ndjson`) requires to be reformatted as an array of structs.
@atuttle
Copy link
Owner

atuttle commented Oct 1, 2020

Interesting. 🤔

I assume the CSP reports part is for automted CSP violation reporting?

Adding NDJson support is an interesting idea. I don't have any problems with it, especially since it's just a couple of lines. I assume you have a use-case in mind?


The code additions look good to me. We'll need some documentation to go with them. Would you mind also submitting a PR to @atuttle/TaffyDocs so that I can merge both at the same time? It should be added to the 3.3.0.md file.

since these are not new methods or configuration, and for lack of an existing designated location for them, please add a section immediately under Override the request method with a header with the heading "Special case request content types" with a brief description of each, and any relevant canonical links that would be helpful.

@JamoCA
Copy link
Contributor Author

JamoCA commented Oct 2, 2020

I recently worked with Campaigner.com. Their webhook options were either 1) a multiple form post (with duplicate field names) or 2) ndJSON. Since ndJSON isn't valid JSON, the Taffy core was ignoring it unless the string is wrapped in an array.

All of my personal tests were successful, but their webhook was still broken. I finally discovered that their webhook wasn't sending content-type=application/x-ndjson headers, so I had to add the following logic until they could communicate the full RFC4288 specification to their team.

NOTE: Add this before the code you just added if you wish. You could even modify it in the case an invalid application/json content-type header is used.

    <cfif not len(requestObj.contentType) and isSimpleValue(requestObj.body)
        and len(requestObj.body) and isJSON("[" & javacast("string", requestObj.body).replace("\n", ",") & "]")>
        <cfset requestObj.contentType = "application/x-ndjson">
    </cfif>

@atuttle
Copy link
Owner

atuttle commented Oct 2, 2020

I recently worked with Campaigner.com. Their webhook options were either 1) a multiple form post (with duplicate field names) or 2) ndJSON. Since ndJSON isn't valid JSON, the Taffy core was ignoring it unless the string is wrapped in an array.

All of my personal tests were successful, but their webhook was still broken. I finally discovered that their webhook wasn't sending content-type=application/x-ndjson headers, so I had to add the following logic until they could communicate the full RFC4288 specification to their team.

Sounds good to me. This should make a fine addition.

NOTE: Add this before the code you just added if you wish. You could even modify it in the case an invalid application/json content-type header is used.

    <cfif not len(requestObj.contentType) and isSimpleValue(requestObj.body)
        and len(requestObj.body) and isJSON("[" & javacast("string", requestObj.body).replace("\n", ",") & "]")>
        <cfset requestObj.contentType = "application/x-ndjson">
    </cfif>

I don't fully follow what you're suggesting here. FYI if you make the change to your patch-2 branch and push it up to github, that will automatically update the PR here.

Update to [ndJSON](http://ndjson.org/) string detection as some service providers don't follow [RFC4288](https://www.ietf.org/rfc/rfc4288.txt) and fail to pass `application/x-ndjson` as the `content-type` header.
Copy link
Contributor Author

@JamoCA JamoCA left a comment

Choose a reason for hiding this comment

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

Updates to ndJSON detection as some service provider don't pass correct content-type headers. Also update to detect browser CSP submissions as json.

@atuttle
Copy link
Owner

atuttle commented Oct 4, 2020

That makes sense, thanks. Code updates look good. Just need some sort of documentation about these improvements so they don't become forgotten/undocumented features.

A major data provider uses ndJSON, but doesn't send `content-type` headers. Add support to correctly set type to `application/json` if body is JSON without any new lines.
@JamoCA
Copy link
Contributor Author

JamoCA commented Oct 5, 2020

I've updated the documentation. I also updated the detection based on my experience with a third-party provider that isn't passing any content-type headers and may pass multi or single line ndJSON. (In this case on a single recorded, a new line character may not be submitted.)

<cfif isSimpleValue(requestObj.body) and listlen(requestObj.Body,chr(10)) GT 1 and not isJSON(requestObj.body) and isJSON("[" & javacast("string", requestObj.body).replace("\n", ",") & "]")>
<cfif not len(requestObj.contentType) and isSimpleValue(requestObj.body) and isJSON(requestObj.body)>
<cfset requestObj.contentType = "application/json">
<cfelseif isSimpleValue(requestObj.body) and listlen(requestObj.Body,chr(10)) GT 1 and not isJSON(requestObj.body) and isJSON("[" & javacast("string", requestObj.body).replace("\n", ",") & "]")>
Copy link
Owner

Choose a reason for hiding this comment

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

Just had a thought: what if there's only 1 row in the ndJson body? your listLen() param will evaluate to false. 🤔

@atuttle
Copy link
Owner

atuttle commented Oct 6, 2020

Also:

I've updated the documentation.

Where? I don't see a PR in the TaffyDocs repo for it, nor do I see it in any of the comments in this PR. Granted it's getting late here and my eyes are tired, so maybe I'm just missing it.

@atuttle atuttle modified the milestones: Taffy-3.3, Taffy-NEXT Dec 17, 2021
@atuttle atuttle added the Semver-Minor This change will necessitate a minor version bump label Dec 17, 2021
@atuttle atuttle removed this from the Taffy-NEXT milestone Jan 15, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CSP Support ("application/csp-report" content type)
2 participants