Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ValidatePreconditions always fails if the supplied DateTimeOffset has non-zero milliseconds #424

Closed
cmeeren opened this issue Jun 2, 2020 · 8 comments
Labels
bug Reproducible bug

Comments

@cmeeren
Copy link

cmeeren commented Jun 2, 2020

Consider the following DateTimeOffset:

DateTimeOffset(2000, 1, 1, 0, 0, 0, TimeSpan.Zero)

This matches the following If-Unmodified-Since value (obtained by calling .ToString("R") on the DateTimeOffset):

"Sat, 01 Jan 2000 00:00:00 GMT"

And if I pass that DateTimeOffset to HttpContext.ValidatePreconditions, then that header value shown above will validate successfully.

However, most DateTimeOffset values are not exact seconds. If I add between 1 and 999 milliseconds

DateTimeOffset(2000, 1, 1, 0, 0, 0, 1, TimeSpan.Zero)
DateTimeOffset(2000, 1, 1, 0, 0, 0, 999, TimeSpan.Zero)

then, even though .ToString("R") returns the exact same string, the validation will fail, and AFAIK there will be no way of making it pass, because the resolution of the If-Unmodified-Since header is seconds.

This can be worked around in user code by manually rounding the DateTimeOffset down to the nearest second before passing the value to ValidatePreconditions, but ideally this should be fixed in Giraffe.

@cmeeren cmeeren changed the title ValidatePreconditions always fails if supplied DateTimeOffset is not exact seconds ValidatePreconditions always fails if the supplied DateTimeOffset has non-zero milliseconds Jun 2, 2020
@cmeeren
Copy link
Author

cmeeren commented Jun 4, 2020

Could anyone confirm whether this is a bug in Giraffe? It's blocking a lot of work for us, so I need to know whether I should temporarily work around this or if there will be a quick fix to Giraffe, or perhaps I have misunderstood the HTTP spec.

@TheAngryByrd
Copy link
Member

Since we're using DateTimeOffset directly in

match requestHeaders.IfUnmodifiedSince.Value > DateTimeOffset.UtcNow
|| requestHeaders.IfUnmodifiedSince.Value >= lastModified with
and
match requestHeaders.IfModifiedSince.Value <= DateTimeOffset.UtcNow
&& requestHeaders.IfModifiedSince.Value < lastModified with

This check will fail. We'd have to go through the process of zeroing out the millisecond field.

In the mean time to unblock you, can you normalize the DateTimeOffset you're passing in to zero out the milliseconds?

@TheAngryByrd
Copy link
Member

@dustinmoris what are you thoughts about fixing this? Since the format doesn't seem to include milliseconds and as specified in this RFC, should we go about zeroing out the milliseconds for the consumer?

@cmeeren
Copy link
Author

cmeeren commented Jun 4, 2020

Also see this SO answer which also says milliseconds is invalid (and refers to the RFC).

cmeeren added a commit to cmeeren/Felicity that referenced this issue Jun 5, 2020
@cmeeren
Copy link
Author

cmeeren commented Jun 8, 2020

Note that I tried working around it using

dt.AddMilliseconds (float (-dt.Millisecond))

but that doesn't work in all cases, I guess due to rounding errors. The following seems to work, however:

dt.AddTicks(-(dt.Ticks % TimeSpan.TicksPerSecond))

cmeeren added a commit to cmeeren/Felicity that referenced this issue Jun 8, 2020
@TheAngryByrd TheAngryByrd added the bug Reproducible bug label Jun 9, 2020
@dustinmoris
Copy link
Member

should we go about zeroing out the milliseconds for the consumer?

Just by quickly reading through this thread and some links posted here I think that's what we should do.

dustinmoris added a commit that referenced this issue Jun 12, 2020
@dustinmoris
Copy link
Member

I just created a PR for the two of you to review. I might have misunderstood this so please feel free to correct me if I've got it wrong!

@dustinmoris
Copy link
Member

Fixed with 5.0.0-rc-2.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Reproducible bug
Projects
None yet
Development

No branches or pull requests

3 participants