Repository navigation
Stress-test fixes: arch-merge LayerNorm remap, CORS whitelist, job au… - #30
Merged
Merged
Conversation
…th order, nginx tuning Pre-public-launch stress test against mergese.usask.ca turned up four fixable issues. This closes the three code-level ones and documents the fourth as a deploy-time nginx update. B1 - CRITICAL: Architecture-greedy merge fails on server-mounted fine-tunes Older HuggingFace Transformers (<=4.0) saved LayerNorm params under `LayerNorm.gamma`/`.beta`; modern Transformers names them `.weight`/`.bias`. `AutoModel.from_pretrained` auto-remaps on load, but the strict=True `load_state_dict` path used by the architecture-greedy eval does not, so the hill-climb's first candidate load blows up with 48 missing keys. Fix: a module-level `_normalize_layernorm_keys` runs before every load_state_dict in `_build_architecture_eval_fn`; it's a no-op on modern checkpoints (returns the same dict object) and a pure-python key rewrite on legacy ones. Two unit tests pin both branches. B5 - CORS reflected any Origin `CORS(app)` with no args echoed whatever `Origin:` the client sent as `Access-Control-Allow-Origin`. Credentials aren't shared cross-origin (bearer-token auth, not cookies) so there's no account takeover vector, but reflecting unknown origins fails least-privilege and lets any site trigger CORS-enabled XHR to our API on behalf of a visitor. Fix: whitelist https://mergese.usask.ca + the two localhost dev ports. `MERGESE_CORS_ORIGINS=https://foo,https://bar` extends the list without a redeploy. Two tests pin "unknown origin -> no reflection" and "whitelisted origin -> reflected". B6 - Job endpoints leaked existence (404 for missing id, 401 for existing) The 6 handlers at /api/jobs/<id>{,/log,/stream,/result,/cancel,/download} called JOBS.get() BEFORE _require_owner(), so an unauthenticated caller could distinguish missing (404) from existing (401). Not practically exploitable (ids are 48 bits of random hex) but worth the one-line fix: _authenticate() runs first, so unauth is 401 regardless of id state. One parametrized test covers all 5 GET variants + a dedicated cancel test. B3/B4 - 503 under modest bursts + no body-size cap on JSON endpoints The deploy/nginx.conf already had rate-limit zones; the fix is three improvements: * limit_req_status 429 so clients see a clear "slow down" instead of a scary "service unavailable" * a dedicated mergese_anon zone (2 r/s burst=5) for /api/v1/anon-token, which blocks the single gunicorn worker on an outbound Turnstile verify * per-location client_max_body_size for /api/v1/anon-token (8k), /api/v1/limits (8k) and /api/health (1k) - previously the 3g default let a client stream 50MB of JSON before Flask rejected it (Infra change: operator applies by replacing /etc/nginx/sites-available/ mergese with the new deploy/nginx.conf and `sudo nginx -s reload`.) Tests: 117 passed, 6 skipped (unchanged). Three new tests cover B1 remap behavior, B5 origin whitelist, B6 auth-first ordering. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…th order, nginx tuning
Pre-public-launch stress test against mergese.usask.ca turned up four fixable issues. This closes the three code-level ones and documents the fourth as a deploy-time nginx update.
B1 - CRITICAL: Architecture-greedy merge fails on server-mounted fine-tunes
Older HuggingFace Transformers (<=4.0) saved LayerNorm params under
LayerNorm.gamma/.beta; modern Transformers names them.weight/.bias.AutoModel.from_pretrainedauto-remaps on load, but the strict=Trueload_state_dictpath used by the architecture-greedy eval does not,so the hill-climb's first candidate load blows up with 48 missing keys.
Fix: a module-level
_normalize_layernorm_keysruns before everyload_state_dict in
_build_architecture_eval_fn; it's a no-op on moderncheckpoints (returns the same dict object) and a pure-python key rewrite
on legacy ones. Two unit tests pin both branches.
B5 - CORS reflected any Origin
CORS(app)with no args echoed whateverOrigin:the client sent asAccess-Control-Allow-Origin. Credentials aren't shared cross-origin(bearer-token auth, not cookies) so there's no account takeover vector,
but reflecting unknown origins fails least-privilege and lets any site
trigger CORS-enabled XHR to our API on behalf of a visitor.
Fix: whitelist https://mergese.usask.ca + the two localhost dev ports.
MERGESE_CORS_ORIGINS=https://foo,https://barextends the list withouta redeploy. Two tests pin "unknown origin -> no reflection" and
"whitelisted origin -> reflected".
B6 - Job endpoints leaked existence (404 for missing id, 401 for existing)
The 6 handlers at /api/jobs/{,/log,/stream,/result,/cancel,/download}
called JOBS.get() BEFORE _require_owner(), so an unauthenticated caller
could distinguish missing (404) from existing (401). Not practically
exploitable (ids are 48 bits of random hex) but worth the one-line fix:
_authenticate() runs first, so unauth is 401 regardless of id state.
One parametrized test covers all 5 GET variants + a dedicated cancel test.
B3/B4 - 503 under modest bursts + no body-size cap on JSON endpoints
The deploy/nginx.conf already had rate-limit zones; the fix is three
improvements:
* limit_req_status 429 so clients see a clear "slow down" instead of a scary "service unavailable"
* a dedicated mergese_anon zone (2 r/s burst=5) for /api/v1/anon-token, which blocks the single gunicorn worker on an outbound Turnstile verify
* per-location client_max_body_size for /api/v1/anon-token (8k), /api/v1/limits (8k) and /api/health (1k) - previously the 3g default let a client stream 50MB of JSON before Flask rejected it
(Infra change: operator applies by replacing /etc/nginx/sites-available/
mergese with the new deploy/nginx.conf and
sudo nginx -s reload.)Tests: 117 passed, 6 skipped (unchanged). Three new tests cover B1 remap behavior, B5 origin whitelist, B6 auth-first ordering.
Summary
Risk + rollback
Testing
pytestpasses locallypython server/app.pyand acurlagainst the changed endpoint.github/workflows/ci.ymlstill passesDeploy note (fill if the merge needs an operator action beyond
git pull && systemctl restart)