ETT-477 Update `under_server` references in imgsrv by moseshll · Pull Request #196 · hathitrust/babel

moseshll · 2026-03-18T16:30:21Z

Remove code that de-restricts when not under server (e.g., $self->restricted(0) unless ( Debug::DUtils::under_server() );)
- SRV/Article/EPUB.pm
- SRV/Article/PDF.pm
- SRV/Volume/Base.pm
- SRV/Volume/EPUB.pm
- SRV/Volume/HTML.pm
- SRV/Volume/PDF.pm
- SRV/Volume/Remediated/Bundle.pm
Remove conditional use of SRV::Utils::Stream for output (e.g., if ( ! $self->output_filename || Debug::DUtils::under_server() ))
- SRV/Article/PDF.pm
- SRV/Volume/PDF.pm
Completely de-restrict SRV/Cover.pm
Always translate to character entities in DEBUG output
- SRV/SearchUtils.pm
Explicitly initialize super to undef in _default_params() since I believe the existing code is wrong
- SRV/Volume/Base.pm
Miscellaneous logic simplifications
- SRV/Image.pm
- SRV/Utils.pm
Update psgi apps to use SRV::Utils::under_server
- FIXME: do we even need that?
Remove Article/JATS support

- Remove code that de-restricts when not under server (e.g., `$self->restricted(0) unless ( Debug::DUtils::under_server() );`) - `SRV/Article/EPUB.pm` - `SRV/Article/PDF.pm` - `SRV/Volume/Base.pm` - `SRV/Volume/EPUB.pm` - `SRV/Volume/HTML.pm` - `SRV/Volume/PDF.pm` - `SRV/Volume/Remediated/Bundle.pm` - Remove conditional use of `SRV::Utils::Stream` for output (e.g., `if ( ! $self->output_filename || Debug::DUtils::under_server() )`) - `SRV/Article/PDF.pm` - `SRV/Volume/PDF.pm` - Completely de-restrict SRV/Cover.pm - Always translate to character entities in DEBUG output - `SRV/SearchUtils.pm` - Explicitly initialize `super` to `undef` in `_default_params()` since I believe the existing code is wrong - `SRV/Volume/Base.pm` - Miscellaneous logic simplifications - `SRV/Image.pm` - `SRV/Utils.pm` - Update psgi apps to use `SRV::Utils::under_server` - FIXME: do we even need that?

moseshll · 2026-03-18T16:37:18Z

 builder {

-    if ( under_server() ) {
+    if ( SRV::Utils::under_server() ) {


The question is, do we even need these at all? Applies to this file and imgsrv.psgi as well.

I'm not sure how I feel about URLFixer in general. If there are percent-escaped ';' or '=' characters in the URL, then it will unescape the entire URL and redirect to that. That's a problem we see with the ChatGPT-created queries to ssd.

I could imagine some kind of redirection attack though -- basically tricking imgsrv to redirecting to something unexpected - but I'm not sure exactly what the threat model would be.

It may be worthwhile seeing if there's evidence of legit requests to imgsrv that would trigger this (i.e. requests to imgsrv containing %3B or %3D), and if we can't find any, we just disable URLFixer entirely.

I honestly can't tell. It appears that the redirect is sticking the URL with escaped values into the http_referer, so I can't see what the upstream source is, that is, whether it's one of ours or something else that might hold some meaning. I will rely on some more guidance to answer this question, perhaps, and expect to return to this question.

Update: we can't completely remove under_server or URLFixer as things stand now. So here are two sample requests from the playwright tests. The second is called with Plack::App::Command so the actual download action is not under_server(). URLFixer is only enabled when under_server(), the (second, below) request that comes back from the frontend contains %3d and... for some reason it's bad to fix it??? It looks like doing so would result in decoding a part of the URL (download_url) that is meant to be left encoded. Enabling URLFixer across the board kills the playwright tests for some reason. Not sure why. I'm not sure if this is in scope for the current ticket. Why on earth are these URLs so bloody convoluted?

- /cgi/imgsrv/download/epub?id=test.pd_open&callback=tunnelCallback&_=1774990829729 - /epub?progress_filepath=/htapps/babel/cache/download/2K16.118abf7093dae5452990834aa55749f1430a1454497252f63dab189fcec3707780/2K16.118abf7093dae5452990834aa55749f1430a1454497252f63dab189fcec3707780__progress&download_url=/cgi/imgsrv/download/epub?id=test.pd_open%3Bmarker=2K16.118abf7093dae5452990834aa55749f1430a1454497252f63dab189fcec3707780%3Battachment=1&id=test.pd_open

After some tracing I understand better what is happening. image.psgi and download.psgi can both be called via Plack::App::Command->run and thus with under_server FALSE. I didn't realize that background processing was done by the bucket chain of

SRV::Volume::Base->_background (or subclass) SRV::Utils::run_command IPC::Run::run bin/start.sh bin/pdf (or similar) Plack::App::Command->run(..., apps/download.psgi) (or imgsrv.psgi)

and by the time this gets to URLFixer (yes ultimately we're talking about that) the REQUEST_URI in the environment is munged all to heck. So I would guess that's why that particular middleware is selectively enabled.

To help answer the original question, however, %3B and %3D only seem to be showing up in unauthenticated requests from outside the US. I would tentatively recommend disabling URLFixer, though perhaps not removing it right away.

moseshll · 2026-03-20T16:13:57Z

-        # this can only be passed from the command line
-        $params{super} = undef;
-    }
-


I believe the existing code should be if instead of unless because it makes no sense otherwise based on the comment. I don't know if this change is more secure big picture wise, but it is simpler.

Yeah, this doesn't make any sense. We should check if we remove it that debug=super still works & works only for logged-in users with appropriate permissions.

aelkiss

I have a number of questions, mostly if we should get rid of some of the other under_server checks entirely, and making sure my understanding is the same.

aelkiss · 2026-03-20T20:30:09Z

 builder {

-    if ( under_server() ) {
+    if ( SRV::Utils::under_server() ) {


I'm not sure how I feel about URLFixer in general. If there are percent-escaped ';' or '=' characters in the URL, then it will unescape the entire URL and redirect to that. That's a problem we see with the ChatGPT-created queries to ssd.

I could imagine some kind of redirection attack though -- basically tricking imgsrv to redirecting to something unexpected - but I'm not sure exactly what the threat model would be.

It may be worthwhile seeing if there's evidence of legit requests to imgsrv that would trigger this (i.e. requests to imgsrv containing %3B or %3D), and if we can't find any, we just disable URLFixer entirely.

aelkiss · 2026-03-20T20:31:04Z

    enable "PopulateENV", app_name => 'imgsrv';

-    enable_if { (under_server() && $ENV{HT_DEV}) } 'StackTrace';
+    enable_if { (SRV::Utils::under_server() && $ENV{HT_DEV}) } 'StackTrace';


Can we think of any reason not to enable solely based on HT_DEV or (maybe better) the plack environment?

aelkiss · 2026-03-20T20:32:27Z

+    enable_if { (SRV::Utils::under_server() && ! $ENV{HT_DEV}) } "HTTPExceptions", rethrow => 0;

-    if ( under_server() ) {
+    if ( SRV::Utils::under_server() ) {


I'd be inclined to just always enable these -- given we do local development in docker, I can't think of a case where we wouldn't be under_server.

aelkiss · 2026-03-20T20:39:21Z

-        # this can only be passed from the command line
-        $params{super} = undef;
-    }
-


Yeah, this doesn't make any sense. We should check if we remove it that debug=super still works & works only for logged-in users with appropriate permissions.

aelkiss · 2026-03-20T20:41:53Z

    my $ext = shift;
    my $output_filename =
-        SRV::Utils::generate_output_filename($env, [ $self->file, $self->mode, 'full', $self->size, '0', $self->quality, $self->restricted, 'ZZZ' ], $ext);
+        SRV::Utils::generate_output_filename($env, [ $self->file, $self->mode, 'full', $self->size, '0', $self->quality, 'ZZZ' ], $ext);


Just removing $self->restricted here threw me for a loop, but it looks like generate_output_filename just makes a hash of all the options, and if covers are never restricted then having it in the hash doesn't matter. It does change the cache key for existing cover images, though, so I might be inclined to keep it in the hash?

aelkiss · 2026-03-20T20:42:43Z

-    my $restricted = $self->restricted;
-    unless ( defined $restricted ) {
-        # $restricted = $C->get_object('Access::Rights')->assert_final_access_status($C, $gId) ne 'allow';
-        $restricted = $$env{'psgix.restricted'};


Could there be other ways (throttling, etc) where psgix.restricted gets set, even though in general covers should be unrestricted?

Or I guess the point is that restricted could be set, but it doesn't matter because we serve the cover anyway?

moseshll added 2 commits March 18, 2026 12:07

Remove unused Article/JATS support.

7f27830

moseshll commented Mar 18, 2026

View reviewed changes

download.psgi should have a newline at end of file

f7dcdd8

moseshll commented Mar 20, 2026

View reviewed changes

moseshll marked this pull request as ready for review March 20, 2026 16:14

moseshll requested a review from aelkiss March 20, 2026 16:14

aelkiss reviewed Mar 20, 2026

View reviewed changes

Conversation

moseshll commented Mar 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moseshll Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moseshll Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelkiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

moseshll Mar 31, 2026 •

edited

Loading

moseshll Apr 1, 2026 •

edited

Loading