- last update
about 1 year ago Composer error. Unable to continue. - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
This works in my testing.
It adds a new pre render callback to any #type => html_tag elements that are of #tag script.
Turn it on with a checkbox.
Uses a lazy builder so its render cache safe
Adds an update hook and test coverage as well
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Please credit @dpi who came up with the element_info_alter idea
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Per chat with @larowlan this looks good to me at glance, but will ask @Rajeshreeputra for a proper review as they've volunteered to help maintain seckit ( 💬 Offering to maintain Security Kit Active ).
- Assigned to Rajeshreeputra
- Issue was unassigned.
- 🇦🇺Australia dpi Perth, Australia
Thanks, good to see this can be automated for third parties that participate in attaching scripts consistently with
html_tag
. - 🇦🇺Australia dpi Perth, Australia
Sorry about the unassign, must be a stale form^
- Assigned to Rajeshreeputra
- Status changed to Needs work
about 1 year ago 10:08am 19 June 2023 - 🇮🇳India Rajeshreeputra Pune
Patch applied cleanly:
On existing site and ran database update I can see option to enable Nounce under CSP.
Enabled the nounce and verified same it work as expected.
Overall changes looks good, the only weird thing noticed is:
+ /** + * Implements hook_form_FORM_ID_alter(). + */ +function seckit_test_form_seckit_settings_form_alter(array &$form, FormStateInterface $form_state, string $form_id): void { + $form['test_script'] = [ + '#type' => 'html_tag', + '#weight' => 50, + '#tag' => 'script', + '#attributes' => ['id' => 'testScript'], + '#value' => Markup::create('console.log("👋");'), + ]; +}
'#value' => Markup::create('console.log("👋");'), - this special characters in console log.
- Status changed to RTBC
about 1 year ago 11:03am 19 June 2023 - 🇦🇺Australia dpi Perth, Australia
Special characters look like a good sanity test to make sure things aren't encoded oddly / hashed correctly.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Yep, that's just test code, only used in the test
Happy to change it to any other inline JS - 🇨🇦Canada gapple
Adding a nonce to every inline script
html_tag
element gives me some concern, compared to requiring an attribute on the render element to specifically opt-in to having a nonce applied. I'm open to "it should be reasonable to assume that any content is sanitized / restricted from editing by non-privileged users / etc before being added to an html_tag element (and user's shouldn't be able to arbitrarily insert new html_tag elements)", but I'm wary of this feature being used to make warnings go away without exploring ways to address the root cause, and opening up unintended leniency in a site's policy.As more of a user knowledge or integration issue, adding a nonce to the policy will disable
'unsafe-inline'
and block any other scripts that rely on it.
While 9.5+ / 10.0+ no longer require'unsafe-inline'
forcore/drupal.ajax
to function, if any inline scripts are added to the page through a mechanism other than ahtml_tag
element (e.g. directly in a theme template, or with a raw markup element), they would be blocked by the policy if a nonce is included.
Any site continuing to use CKEditor 4 (from core in 9.x or contrib in 10.x) will break if this feature is used since CKE4 relies on inline attributes (which can't have a nonce applied). - Issue was unassigned.
- 🇦🇺Australia geoffreyr
@larowlan's patch works nicely for inline scripts, but to be able to use the 'strict-dynamic' directive, the nonce needs to be added to every script tag, not just inline JS. At some point I might take a crack at tweaking the patch to provide configuration for linked scripts as well, so we can harden our CSP config a bit further. That said, I'm probably missing something that's already out there that would make this work...
- last update
about 1 year ago 2 fail - 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
This seems like it's a pretty decent implementation of the feature.
Whether sites will actually want to use it is a somewhat different question; as with many aspects of CSP (and seckit's other features) it's hard to determine exactly what sane defaults should be; turning everything on will almost certainly break things.
Seems like a useful addition to the toolkit that the module provides though.
- Status changed to Needs work
about 1 year ago 2:18pm 27 July 2023 - 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Looks like 3x tests are failing for D9:
- Drupal\Tests\seckit\Functional\SecKitTestCaseTest::testCspWithoutVendorPrefixes
- Drupal\Tests\seckit\Functional\SecKitTestCaseTest::testCspWithCspVendorPrefix
- Drupal\Tests\seckit\Functional\SecKitTestCaseTest::testCspWithWebkitCspVendorPrefix
In each case, the new nonce is being added to the CSP but that's not what the assertion expects - e.g. the test got the following CSP header:
"default-src *; script-src * 'nonce--8mf3Awmh4ut1257OFh-xWL-yGx8g65B'; object-src *; style-src *; img-src *; media-src *; frame-src *; frame-ancestors *; child-src *; font-src *; connect-src *; report-uri /subdirectory/report-csp-violation; upgrade-insecure-requests"
...but expected:
"default-src *; script-src *; object-src *; style-src *; img-src *; media-src *; frame-src *; frame-ancestors *; child-src *; font-src *; connect-src *; report-uri /subdirectory/report-csp-violation; upgrade-insecure-requests"
So...
1) Do we expect the nonce to appear like this within the header?
2) If so, can we adjust these tests accordingly?
FWIW it looks like a fluke that the nonce began with a dash in the example above resulting in two consecutive dashes, this wasn't the case in the other two test fails.
- last update
about 1 year ago 2 fail - last update
6 months ago Patch Failed to Apply