hard error for bad max-age

[jamesstidard]

See: #1810

Thanks

[codecov]

Codecov Report

Merging #1809 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1809   +/-   ##
=======================================
  Coverage   91.94%   91.94%           
=======================================
  Files          23       23           
  Lines        2309     2309           
  Branches      428      428           
=======================================
  Hits         2123     2123           
  Misses        143      143           
  Partials       43       43

Impacted Files	Coverage Δ
sanic/cookies.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60b4efa...1bdc9ac. Read the comment docs.

[Tronic]

Would you care to elaborate on the choice of type checking logic, and on the change to make non-integer float in tests a failure?

[jamesstidard]

Hey @Tronic, sure, sorry.

First, here's a bit of context that I maybe didn't cover in my issue very clearly.

I had some code that set this attribute as a float, and this worked in previous versions of Sanic. I then updated Sanic, and the float value of 604,800.0 was being silently coerced into 0.

It actually took me quite a while to figure out this was the problem for multiple reasons:

1. The code worked before

2. Debugging why cookies are not being set in browsers is generally quite hard. Browsers don't seem to log a reason of why they choose to ignore invalid cookies.

3. I was moving domains at the same time and the whole samesite attribute was new to me, so I went off down a rabbit hole thinking it had to do with that. Not spotting that actually now my max age was being set to 0.

4. choice of type checking logic

I think generally, it's better practice for code to error, then make implicit error corrections. So a hard error here would have let me catch this right away.

The reason this logic was changed in Sanic in the first place #1452 was to ensure that the value was an integer value and not some float. So I wrote this to allow any kind of integer looking thing (string integer, int, or float integer). Otherwise throw an error.

In retrospect I think that allowing float integers is also probably bad. If people are programatically generating their max-age length based on some dynamic values, then they might only hand in a float with integer values during their testing. However, later in production some requests may try and set non-integer float values. So it's probably better to error if a float and force the user to explicitly math.ceil/math.floor it into an int. I will update my pull request with that. I think I was just trying to be a little forgiving (like how the code currently accepts str type).

2. make non-integer float in tests a failure

For the reasons above for having a hard error. It prevents 'unexpected behaviour' for someone that accidentally hands in a float value >0 being silently converted to a 0 value.

This is similar to how there is a hard error for handing in a non datetime.datetime object for expiry; it forces the programmer to make a simple fix and not missing a subtle bug as I did.

I updated the pytest test to reflect the above. Though now I'll update it again to just not allow any float.

I hope that makes some sense. Would you agree?

Sorry if this pull request comes across pedantic, I just felt like I lost quite a lot of time to this and that I could save others the same trouble.

[Tronic]

Python uses float seconds as its basic time unit, so any time calculation like max_age = expiry_time - time.time() produces a float that is generally not an integer but that can be safely rounded down. Doing so would not affect your original problem, where float 604_800.0 should turn into max-age=604800 if I am not mistaken.

Negative values should probably also be considered errors (raise ValueError?).

[jamesstidard]

Ah yes, hadn't considered negatives, I can add a check for that.

your example is exactly my original code that produced this problem in the first place for me :).

I had considered doing a floor or ceiling automatically on the value, but felt that is a similar problem as the original error. The Sanic implementation making an implicit choice for you that you may not expect.

Though I'm not too picky, and the Expires works in the same way (flooring to the closet second), so it will be consistent:

expires = datetime(2000, 1,1,3, 2, 1, 999999)
expires.strftime("%a, %d-%b-%Y %T GMT")
'Sat, 01-Jan-2000 03:02:01 GMT'

I'll add those changes. Thanks

[Tronic]

Related #1457 with similar intention.

[jamesstidard]

I’ll close this seeing that this has been addressed in another issue