Repository navigation
Fix/middleware startup - #267
Open
Saritahimthani wants to merge 4 commits into
Open
Saritahimthani wants to merge 4 commits into
Saritahimthani wants to merge 4 commits into
Conversation
Challenge accept now saves a PvpGame (gameId, white, black, status), and GET /challenge/game/:gameId lets the chess server verify a joining player's identity against it instead of trusting the client. Adds the service-key-gated POST /internal/gameResults, validated against the saved PvpGame (unknown gameId -> 404, mismatched players -> 400), idempotent on gameId. Removes the player-facing POST /gameResults entirely - results can now only be written by the chess server, authenticated with CHESS_SERVICE_KEY. See "PvP game results: server-authoritative reporting" (v2), T1-T3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ts own key newpvpgame now calls GET /challenge/game/:gameId with the joining player's own JWT and seats them under the username the middleware's auth resolves (`you`), never the client-claimed username - a mismatch is rejected with gameerror instead of silently corrected. createOrJoinPvpGame takes white/black from that response only. Deletes the Authentication header bug at the old EventHandlers.js:60 rather than renaming it - the whole player-token report path is gone. reportGameResult now builds its request with the new pure reporting/resultRequest.js and authenticates with CHESS_SERVICE_KEY instead of relaying a player's token. Seats no longer store player credentials for reporting. Adds validateEnvironment.js (new - no such startup check existed) so a production boot without CHESS_SERVICE_KEY/MIDDLEWARE_URL fails fast instead of silently skipping every report, plus .env.example. Adds the cross-service contract test (T4b): it imports resultRequest.js by relative path and sends its exact output through the real /internal/gameResults route (real service-key check, real models) so the two services can't each pass their own tests while disagreeing - which is how the header bug went unnoticed in v1. Updates the design doc and feature guide to describe the new trust model, replacing the known v1 limitation. See "PvP game results: server-authoritative reporting" (v2), T4, T4b, T7. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Not run yet - needs the production count check and Devin's call on whether legacy records should be excluded from chess score first. Idempotent (only touches documents missing `source`), safe to run once those are answered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rebasing the 3 PvP commits onto current upstream/main (now includes Karthik's/Jimmy's merged currency-ledger work and a separate signed-environment-separation effort) surfaced two real breaks, beyond the straightforward conflicts in challenge.js/server.js and the duplicate validateEnvironment.js/.env.example already resolved in the rebase itself: - gameResults.source was required: true, which broke the already- merged tests/backfillCurrencyLedger.test.js - its fixtures create GameResults documents with no source field. Changed to default: "legacy-unverified" instead of required: true: any write that doesn't explicitly claim source: "chessServer" (only internalGameResults.js does) now safely falls back to unverified rather than failing. Strictly more correct than required, and non-breaking for existing callers. - challenge.pvpgame.test.js's acceptedGame() helper called POST /challenge and POST /challenge/:id/accept with no identity header. Both routes are now behind requireAuth with identity enforcement (added upstream, independently of this plan) - the caller must be fromUsername to create a challenge, and only toUsername may accept it. Updated the helper to authenticate as the right player on each call. All 346 middleware + 40 chessServer tests pass on top of current main. Co-Authored-By: Claude Sonnet 5 <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
1 participant
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
[Briefly describe what this PR does and why]
Type of Change
Key Changes
[Describe the main areas of code affected and what was done, e.g., "Auth: Split signup into Parent/Mentor flows" or "Dashboard: Added parent dashboard and child progress view"]
Testing
Bugs Fixed (if applicable)
TODO (Follow-up Work)
Additional Notes
[Any extra context, dependencies, or deployment considerations]