The README documented access_control_allow_origin, upstream_fetch_timeout, upstream_max_response_size and downstream_timeout, but pixa did not know them, so a config that followed the README aborted startup as having unknown keys. Each is now a setting with its PIXA_ variable, defaulting to the formerly fixed value: *, 30s, 50 MiB and 60s.
Durations are Go duration strings (30s, 2m) and must be positive; a bare number is refused. The size is a whole number of bytes from 1 to 1 GiB. An invalid value aborts startup naming the key, its variable and the value.
access_control_allow_origin is * or one http or https origin; README.md states the full rule for its host and port. The CORS middleware compares the value exactly, so anything else would match no browser request; it aborts startup. Where CORS applies is unchanged (#98).
downstream_timeout replaces the HTTPWriteTimeout constant for both the server's write timeout and the per-request timeout.
upstream_max_response_size still sets the image processor's input limit.
Disclosures:
Owner-approved edits to existing tests: TestEnvironmentSetsEveryKey sets the four new variables; TestNewHTTPServerTimeouts checks against downstream_timeout; newTestServer in the login rate limit tests uses the default downstream_timeout.
Judgement call: 1 GiB as the size maximum; the processor reads one byte past the limit, which overflowed at the largest 64-bit value.
Judgement call: refusing any origin other than * or one origin is my reading of "invalid" for this key.
Unverified item: no test covers the per-request timeout or the two upstream values reaching the fetcher.
Model: opus-5-5
The README documented `access_control_allow_origin`, `upstream_fetch_timeout`, `upstream_max_response_size` and `downstream_timeout`, but pixa did not know them, so a config that followed the README aborted startup as having unknown keys. Each is now a setting with its `PIXA_` variable, defaulting to the formerly fixed value: `*`, `30s`, 50 MiB and `60s`.
- Durations are Go duration strings (`30s`, `2m`) and must be positive; a bare number is refused. The size is a whole number of bytes from 1 to 1 GiB. An invalid value aborts startup naming the key, its variable and the value.
- `access_control_allow_origin` is `*` or one `http` or `https` origin; `README.md` states the full rule for its host and port. The CORS middleware compares the value exactly, so anything else would match no browser request; it aborts startup. Where CORS applies is unchanged (https://git.eeqj.de/sneak/pixa/issues/98).
- `downstream_timeout` replaces the `HTTPWriteTimeout` constant for both the server's write timeout and the per-request timeout.
- `upstream_max_response_size` still sets the image processor's input limit.
Disclosures:
- Owner-approved edits to existing tests: `TestEnvironmentSetsEveryKey` sets the four new variables; `TestNewHTTPServerTimeouts` checks against `downstream_timeout`; `newTestServer` in the login rate limit tests uses the default `downstream_timeout`.
- Judgement call: 1 GiB as the size maximum; the processor reads one byte past the limit, which overflowed at the largest 64-bit value.
- Judgement call: refusing any origin other than `*` or one origin is my reading of "invalid" for this key.
- Unverified item: no test covers the per-request timeout or the two upstream values reaching the fetcher.
Model: opus-5-5
internal/config/config.go:622 (validateAccessControlAllowOrigin): a value with a * inside it passes the startup check, for example https://*, https://*.example.com or https://*example.com. The CORS middleware (go-chi/cors) reads a * inside an origin as a pattern, so https://*example.com also lets https://evilexample.com read responses, and https://* lets every https site read them. README.md and config.example.yml say that anything other than * or one origin aborts startup. Acceptable: any value that contains * and is not exactly * aborts startup naming the key, its variable and the value, with a case for it in invalidSizeAndOriginCases (internal/config/config_validation_internal_test.go).
internal/middleware/middleware_internal_test.go:12-14: the comment on TestCORSAnswersWithConfiguredOrigin is not a readable sentence ("checks that the CORS middleware uses access_control_allow_origin: "*" lets any origin read responses, ..."). Acceptable: one plain sentence, for example: the CORS middleware answers with access_control_allow_origin, where * lets any origin read responses and a single origin lets only that origin read them.
Test edits: the changes to TestEnvironmentSetsEveryKey and TestNewHTTPServerTimeouts extend each test without dropping or loosening any assertion.
Model: opus-5-5
**FAIL**
1. `internal/config/config.go:622` (`validateAccessControlAllowOrigin`): a value with a `*` inside it passes the startup check, for example `https://*`, `https://*.example.com` or `https://*example.com`. The CORS middleware (go-chi/cors) reads a `*` inside an origin as a pattern, so `https://*example.com` also lets `https://evilexample.com` read responses, and `https://*` lets every https site read them. `README.md` and `config.example.yml` say that anything other than `*` or one origin aborts startup. Acceptable: any value that contains `*` and is not exactly `*` aborts startup naming the key, its variable and the value, with a case for it in `invalidSizeAndOriginCases` (`internal/config/config_validation_internal_test.go`).
2. `internal/middleware/middleware_internal_test.go:12-14`: the comment on `TestCORSAnswersWithConfiguredOrigin` is not a readable sentence ("checks that the CORS middleware uses access_control_allow_origin: "*" lets any origin read responses, ..."). Acceptable: one plain sentence, for example: the CORS middleware answers with `access_control_allow_origin`, where `*` lets any origin read responses and a single origin lets only that origin read them.
Test edits: the changes to `TestEnvironmentSetsEveryKey` and `TestNewHTTPServerTimeouts` extend each test without dropping or loosening any assertion.
Model: opus-5-5
Rework for the review at https://git.eeqj.de/sneak/pixa/pulls/142#issuecomment-104130:
1. Fixed: 4afe8c3 adds the three cases to `invalidSizeAndOriginCases`; d2e2d77 makes a value that contains `*` and is not exactly `*` abort startup.
2. Fixed in 1cd47cb.
Model: opus-5-5
internal/config/config.go:625 (validateAccessControlAllowOrigin): the check only confirms the value reads back as scheme plus host; it does not check that the host is a host name or IP address, or that the port is a port. https://a.com,b.com, https://example.com:, https://:8443, https://example.com:99999 and https://exämple.com all start pixa. No browser sends such an origin, so the CORS middleware silently lets no site read responses, while README.md and config.example.yml say anything other than * or one origin aborts startup. Acceptable: the host must be a host name (ASCII letters, digits, hyphens, dots) or an IP address, and a port, when given, a number from 1 to 65535. Anything else aborts startup naming the key, its variable and the value, with these cases in invalidSizeAndOriginCases.
internal/config/config.go:581: upstream_max_response_size: 9223372036854775807, the largest value the check accepts, starts pixa. But the image processor's limit (internal/imageprocessor/imageprocessor.go:146, maxInputBytes+1) then wraps negative, no bytes are read, and every image fails to decode. Acceptable: a documented maximum above which startup aborts naming the key, its variable and the value, or a processor limit that works for the largest accepted value, with a test for it.
Commits 4afe8c3, d2e2d77 and 1cd47cb name sneak <sneak@sneak.berlin> as author, but the owner did not write them. A squash merge with Gitea's default message adds a Co-authored-by: line for each commit author. Acceptable: the same author identity as a84a6fd and eaa4020.
Model: opus-5-5
**FAIL**
1. `internal/config/config.go:625` (`validateAccessControlAllowOrigin`): the check only confirms the value reads back as scheme plus host; it does not check that the host is a host name or IP address, or that the port is a port. `https://a.com,b.com`, `https://example.com:`, `https://:8443`, `https://example.com:99999` and `https://exämple.com` all start pixa. No browser sends such an origin, so the CORS middleware silently lets no site read responses, while `README.md` and `config.example.yml` say anything other than `*` or one origin aborts startup. Acceptable: the host must be a host name (ASCII letters, digits, hyphens, dots) or an IP address, and a port, when given, a number from 1 to 65535. Anything else aborts startup naming the key, its variable and the value, with these cases in `invalidSizeAndOriginCases`.
2. `internal/config/config.go:581`: `upstream_max_response_size: 9223372036854775807`, the largest value the check accepts, starts pixa. But the image processor's limit (`internal/imageprocessor/imageprocessor.go:146`, `maxInputBytes+1`) then wraps negative, no bytes are read, and every image fails to decode. Acceptable: a documented maximum above which startup aborts naming the key, its variable and the value, or a processor limit that works for the largest accepted value, with a test for it.
3. Commits 4afe8c3, d2e2d77 and 1cd47cb name `sneak <sneak@sneak.berlin>` as author, but the owner did not write them. A squash merge with Gitea's default message adds a `Co-authored-by:` line for each commit author. Acceptable: the same author identity as a84a6fd and eaa4020.
Model: opus-5-5
Fixed: 8474ed3 adds the cases; 0b756c4 checks the host with net/netip or a host-name character check and the port with strconv.
Fixed: a 1 GiB maximum, stated in config.example.yml, tested at the maximum and above it (8474ed3, 0b756c4).
Fixed: every commit on the branch is now authored as clawbot.
Judgement call: the bad origins in invalidSizeAndOriginCases are now one list checked the same way, so the function fits the linter's length limit; no case was dropped.
Model: opus-5-5
Rework for the review at https://git.eeqj.de/sneak/pixa/pulls/142#issuecomment-104171:
1. Fixed: 8474ed3 adds the cases; 0b756c4 checks the host with `net/netip` or a host-name character check and the port with `strconv`.
2. Fixed: a 1 GiB maximum, stated in `config.example.yml`, tested at the maximum and above it (8474ed3, 0b756c4).
3. Fixed: every commit on the branch is now authored as clawbot.
Judgement call: the bad origins in `invalidSizeAndOriginCases` are now one list checked the same way, so the function fits the linter's length limit; no case was dropped.
Model: opus-5-5
internal/config/config.go:634 (validateAccessControlAllowOrigin): values that no browser sends as an origin still start pixa. A browser leaves out the scheme's default port, writes a port without leading zeros, reads a host whose last part (after the final dot) is a number as an IPv4 address (rewriting it, or refusing the URL when it is not a valid address), and writes an IPv6 address in its shortest form. So https://example.com:443, http://example.com:80, https://example.com:08080, https://01.2.3.4, https://10.0.0, https://192.168.1.256, https://example.123 and https://[0:0:0:0:0:0:0:1] are accepted, but the CORS middleware compares the value exactly, so no browser request ever matches, while README.md and config.example.yml say anything other than * or one origin aborts startup. Acceptable: each of these aborts startup naming the key, its variable and the value: a port equal to the scheme's default or written with a leading zero; a host whose last part is a number (such as 1234 or 0x7f) unless the whole host is an IPv4 address net/netip accepts; an IPv6 address not written the way netip.Addr.String() writes it. Add these cases to invalidSizeAndOriginCases.
README.md:203 and config.example.yml:81: both say an origin is "scheme and host only", but a port is accepted (http://localhost:3000 is in TestOriginWithPortOrAnyOriginIsAccepted). Acceptable: scheme, host and optional port, as TODO.md says.
Judgement call: an origin with a scheme other than http or https, such as file://example.com, also starts pixa and is not raised, since browser extensions and app web views send origins with their own schemes.
Model: opus-5-5
**FAIL**
1. `internal/config/config.go:634` (`validateAccessControlAllowOrigin`): values that no browser sends as an origin still start pixa. A browser leaves out the scheme's default port, writes a port without leading zeros, reads a host whose last part (after the final dot) is a number as an IPv4 address (rewriting it, or refusing the URL when it is not a valid address), and writes an IPv6 address in its shortest form. So `https://example.com:443`, `http://example.com:80`, `https://example.com:08080`, `https://01.2.3.4`, `https://10.0.0`, `https://192.168.1.256`, `https://example.123` and `https://[0:0:0:0:0:0:0:1]` are accepted, but the CORS middleware compares the value exactly, so no browser request ever matches, while `README.md` and `config.example.yml` say anything other than `*` or one origin aborts startup. Acceptable: each of these aborts startup naming the key, its variable and the value: a port equal to the scheme's default or written with a leading zero; a host whose last part is a number (such as `1234` or `0x7f`) unless the whole host is an IPv4 address `net/netip` accepts; an IPv6 address not written the way `netip.Addr.String()` writes it. Add these cases to `invalidSizeAndOriginCases`.
2. `README.md:203` and `config.example.yml:81`: both say an origin is "scheme and host only", but a port is accepted (`http://localhost:3000` is in `TestOriginWithPortOrAnyOriginIsAccepted`). Acceptable: scheme, host and optional port, as `TODO.md` says.
Judgement call: an origin with a scheme other than `http` or `https`, such as `file://example.com`, also starts pixa and is not raised, since browser extensions and app web views send origins with their own schemes.
Model: opus-5-5
Not covered: a host whose last part is written in hexadecimal, such as 0x7f, contains a letter, so it still starts pixa under the rule I was given.
A scheme other than http or https, such as file://example.com, now aborts startup, and a host name ending in a dot, such as example.com., does too, since its last part is empty.
Model: opus-5-5
Rework for the review at https://git.eeqj.de/sneak/pixa/pulls/142#issuecomment-104284:
1. Fixed: 37372cd adds the cases to `invalidSizeAndOriginCases`; 1d707c9 rewrites the check as one function whose comment states the rule.
2. Fixed in 1d707c9.
- Not covered: a host whose last part is written in hexadecimal, such as `0x7f`, contains a letter, so it still starts pixa under the rule I was given.
- A scheme other than `http` or `https`, such as `file://example.com`, now aborts startup, and a host name ending in a dot, such as `example.com.`, does too, since its last part is empty.
Model: opus-5-5
README.md:201-205 and config.example.yml:79-82 do not state the origin rule the code enforces. "scheme, host and optional port, exactly as the browser sends it" does not say that only http and https are accepted, that the host must be a lowercase host name (letters, digits, hyphens, dots, with a letter in its last part) or an IP address (IPv6 in brackets, in its shortest form), or that a port with a leading zero or equal to the scheme's default is refused. It also promises that an origin a browser does send, such as a browser extension's chrome-extension://..., is accepted, while it aborts startup. Acceptable: both files state the rule itself: *, or http/https, such a host, and an optional port 1-65535 with no leading zero that is not the scheme's default.
internal/config/config_validation_internal_test.go:749 (badOrigins): no case covers the scheme being lowercase. An upper or mixed case scheme such as HTTPS://example.com is refused only by the final comparison in validateAccessControlAllowOrigin, and that comparison could ignore case with every test still passing. Acceptable: add HTTPS://example.com to badOrigins.
The PR body is about 280 words, over the limit of about 250. Acceptable: shorten it to about 250 words or fewer.
Model: opus-5-5
**FAIL**
1. `README.md:201-205` and `config.example.yml:79-82` do not state the origin rule the code enforces. "scheme, host and optional port, exactly as the browser sends it" does not say that only `http` and `https` are accepted, that the host must be a lowercase host name (letters, digits, hyphens, dots, with a letter in its last part) or an IP address (IPv6 in brackets, in its shortest form), or that a port with a leading zero or equal to the scheme's default is refused. It also promises that an origin a browser does send, such as a browser extension's `chrome-extension://...`, is accepted, while it aborts startup. Acceptable: both files state the rule itself: `*`, or `http`/`https`, such a host, and an optional port 1-65535 with no leading zero that is not the scheme's default.
2. `internal/config/config_validation_internal_test.go:749` (`badOrigins`): no case covers the scheme being lowercase. An upper or mixed case scheme such as `HTTPS://example.com` is refused only by the final comparison in `validateAccessControlAllowOrigin`, and that comparison could ignore case with every test still passing. Acceptable: add `HTTPS://example.com` to `badOrigins`.
3. The PR body is about 280 words, over the limit of about 250. Acceptable: shorten it to about 250 words or fewer.
Model: opus-5-5
Judgement call: the TODO.md entry for this work repeated "exactly as the browser sends it", so it now points to README.md instead.
Model: opus-5-5
Rework of the review at https://git.eeqj.de/sneak/pixa/pulls/142#issuecomment-104319:
1. Fixed in 741c4f7: `README.md` and `config.example.yml` now state the rule.
2. Added in 5e335fb.
3. Body shortened.
- Judgement call: the `TODO.md` entry for this work repeated "exactly as the browser sends it", so it now points to `README.md` instead.
Model: opus-5-5
PASS. Judgement call: I read the issue's "a valid value takes effect" as met by the tests at config level, although, as the PR body says, no test checks that upstream_fetch_timeout and upstream_max_response_size reach the fetcher or that downstream_timeout reaches the per-request timeout.
Model: opus-5-5
PASS. Judgement call: I read the issue's "a valid value takes effect" as met by the tests at config level, although, as the PR body says, no test checks that `upstream_fetch_timeout` and `upstream_max_response_size` reach the fetcher or that `downstream_timeout` reaches the per-request timeout.
Model: opus-5-5
internal/server/login_rate_limit_internal_test.go:53 (newTestServer, added by #143): its config leaves DownstreamTimeout at zero. With this change, every request in the login rate limit tests therefore runs with a per-request timeout of zero. Its context has already expired when the handler runs, and the timeout middleware tries to answer 504 after each handler. These tests no longer run against the routes pixa serves, and any change that makes the login path respect the request context fails all of them for a reason unrelated to rate limiting. Acceptable: newTestServer sets DownstreamTimeout to config.DefaultDownstreamTimeout. This edits an existing test, so it needs the owner's approval before it is made.
Model: opus-5-5
**FAIL**
1. `internal/server/login_rate_limit_internal_test.go:53` (`newTestServer`, added by https://git.eeqj.de/sneak/pixa/pulls/143): its config leaves `DownstreamTimeout` at zero. With this change, every request in the login rate limit tests therefore runs with a per-request timeout of zero. Its context has already expired when the handler runs, and the timeout middleware tries to answer 504 after each handler. These tests no longer run against the routes pixa serves, and any change that makes the login path respect the request context fails all of them for a reason unrelated to rate limiting. Acceptable: `newTestServer` sets `DownstreamTimeout` to `config.DefaultDownstreamTimeout`. This edits an existing test, so it needs the owner's approval before it is made.
Model: opus-5-5
Failing tests for access_control_allow_origin, upstream_fetch_timeout,
upstream_max_response_size and downstream_timeout: their defaults,
valid values from the file and the environment, invalid values aborting
startup naming the key or variable and the value, the CORS middleware
answering with the configured origin, and the server's write timeout
coming from downstream_timeout. They do not compile until the settings
exist.
Model: opus-5-5
access_control_allow_origin, upstream_fetch_timeout,
upstream_max_response_size and downstream_timeout were in the README
but unknown to pixa, so a config that followed it aborted startup. Each
is now a setting with its PIXA_ variable, defaulting to the value that
was fixed in the code: *, 30s, 50 MiB and 60s. Durations are positive Go
duration strings; the size is a positive whole number of bytes; the
origin is * or one scheme and host. An invalid value aborts startup
naming the key and value. downstream_timeout replaces HTTPWriteTimeout
for the server's write timeout and the per-request timeout.
Model: opus-5-5
Failing cases for access_control_allow_origin set to https://*,
https://*.example.com and https://*example.com. The CORS middleware
reads a * inside an origin as a pattern, so these would let other sites
read responses; each must abort startup naming the key and the value.
Model: opus-5-5
access_control_allow_origin accepted values such as https://* or
https://*example.com, which the CORS middleware reads as a pattern that
lets other sites read responses. Any value that contains * and is not
exactly * now aborts startup naming the key, its variable and the value.
Model: opus-5-5
Failing cases: access_control_allow_origin with two hosts, an empty
port, no host, port 0, port 99999 or a non-ASCII host name, and
upstream_max_response_size above 1 GiB, up to the largest 64-bit
integer, where the image processor's limit wraps negative. Each must
abort startup naming the key and the value. The bad origins are now one
list so the table fits the linter's function length limit; the size
test uses 1 GiB to show the maximum itself is accepted, and IPv4 and
IPv6 origins are shown to be accepted.
Model: opus-5-5
access_control_allow_origin now needs a host name (ASCII letters,
digits, hyphens, dots) or an IP address, and a port, when given, from 1
to 65535, read with net/url, net/netip and strconv. Two hosts, an empty
port, no host, a bad port or a non-ASCII host name abort startup, as a
* inside the value already did.
upstream_max_response_size above 1 GiB aborts startup: the image
processor reads one byte past the limit, which wrapped negative at the
largest 64-bit value, and a response is held whole in memory.
config.example.yml states the maximum.
Model: opus-5-5
Adds the review's examples to invalidSizeAndOriginCases: a scheme's
default port, a port with a leading zero, hosts a browser reads as an
IPv4 address or refuses, an IPv6 address not in its shortest form,
plus a scheme other than http or https and a host name in upper case.
Model: opus-5-5
access_control_allow_origin is now "*", or http or https, a host that is
an IP address as net/netip writes it (IPv6 in brackets) or a lowercase
host name whose last part contains a letter, and an optional port 1-65535
with no leading zero that is not the scheme's default. The value must
equal the origin rebuilt from those parts; anything else aborts startup
naming the key, its variable and the value. README.md and
config.example.yml say an origin is scheme, host and optional port,
exactly as the browser sends it.
Model: opus-5-5
Both now say what access_control_allow_origin accepts, instead of
"exactly as the browser sends it", and that another scheme, such as a
browser extension's, aborts startup.
Model: opus-5-5
newTestServer left downstream_timeout at zero, so every request in those
tests ran with an already expired per-request timeout. It now uses
config.DefaultDownstreamTimeout, as a real config would. Approved by the
owner on the issue.
Model: opus-5-5
Fixed in 2c4dfe9: newTestServer sets DownstreamTimeout to config.DefaultDownstreamTimeout, as approved at #61 (comment). The other lines of that config differ only by the alignment make fmt gives them.
Rebased onto current next. Conflicts were only in README.md (the trusted_proxies text from #150 kept, with this PR's three setting descriptions after it) and TODO.md (this PR's entry placed among the new ones by date). config.example.yml and the code merged without conflict, and the PR's own changes are otherwise the same as before. The PR body now lists the approved test edits.
Model: opus-5-5
Rework for the review at https://git.eeqj.de/sneak/pixa/pulls/142#issuecomment-104872:
1. Fixed in 2c4dfe9: `newTestServer` sets `DownstreamTimeout` to `config.DefaultDownstreamTimeout`, as approved at https://git.eeqj.de/sneak/pixa/issues/61#issuecomment-106048. The other lines of that config differ only by the alignment `make fmt` gives them.
Rebased onto current `next`. Conflicts were only in `README.md` (the `trusted_proxies` text from https://git.eeqj.de/sneak/pixa/issues/150 kept, with this PR's three setting descriptions after it) and `TODO.md` (this PR's entry placed among the new ones by date). `config.example.yml` and the code merged without conflict, and the PR's own changes are otherwise the same as before. The PR body now lists the approved test edits.
Model: opus-5-5
PASS. The four settings reach the fetcher, the CORS middleware, the server write timeout and the per-request timeout alongside everything now on next.
Model: opus-5-5
PASS. The four settings reach the fetcher, the CORS middleware, the server write timeout and the per-request timeout alongside everything now on `next`.
Model: opus-5-5
clawbot
merged commit 56217cbf4a into next2026-09-29 08:49:19 +02:00
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.
The README documented
access_control_allow_origin,upstream_fetch_timeout,upstream_max_response_sizeanddownstream_timeout, but pixa did not know them, so a config that followed the README aborted startup as having unknown keys. Each is now a setting with itsPIXA_variable, defaulting to the formerly fixed value:*,30s, 50 MiB and60s.30s,2m) and must be positive; a bare number is refused. The size is a whole number of bytes from 1 to 1 GiB. An invalid value aborts startup naming the key, its variable and the value.access_control_allow_originis*or onehttporhttpsorigin;README.mdstates the full rule for its host and port. The CORS middleware compares the value exactly, so anything else would match no browser request; it aborts startup. Where CORS applies is unchanged (#98).downstream_timeoutreplaces theHTTPWriteTimeoutconstant for both the server's write timeout and the per-request timeout.upstream_max_response_sizestill sets the image processor's input limit.Disclosures:
TestEnvironmentSetsEveryKeysets the four new variables;TestNewHTTPServerTimeoutschecks againstdownstream_timeout;newTestServerin the login rate limit tests uses the defaultdownstream_timeout.*or one origin is my reading of "invalid" for this key.Model: opus-5-5
FAIL
internal/config/config.go:622(validateAccessControlAllowOrigin): a value with a*inside it passes the startup check, for examplehttps://*,https://*.example.comorhttps://*example.com. The CORS middleware (go-chi/cors) reads a*inside an origin as a pattern, sohttps://*example.comalso letshttps://evilexample.comread responses, andhttps://*lets every https site read them.README.mdandconfig.example.ymlsay that anything other than*or one origin aborts startup. Acceptable: any value that contains*and is not exactly*aborts startup naming the key, its variable and the value, with a case for it ininvalidSizeAndOriginCases(internal/config/config_validation_internal_test.go).internal/middleware/middleware_internal_test.go:12-14: the comment onTestCORSAnswersWithConfiguredOriginis not a readable sentence ("checks that the CORS middleware uses access_control_allow_origin: "*" lets any origin read responses, ..."). Acceptable: one plain sentence, for example: the CORS middleware answers withaccess_control_allow_origin, where*lets any origin read responses and a single origin lets only that origin read them.Test edits: the changes to
TestEnvironmentSetsEveryKeyandTestNewHTTPServerTimeoutsextend each test without dropping or loosening any assertion.Model: opus-5-5
Rework for the review at #142 (comment):
4afe8c3adds the three cases toinvalidSizeAndOriginCases;d2e2d77makes a value that contains*and is not exactly*abort startup.1cd47cb.Model: opus-5-5
FAIL
internal/config/config.go:625(validateAccessControlAllowOrigin): the check only confirms the value reads back as scheme plus host; it does not check that the host is a host name or IP address, or that the port is a port.https://a.com,b.com,https://example.com:,https://:8443,https://example.com:99999andhttps://exämple.comall start pixa. No browser sends such an origin, so the CORS middleware silently lets no site read responses, whileREADME.mdandconfig.example.ymlsay anything other than*or one origin aborts startup. Acceptable: the host must be a host name (ASCII letters, digits, hyphens, dots) or an IP address, and a port, when given, a number from 1 to 65535. Anything else aborts startup naming the key, its variable and the value, with these cases ininvalidSizeAndOriginCases.internal/config/config.go:581:upstream_max_response_size: 9223372036854775807, the largest value the check accepts, starts pixa. But the image processor's limit (internal/imageprocessor/imageprocessor.go:146,maxInputBytes+1) then wraps negative, no bytes are read, and every image fails to decode. Acceptable: a documented maximum above which startup aborts naming the key, its variable and the value, or a processor limit that works for the largest accepted value, with a test for it.4afe8c3,d2e2d77and1cd47cbnamesneak <sneak@sneak.berlin>as author, but the owner did not write them. A squash merge with Gitea's default message adds aCo-authored-by:line for each commit author. Acceptable: the same author identity asa84a6fdandeaa4020.Model: opus-5-5
Rework for the review at #142 (comment):
8474ed3adds the cases;0b756c4checks the host withnet/netipor a host-name character check and the port withstrconv.config.example.yml, tested at the maximum and above it (8474ed3,0b756c4).Judgement call: the bad origins in
invalidSizeAndOriginCasesare now one list checked the same way, so the function fits the linter's length limit; no case was dropped.Model: opus-5-5
FAIL
internal/config/config.go:634(validateAccessControlAllowOrigin): values that no browser sends as an origin still start pixa. A browser leaves out the scheme's default port, writes a port without leading zeros, reads a host whose last part (after the final dot) is a number as an IPv4 address (rewriting it, or refusing the URL when it is not a valid address), and writes an IPv6 address in its shortest form. Sohttps://example.com:443,http://example.com:80,https://example.com:08080,https://01.2.3.4,https://10.0.0,https://192.168.1.256,https://example.123andhttps://[0:0:0:0:0:0:0:1]are accepted, but the CORS middleware compares the value exactly, so no browser request ever matches, whileREADME.mdandconfig.example.ymlsay anything other than*or one origin aborts startup. Acceptable: each of these aborts startup naming the key, its variable and the value: a port equal to the scheme's default or written with a leading zero; a host whose last part is a number (such as1234or0x7f) unless the whole host is an IPv4 addressnet/netipaccepts; an IPv6 address not written the waynetip.Addr.String()writes it. Add these cases toinvalidSizeAndOriginCases.README.md:203andconfig.example.yml:81: both say an origin is "scheme and host only", but a port is accepted (http://localhost:3000is inTestOriginWithPortOrAnyOriginIsAccepted). Acceptable: scheme, host and optional port, asTODO.mdsays.Judgement call: an origin with a scheme other than
httporhttps, such asfile://example.com, also starts pixa and is not raised, since browser extensions and app web views send origins with their own schemes.Model: opus-5-5
Rework for the review at #142 (comment):
37372cdadds the cases toinvalidSizeAndOriginCases;1d707c9rewrites the check as one function whose comment states the rule.1d707c9.0x7f, contains a letter, so it still starts pixa under the rule I was given.httporhttps, such asfile://example.com, now aborts startup, and a host name ending in a dot, such asexample.com., does too, since its last part is empty.Model: opus-5-5
FAIL
README.md:201-205andconfig.example.yml:79-82do not state the origin rule the code enforces. "scheme, host and optional port, exactly as the browser sends it" does not say that onlyhttpandhttpsare accepted, that the host must be a lowercase host name (letters, digits, hyphens, dots, with a letter in its last part) or an IP address (IPv6 in brackets, in its shortest form), or that a port with a leading zero or equal to the scheme's default is refused. It also promises that an origin a browser does send, such as a browser extension'schrome-extension://..., is accepted, while it aborts startup. Acceptable: both files state the rule itself:*, orhttp/https, such a host, and an optional port 1-65535 with no leading zero that is not the scheme's default.internal/config/config_validation_internal_test.go:749(badOrigins): no case covers the scheme being lowercase. An upper or mixed case scheme such asHTTPS://example.comis refused only by the final comparison invalidateAccessControlAllowOrigin, and that comparison could ignore case with every test still passing. Acceptable: addHTTPS://example.comtobadOrigins.Model: opus-5-5
Rework of the review at #142 (comment):
741c4f7:README.mdandconfig.example.ymlnow state the rule.5e335fb.TODO.mdentry for this work repeated "exactly as the browser sends it", so it now points toREADME.mdinstead.Model: opus-5-5
PASS. Judgement call: I read the issue's "a valid value takes effect" as met by the tests at config level, although, as the PR body says, no test checks that
upstream_fetch_timeoutandupstream_max_response_sizereach the fetcher or thatdownstream_timeoutreaches the per-request timeout.Model: opus-5-5
FAIL
internal/server/login_rate_limit_internal_test.go:53(newTestServer, added by #143): its config leavesDownstreamTimeoutat zero. With this change, every request in the login rate limit tests therefore runs with a per-request timeout of zero. Its context has already expired when the handler runs, and the timeout middleware tries to answer 504 after each handler. These tests no longer run against the routes pixa serves, and any change that makes the login path respect the request context fails all of them for a reason unrelated to rate limiting. Acceptable:newTestServersetsDownstreamTimeouttoconfig.DefaultDownstreamTimeout. This edits an existing test, so it needs the owner's approval before it is made.Model: opus-5-5
clawbot referenced this pull request2026-09-29 04:08:37 +02:00
2a8ce05335to2c4dfe929aRework for the review at #142 (comment):
2c4dfe9:newTestServersetsDownstreamTimeouttoconfig.DefaultDownstreamTimeout, as approved at #61 (comment). The other lines of that config differ only by the alignmentmake fmtgives them.Rebased onto current
next. Conflicts were only inREADME.md(thetrusted_proxiestext from #150 kept, with this PR's three setting descriptions after it) andTODO.md(this PR's entry placed among the new ones by date).config.example.ymland the code merged without conflict, and the PR's own changes are otherwise the same as before. The PR body now lists the approved test edits.Model: opus-5-5
PASS. The four settings reach the fetcher, the CORS middleware, the server write timeout and the per-request timeout alongside everything now on
next.Model: opus-5-5