Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/extensions/email_provider_rate_limit_extension.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { flattenErrors, statusCodeForError } from "lib/graphqlErrorHandler"
Copy link
Member

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.


// 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) => {
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these true/false return values do anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just the early-returning of the .some iterator. I can double-check this snippet (it may or may not have been AI-generated).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use something other than some here? To joey's point, return true / false means that we should / should not return a result from some to be collected into a variable. If we're simply iterating over an array to check for values to be merged into an object, we can use forEach, which doesn't return anything and reads more correctly.

Copy link
Member

Choose a reason for hiding this comment

The 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
}
44 changes: 44 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -84,10 +85,17 @@ function createExtensions(document, result, requestID, userAgent) {
? fetchLoggerRequestDone(requestID, userAgent)
: {}

// Should this introspect xapp token/roles for `email_provider` instead?
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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'.

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

Braze Sender 954a8af2d130500d09e736a5e8ce1db59da5c25f; Metaphysics

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: {}`
Expand Down Expand Up @@ -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) {
Copy link
Member

@damassi damassi Jul 4, 2024

Choose a reason for hiding this comment

The 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 function declarations anywhere, not for years. Please use arrow functions

const userAgent = req.headers["user-agent"] || ""
if (!userAgent.includes("Braze")) {
return next()
}

const originalEnd = res.end

res.end = function (...args) {
Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 context used for passing request data around. Is it not possible to use that to make the 429 status available, as opposed to rendering it into the extensions response?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose one could use context for that, but that's a GraphQL-server-concept (as in, within the scope of the special express-graphql sub-app mounted at our /v2 endpoint) - and so this middleware (which is a totally separate Express middleware mounted before that sub-app) doesn't have access to that context.

In general trying to implement some version of this was tricky for exactly this reason!

  • Can't do it natively with express-graphql as there's no parseResponse or any other 'hook' available -> leading to possible patch.
  • Can't do it natively with Express middleware as that won't have access to the completed response, unless there's some tricksy res.on("end", ... overriding - and then this approach implies that we do need to store the 'this is an email provider request that resulted in rate limiting' fact in the response somewhere.


if (response.extensions && response.extensions.emailProviderLimited) {
res.status(429).json({ error: "rate limit reached, try again later" })
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
})
Expand Down
38 changes: 38 additions & 0 deletions src/integration/__tests__/email_provider_rate_limit.test.ts
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")
})
})