-
-
Couldn't load subscription status.
- Fork 246
Integrate graphql-codegen to generate GraphQL types #2155
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
Changes from 6 commits
5a21617
8402bf6
9a3a5e8
441e25c
5c8b6e1
55f86cc
0e7055d
dd5d2fe
1fc0818
09b8ca5
a239467
d71e394
7bb3bc6
f15fe68
e17d036
cb90877
bcf8d26
4babd73
f1c7653
3316cce
6bb7ed0
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 |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| .pnpm-store/ | ||
| pnpm-lock.yaml | ||
| src/types/__generated__/ | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,52 @@ | ||||||||||||||||||||||||||||||||||||||||||
| import { CodegenConfig } from '@graphql-codegen/cli' | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const PUBLIC_API_URL = process.env.PUBLIC_API_URL || 'http://localhost:8000' | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
rudransh-shrivastava marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||
| const createCodegenConfig = async (): Promise<CodegenConfig> => { | ||||||||||||||||||||||||||||||||||||||||||
| const response = await fetch(`${PUBLIC_API_URL}/csrf/`, { | ||||||||||||||||||||||||||||||||||||||||||
| credentials: 'include', | ||||||||||||||||||||||||||||||||||||||||||
| method: 'GET', | ||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
| const createCodegenConfig = async (): Promise<CodegenConfig> => { | |
| const response = await fetch(`${PUBLIC_API_URL}/csrf/`, { | |
| credentials: 'include', | |
| method: 'GET', | |
| }) | |
| const createCodegenConfig = async (): Promise<CodegenConfig> => { | |
| const controller = new AbortController() | |
| const timeout = setTimeout(() => controller.abort(), 5000) | |
| const response = await fetch(`${PUBLIC_API_URL}/csrf/`, { | |
| method: 'GET', | |
| signal: controller.signal, | |
| }) |
🤖 Prompt for AI Agents
In frontend/graphql-codegen.ts around lines 5 to 10, the fetch call to
`${PUBLIC_API_URL}/csrf/` incorrectly includes credentials: 'include' (no-op in
Node) and lacks a timeout, risking hanging; remove the credentials option,
create an AbortController, pass its signal to fetch, set a short timeout (e.g.,
5s) that calls controller.abort(), and ensure you clear the timeout after fetch
completes and properly catch/handle the abort error so the function fails fast
when /csrf/ is slow or down.
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.
🛠️ Refactor suggestion
Harden CSRF extraction and clear timeout.
Be resilient to different payload shapes and fall back to Set-Cookie. Fail fast if still missing.
- if (!response.ok) {
+ clearTimeout(timeout)
+ if (!response.ok) {
throw new Error(`Failed to fetch CSRF token: ${response.status} ${response.statusText}`)
}
- const data = await response.json()
- const csrfToken = data.csrftoken
+ type CsrfResponse = { csrftoken?: string; csrfToken?: string; token?: string }
+ const data = (await response.json().catch(() => ({}))) as CsrfResponse
+ let csrfToken = data.csrftoken ?? data.csrfToken ?? data.token
+ if (!csrfToken) {
+ const setCookie = response.headers.get('set-cookie') ?? ''
+ const m = setCookie.match(/(?:^|;\s*)csrftoken=([^;]+)/i)
+ if (m) csrfToken = m[1]
+ }
+ if (!csrfToken) {
+ throw new Error('Failed to extract CSRF token from CSRF endpoint response')
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!response.ok) { | |
| throw new Error(`Failed to fetch CSRF token: ${response.status} ${response.statusText}`) | |
| } | |
| const data = await response.json() | |
| const csrfToken = data.csrftoken | |
| clearTimeout(timeout) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch CSRF token: ${response.status} ${response.statusText}`) | |
| } | |
| type CsrfResponse = { csrftoken?: string; csrfToken?: string; token?: string } | |
| const data = (await response.json().catch(() => ({}))) as CsrfResponse | |
| let csrfToken = data.csrftoken ?? data.csrfToken ?? data.token | |
| if (!csrfToken) { | |
| const setCookie = response.headers.get('set-cookie') ?? '' | |
| const m = setCookie.match(/(?:^|;\s*)csrftoken=([^;]+)/i) | |
| if (m) csrfToken = m[1] | |
| } | |
| if (!csrfToken) { | |
| throw new Error('Failed to extract CSRF token from CSRF endpoint response') | |
| } |
🤖 Prompt for AI Agents
In frontend/graphql-codegen.ts around lines 11 to 16, the code assumes the CSRF
token is at data.csrftoken and doesn't clear the fetch timeout or handle
alternate payloads; update the logic to: parse JSON safely (guarding for
data.csrftoken, data.csrfToken, data.token, or nested properties), if JSON
parsing fails or none of those fields exist, check the
response.headers.get('set-cookie') (or other cookie headers) for a csrf token
fallback, and ensure any timeout/abort controller is cleared or aborted cleanup
is performed; if after these attempts the token is still missing, throw an
explicit error immediately to fail fast.
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.
Add Referer and normalize CSRF header casing (Django requires it over HTTPS).
Without a Referer that matches the target origin, Django will reject POST introspection requests even with a valid CSRF token. Also prefer the conventional header casing.
Apply:
headers: {
- Cookie: `csrftoken=${csrfToken}`,
- 'x-csrftoken': `${csrfToken}`,
+ Cookie: `csrftoken=${csrfToken}`,
+ 'X-CSRFToken': csrfToken,
+ Referer: PUBLIC_API_URL,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [`${PUBLIC_API_URL}/graphql/`]: { | |
| headers: { | |
| Cookie: `csrftoken=${csrfToken}`, | |
| 'x-csrftoken': `${csrfToken}`, | |
| }, | |
| }, | |
| }, | |
| [`${PUBLIC_API_URL}/graphql/`]: { | |
| headers: { | |
| Cookie: `csrftoken=${csrfToken}`, | |
| 'X-CSRFToken': csrfToken, | |
| Referer: PUBLIC_API_URL, | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In frontend/graphql-codegen.ts around lines 43 to 49, the request headers are
missing a Referer and use a nonstandard CSRF header casing; add a Referer header
that matches the target origin (e.g. PUBLIC_API_URL with trailing slash) and
change the header name from 'x-csrftoken' to the conventional 'X-CSRFToken' so
Django accepts POST introspection over HTTPS; keep the Cookie csrftoken as-is
and ensure the Referer value is exactly the API origin expected by Django.
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.
django does not reject them, it works fine.
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.
@rudransh-shrivastava Thank you for clarifying! I appreciate you testing this and confirming it works with your current Django configuration.
My suggestion was based on Django's default CSRF protection behavior, particularly for HTTPS environments where Django typically requires a Referer header that matches the target origin. However, since you've verified it works fine in your setup, it's likely that:
- Your Django configuration has appropriate CSRF settings for your use case
- The GraphQL introspection might be using GET requests rather than POST
- Your development/deployment environment doesn't trigger Django's stricter CSRF validation
Since you've confirmed the current implementation works, there's no need to make these changes. Thanks for the feedback!
✏️ Learnings added
Learnt from: rudransh-shrivastava
PR: OWASP/Nest#2155
File: frontend/graphql-codegen.ts:0-0
Timestamp: 2025-08-31T13:48:09.785Z
Learning: In the OWASP/Nest project, Django's GraphQL endpoint accepts CSRF tokens via 'x-csrftoken' header (lowercase) without requiring a Referer header, working fine in their configuration for GraphQL codegen introspection.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: rudransh-shrivastava
PR: OWASP/Nest#2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.180Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |||||||||||||||||
| "dev": "next dev --port 3000", | ||||||||||||||||||
| "format": "prettier --log-level warn --write .", | ||||||||||||||||||
| "format:check": "prettier --check .", | ||||||||||||||||||
| "graphql-codegen": "graphql-codegen --config graphql-codegen.ts", | ||||||||||||||||||
|
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. 🛠️ Refactor suggestion Generate types automatically during builds. Prevent stale/missing types by invoking codegen before build. "scripts": {
- "build": "next build",
+ "build": "pnpm run graphql-codegen && next build",
"build:turbo": "next build --turbo",
@@
- "graphql-codegen": "graphql-codegen --config graphql-codegen.ts",
+ "graphql-codegen": "graphql-codegen --config graphql-codegen.ts",
+ "predev": "pnpm run graphql-codegen",
+ "pretest:unit": "pnpm run graphql-codegen",📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsThere 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. how is this possible if we run backend separately? if we invoke codegen before build it may fail as backend may or may not be available. Codegen requires a server to query the GraphQL endpoint 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. @rudransh-shrivastava You're absolutely right! That's a critical oversight on my part. Running codegen during build would indeed fail when the backend isn't available, which is common in CI/CD pipelines or local development scenarios. Here are better approaches to handle this: Option 1: Use a static schema file // In graphql-codegen.ts, replace the schema URL with a local file
- schema: `${PUBLIC_API_URL}/graphql/`,
+ schema: './schema.graphql',Option 2: Make codegen conditional {
"scripts": {
"graphql-codegen": "graphql-codegen --config graphql-codegen.ts",
"graphql-codegen:safe": "graphql-codegen --config graphql-codegen.ts || echo 'Codegen skipped - server unavailable'",
"build": "pnpm run graphql-codegen:safe && next build"
}
}Option 3: Separate CI step Given that you've already committed the generated types, Option 3 seems to align with your current approach. The types would be generated when the backend is available and committed to the repo. What's your preference for handling this in your deployment pipeline? 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 would like to know if there's a way to run graphql-codegen after backend is available? This is for local development purposes. I am assuming that for CI jobs -- all generated files are already present. 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.
The first solution with |
||||||||||||||||||
| "lighthouse-ci": "lhci autorun", | ||||||||||||||||||
| "lighthouse-ci:desktop": "lhci autorun --collect.settings.preset=desktop", | ||||||||||||||||||
| "lint": "eslint . --config eslint.config.mjs --fix --max-warnings=0", | ||||||||||||||||||
|
|
@@ -68,6 +69,9 @@ | |||||||||||||||||
| "@axe-core/react": "^4.10.2", | ||||||||||||||||||
| "@eslint/eslintrc": "^3.3.1", | ||||||||||||||||||
| "@eslint/js": "^9.34.0", | ||||||||||||||||||
| "@graphql-codegen/cli": "^5.0.7", | ||||||||||||||||||
| "@graphql-codegen/typescript": "^4.1.6", | ||||||||||||||||||
| "@graphql-codegen/typescript-operations": "^4.6.1", | ||||||||||||||||||
| "@lhci/cli": "^0.15.1", | ||||||||||||||||||
| "@playwright/test": "^1.55.0", | ||||||||||||||||||
| "@swc/core": "^1.13.5", | ||||||||||||||||||
|
|
||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.