Hi,
At the moment, I can tell the seckit module to use relative URLs for the "report-uri" directive. I am thinking of using a public CSP receiver such as https://report-uri.io/ . But to do that, I have to be able to enter an absolute URL for the "report-uri" directive. Are you going to entertain such an idea if I am able to provide a patch?

Thanks,
Adnan

CommentFileSizeAuthor
#11 2656292-11.patch10.14 KBmcdruid
#6 seckit-absolute-uri-2656292-6.patch3.22 KBsvenryen
#5 seckit-report-uri-2656292-5.patch10.12 KBprogga

Comments

progga created an issue. See original summary.

jweowu’s picture

jweowu commented

Apologies for the delayed response, and yes, I would be happy to add such support.

Please do go ahead with a patch.

progga’s picture

progga commented
Assigned: Unassigned » progga

Thanks for the go ahead signal. I am working on it now. I will use the following paths and URLs in the web test case file:
- report-csp-violation
- /report-csp-violation
- https://report-uri.io/report/DrupalSeckitTest
- //report-uri.io/report/DrupalSeckitTest

Just let me know if you want any other paths or URLs.

progga’s picture

progga commented

Before changing any code, I ran the existing tests. The tests took about an hour to run! Here is the output:

$ drush test-run SecKitTestCase --uri=http://localhost/drupal-7.x/
Security Kit functionality 392 passes, 2 fails, 0 exceptions, and 112 debug messages       [error]
Test SecKitTestCase->testDisabledXContentTypeOptions() failed: X-Content-Type-Options is   [error]
disabled.
Test SecKitTestCase->testOriginDeny() failed: Request is denied.                           [error]
progga’s picture

progga commented
Assigned: progga » Unassigned
Status: Active » Needs review
StatusFileSize
new seckit-report-uri-2656292-5.patch10.12 KB

Patch is ready for review.

Few points to note:
- I haven't run the coding standard checker as it was giving too many errors in the existing code. Hope it's okay.
- The two test errors mentioned in comment #4 still exists. It doesn't look like I have broken anything new.
- I am happy to provide a similar patch for the 8.x-1.x branch if necessary.
- The report-uri URL https://report-uri.io/report/DrupalSeckitTest used in the test file do exists. report-uri.io is a free service, so it is easy to test :)

Thanks,
Adnan

svenryen’s picture

svenryen commented
StatusFileSize
new seckit-absolute-uri-2656292-6.patch3.22 KB

The patch from #5 didn't apply. Here's a patch that fixes the problem (with a different approach).

catalano’s picture

catalano commented

I thought this was included in release 1.7. According to this issue thread (https://www.drupal.org/project/seckit/issues/2091627) item #5 indicates that it was applied to release 1.7.

jweowu’s picture

jweowu commented

No, the changes in #2091627: CSP report-uri and policy-uri directives are relative to current URL rather than to base URL fixed a bug which caused the wrong URLs to be generated, but the configured values still had to be relative. This issue is to allow absolute URL values to be configured, so that an external domain can be used.

catalano’s picture

catalano commented

I see. Are they any patches available that are awaiting being built into a release?

mcdruid’s picture

mcdruid commented
Related issues: +#2980962: Allow absolute URLs in the report-uri directive

Linking the D8 issue for the same thing.

mcdruid’s picture

mcdruid commented
StatusFileSize
new 2656292-11.patch10.14 KB

It looks like patch #6 was created from outside the seckit directory, and removes a lot of useful things like validation and tests.

I made a couple of small tweaks to the patch from #5 - mostly coding standards / consistency (partly with the D8 patch).

I think this looks good - my only niggle is that the tests it adds seem quite long-winded and repetitive. They could possibly be refactored to loop over an array of test data or something similar (see the D8 patch in the linked issue).

AFAICS the only test which fails though is "X-Content-Type-Options is disabled." which is being addressed in other issues.

  • mcdruid committed 25e04b4 on 7.x-1.x authored by progga 
    Issue #2656292 by progga, mcdruid, svenryen: Absolute URL for report-uri
mcdruid’s picture

mcdruid commented
Status: Needs review » Fixed

Committed without refactoring the tests; if anyone wants to do some work polishing them, patches welcome.

Thanks to all who contributed!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.