Warn when cookie domain is set to an IP #2105

jjoliveira · 2016-12-04T22:04:34Z

Possible solution for issue #2007, it gives a warning message when an IP is set as SESSION_COOKIE_DOMAIN as user untitaker asked us to do, SERVER_DOMAIN isn't checked because he didn't remembered why it was needed, but the function is_IP in /flask/helpers is prepared to it. If you have any doubt feel free to ask.

update

flask/helpers.py

@@ -958,3 +958,33 @@ def total_seconds(td):
    :rtype: int
    """
    return td.days * 60 * 60 * 24 + td.seconds
+
+def is_IP(string, var_name):


flask/sessions.py

@@ -332,6 +332,7 @@ def open_session(self, app, request):

    def save_session(self, app, session, response):
        domain = self.get_cookie_domain(app)
+        is_IP(domain, "SESSION_COOKIE_DOMAIN")


ThiefMaster · 2016-12-04T22:18:54Z

I think instead of string operations the proper way to check whether it's an IP is using socket.inet_pton() and returning True if it doesn't raise, otherwise False.

Something like this:

for family in (socket.AF_INET, socket.AF_INET6):
    try:
        socket.inet_pton(family, ip)
    except socket.error:
        pass
    else:
        return True
return False

jjoliveira · 2016-12-04T22:39:27Z

@ThiefMaster What should be the category of the warning message?

ThiefMaster · 2016-12-04T22:41:52Z

maybe RuntimeWarning... I'd check if Flask uses any other warnings (that are not deprecation-related) and possibly use the same type

Also, tests are failing since you don't handle the case where the domain is unset, i.e. None

jjoliveira · 2016-12-04T23:10:55Z

@ThiefMaster How can I get the socket of the session when I call the function is_ip() in the save_session() function?

ThiefMaster · 2016-12-04T23:11:30Z

It's a function from python's socket module, so you simple import socket at the top of the file...

flask/sessions.py

@@ -332,7 +332,8 @@ def open_session(self, app, request):

    def save_session(self, app, session, response):
        domain = self.get_cookie_domain(app)
-        is_IP(domain, "SESSION_COOKIE_DOMAIN")
+        if domain != None:


flask/sessions.py

@@ -332,6 +332,8 @@ def open_session(self, app, request):

    def save_session(self, app, session, response):
        domain = self.get_cookie_domain(app)
+        if domain != None:
+            is_ip(domain, "SESSION_COOKIE_DOMAIN", self)


jjoliveira · 2016-12-04T23:41:15Z

@ThiefMaster Sorry for that, in my last commit all the unit tests are passing. I also did the changes you asked for.

flask/sessions.py

@@ -332,6 +333,9 @@ def open_session(self, app, request):

    def save_session(self, app, session, response):
        domain = self.get_cookie_domain(app)
+        if domain is not None:


flask/sessions.py

@@ -332,6 +333,9 @@ def open_session(self, app, request):

    def save_session(self, app, session, response):
        domain = self.get_cookie_domain(app)
+        if domain is not None:
+            if is_ip(domain):
+                warnings.warn("IP introduced in SESSION_COOKIE_DOMAIN", RuntimeWarning)


davidism · 2017-05-12T05:35:17Z

@untitaker can you make a decision on this?

untitaker · 2017-05-12T10:11:06Z

I think the code is fine except for my review comments.

…

On Thu, May 11, 2017 at 10:35:23PM -0700, David Lord wrote: @untitaker can you make a decision on this? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #2105 (comment)

davidism · 2017-05-12T17:52:26Z

Setting SESSION_COOKIE_DOMAIN = '127.0.0.1:5000' works fine. The issue is when it's detected through SERVER_NAME instead, in which case Flask adds a '.' before the "domain", which Chrome doesn't like (rightly so, an IP isn't a domain). We can just use the is_ip function to decide to remove the '.' prefix and things will work as expected.

davidism · 2017-05-13T18:32:07Z

I don't like running is_ip for every request when ideally domain will not be an IP.

davidism · 2017-05-14T05:00:55Z

Continued in #2282.

mferreira96 and others added 3 commits December 4, 2016 17:48

Merge pull request #3 from pallets/master

e1639f3

update

first attempt to solve issue #2007

7870862

is_IP() made more versatile and added some comments

21dea9f

ThiefMaster reviewed Dec 4, 2016

View reviewed changes

flask/helpers.py

@@ -958,3 +958,33 @@ def total_seconds(td):

:rtype: int

"""

return td.days * 60 * 60 * 24 + td.seconds

def is_IP(string, var_name):

This comment was marked as off-topic.

Sign in to view

ThiefMaster reviewed Dec 4, 2016

View reviewed changes

flask/sessions.py

@@ -332,6 +332,7 @@ def open_session(self, app, request):

def save_session(self, app, session, response):

domain = self.get_cookie_domain(app)

is_IP(domain, "SESSION_COOKIE_DOMAIN")

This comment was marked as off-topic.

Sign in to view

is_IP() renamed to is_ip() and now when domain is handled

2c8a44f

ThiefMaster reviewed Dec 4, 2016

View reviewed changes

flask/sessions.py

@@ -332,7 +332,8 @@ def open_session(self, app, request):

def save_session(self, app, session, response):

domain = self.get_cookie_domain(app)

is_IP(domain, "SESSION_COOKIE_DOMAIN")

if domain != None:

This comment was marked as off-topic.

Sign in to view

ThiefMaster reviewed Dec 4, 2016

View reviewed changes

flask/sessions.py

@@ -332,6 +332,8 @@ def open_session(self, app, request):

def save_session(self, app, session, response):

domain = self.get_cookie_domain(app)

if domain != None:

is_ip(domain, "SESSION_COOKIE_DOMAIN", self)

This comment was marked as off-topic.

Sign in to view

asked changes made, code tested and flask unittests all passed

56b33ab

untitaker reviewed Dec 7, 2016

View reviewed changes

davidism changed the title ~~#2007~~ Warn when cookie domain is set to an IP Apr 19, 2017

davidism mentioned this pull request May 14, 2017

Refactor session cookie domain #2282

Merged

davidism closed this May 14, 2017

github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when cookie domain is set to an IP #2105

Warn when cookie domain is set to an IP #2105

jjoliveira commented Dec 4, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

ThiefMaster commented Dec 4, 2016 •

edited

Loading

jjoliveira commented Dec 4, 2016

ThiefMaster commented Dec 4, 2016

jjoliveira commented Dec 4, 2016

ThiefMaster commented Dec 4, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

jjoliveira commented Dec 4, 2016 •

edited

Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

davidism commented May 12, 2017

untitaker commented May 12, 2017 via email

davidism commented May 12, 2017

davidism commented May 13, 2017 •

edited

Loading

davidism commented May 14, 2017

Warn when cookie domain is set to an IP #2105

Warn when cookie domain is set to an IP #2105

Conversation

jjoliveira commented Dec 4, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

ThiefMaster commented Dec 4, 2016 • edited Loading

jjoliveira commented Dec 4, 2016

ThiefMaster commented Dec 4, 2016

jjoliveira commented Dec 4, 2016

ThiefMaster commented Dec 4, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

jjoliveira commented Dec 4, 2016 • edited Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

davidism commented May 12, 2017

untitaker commented May 12, 2017 via email

davidism commented May 12, 2017

davidism commented May 13, 2017 • edited Loading

davidism commented May 14, 2017

ThiefMaster commented Dec 4, 2016 •

edited

Loading

jjoliveira commented Dec 4, 2016 •

edited

Loading

davidism commented May 13, 2017 •

edited

Loading