close_issue (crates/gitlawb-node/src/api/issues.rs) resolves the repo, checks whether the caller is the owner, and for any non-owner falls back to comparing the caller against the issue's author. The author lives in the issue's git-JSON blob rather than a DB column, so establishing it means reading the repo, and reading it usefully means reading a current copy: on a node whose local mirror is stale, a legitimate author's own issue is invisible and they get a 403. So the pre-check calls acquire_fresh, which does a HEAD plus a full archive GET.
That puts a full object-storage fetch before the authorization decision, on a route any self-registered identity can reach. Two things follow, and they need separating because one of them is already someone else's PR.
The rate-limit half is already owned by #196. Its diff applies .write_braked(&write_ip_limiter) to issue_write_routes, so the "no rate limit" comment currently in server.rs stops being true when it merges. Nothing to do here for that half, and this issue should not duplicate it.
What #196 does not close is the ordering. A per-IP rate limit caps how often an unauthorized caller can do this; it does not make the work legitimate, and it does not bound the cost of any single request, which is a whole archive. The per-IP key is also weaker than it looks in the current fleet: GITLAWB_TRUSTED_PROXY is unset on two of the nodes (#264), which collapses the limiter key. So the residual is that a caller who is about to be refused still causes a full archive transfer first.
Also missing, and unrelated to cost: close_issue never calls authorize_repo_read. Every sibling issue handler does. So the pre-authorization fetch happens for private repos too, on behalf of callers with no read access to them.
For the record on what changed and what did not: this is not a regression against main. There, close_issue called acquire_write first, which downloaded and took the advisory lock before any authorization check, so the same fetch happened and held the lock while it did. The branch for #279 moved the authorization above the lock, which strictly improved it. The fetch is what is left.
A sketch that would close it, needing a design call rather than a patch: add the missing read gate, then make the pre-check local-only and optimistic, refusing without a fetch only when the local copy positively names a different author, and letting "cannot establish" fall through to the authoritative owner-or-author check that now runs under the guard. That drops the pre-authorization fetch entirely. It leaves a narrower lever, since a caller can still reach the lock by naming an issue absent from the local copy, and it is worth deciding whether that trade is the right one before implementing it.
Worth noting the project already reached the opposite conclusion at the only other unlocked acquire_fresh site: git_info_refs in crates/gitlawb-node/src/api/repos.rs puts its brake before the acquire, with a comment saying the fresh-acquire must not be reachable unauthenticated and unlimited. The same reasoning applies here; this route just never got it.
Established by reading the code and #196's diff. Not driven against a running node.
close_issue(crates/gitlawb-node/src/api/issues.rs) resolves the repo, checks whether the caller is the owner, and for any non-owner falls back to comparing the caller against the issue's author. The author lives in the issue's git-JSON blob rather than a DB column, so establishing it means reading the repo, and reading it usefully means reading a current copy: on a node whose local mirror is stale, a legitimate author's own issue is invisible and they get a 403. So the pre-check callsacquire_fresh, which does a HEAD plus a full archive GET.That puts a full object-storage fetch before the authorization decision, on a route any self-registered identity can reach. Two things follow, and they need separating because one of them is already someone else's PR.
The rate-limit half is already owned by #196. Its diff applies
.write_braked(&write_ip_limiter)toissue_write_routes, so the "no rate limit" comment currently inserver.rsstops being true when it merges. Nothing to do here for that half, and this issue should not duplicate it.What #196 does not close is the ordering. A per-IP rate limit caps how often an unauthorized caller can do this; it does not make the work legitimate, and it does not bound the cost of any single request, which is a whole archive. The per-IP key is also weaker than it looks in the current fleet:
GITLAWB_TRUSTED_PROXYis unset on two of the nodes (#264), which collapses the limiter key. So the residual is that a caller who is about to be refused still causes a full archive transfer first.Also missing, and unrelated to cost:
close_issuenever callsauthorize_repo_read. Every sibling issue handler does. So the pre-authorization fetch happens for private repos too, on behalf of callers with no read access to them.For the record on what changed and what did not: this is not a regression against
main. There,close_issuecalledacquire_writefirst, which downloaded and took the advisory lock before any authorization check, so the same fetch happened and held the lock while it did. The branch for #279 moved the authorization above the lock, which strictly improved it. The fetch is what is left.A sketch that would close it, needing a design call rather than a patch: add the missing read gate, then make the pre-check local-only and optimistic, refusing without a fetch only when the local copy positively names a different author, and letting "cannot establish" fall through to the authoritative owner-or-author check that now runs under the guard. That drops the pre-authorization fetch entirely. It leaves a narrower lever, since a caller can still reach the lock by naming an issue absent from the local copy, and it is worth deciding whether that trade is the right one before implementing it.
Worth noting the project already reached the opposite conclusion at the only other unlocked
acquire_freshsite:git_info_refsincrates/gitlawb-node/src/api/repos.rsputs its brake before the acquire, with a comment saying the fresh-acquire must not be reachable unauthenticated and unlimited. The same reasoning applies here; this route just never got it.Established by reading the code and #196's diff. Not driven against a running node.