Add support for global and per-route security headers in PagesRouter #9958
Conversation
|
🚀 Thanks for opening this pull request! |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-08T13:46:04.940ZApplied to files:
📚 Learning: 2025-12-02T06:55:53.788ZApplied to files:
🧬 Code graph analysis (1)src/Options/Definitions.js (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
J1vvoo
force-pushed
the
test/pagesrouter-secure-headers
branch
from
15828af to
f872368
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Routers/PagesRouter.js (1)
260-314: Route-level headers are dropped for localized/custom-URL pages
goToPageacceptsrouteHeadersand passes them toredirectResponse/pageResponseonly in the non-localized, non-custom-URL branch. That means:
- When
pages.enableLocalizationistrueand a locale is present, the localized branch never forwardsrouteHeaders.- When a
customUrlis configured for a page,redirectResponseis called withoutrouteHeaders.mountCustomRoutescorrectly extractsheaders: routeHeadersfrom the handler and passes them intogoToPage, but those headers are silently ignored in the above scenarios.As a result, per-route headers won’t be applied for localized pages or custom URLs, which is particularly problematic for security-related headers.
Consider updating
goToPageto always propagaterouteHeaders:const customUrl = config.pages.customUrls[page.id]; if (customUrl && !Utils.isPath(customUrl)) { - return this.redirectResponse(customUrl, params); + return this.redirectResponse(customUrl, params, routeHeaders); } // ... if (config.pages.enableLocalization && locale) { return Utils.getLocalizedPath(defaultPath, locale).then(({ path, subdir }) => redirect - ? this.redirectResponse( - this.composePageUrl(defaultFile, config.publicServerURL, subdir), - params - ) - : this.pageResponse(path, params, placeholders) + ? this.redirectResponse( + this.composePageUrl(defaultFile, config.publicServerURL, subdir), + params, + routeHeaders + ) + : this.pageResponse(path, params, placeholders, routeHeaders) ); } else { return redirect ? this.redirectResponse(defaultUrl, params, routeHeaders) : this.pageResponse(defaultPath, params, placeholders, routeHeaders); }With this change, the existing calls from
mountCustomRoutes(goToPage(req, page, query, false, routeHeaders)) will behave consistently regardless of localization or custom URLs.Also applies to: 576-609, 711-732
🧹 Nitpick comments (4)
src/Options/Definitions.js (1)
782-787: Ensure newpages.headers/ routeheadersare backed by Options/index.js and docsThe new
PagesOptions.headersandPagesRoute.headersentries look structurally fine, but since this file is generated, please confirm:
- The
headersfields are defined insrc/Options/index.jsand this file was produced vianpm run definitions, not edited directly.- The corresponding docs (in
src/Options/docs.jsand any user-facing configuration docs) mentionpages.headersand route-levelheadersso users can actually discover and configure them (including viaPARSE_SERVER_PAGES_HEADERS).You may also want to tighten the help text to clarify that these headers are applied to HTML page/redirect responses produced by
PagesRouterrather than literally all possible responses (e.g. JSON XHR responses from the same router).Based on learnings, this keeps generator source and docs aligned.
Also applies to: 805-810
src/Routers/PagesRouter.js (3)
69-76: Avoid duplicating pages config state (_pagesOptionsvspagesConfig)
this._pagesOptionsandthis.pagesConfigare both set to the samepagesobject and then used in different parts of the class. This works, but it introduces two sources of truth for the same config. Consider standardizing on one property (e.g.this.pagesConfig) and derivingglobalHeadersfrom it to reduce the chance of future divergence.
432-495: Globalpages.headersare now applied to non-HTML static assets as well
pageResponseandfileResponseboth mergethis.globalHeadersinto the returnedheaders. BecausestaticRouteusesfileResponsefor non-HTML resources (e.g. images, CSS, JS), global headers frompages.headerswill now also be attached to those static assets, not just HTML pages.That may be fine (and often desirable) for security headers, but it slightly diverges from the “global HTML-page headers” wording in the option help text. If you intend headers strictly for HTML/redirect responses, you may want to:
- Either gate
globalHeadersinfileResponsebased onpath.endsWith('.html'), or- Relax the option help text to describe that headers apply to all responses produced via
PagesRouter’s helpers (including static assets).The current merge order
{ ...this.globalHeaders, ...routeHeaders, ...paramHeaders }looks good: route- and param-derived headers override global ones as expected.
748-775: Clarify and surfacepages.secureHeaders/pages.customHeadersas first-class optionsThe new
expressRouter()middleware:
- Enables clickjacking protection headers by default unless
pages.secureHeaders === false.- Applies any
pages.customHeaderson every request handled by this router.This is a solid pattern, but a couple of points are worth tightening up:
Config surface alignment
options.secureHeadersandoptions.customHeadersare read fromthis._pagesOptions, but there are no corresponding entries for these fields inPagesOptionswithinOptions/Definitions.js. If these are meant to be supported configuration options (especially via environment variables), they should be added toPagesOptionsinsrc/Options/index.jsand the definitions/docs regenerated so users can discover and set them consistently.Default behavior vs backward compatibility
BecauseenableSecureHeadersdefaults totrue, existing deployments will suddenly getX-Frame-Options: DENYandContent-Security-Policy: frame-ancestors 'none'on all pages served byPagesRouter. That’s usually a good security default, but it may be a breaking change for apps that intentionally embed these pages in iframes. Please double-check with maintainers whether this should be opt-in (secureHeaders: true) or opt-out as coded here.If you keep the current default, it would be helpful to document
pages.secureHeaders/pages.customHeadersprominently so operators know how to override or disable these headers when needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Options/Definitions.js(2 hunks)src/Routers/PagesRouter.js(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
src/Options/Definitions.js
🧬 Code graph analysis (1)
src/Routers/PagesRouter.js (2)
src/GraphQL/loaders/filesMutations.js (1)
headers(12-12)src/PromiseRouter.js (2)
headers(143-143)url(140-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Benchmarks
J1vvoo
force-pushed
the
test/pagesrouter-secure-headers
branch
from
f872368 to
6001107
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Routers/PagesRouter.js (1)
260-314: Inconsistent header propagation in early return paths.The
routeHeadersparameter is only passed toredirectResponseandpageResponsein the final return statements (lines 312-313), but not in earlier return paths:
- Line 291: Custom URL redirect doesn't pass
routeHeaders- Lines 304-308: Localized page responses don't pass
routeHeadersThis means route-specific headers (including security headers from custom routes) won't be applied when custom URLs are configured or localization is enabled, creating inconsistent security behavior.
Apply this diff to ensure consistent header propagation:
// If custom URL is set redirect to it without localization const customUrl = config.pages.customUrls[page.id]; if (customUrl && !Utils.isPath(customUrl)) { - return this.redirectResponse(customUrl, params); + return this.redirectResponse(customUrl, params, routeHeaders); } // Get JSON placeholders let placeholders = {}; if (config.pages.enableLocalization && config.pages.localizationJsonPath) { placeholders = this.getJsonPlaceholders(locale, params); } // Send response if (config.pages.enableLocalization && locale) { return Utils.getLocalizedPath(defaultPath, locale).then(({ path, subdir }) => redirect ? this.redirectResponse( this.composePageUrl(defaultFile, config.publicServerURL, subdir), - params + params, + routeHeaders ) - : this.pageResponse(path, params, placeholders) + : this.pageResponse(path, params, placeholders, routeHeaders) ); } else {
🧹 Nitpick comments (1)
src/Routers/PagesRouter.js (1)
764-768: Consider checking existing headers before applying customHeaders.The
customHeadersare applied unconditionally, which means they will override any headers set by external middleware. While this might be intentional to ensure configured headers always apply, it could be surprising behavior.Consider whether
customHeadersshould:
- Always override (current behavior) - ensures configured headers are enforced
- Only apply if not already set (like security headers) - allows more flexibility
If you want to allow external middleware to take precedence, apply this diff:
if (options.customHeaders) { Object.entries(options.customHeaders).forEach(([key, value]) => { - res.set(key, value); + if (!res.get(key)) { + res.set(key, value); + } }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Options/Definitions.js(2 hunks)src/Routers/PagesRouter.js(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Options/Definitions.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Benchmarks
🔇 Additional comments (7)
src/Routers/PagesRouter.js (7)
71-72: LGTM!The constructor properly stores the pages options and initializes global headers with a safe fallback to an empty object.
432-473: LGTM!The header merging logic correctly prioritizes headers from most specific to least specific: parameter headers override route headers, which override global headers. The implementation properly uses the spread operator to achieve this precedence.
576-610: LGTM!The header merging logic is consistent with
pageResponse, correctly prioritizing parameter headers over route headers over global headers.
720-730: LGTM!The custom route handler correctly extracts headers from the route response and propagates them through
goToPage, enabling per-route header customization as intended.
748-775: Verify scope against PR objectives for issue #9951.The implementation correctly wraps the parent router with middleware to apply security headers (X-Frame-Options and Content-Security-Policy) before route handling. However, confirm whether this PagesRouter-specific approach aligns with the issue requirements—if the goal is ParseServer-wide header coverage, verify that security headers are also implemented at the application level, not solely in PagesRouter routes.
480-495: Verify header handling in staticRoute context.The code correctly merges
globalHeadersandrouteHeadersinfileResponse. However, without confirming the actual usage patterns ofstaticRouteand whether it intentionally omitsrouteHeaders, the security and design implications cannot be assessed. Verify that static route behavior aligns with security requirements and design intent.
753-762: Verify the security headers default behavior and header precedence logic.The middleware sets security headers (
X-Frame-Options: DENYandContent-Security-Policy: frame-ancestors 'none') only if they aren't already present. Since this middleware runs before route handlers, theres.get()checks only detect headers set by external middleware that runs earlier.Clarify the intended behavior:
- Should route-specific headers (from
pageResponse/redirectResponse) be able to override these security defaults?- How does PromiseRouter merge headers set via
res.set()in middleware with headers returned in the response object?If route handlers should be able to override security headers, consider applying these defaults at a later stage or checking both res headers and response object headers.
There was a problem hiding this comment.
Check the contribution guide for how to add a new option.
|
@coderabbitai When reviewing a PR that contains changes to the Parse Server options, always make sure the changes are reflected in src/Options/Definitions.js, src/Options/docs.js and src/Options/index.js. The changes are made originally in index.js and then via |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 608 🏁 Script executed: Length of output: 521 🏁 Script executed: Length of output: 19811 🏁 Script executed: Length of output: 12941 🏁 Script executed: Length of output: 1014 @mtrezza, I've verified the Parse Server options files and found an issue with the workflow in this PR: Current state:
The issue: Required fix:
export interface PagesOptions {
// ... existing properties ...
/* Global headers applied to all PagesRouter responses.
:DEFAULT: {} */
headers: ?Object;
}
export interface PagesRoute {
// ... existing properties ...
/* Headers applied only to this specific page route.
:DEFAULT: {} */
headers: ?Object;
}
I'll remember this verification requirement for future Parse Server PRs. Thank you for the guidance! ✏️ Learnings added
🧠 Learnings used |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9958 +/- ##
==========================================
+ Coverage 93.08% 93.71% +0.63%
==========================================
Files 188 188
Lines 15298 15314 +16
Branches 177 177
==========================================
+ Hits 14240 14352 +112
+ Misses 1046 950 -96
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
Pull Request
Issue
Closes: #9951
Approach
Implemented global and per-route security header support in PagesRouter.
pages.headersfor global HTML-page headersheadersoption for each custom routeTasks
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.