Re: [PATCH] Error page format codes upgrade

On 06/12/2013 02:13 AM, Amos Jeffries wrote:
>>> If we use fatal() here anyone loading a template with future defined
>>> codes or typos will crash Squid every time that error is supposed to be
>>> displayed.

>> Can Squid validate error templates at startup and quit if templates are
>> invalid? That would avoid runtime crashes without hiding potentially
>> serious problems from the admin.

You may want to skip the rant below and go to the very last paragraph in
this email. I might have found a way to resolve this issue in that
paragraph.

> That would be a part of the internal cache feature which is omitted from
> this patch.

Error page validation and internal caching are not related IMO. And we
already have examples of startup-time macro validation in the
configuration files. We can apply the same approach here, without
implementing any internal cache.

> I don't think it is a serious problem. Just some technical token
> lettering is displayed intead of potentially useful info.

I think it may be a serious problem for those who value high-quality
error pages because they represent their organization or service. Not a
problem at all for those who do not care what the error page says. Most
use cases fall somewhere in-between those two extremes, of course.

> All templates
> should be tested before use anyway to check that the desired output is
> displayed at the desired time.

I do not know of any practical way to do that except for Squid to
validate the templates at startup time.

>>> The assumption that they meanst %% is not great, but we do
>>> have to choose between hiding the unknown token and displaying it
>>> un-expanded and since libformat tokens are variable in length we don't
>>> really have the hide option without potentially screwing something else
>>> up as well. This way at least it will show up during trivial testing by
>>> the admin.
>> What trivial test would expose these problems if Squid does not complain
>> about them? I cannot think of any (not to mention that most admins would
>> not know they should run those tests, even if they are trivial).
>
> The trivial test of pretending to be a user and loading a forbidden URL
> or whatever the use-case they are altering the templates for is.

Correctly triggering certain error pages is far from trivial! And
visually checking that an error page does not contain unexpanded macros
is rather error prone so some additional automation would be needed to
solve that problem as well. Squid is in a much better position to do that.

> Not that I'm disagreeing with you on the idea. Just that it is a design
> decision that toggles between small internal conversion patch (current)
> and major new feature addition (TODO).

If the small internal conversion patch disables an existing useful
feature, it is fair to ask the developer to fix the patch. What you
could be arguing instead is that your patch does _not_ disable any
startup checks (because we currently do not check error pages during
startup), it just makes them non-fatal during runtime. If that is true,
that would be a valid argument (and you would not be responsible for
fixing this). Is that true?

Thank you,

Alex.
Received on Wed Jun 12 2013 - 16:04:11 MDT