atuttle / Taffy

:candy: The REST Web Service framework for ColdFusion and Lucee
http://taffy.io
Other
226 stars 118 forks source link

Add support for CSP Reporting & ndJSON #397

Closed JamoCA closed 2 years ago

JamoCA commented 4 years ago

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.

atuttle commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

atuttle commented 4 years ago

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.

JamoCA commented 4 years ago

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.)

atuttle commented 4 years ago

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.