bug with Plack::Middleware::ConditionalGET and ETags
amaxcz opened this issue · 6 comments
This is very bad part of code, it's working incorrect with checks = "1",""
unless (grep !$_, @checks) {
suggested fix is:
--- ConditionalGET.pm.orig 2017-12-07 12:11:59.564776214 +0100
+++ ConditionalGET.pm 2017-02-22 04:00:35.000000000 +0100
@@ -20,7 +20,7 @@
my @checks = ( $self->etag_matches($h, $env), $self->not_modified_since($h, $env) )
or return;
+ if (grep {$_ eq 1 } @checks) {
- unless (grep !$_, @checks) {
$res->[0] = 304;
$h->remove($_) for qw( Content-Type Content-Length Content-Disposition );
if ($res->[2]) {
actual bug is: array @checks is
[
[0] 1,
[1] ""
]
the construction
unless (grep !$_, @checks) {
print "blabla";
}
works incorrect. absolutely.
you can easy experiment with this:
#!/usr/bin/perl
use strict;
my @checks = ("1","");
unless (grep !$_, @checks) {
print "WORKS!!! \n";
}
after that compare with :
#!/usr/bin/perl
use strict;
my @checks = ("1","");
if (grep {$_ eq 1 } @checks) {
print "WORKS!!! \n";
}
P.S. the checks array fills with results of subs. there can be 1 (true) or "" (false). If we have ETags via check
$self->etag_matches($h, $env)
it will result = 1
so UNLESS + GREP for !$_ fails .
has fixed comments, sorry
No that's not what i asked. You're only explaining why the code is buggy but do not tell me how to trigger it. Please explain it without showing the code and values, and instead what you're doing as a user of the middleware and what you're expecting. #606 sounds related but i still can't see how.
I went ahead and re-read the code, and you probably didn't read the comment: https://github.com/plack/Plack/blob/master/lib/Plack/Middleware/ConditionalGET.pm#L18-L19
# check both ETag and If-Modified-Since, and at least one should exist
# and all present headers should match, not either.
so if either of this If-* didn't match and returned '', it should not set 304. This code is working as intended.
This is very bad part of code, it's working incorrect with checks = "1",""
Before making such a claim, re-read the code, comment and tests to make sure the intent. Also, when reporting a bug, you should explain why you think is a bug with a concrete example, instead of saying "this code is buggy and here's a fix".
Now, if you think that if either (not both) ETag and If-Modified-Since matches it should 304, we can now discuss, but i believe the right behavior is that if both If-None-Match and If-Modified-Since are sent and either of them fail, that's a change in the resource and it should not return stale 304.