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

Comments

dakku created an issue. See original summary.

dakku’s picture

dakku commented
StatusFileSize
new 2925265.patch4.3 KB
dakku’s picture

dakku commented
Status: Active » Needs review
dakku’s picture

dakku commented
StatusFileSize
new Screen Shot 2017-11-21 at 4.59.41 pm.png53.3 KB
dakku’s picture

dakku commented
Issue summary: View changes
snufkin’s picture

snufkin 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 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 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 commented
StatusFileSize
new 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 commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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