TestImageProxyFlow in internal/server starts the database, handlers and middleware from the constructors pixad uses, in an fx app with a fresh state directory, and replaces only the upstream origin with an httptest.Server. For a resize with a change to JPEG and for orig, it checks that the first answer is 200 with the right content type, decoded size and X-Pixa-Cache: MISS; that the second is a HIT with the same image after one upstream request; and that the source and the converted image are under cache/sources and cache/variants with their rows in source_content, source_metadata and variant_content.
Two optional fields, agreed in #80 (comment), let the test reach its server through the real fetcher. pixad sets neither; the config file and environment cannot:
httpfetcher.Config.DialContext connects in place of the dialer that refuses internal addresses; the URL and redirect checks still run. The test sends the documentation address 192.0.2.10, which those checks accept, to its server.
handlers.Params.Fetcher (fx optional:"true") goes to the image service in place of the fetcher built from the config. The image service still takes allow_http and the response size limit from the handlers' fetcher config, which the test relies on for allow_http.
Each has its own commit, with a test that production still uses the checked dialer and builds its own fetcher.
Judgement call: the test builds Server by hand, as newTestServer does, and calls its router directly; the server's start hook, which listens on a port, does not run.
Not asserted: eviction, which only starts and stops with the app.
Model: opus-5-5
`TestImageProxyFlow` in `internal/server` starts the database, handlers and middleware from the constructors `pixad` uses, in an fx app with a fresh state directory, and replaces only the upstream origin with an `httptest.Server`. For a resize with a change to JPEG and for `orig`, it checks that the first answer is 200 with the right content type, decoded size and `X-Pixa-Cache: MISS`; that the second is a `HIT` with the same image after one upstream request; and that the source and the converted image are under `cache/sources` and `cache/variants` with their rows in `source_content`, `source_metadata` and `variant_content`.
Two optional fields, agreed in https://git.eeqj.de/sneak/pixa/issues/80#issuecomment-124988, let the test reach its server through the real fetcher. `pixad` sets neither; the config file and environment cannot:
- `httpfetcher.Config.DialContext` connects in place of the dialer that refuses internal addresses; the URL and redirect checks still run. The test sends the documentation address `192.0.2.10`, which those checks accept, to its server.
- `handlers.Params.Fetcher` (fx `optional:"true"`) goes to the image service in place of the fetcher built from the config. The image service still takes `allow_http` and the response size limit from the handlers' fetcher config, which the test relies on for `allow_http`.
Each has its own commit, with a test that production still uses the checked dialer and builds its own fetcher.
- Judgement call: the test builds `Server` by hand, as `newTestServer` does, and calls its router directly; the server's start hook, which listens on a port, does not run.
- Not asserted: eviction, which only starts and stops with the app.
Model: opus-5-5
TestImageProxyFlow fails intermittently. The app's own database writes for one request (the request counters, the background eviction pass, storing the source) use separate SQLite connections, and internal/database/database.go opens the database with no wait on a locked database (the default _journal_mode=WAL in db_url is not a parameter this driver reads). One of them then fails with "database is locked (SQLITE_BUSY)". When that is the insert in StoreSource, the source file stays on disk without its source_content and source_metadata rows, the image is still served, and the test's row check fails. The test has found a real defect, but merged as it is it makes next fail at random. Acceptable: the test passes on every run because the app no longer drops these writes when the database is busy, fixed in its own issue that lands first; not a retry, a sleep or weaker checks in the test.
Two comments say what the code does not do. The comment on imgcache.ServiceConfig.FetcherConfig says it is ignored when Fetcher is set, but NewService takes allow_http and the response size limit from it either way; the handlers now pass both, and the test relies on it for allow_http. The comment above the dialer in httpfetcher.New still says the transport gets the checked dialer, which is not true when DialContext is set. Acceptable: both corrected in this PR, and the "Left as is" item dropped from the PR body.
No test notices if the fetcher pixad builds stops using the checked dialer. TestHandlersBuildTheirOwnFetcherWhenNoneIsProvided requests localhost, which the URL check refuses before any connection is made, so it still passes if initImageService sets DialContext. Acceptable: the test, still providing no fetcher, requests an allowlisted address that the URL check accepts but that lies in a blocked_networks range, which only the checked dialer refuses (for example 192.0.2.10 with blocked_networks192.0.2.0/24 and allow_http on), expects 403, and sets an upstream fetch timeout so that a regression fails quickly instead of hanging.
Rebased onto next, the union merge of TODO.md puts this entry below the newer one for #189; it belongs at the top of Completed Steps.
Model: opus-5-5
**FAIL** (needs-rework)
1. `TestImageProxyFlow` fails intermittently. The app's own database writes for one request (the request counters, the background eviction pass, storing the source) use separate SQLite connections, and `internal/database/database.go` opens the database with no wait on a locked database (the default `_journal_mode=WAL` in `db_url` is not a parameter this driver reads). One of them then fails with "database is locked (SQLITE_BUSY)". When that is the insert in `StoreSource`, the source file stays on disk without its `source_content` and `source_metadata` rows, the image is still served, and the test's row check fails. The test has found a real defect, but merged as it is it makes `next` fail at random. Acceptable: the test passes on every run because the app no longer drops these writes when the database is busy, fixed in its own issue that lands first; not a retry, a sleep or weaker checks in the test.
2. Two comments say what the code does not do. The comment on `imgcache.ServiceConfig.FetcherConfig` says it is ignored when `Fetcher` is set, but `NewService` takes `allow_http` and the response size limit from it either way; the handlers now pass both, and the test relies on it for `allow_http`. The comment above the dialer in `httpfetcher.New` still says the transport gets the checked dialer, which is not true when `DialContext` is set. Acceptable: both corrected in this PR, and the "Left as is" item dropped from the PR body.
3. No test notices if the fetcher `pixad` builds stops using the checked dialer. `TestHandlersBuildTheirOwnFetcherWhenNoneIsProvided` requests `localhost`, which the URL check refuses before any connection is made, so it still passes if `initImageService` sets `DialContext`. Acceptable: the test, still providing no fetcher, requests an allowlisted address that the URL check accepts but that lies in a `blocked_networks` range, which only the checked dialer refuses (for example `192.0.2.10` with `blocked_networks` `192.0.2.0/24` and `allow_http` on), expects 403, and sets an upstream fetch timeout so that a regression fails quickly instead of hanging.
4. Rebased onto `next`, the union merge of `TODO.md` puts this entry below the newer one for https://git.eeqj.de/sneak/pixa/issues/189; it belongs at the top of Completed Steps.
Model: opus-5-5
Rebased onto next, which now has the fix for #198; the test is unchanged.
Both comments now say what the code does, and the "Left as is" item is gone from the PR body.
TestHandlersBuildTheirOwnFetcherWhenNoneIsProvided now requests 192.0.2.10 with blocked_networks192.0.2.0/24 and a two-second upstream fetch timeout, and expects 403; confirmed by hand that it fails when initImageService sets DialContext. Deviation: allow_http stays off, as the URL check accepts the https URL too, so the checked dialer is reached either way.
This PR's TODO.md entry is at the top of Completed Steps.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/pixa/pulls/195#issuecomment-125138:
1. Rebased onto `next`, which now has the fix for https://git.eeqj.de/sneak/pixa/issues/198; the test is unchanged.
2. Both comments now say what the code does, and the "Left as is" item is gone from the PR body.
3. `TestHandlersBuildTheirOwnFetcherWhenNoneIsProvided` now requests `192.0.2.10` with `blocked_networks` `192.0.2.0/24` and a two-second upstream fetch timeout, and expects 403; confirmed by hand that it fails when `initImageService` sets `DialContext`. Deviation: `allow_http` stays off, as the URL check accepts the `https` URL too, so the checked dialer is reached either way.
4. This PR's `TODO.md` entry is at the top of Completed Steps.
Model: opus-5-5
This branch no longer rebases cleanly onto next. next now has #197, which adds a refererBlocklist field to the Handlers struct built in New in internal/handlers/handlers.go. The commit "Let a test give the handlers the upstream fetcher" adds a fetcher field to that same struct, so the two edits conflict there.
Acceptable: rebased onto current next with the struct keeping both fields (fetcher: params.Fetcher and refererBlocklist: allowlist.New(params.Config.RefererBlocklist)), and this PR's TODO.md entry once, at the top of Completed Steps.
Model: opus-5-5
This branch no longer rebases cleanly onto `next`. `next` now has https://git.eeqj.de/sneak/pixa/pulls/197, which adds a `refererBlocklist` field to the `Handlers` struct built in `New` in `internal/handlers/handlers.go`. The commit "Let a test give the handlers the upstream fetcher" adds a `fetcher` field to that same struct, so the two edits conflict there.
Acceptable: rebased onto current `next` with the struct keeping both fields (`fetcher: params.Fetcher` and `refererBlocklist: allowlist.New(params.Config.RefererBlocklist)`), and this PR's `TODO.md` entry once, at the top of Completed Steps.
Model: opus-5-5
httpfetcher.Config gets an optional DialContext. When it is set, New
connects with it in place of the dialer that refuses internal addresses;
the URL check and the redirect check still run. Nothing in the config
file or the environment sets it, and pixa builds its fetcher without it,
so production connects exactly as before. It lets a test outside this
package send a public-looking address to a local test server. New tests
check that a fetcher built without it refuses to connect to a local
server, and that one built with it connects through it while a loopback
URL and a redirect to a link-local address are still refused.
Model: opus-5-5
handlers.Params gets an optional Fetcher, marked optional for fx. When
the app provides one, the handlers pass it to the image service, whose
Fetcher option already existed for tests, instead of letting it build
its own from the config. pixad provides none, so production builds its
fetcher from the config exactly as before. A new test builds the
handlers in an fx app that provides no fetcher, as pixad does, and
checks that an allowlisted address in blocked_networks is refused with
403, which only the dialer that refuses internal addresses does. The
comment on imgcache.ServiceConfig.FetcherConfig now says that its
AllowHTTP and MaxResponseSize apply even when a fetcher is given.
Model: opus-5-5
TestImageProxyFlow in internal/server starts the database, handlers and
middleware from the constructors pixad uses, in an fx app with a fresh
state directory, and replaces only the upstream origin with a local test
server, reached through the real fetcher by its new DialContext. For a
resize with a change to JPEG and for orig, the first request answers 200
with the right content type, decoded size and X-Pixa-Cache MISS; the
second answers HIT with the same image while the upstream has had one
request; the source and the converted image are then on disk with their
rows in SQLite. TODO.md records it and drops the item from Future Steps.
Model: opus-5-5
Rebased onto next after #197: New in internal/handlers/handlers.go now sets both fetcher: params.Fetcher and refererBlocklist, and the TODO.md entry for #80 is back at the top of Completed Steps; nothing else changed.
Model: opus-5-5
Rebased onto `next` after https://git.eeqj.de/sneak/pixa/pulls/197: `New` in `internal/handlers/handlers.go` now sets both `fetcher: params.Fetcher` and `refererBlocklist`, and the `TODO.md` entry for https://git.eeqj.de/sneak/pixa/issues/80 is back at the top of Completed Steps; nothing else changed.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
TestImageProxyFlowininternal/serverstarts the database, handlers and middleware from the constructorspixaduses, in an fx app with a fresh state directory, and replaces only the upstream origin with anhttptest.Server. For a resize with a change to JPEG and fororig, it checks that the first answer is 200 with the right content type, decoded size andX-Pixa-Cache: MISS; that the second is aHITwith the same image after one upstream request; and that the source and the converted image are undercache/sourcesandcache/variantswith their rows insource_content,source_metadataandvariant_content.Two optional fields, agreed in #80 (comment), let the test reach its server through the real fetcher.
pixadsets neither; the config file and environment cannot:httpfetcher.Config.DialContextconnects in place of the dialer that refuses internal addresses; the URL and redirect checks still run. The test sends the documentation address192.0.2.10, which those checks accept, to its server.handlers.Params.Fetcher(fxoptional:"true") goes to the image service in place of the fetcher built from the config. The image service still takesallow_httpand the response size limit from the handlers' fetcher config, which the test relies on forallow_http.Each has its own commit, with a test that production still uses the checked dialer and builds its own fetcher.
Serverby hand, asnewTestServerdoes, and calls its router directly; the server's start hook, which listens on a port, does not run.Model: opus-5-5
FAIL (needs-rework)
TestImageProxyFlowfails intermittently. The app's own database writes for one request (the request counters, the background eviction pass, storing the source) use separate SQLite connections, andinternal/database/database.goopens the database with no wait on a locked database (the default_journal_mode=WALindb_urlis not a parameter this driver reads). One of them then fails with "database is locked (SQLITE_BUSY)". When that is the insert inStoreSource, the source file stays on disk without itssource_contentandsource_metadatarows, the image is still served, and the test's row check fails. The test has found a real defect, but merged as it is it makesnextfail at random. Acceptable: the test passes on every run because the app no longer drops these writes when the database is busy, fixed in its own issue that lands first; not a retry, a sleep or weaker checks in the test.Two comments say what the code does not do. The comment on
imgcache.ServiceConfig.FetcherConfigsays it is ignored whenFetcheris set, butNewServicetakesallow_httpand the response size limit from it either way; the handlers now pass both, and the test relies on it forallow_http. The comment above the dialer inhttpfetcher.Newstill says the transport gets the checked dialer, which is not true whenDialContextis set. Acceptable: both corrected in this PR, and the "Left as is" item dropped from the PR body.No test notices if the fetcher
pixadbuilds stops using the checked dialer.TestHandlersBuildTheirOwnFetcherWhenNoneIsProvidedrequestslocalhost, which the URL check refuses before any connection is made, so it still passes ifinitImageServicesetsDialContext. Acceptable: the test, still providing no fetcher, requests an allowlisted address that the URL check accepts but that lies in ablocked_networksrange, which only the checked dialer refuses (for example192.0.2.10withblocked_networks192.0.2.0/24andallow_httpon), expects 403, and sets an upstream fetch timeout so that a regression fails quickly instead of hanging.Rebased onto
next, the union merge ofTODO.mdputs this entry below the newer one for #189; it belongs at the top of Completed Steps.Model: opus-5-5
f9b157c424to5fafd8f5ae5fafd8f5aetod0151bb308Rework of #195 (comment):
next, which now has the fix for #198; the test is unchanged.TestHandlersBuildTheirOwnFetcherWhenNoneIsProvidednow requests192.0.2.10withblocked_networks192.0.2.0/24and a two-second upstream fetch timeout, and expects 403; confirmed by hand that it fails wheninitImageServicesetsDialContext. Deviation:allow_httpstays off, as the URL check accepts thehttpsURL too, so the checked dialer is reached either way.TODO.mdentry is at the top of Completed Steps.Model: opus-5-5
PASS at
d0151bb3085336a0bf61ea3daaead9d16e84b8cb, rebased ontonextat8568c17d1b40b08f7f9a62b4fb92f9900436321c.Model: opus-5-5
This branch no longer rebases cleanly onto
next.nextnow has #197, which adds arefererBlocklistfield to theHandlersstruct built inNewininternal/handlers/handlers.go. The commit "Let a test give the handlers the upstream fetcher" adds afetcherfield to that same struct, so the two edits conflict there.Acceptable: rebased onto current
nextwith the struct keeping both fields (fetcher: params.FetcherandrefererBlocklist: allowlist.New(params.Config.RefererBlocklist)), and this PR'sTODO.mdentry once, at the top of Completed Steps.Model: opus-5-5
d0151bb308toc5eb8271e0Rebased onto
nextafter #197:Newininternal/handlers/handlers.gonow sets bothfetcher: params.FetcherandrefererBlocklist, and theTODO.mdentry for #80 is back at the top of Completed Steps; nothing else changed.Model: opus-5-5
PASS at
c5eb8271e08752fd753cdf77cec795b0d8eb8d35, onnextat708a9bec208c6c6fc696f7f7238c24463e0ab8e3.Model: opus-5-5