Repository navigation
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 44 |
🟢 Coverage 92.75% diff coverage
Metric Results Coverage variation Report missing for 4bb1fee1 Diff coverage ✅ 92.75% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (4bb1fee) Report Missing Report Missing Report Missing Head commit (0169f3b) 94146 27948 29.69% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3973) 69 64 92.75% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
asaah262
approved these changes
Nov 13, 2026
Check that a user exports only their own personal data in a new middleware, RequireOwnUserID, so that the check can run before other middlewares on the route, such as a rate limit. The route passes the name of its user id parameter to it. The handler keeps its own check as well, as a safety net: exporting the data of one user to another must never happen, also when the route is changed by mistake. When that check catches a request, it logs an error, since the middleware must be missing. Both unescape the user id from the path, like the other handlers of the users endpoint do, and reject an empty one. The middleware answers 401 when there is no authenticated user. Requests for other users still get a 400, now with a JSON error body instead of a plain text one. Assisted-by: Claude Code:claude-opus-5-5
Each personal data export collects all events of the user and writes them into the user's personal space, so starting exports over and over puts load on the system. Limit how many exports a user can start to 5 per minute, configurable with GRAPH_EXPORT_PERSONAL_DATA_RATE_LIMIT (0 disables the limit). Requests beyond it get a 429 with a Retry-After header. The limit uses a new middleware, RateLimitPerAuthenticatedUser. It counts requests by the id of the authenticated user rather than by the URL, so that variations of the URL, such as a trailing slash, can't get around it. It comes after the check that users only export their own data, so that requests for other users don't count against any limit. The counters are kept in memory by every instance of the graph service. This adds the dependency github.com/go-chi/httprate v0.16.1 (MIT), from the authors of the chi router that the service already uses. It is used instead of a limiter of our own, because counting requests in sliding time windows correctly under concurrency is easy to get wrong, and the library is maintained and tested for exactly that. It brings no further modules: the two it needs are already required. There are no known vulnerabilities in it, and govulncheck reports none. Assisted-by: Claude Code:claude-opus-5-5
A request to export the personal data beyond the rate limit got the plain text 429 of httprate, while the other errors of graph, also those of the same route, are JSON with an error code. RateLimitPerAuthenticatedUser takes a handler for the response beyond the limit now, so that a service can answer in its own format; nil keeps the plain text 429. Graph answers with activityLimitReached. The Retry-After header is set before the handler is called, so it stays. Assisted-by: Claude Code:claude-opus-5-5
butonic
force-pushed
the
feat/graph-export-personal-data-rate-limit
branch
from
November 13, 2026 17:26
9ad2c55 to
0169f3b
Compare
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.
Limits how many personal data exports a user can start: 5 per minute by default, configurable with
GRAPH_EXPORT_PERSONAL_DATA_RATE_LIMIT(0 disables the limit). Requests beyond it get429 Too Many Requestswith aRetry-Afterheader.Changes
RequireOwnUserID(graph middleware): checks that users only export their own data before the rate limit runs, so that requests for other users don't count against any limit. The route passes the name of its user id parameter. They still get a 400, now with a JSON error body instead of plain text. A code comment notes that 403 would arguably fit better; it stays 400 so that clients aren't affected.RateLimitPerAuthenticatedUser(pkg/middleware): a reusable rate limit that counts requests by the id of the authenticated user, not by the URL, so that variations of the URL (e.g. a trailing slash, which the router strips for routing but which stays in the URL) can't get around it. It requires an authenticated user in the context and answers 401 without one.Not part of this PR: limiting how many exports run at the same time, see #3970.
New dependency
This adds
github.com/go-chi/httpratev0.16.1 (MIT), by the authors of the chi router that graph already uses.github.com/zeebo/xxh3andmy-woods-we.pages.dev/klauspost/cpuid/v2, which are already required at newer versions.Retry-Afteris always the full window rather than the remaining time (Retry-After header should maybe not be static go-chi/httprate#47, fix in webfinger #64).Reviewers can skip
vendor/in the diff.Tests
apiGraph/userGDPRExport.featurepasses on posix and decomposed.AI assistance: Claude Code (claude-opus-5-5) analysed the options, wrote the first draft of the code, the tests and this description, and checked the dependency for licenses and vulnerabilities.