Image handler

<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23discussion_r20791685%22%2C%20%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23discussion_r20791748%22%2C%20%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23discussion_r20791782%22%2C%20%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23discussion_r20791894%22%2C%20%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23discussion_r20792050%22%2C%20%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23discussion_r20792326%22%2C%20%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23discussion_r20792436%22%2C%20%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23issuecomment-64203609%22%5D%2C%20%22comments%22%3A%20%7B%22Pull%20b17779064c1437b02a27b24e4655c70a3cd6bde7%20tcl/image_file.tcl%2022%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23discussion_r20791782%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%27d%20prefer%20we%20throw%20an%20error%20rather%20than%20return%20a%20not-found%22%2C%20%22created_at%22%3A%20%222014-11-24T14%3A19%3A09Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/486212%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/bvw%22%7D%7D%2C%20%7B%22body%22%3A%20%22hard%20error%20or%20USER%20error%20%3F%5Cr%5Cn%5Cr%5CnDo%20we%20want%20an%20support%20email%20everytime%20someone%20requests%20an%20image%20%3E%202560x2560%5Cr%5Cn%3F%5Cr%5Cn%5Cr%5CnOn%2024%20November%202014%20at%2014%3A19%2C%20Bernhard%20van%20Woerden%20%3Cnotifications%40github.com%5Cr%5Cn%3E%20wrote%3A%5Cr%5Cn%5Cr%5Cn%3E%20In%20tcl/image_file.tcl%3A%5Cr%5Cn%3E%5Cr%5Cn%3E%20%3E%20%40%40%20-172%2C6%20%2B173%2C12%20%40%40%20proc%20qc%3A%3Aimage_handler%20%7Bcache_dir%20%7Bimage_redirect_handler%20UNDEF%7D%7D%20%7B%5Cr%5Cn%3E%20%3E%20%20%20%20%20%20%20%20%20%20%23log%20Debug%20%5C%22Image%20Handler%20-%20Invalid%20image%20url%5C%22%5Cr%5Cn%3E%20%3E%20%20%20%20%20%20%20%20%20%20return%20%5Bns_returnnotfound%5D%5Cr%5Cn%3E%20%3E%20%20%20%20%20%20%7D%5Cr%5Cn%3E%20%3E%20%2B%5Cr%5Cn%3E%20%3E%20%2B%20%20%20%20%23%20Check%20requested%20max%20width%20and%20height%20does%20not%20exceed%20allowed%20max%20width%20and%20height%5Cr%5Cn%3E%20%3E%20%2B%20%20%20%20if%20%7B%20%28%24allowed_max_width%20ne%20%5C%22%5C%22%20%26%26%20%24max_width%20%3E%20%24allowed_max_width%29%20%7C%7C%20%28%24allowed_max_height%20ne%20%5C%22%5C%22%20%26%26%20%24max_height%20%3E%20%24allowed_max_height%29%20%7D%20%7B%5Cr%5Cn%3E%20%3E%20%2B%20%20%20%20%20%20%20%20%23log%20Debug%20%5C%22Image%20Handler%20-%20requested%20%24%7Bmax_width%7Dx%24%7Bmax_height%7D%20image%20exceeds%20max%20width%20and%20height%20permitted%20%24%7Ballowed_max_width%7Dx%24%7Ballowed_max_height%7D%5C%22%5Cr%5Cn%3E%20%3E%20%2B%20%20%20%20%20%20%20%20return%20%5Bns_returnnotfound%5D%5Cr%5Cn%3E%5Cr%5Cn%3E%20I%27d%20prefer%20we%20throw%20an%20error%20rather%20than%20return%20a%20not-found%5Cr%5Cn%3E%5Cr%5Cn%3E%20%5Cu2014%5Cr%5Cn%3E%20Reply%20to%20this%20email%20directly%20or%20view%20it%20on%20GitHub%5Cr%5Cn%3E%20%3Chttps%3A//github.com/qcode-software/qcode-tcl/pull/107/files%23r20791782%3E.%5Cr%5Cn%3E%5Cr%5Cn%5Cr%5Cn%5Cr%5Cn%5Cr%5Cn--%20%5Cr%5CnKind%20Regards%2C%5Cr%5CnDaniel%20Clark%5Cr%5CnQcode%20Software%20Limited%5Cr%5Cnwww.qcode.co.uk%5Cr%5CnTel%3A%20%2B44%280%29%201463%20896%20482%22%2C%20%22created_at%22%3A%20%222014-11-24T14%3A20%3A59Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3089689%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/daniel-qcode%22%7D%7D%2C%20%7B%22body%22%3A%20%22yeah%20probably%20not.%20What%20about%20one%20of%20the%20other%20400%20HTTP%20error%20codes%20%3F%5Cr%5CnIs%20there%20a%20more%20precise%20one%20we%20could%20return%20%3F%5Cr%5Cn%3Chttp%3A//www.w3.org/Protocols/rfc2616/rfc2616-sec10.html%3E%22%2C%20%22created_at%22%3A%20%222014-11-24T14%3A28%3A14Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/486212%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/bvw%22%7D%7D%2C%20%7B%22body%22%3A%20%22perhaps%20%60ns_returnbadrequest%20reason%60%22%2C%20%22created_at%22%3A%20%222014-11-24T14%3A29%3A58Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/486212%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/bvw%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20tcl/image_file.tcl%3AL173-185%22%7D%2C%20%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23issuecomment-64203609%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%40bvw%20updated%20pull%20request%22%2C%20%22created_at%22%3A%20%222014-11-24T14%3A49%3A16Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3089689%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/daniel-qcode%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%2C%20%22Pull%20b17779064c1437b02a27b24e4655c70a3cd6bde7%20tcl/image_file.tcl%205%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/qcode-software/qcode-tcl/pull/107%23discussion_r20791685%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22could%20we%20use%20%60max_width%60%20rather%20than%20%60allowed_max_width%60%20%22%2C%20%22created_at%22%3A%20%222014-11-24T14%3A17%3A38Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/486212%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/bvw%22%7D%7D%2C%20%7B%22body%22%3A%20%22just%20using%20max_width%20would%20collide%20with%20max_width%20of%20the%20requested%20image%5Cr%5Cnsize%5Cr%5Cn%5Cr%5CnOn%2024%20November%202014%20at%2014%3A17%2C%20Bernhard%20van%20Woerden%20%3Cnotifications%40github.com%5Cr%5Cn%3E%20wrote%3A%5Cr%5Cn%5Cr%5Cn%3E%20In%20tcl/image_file.tcl%3A%5Cr%5Cn%3E%5Cr%5Cn%3E%20%3E%20%40%40%20-159%2C11%20%2B159%2C12%20%40%40%20proc%20qc%3A%3Aimage_data%20%7Bcache_dir%20file_id%20max_width%20max_height%7D%20%7B%5Cr%5Cn%3E%20%3E%20%20%20%20%20%20return%20%5Bqc%3A%3Aimage_cache_data%20%24cache_dir%20%24file_id%20%24max_width%20%24max_height%5D%5Cr%5Cn%3E%20%3E%20%20%7D%5Cr%5Cn%3E%20%3E%5Cr%5Cn%3E%20%3E%20-proc%20qc%3A%3Aimage_handler%20%7Bcache_dir%20%7Bimage_redirect_handler%20UNDEF%7D%7D%20%7B%5Cr%5Cn%3E%20%3E%20%2Bproc%20qc%3A%3Aimage_handler%20%7Bcache_dir%20%7Bimage_redirect_handler%20%5C%22qc%3A%3Aimage_redirect_handler%5C%22%7D%20%7Ballowed_max_width%202560%7D%20%7Ballowed_max_height%202560%7D%7D%20%7B%5Cr%5Cn%3E%5Cr%5Cn%3E%20could%20we%20use%20max_width%20rather%20than%20allowed_max_width%5Cr%5Cn%3E%5Cr%5Cn%3E%20%5Cu2014%5Cr%5Cn%3E%20Reply%20to%20this%20email%20directly%20or%20view%20it%20on%20GitHub%5Cr%5Cn%3E%20%3Chttps%3A//github.com/qcode-software/qcode-tcl/pull/107/files%23r20791685%3E.%5Cr%5Cn%3E%5Cr%5Cn%5Cr%5Cn%5Cr%5Cn%5Cr%5Cn--%20%5Cr%5CnKind%20Regards%2C%5Cr%5CnDaniel%20Clark%5Cr%5CnQcode%20Software%20Limited%5Cr%5Cnwww.qcode.co.uk%5Cr%5CnTel%3A%20%2B44%280%29%201463%20896%20482%22%2C%20%22created_at%22%3A%20%222014-11-24T14%3A18%3A34Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3089689%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/daniel-qcode%22%7D%7D%2C%20%7B%22body%22%3A%20%22right%20okay.%22%2C%20%22created_at%22%3A%20%222014-11-24T14%3A23%3A40Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/486212%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/bvw%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20tcl/image_file.tcl%3AL159-171%22%7D%7D%7D'></a>
<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>
- [x] <a href='#crh-comment-Pull b17779064c1437b02a27b24e4655c70a3cd6bde7 tcl/image_file.tcl 5'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/qcode-software/qcode-tcl/pull/107#discussion_r20791685'>File: tcl/image_file.tcl:L159-171</a></b>
- <a href='https://github.com/bvw'><img border=0 src='https://avatars.githubusercontent.com/u/486212?v=3' height=16 width=16'></a> could we use `max_width` rather than `allowed_max_width`
- <a href='https://github.com/daniel-qcode'><img border=0 src='https://avatars.githubusercontent.com/u/3089689?v=3' height=16 width=16'></a> just using max_width would collide with max_width of the requested image
size
On 24 November 2014 at 14:17, Bernhard van Woerden <notifications@github.com
> wrote:
> In tcl/image_file.tcl:
>
> > @@ -159,11 +159,12 @@ proc qc::image_data {cache_dir file_id max_width max_height} {
> >      return [qc::image_cache_data $cache_dir $file_id $max_width $max_height]
> >  }
> >
> > -proc qc::image_handler {cache_dir {image_redirect_handler UNDEF}} {
> > +proc qc::image_handler {cache_dir {image_redirect_handler "qc::image_redirect_handler"} {allowed_max_width 2560} {allowed_max_height 2560}} {
>
> could we use max_width rather than allowed_max_width
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/qcode-software/qcode-tcl/pull/107/files#r20791685>.
>
--
Kind Regards,
Daniel Clark
Qcode Software Limited
www.qcode.co.uk
Tel: +44(0) 1463 896 482
- <a href='https://github.com/bvw'><img border=0 src='https://avatars.githubusercontent.com/u/486212?v=3' height=16 width=16'></a> right okay.
- [ ] <a href='#crh-comment-Pull b17779064c1437b02a27b24e4655c70a3cd6bde7 tcl/image_file.tcl 22'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/qcode-software/qcode-tcl/pull/107#discussion_r20791782'>File: tcl/image_file.tcl:L173-185</a></b>
- <a href='https://github.com/bvw'><img border=0 src='https://avatars.githubusercontent.com/u/486212?v=3' height=16 width=16'></a> I'd prefer we throw an error rather than return a not-found
- <a href='https://github.com/daniel-qcode'><img border=0 src='https://avatars.githubusercontent.com/u/3089689?v=3' height=16 width=16'></a> hard error or USER error ?
Do we want an support email everytime someone requests an image > 2560x2560
?
On 24 November 2014 at 14:19, Bernhard van Woerden <notifications@github.com
> wrote:
> In tcl/image_file.tcl:
>
> > @@ -172,6 +173,12 @@ proc qc::image_handler {cache_dir {image_redirect_handler UNDEF}} {
> >          #log Debug "Image Handler - Invalid image url"
> >          return [ns_returnnotfound]
> >      }
> > +
> > +    # Check requested max width and height does not exceed allowed max width and height
> > +    if { ($allowed_max_width ne "" && $max_width > $allowed_max_width) || ($allowed_max_height ne "" && $max_height > $allowed_max_height) } {
> > +        #log Debug "Image Handler - requested ${max_width}x${max_height} image exceeds max width and height permitted ${allowed_max_width}x${allowed_max_height}"
> > +        return [ns_returnnotfound]
>
> I'd prefer we throw an error rather than return a not-found
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/qcode-software/qcode-tcl/pull/107/files#r20791782>.
>
--
Kind Regards,
Daniel Clark
Qcode Software Limited
www.qcode.co.uk
Tel: +44(0) 1463 896 482
- <a href='https://github.com/bvw'><img border=0 src='https://avatars.githubusercontent.com/u/486212?v=3' height=16 width=16'></a> yeah probably not. What about one of the other 400 HTTP error codes ?
Is there a more precise one we could return ?
<http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html>
- <a href='https://github.com/bvw'><img border=0 src='https://avatars.githubusercontent.com/u/486212?v=3' height=16 width=16'></a> perhaps `ns_returnbadrequest reason`
- [ ] <a href='#crh-comment-General Comment'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/qcode-software/qcode-tcl/pull/107#issuecomment-64203609'>General Comment</a></b>
- <a href='https://github.com/daniel-qcode'><img border=0 src='https://avatars.githubusercontent.com/u/3089689?v=3' height=16 width=16'></a> @bvw updated pull request


<a href='https://www.codereviewhub.com/qcode-software/qcode-tcl/pull/107?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/qcode-software/qcode-tcl/pull/107?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/qcode-software/qcode-tcl/pull/107'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>
<a href='#crh-end'></a>