-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: propagate a 429 status code if email-provider rate limiting occurs #5801
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { flattenErrors, statusCodeForError } from "lib/graphqlErrorHandler" | ||
|
||
// Log in `extensions` if this request was by an email provider and resulted | ||
// in a 429 rate limit error from a downstream request made using `fetch`. | ||
export const emailProviderRateLimitExtension = (_documentAST, result) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ie, with the named export here, and the file name being snake case, that creates a 1:2 naming representation in the codebase, which is unnecessary. |
||
const extensions = {} | ||
|
||
if (result.errors && result.errors.length) { | ||
result.errors.some((err) => { | ||
flattenErrors(err).some((e) => { | ||
const httpStatusCode = statusCodeForError(e) | ||
if (httpStatusCode === 429) { | ||
extensions["emailProviderLimited"] = true | ||
return true | ||
} | ||
return false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's just the early-returning of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use something other than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something like: const extensions = result.errors?
.flatMap(flattenErrors)
.reduce((acc, error) => {
const httpStatusCode = statusCodeForError(error)
if (httpStatusCode === 429) {
acc.emailProviderLimited = true
}
return acc
}, {})
return extensions |
||
}) | ||
}) | ||
} | ||
|
||
return extensions | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ import { principalFieldDirectiveValidation } from "directives/principleField/pri | |
import * as Sentry from "@sentry/node" | ||
import { bodyParserMiddleware } from "lib/bodyParserMiddleware" | ||
import { initilizeFeatureFlags } from "lib/featureFlags" | ||
import { emailProviderRateLimitExtension } from "extensions/email_provider_rate_limit_extension" | ||
|
||
// Initialize Unleash feature flags as early as possible | ||
initilizeFeatureFlags() | ||
|
@@ -84,10 +85,17 @@ function createExtensions(document, result, requestID, userAgent) { | |
? fetchLoggerRequestDone(requestID, userAgent) | ||
: {} | ||
|
||
// Should this introspect xapp token/roles for `email_provider` instead? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wondered this same thing. It would be more reliable, but more complex code-wise. Overall I think it's OK to use the user-agent in this way because Braze is a well-behaved integration. The worst case is that a caller spoofing their user-agent gets this same, conservative treatment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, worst-case winds up being this is applied more often/overzealously. I think it's fine that we try to call it 'email provider' in our code, but somewhere in the stack (ideally maybe kept to one or minimal spots), there is a check that winds up having to mention 'braze'. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may not know and this is a bit of a off topic question but I've wondered for a while about the ID thats part of Braze user-agent:
What is the purpose of that ID? Is that something that is unique for the organization that Braze has a relationship with or perhaps its masking information to make it harder to spoof the user-agent? Does the ID stay constant or can it change at any point? 🤔 |
||
const isEmailProviderRequest = userAgent.includes("Braze") | ||
const emailProviderExtensions = isEmailProviderRequest | ||
brainbicycle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
? emailProviderRateLimitExtension(document, result) | ||
: {} | ||
|
||
const extensions = { | ||
...optionalFieldsExtensions, | ||
...principalFieldExtensions, | ||
...requestLoggerExtensions, | ||
...emailProviderExtensions, | ||
} | ||
|
||
// Instead of an empty hash, which will include `extensions: {}` | ||
|
@@ -248,6 +256,42 @@ export const supportedV2RouteHandler = (req, res, next, server) => { | |
return server(req, res, next) | ||
} | ||
|
||
// Middleware that must be mounted on our GraphQL route _before_ | ||
// our regular GraphQL handler. This is so we can register a new `res.end` | ||
// method that will intercept the response and check if any email provider | ||
// rate limting occurred. | ||
// | ||
// If so, we return a 429 status code and a JSON error message. | ||
app.use("/v2", function (req, res, next) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're gonna chatGPT stuff we should at least follow modern JS conventions and audit the output 😄 we never see these types of anonymous |
||
const userAgent = req.headers["user-agent"] || "" | ||
if (!userAgent.includes("Braze")) { | ||
return next() | ||
} | ||
|
||
const originalEnd = res.end | ||
|
||
res.end = function (...args) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
res.end = originalEnd | ||
const [chunk, encoding, cb] = args | ||
|
||
const contentType = res.get("content-type") || "text/html" | ||
|
||
if (contentType.indexOf("application/json") === 0) { | ||
const response = JSON.parse(chunk) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unfortunate we have to re-parse this. Uninformed question: I've seen There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose one could use In general trying to implement some version of this was tricky for exactly this reason!
|
||
|
||
if (response.extensions && response.extensions.emailProviderLimited) { | ||
res.status(429).json({ error: "rate limit reached, try again later" }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This drops the other response data (including the extensions data), and just returns this tiny error blob, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup |
||
} else { | ||
res.end(chunk, encoding, cb) | ||
} | ||
} else { | ||
res.end(chunk, encoding, cb) | ||
} | ||
} | ||
|
||
return next() | ||
}) | ||
|
||
app.use("/v2", (req, res, next) => { | ||
supportedV2RouteHandler(req, res, next, graphqlServer) | ||
}) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
jest.mock("lib/apis/fetch", () => jest.fn()) | ||
import { HTTPError } from "lib/HTTPError" | ||
import fetch from "lib/apis/fetch" | ||
const mockFetch = fetch as jest.Mock | ||
|
||
describe("rate limiting custom status code and response", () => { | ||
const request = require("supertest") | ||
const app = require("../../index").default | ||
const gql = require("lib/gql").default | ||
|
||
beforeEach(() => { | ||
mockFetch.mockClear() | ||
mockFetch.mockReset() | ||
}) | ||
|
||
it("propagates a 429 for email provider rate limiting", async () => { | ||
mockFetch.mockRejectedValueOnce( | ||
new HTTPError("Braze rate limit reached", 429) | ||
) | ||
|
||
const response = await request(app) | ||
.post("/v2") | ||
.set("Accept", "application/json") | ||
.set("User-Agent", "Braze") | ||
.send({ | ||
query: gql` | ||
{ | ||
artist(id: "banksy") { | ||
name | ||
} | ||
} | ||
`, | ||
}) | ||
|
||
expect(response.statusCode).toBe(429) | ||
expect(response.body.error).toBe("rate limit reached, try again later") | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but can we rename this file to
emailProviderRateLimitExtension
so that it matches idiomatic JS? We have snake_case in MP, but that's very old.