If-Modified-Since is not ignored if request contains If-None-Match

jshttp / fresh

HTTP request freshness testing

MIT License

161 stars 28 forks source link

If-Modified-Since is not ignored if request contains If-None-Match #35

Closed marco2216 closed 1 month ago

marco2216 commented 1 year ago

According to the latest HTTP RFC,

A recipient MUST ignore If-Modified-Since if the request contains an If-None-Match header field; the condition in If-None-Match is considered to be a more accurate replacement for the condition in If-Modified-Since, and the two are only combined for the sake of interoperating with older intermediaries that might not implement If-None-Match.

fresh does not ignore If-Modified-Since, it returns false if the If-Modified-Since is before last-modified. I would expect it to return true immediately if If-None-Match matches the ETag header.

dougwilson commented 1 year ago

Ah, yes, it looks like this is a new change in the spec, as that requirement was not part of RFC 7232, AFAICT.

marco2216 commented 1 year ago

I think it was?

dougwilson commented 1 year ago

Oh, you're right. That's why I said as far as I can tell. I guess I assumed it would be in the same section in both RFC, doh.

marco2216 commented 1 year ago

If you'd like, I could try to create a PR addressing the issue?

dougwilson commented 1 year ago

Hi @marco2216 you can, though sorry I didn't post yet, just before you go through too much trouble, I did already start on the develop branch a couple hours ago. I just noticed that I need to switch the project over to GH Actions to finish it off, and will just need to wait until after I'm done with work today to work on that since it's more involved.

marco2216 commented 1 year ago

Oh I see, I'll let you handle it then 👌

bigbigDreamer commented 1 year ago

Hi, sorry to bother you. May I ask if there has been any progress on this matter, or what the follow-up plan is, or if there is a timeline? @dougwilson

dougwilson commented 1 year ago

Hi @bigbigDreamer apologies for that! It just got lost in the sea of notifications for me and then fell off more as I got some unrelated security reports I had to triage and working on fixing one of them currently. I think I already pushed up the changeset, so I will get a release made and pushed here.

UlisesGascon commented 5 months ago

If you'd like, I could try to create a PR addressing the issue?

Hi @marco2216! Are you still interested in working on this PR? We've completed the migration to GitHub already :)

bigbigDreamer commented 5 months ago

If you'd like, I could try to create a PR addressing the issue?

Hi @marco2216! Are you still interested in working on this PR? We've completed the migration to GitHub already :)

Hi @UlisesGascon , I fix this, can u help me review #38

All tests passed!