What are your thoughts on implementing

$secure = true|false
$httponly = true|false

I propose we can do the following:
- extend the form BasicSettingsForm to provide 2 additional fields
- by default secure can be set to false
- by default httponly can be set to false
- extend the setrawcookie to use config settings.

What are your thoughts? See attched patch!

security

CommentFileSizeAuthor
#10 2925265-1.patch4.26 KBdakku
#4 Screen Shot 2017-11-21 at 4.59.41 pm.png53.3 KBdakku
#2 2925265.patch4.3 KBdakku
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dakku created an issue. See original summary.

dakku’s picture

dakku CreditAttribution: dakku as a volunteer commented
FileSize
2925265.patch4.3 KB
dakku’s picture

dakku CreditAttribution: dakku as a volunteer commented
Status: Active » Needs review
dakku’s picture

dakku CreditAttribution: dakku as a volunteer commented
FileSize
Screen Shot 2017-11-21 at 4.59.41 pm.png53.3 KB
dakku’s picture

dakku CreditAttribution: dakku as a volunteer commented
Issue summary: View changes
snufkin’s picture

snufkin CreditAttribution: snufkin at Cheppers for GatherContent commented

I think this would be a very useful addition and improvement to the module. But with making it default on the config retrieval part are we risking breaking legacy sites which do not have https?

dakku’s picture

dakku CreditAttribution: dakku as a volunteer commented

Hey Balazs,
cheers for the feedback. By default, both the options are unchecked. Therefore, legacy sites shouldnt be affected. This will only take effect if the option(s) are set.

+++ b/config/install/simplesamlphp_auth.settings.yml
@@ -23,3 +23,5 @@ sync:
   user_name: true
 autoenablesaml: false
 debug: false
+secure: false
+httponly: false
snufkin’s picture

snufkin CreditAttribution: snufkin at Cheppers for GatherContent commented

Then thumbs up from me! There are two warnings for missing newlines from dredit, but apart from that I don't see why we shouldn't add this.

  • dakku committed 50b7be9 on 8.x-3.x 
    Issue #2925265 by dakku: Protect Against XSS by Enabling Secure and...
dakku’s picture

dakku CreditAttribution: dakku as a volunteer commented
FileSize
2925265-1.patch4.26 KB

Slightly updated patch. Cheers for the review Balazs. This is now merged in and available in RC3

dakku’s picture

dakku CreditAttribution: dakku as a volunteer commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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