feat(structure): Warn when unprotected routes use @requireAuth mutations - #2380
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesMutation route auth check
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many -t build:pack --exclude create-ceda... |
✅ Succeeded | 5s | View ↗ |
nx run-many -t build |
✅ Succeeded | <1s | View ↗ |
nx run-many -t build --output-style=stream |
✅ Succeeded | 1m 20s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-12 06:28:29 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/structure/src/model/__tests__/mutationAuthCheck.test.ts`:
- Around line 115-128: Update the cyclic component fixtures used by
adminPostsCycle to include a `@requireAuth` mutation in one of CycleA or CycleB,
then change the test to assert exactly one
UNPROTECTED_ROUTE_USES_AUTH_GATED_MUTATION warning while retaining the
non-crashing traversal check.
- Around line 26-28: Update the afterEach cleanup for CEDAR_CWD to delete the
process.env property when original_CEDAR_CWD is undefined; otherwise restore the
saved value, preventing a bogus "undefined" environment variable from leaking
between tests.
- Line 17: Update the fixtureDir initialization in mutationAuthCheck.test.ts to
derive the test file directory from import.meta.url using fileURLToPath and
path.dirname, replacing the undefined __dirname reference while preserving the
existing __fixtures__/mutation-auth-check path.
In `@packages/structure/src/model/RWProject.ts`:
- Around line 230-235: Import RWSDLField as a type from ./RWSDLField.js and
annotate the fields variable in the implementableFields handling within
RWProject with RWSDLField[].
In `@packages/structure/src/model/RWRoute.ts`:
- Around line 327-339: In the diagnostic-yielding logic, replace the `as
ExtendedDiagnostic` assertion with a local variable explicitly typed as
`ExtendedDiagnostic`, then yield that variable. Keep the existing object fields
and diagnostic behavior unchanged so TypeScript validates the object structure
directly.
In `@packages/structure/src/model/util/pageMutationUsage.ts`:
- Around line 51-58: Update the queue-processing loop around queue.shift() to
store its result and narrow it with an undefined check before using it as
filePath. Remove the as string assertion, and continue or otherwise safely exit
when shift() returns undefined while preserving the existing visited handling.
- Around line 248-268: Update resolveWithExtensions to strip a trailing .js or
.jsx extension from basePath before testing RESOLVE_EXTENSIONS, so ESM-style
specifiers resolve to corresponding .ts or .tsx sources without producing
candidates such as .js.tsx. Preserve existing direct-file and index resolution
behavior for paths without those extensions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2fad306-c060-4712-93ec-bd8d773cb75b
📒 Files selected for processing (19)
packages/structure/src/errors.tspackages/structure/src/model/RWProject.tspackages/structure/src/model/RWRoute.tspackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/api/src/graphql/posts.sdl.tspackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/redwood.tomlpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/Routes.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/components/CycleA/CycleA.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/components/CycleB/CycleB.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/components/DeletePostButton/DeletePostButton.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/components/PostActions/PostActions.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/pages/AdminPostsCyclePage/AdminPostsCyclePage.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/pages/AdminPostsPage/AdminPostsPage.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/pages/AdminPostsProtectedPage/AdminPostsProtectedPage.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/pages/AdminPostsQueryPage/AdminPostsQueryPage.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/pages/AdminPostsSkipAuthPage/AdminPostsSkipAuthPage.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/pages/AdminPostsTransitivePage/AdminPostsTransitivePage.tsxpackages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/web/src/pages/HomePage/HomePage.tsxpackages/structure/src/model/__tests__/mutationAuthCheck.test.tspackages/structure/src/model/util/pageMutationUsage.ts
Base automatically changed from
tobbe-fix-structure-isprivate-nested-sets
to
main
August 11, 2026 22:29
`cedar check` now emits a Warning when a route not wrapped in <PrivateSet>/<Private> has a page that (transitively, via imports) uses a GraphQL mutation whose root field carries a literal @requireAuth directive in the api-side SDL. Mutations only (auth-gated queries on public pages are a common legitimate pattern), and only Warning severity since shared components can produce deliberate false positives. Fixes #2291
Tobbe
force-pushed
the
tobbe-check-unprotected-auth-mutations
branch
from
August 11, 2026 22:29
eaf4d89 to
2e4f16f
Compare
✅ Deploy Preview for cedarjs canceled.
|
- Resolve .js/.jsx import specifiers back to their .tsx/.ts source in pageMutationUsage.ts, so pages that use extensioned relative imports aren't silently skipped by the transitive mutation walk - Replace `as string`/`as ExtendedDiagnostic` casts with narrowing and an explicit local type, matching the repo's casting guidelines - Annotate the SDL implementable-fields loop variable instead of relying on inference - Fix afterEach leaking the literal string "undefined" into process.env.CEDAR_CWD when it wasn't set before the test - Strengthen the component-cycle test to assert the mutation is reported exactly once (dedup), not just that nothing crashes
|
The changes in this PR are now available on npm. Try them out by running Or try it in a new app with |
1 task
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.
Fixes #2291
Summary
cedar check(via@cedarjs/structure'sprintDiagnostics) now emits a Warning for routes that are not wrapped in<PrivateSet>/<Private>but whose page — transitively, via its import graph — uses a GraphQL mutation whose root field carries a literal@requireAuthdirective in the api-side SDL.How it works:
SDL directive map (
RWProject.requireAuthMutationFields, inpackages/structure/src/model/RWProject.ts): a lazy getter that walksRWProject.sdls→RWSDL.implementableFieldsand buildsMap<mutationFieldName, sdlFilePathRelativeToProjectRoot>for everyMutation-type root field carrying a literal@requireAuthdirective. Each SDL file is parsed in isolation (try/catch per file) so one malformed SDL doesn't hide valid@requireAuthfields declared elsewhere.Transitive import walk (
packages/structure/src/model/util/pageMutationUsage.ts, new module): given a page's file path, does a BFS overImportDeclarationmodule specifiers (resolving relative imports and thesrc/→web/src/alias, trying.tsx/.ts/.jsx/.jsand/index.*), skipping anything that doesn't resolve insideweb/src. In each reachable file it looks forgql/graphqltagged template expressions, concatenates the static (quasi) parts of the template, parses it withgraphql'sparse, and collects the top-level selection field names of anymutationoperation definitions. Results are cached per file at the project level (via aWeakMap<RWProject, …>) since many pages share components. A visited set guards against import cycles.The diagnostic (
RWRoute.*diagnostics()): when a route isn't private and its page resolves, intersects the page's transitively-used mutation fields with the project's@requireAuthmap and yields aWarningper hit, anchored at the route's JSX node, e.g.:A new
RWError.UNPROTECTED_ROUTE_USES_AUTH_GATED_MUTATIONcode is attached.Scoping decisions
@requireAuthonly —@skipAuthand any custom directives are ignored. Keeps the check simple and avoids false positives from project-specific auth directives with different semantics.DiagnosticSeverity.Warning, never Error — this is a best-effort heuristic (see limitations below), so it must never failcedar checkor block CI; it's a nudge, not a hard rule.Known limitations
gql/graphqltemplates are handled best-effort: the static (quasi) parts of the template are concatenated and the interpolated parts are dropped, which is usually fine for extracting the operation's field shape but can occasionally fail to parse — such documents are silently skipped.<DeleteButton>used from both a protected and an unprotected route) can cause the warning to fire even when the actual call site guarding is fine at the page level, purely because the mutation is reachable from an unprotected page's import graph. This is a deliberate trade-off — hence Warning, not Error.export * from './x') or resolve TypeScript path aliases beyond thesrc/→web/src/convention.Testing
yarn workspace @cedarjs/structure build— passCI=1 yarn workspace @cedarjs/structure test— 38/38 tests pass (7 test files)packages/structure/src/model/__tests__/__fixtures__/mutation-auth-check/(not one of the shared repo-root__fixtures__/projects, since those are snapshotted by other packages), with new tests inpackages/structure/src/model/__tests__/mutationAuthCheck.test.tscovering:@requireAuthmutation → warning, with route/mutation/SDL/component names in the message<PrivateSet><Set>…</Set></PrivateSet>(nested-Set case from fix(structure): Detect Private/PrivateSet ancestors beyond the immediate parent #2379) → no warninggqltag) → warning@skipAuth→ no warning@requireAuthquery (not mutation) on an unprotected page → no warningnpx prettier --check/npx eslinton all changed/added files — passexample-todo-main/example-todo-main-with-errorsfixtures used bymodel.test.ts: no snapshot changes. Verified directly thatRWProject.requireAuthMutationFieldsis empty for both (example-todo-main's mutations are all@skipAuth;example-todo-main-with-errors's only@requireAuth-adjacent field is declared undertype Query, nottype Mutation, and one of its SDL files has an intentionally-malformed schema string, which is now isolated per-file rather than aborting the whole map).Stacking
This PR is stacked on #2379 (
tobbe-fix-structure-isprivate-nested-sets), which fixedRWRoute.isPrivateto walk nested<Set>/<PrivateSet>JSX ancestors — this PR's nested-Set warning-suppression test (case 2 above) depends on that fix. Base is set totobbe-fix-structure-isprivate-nested-sets; please retarget tomainafter #2379 merges.