security + code quality hardening (unrelated to #34)#35
Draft
kkolodziej39 wants to merge 2 commits intohenrygd:masterfrom
Draft
security + code quality hardening (unrelated to #34)#35kkolodziej39 wants to merge 2 commits intohenrygd:masterfrom
kkolodziej39 wants to merge 2 commits intohenrygd:masterfrom
Conversation
Unrelated to any new feature — bundle of security fixes, input
validation, URL encoding, and code cleanups discovered during review
of the repo.
Security:
- Validate logo school slug with /^[a-z0-9-]+$/i (prevents path traversal)
- Encode all GraphQL URL parameters with encodeURIComponent (previously
raw string-interpolated into the URL)
- Add AbortSignal.timeout(10000) to all external fetch calls
- Add X-Content-Type-Options: nosniff and X-Frame-Options: DENY headers
- Add res.ok check to /schools-index before parsing JSON
- Fix getDivisionCode returning raw user input as fallback — now throws
- Fix /schedule-alt crash on invalid sport key access
- Year validation on /brackets (/^\d{4}$/ regex)
- Wrap getTodayUrl() in try-catch in /scoreboard handler
Correctness:
- Fix cache key collision: `path + page` could collide (e.g. /stats/foo1
vs /stats/foo?page=1 both produce "/stats/foo1"). Now uses "?page=N"
separator.
- Fix JSON.stringify(undefined) in /game/:id when upstream data missing:
now returns 502 with a descriptive message
- Fix hash-retry loop guard: replace confusing `hashes.length > 2` check
with explicit index counter capped at maxAttempts
Code cleanups:
- Remove dead commented-out code (cache_5m, shouldUseNewEndpoint,
unused Contest game properties)
- Log errors in all catch blocks instead of swallowing them
- Remove redundant cache.has() check in catch-all route (already done
in onBeforeHandle)
- Fix invalid OpenAPI parameter placement on /game/{id}/scoring-summary,
/game/{id}/team-stats, /schedule-alt, and /logo (parameters were at
path level instead of inside the `get:` operation object)
- Fix /logo `dark` query param schema type (boolean -> string; HTTP
query params are strings and the handler compares as string)
Tests: all 24 existing tests pass.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
Owner
|
Sorry, haven't had time to look at this yet. However, I had some bad actors abusing the public API and needed to add additional validations, which introduced merge conflicts. I can fix those when I have time to look at this, but I'm swamped right now so it may be a bit. |
Contributor
Author
|
@henrygd No worries at all, no rush Sorry to hear that is happening and causing those extra issues/effort to prevent those bad actors. Hope all is resolved now! |
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
2 participants
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.
Summary
Bundle of security fixes, input validation, URL encoding, and code cleanups — separated from #34 per reviewer feedback to keep the new feature PR focused.
Changes
Security:
/^[a-z0-9-]+$/i(prevents path traversal)encodeURIComponent(previously raw string-interpolated)AbortSignal.timeout(10000)to all externalfetchcallsX-Content-Type-Options: nosniffandX-Frame-Options: DENYheadersres.okcheck to/schools-indexbefore parsing JSONgetDivisionCodereturning raw user input as fallback — now throws/schedule-altcrash on invalid sport key access/brackets(/^\d{4}$/regex)getTodayUrl()in try-catch in/scoreboardhandlerCorrectness:
path + pagecould collide (e.g./stats/foo1vs/stats/foo?page=1both produce/stats/foo1). Now uses?page=Nseparator./game/:idnow returns 502 when upstream response shape is missing.data, instead of returning the literal string"undefined".hashes.length > 2check with explicit index counter capped atmaxAttempts.Cleanups:
cache_5m,shouldUseNewEndpoint, unused Contest game properties)catchblocks instead of swallowing themcache.has()check in catch-all route (already handled inonBeforeHandle)/game/{id}/scoring-summary,/game/{id}/team-stats,/schedule-alt, and/logo(parameters were at path level instead of inside theget:operation)/logodarkquery param schema type (boolean→string; HTTP query params are strings)Draft status
Opened as draft because I'd like #34 to land first (feature). Happy to mark ready for review once you've merged that one, or any time you prefer.
Test plan
🤖 Generated with Claude Code